Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve reproducibility of autoloader files - add missing sort #12090

Closed
wants to merge 2 commits into from
Closed

Improve reproducibility of autoloader files - add missing sort #12090

wants to merge 2 commits into from

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Aug 22, 2024 •
edited
Loading

While updating PHPDocumentor in Nix (#336145), we encountered instability with the new version of the Composer builder introduced in #308059. In the context of Nix, "unstable" means that the vendor directory produced by Composer was not consistently reproducible.

image

To reproduce the issue locally:

$ git clone https://github.com/phpdocumentor/phpdocumentor
$ cd phpdocumentor
$ git checkout v3.6.0
$ rm -rf vendor0 vendor1
$ composer install
$ mv vendor vendor0
$ rm -rf ~/.cache/composer
$ composer install
$ mv vendor vendor1
$ diffoscope --exclude-directory-metadata recursive vendor0 vendor1
--- vendor0
+++ vendor1
│   --- vendor0/composer
├── +++ vendor1/composer
│ │   --- vendor0/composer/autoload_psr4.php
│ ├── +++ vendor1/composer/autoload_psr4.php
│ │ @@ -4,15 +4,15 @@
│ │  
│ │  $vendorDir = dirname(__DIR__);
│ │  $baseDir = dirname($vendorDir);
│ │  
│ │  return array(
│ │      'phpDocumentor\\Reflection\\' => array($vendorDir . '/phpdocumentor/reflection-common/src', $vendorDir . '/phpdocumentor/type-resolver/src', $vendorDir . '/phpdocumentor/reflection-docblock/src'),
│ │      'phpDocumentor\\JsonPath\\' => array($baseDir . '/incubator/json-path/tests/unit', $vendorDir . '/phpdocumentor/json-path/src'),
│ │ +    'phpDocumentor\\Guides\\' => array($vendorDir . '/phpdocumentor/guides/src', $vendorDir . '/phpdocumentor/guides-graphs/src', $vendorDir . '/phpdocumentor/guides-restructured-text/src', $vendorDir . '/phpdocumentor/guides-markdown/src'),
│ │ -    'phpDocumentor\\Guides\\' => array($vendorDir . '/phpdocumentor/guides-graphs/src', $vendorDir . '/phpdocumentor/guides/src', $vendorDir . '/phpdocumentor/guides-restructured-text/src', $vendorDir . '/phpdocumentor/guides-markdown/src'),
│ │      'phpDocumentor\\GraphViz\\PHPStan\\' => array($vendorDir . '/phpdocumentor/graphviz/src/phpDocumentor/PHPStan'),
│ │      'phpDocumentor\\GraphViz\\' => array($vendorDir . '/phpdocumentor/graphviz/src/phpDocumentor/GraphViz'),
│ │      'phpDocumentor\\' => array($baseDir . '/src/phpDocumentor', $baseDir . '/tests/unit/phpDocumentor', $baseDir . '/tests/integration/phpDocumentor', $baseDir . '/tests/functional/phpDocumentor', $vendorDir . '/phpdocumentor/reflection/src/phpDocumentor'),
│ │      'Webmozart\\Assert\\' => array($vendorDir . '/webmozart/assert/src'),
│ │      'Twig\\' => array($vendorDir . '/twig/twig/src'),
│ │      'Symfony\\Polyfill\\Php83\\' => array($vendorDir . '/symfony/polyfill-php83'),
│ │      'Symfony\\Polyfill\\Php80\\' => array($vendorDir . '/symfony/polyfill-php80'),
│ │   --- vendor0/composer/autoload_static.php
│ ├── +++ vendor1/composer/autoload_static.php
│ │ @@ -491,19 +491,19 @@
│ │  00001ea0: 292c 0a20 2020 2020 2020 2027 7068 7044  ),.        'phpD
│ │  00001eb0: 6f63 756d 656e 746f 725c 5c47 7569 6465  ocumentor\\Guide
│ │  00001ec0: 735c 5c27 203d 3e20 0a20 2020 2020 2020  s\\' => .       
│ │  00001ed0: 2061 7272 6179 2028 0a20 2020 2020 2020   array (.       
│ │  00001ee0: 2020 2020 2030 203d 3e20 5f5f 4449 525f       0 => __DIR_
│ │  00001ef0: 5f20 2e20 272f 2e2e 2720 2e20 272f 7068  _ . '/..' . '/ph
│ │  00001f00: 7064 6f63 756d 656e 746f 722f 6775 6964  pdocumentor/guid
│ │ +00001f10: 6573 2f73 7263 272c 0a20 2020 2020 2020  es/src',.       
│ │ +00001f20: 2020 2020 2031 203d 3e20 5f5f 4449 525f       1 => __DIR_
│ │ +00001f30: 5f20 2e20 272f 2e2e 2720 2e20 272f 7068  _ . '/..' . '/ph
│ │ +00001f40: 7064 6f63 756d 656e 746f 722f 6775 6964  pdocumentor/guid
│ │ +00001f50: 6573 2d67 7261 7068 732f 7372 6327 2c0a  es-graphs/src',.
│ │ -00001f10: 6573 2d67 7261 7068 732f 7372 6327 2c0a  es-graphs/src',.
│ │ -00001f20: 2020 2020 2020 2020 2020 2020 3120 3d3e              1 =>
│ │ -00001f30: 205f 5f44 4952 5f5f 202e 2027 2f2e 2e27   __DIR__ . '/..'
│ │ -00001f40: 202e 2027 2f70 6870 646f 6375 6d65 6e74   . '/phpdocument
│ │ -00001f50: 6f72 2f67 7569 6465 732f 7372 6327 2c0a  or/guides/src',.
│ │  00001f60: 2020 2020 2020 2020 2020 2020 3220 3d3e              2 =>
│ │  00001f70: 205f 5f44 4952 5f5f 202e 2027 2f2e 2e27   __DIR__ . '/..'
│ │  00001f80: 202e 2027 2f70 6870 646f 6375 6d65 6e74   . '/phpdocument
│ │  00001f90: 6f72 2f67 7569 6465 732d 7265 7374 7275  or/guides-restru
│ │  00001fa0: 6374 7572 6564 2d74 6578 742f 7372 6327  ctured-text/src'
│ │  00001fb0: 2c0a 2020 2020 2020 2020 2020 2020 3320  ,.            3 
│ │  00001fc0: 3d3e 205f 5f44 4952 5f5f 202e 2027 2f2e  => __DIR__ . '/.

Additionally, I've added a commit (refactor: DRY) to remove duplicated code and clarify the autoloader files generation process. Please let me know if you'd prefer to keep or revert this change. I removed it, I was not satisfied with my implementation.

@drupol drupol force-pushed the reproducible-autoloader-add-sort branch 3 times, most recently from 76f212c to 1b2c4a6 Compare August 22, 2024 05:35
@drupol drupol marked this pull request as ready for review August 22, 2024 05:38
@drupol drupol changed the title Improve reproducibility of autoloader files - add missing sort Improve reproducibility of autoloader files - add missing sort Aug 22, 2024
@drupol drupol force-pushed the reproducible-autoloader-add-sort branch 2 times, most recently from bfd432d to 9c34034 Compare August 22, 2024 08:18
Seldaek
Seldaek reviewed Aug 22, 2024
View reviewed changes
src/Composer/Autoload/AutoloadGenerator.php Outdated
@@ -268,6 +268,7 @@ public function dump(Config $config, InstalledRepositoryInterface $localRepo, Ro
foreach ($paths as $path) {
$exportedPaths[] = $this->getPathCode($filesystem, $basePath, $vendorPath, $path);
}
sort($exportedPaths);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure but this does not seem safe to me.. If you have a package defining a psr-4 path for a namespace, then another defining the same.. We do want to keep the preference order of packages here, and not just sort alphabetically. This should already be stable sorted via sortPackageMap so I do not understand how you end up with different orders in different cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't fully explain it either, but we can reproduce it locally. I will dig deeper as soon as I can.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added the 2 different autoload_static.php files in the issue message, in case we need them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added it in the first message of this PR

@Seldaek Seldaek added the Bug label Sep 2, 2024
@Seldaek Seldaek added this to the 2.7 milestone Sep 2, 2024
@piotrkwiecinski piotrkwiecinski mentioned this pull request Sep 10, 2024
Closed
@drupol drupol mentioned this pull request Sep 22, 2024
Merged
13 tasks
@stof
Copy link
Contributor

stof commented Sep 24, 2024

If removing --apcu-autoloader fixes the reproducibility of the generated file, something is really weird, as that flag should not have any influence on the autoload_static file.

@drupol
Copy link
Contributor Author

drupol commented Sep 24, 2024

Yes this is weird. I will most probably investigate at some point, but it's not possible yet for me.

@Seldaek Seldaek modified the milestones: 2.7, 2.8 Oct 2, 2024
@drupol
Copy link
Contributor Author

drupol commented Dec 26, 2024 •
edited
Loading

I removed the --apcu-autoloader flag, and we can reproduce the issue, see NixOS/nixpkgs#368328 (comment)

In this PR, I propose a patch, your inputs/feedback is welcome.

@drupol drupol force-pushed the reproducible-autoloader-add-sort branch from 9c34034 to 34f2f39 Compare December 26, 2024 17:23
@drupol drupol mentioned this pull request Dec 26, 2024
Merged
13 tasks
@Seldaek Seldaek mentioned this pull request Jan 9, 2025
Merged
@Seldaek
Copy link
Member

Seldaek commented Jan 9, 2025

Please see #12263 for an alternative fix, which I believe is more correct and appears to resolve the issue as well by forcing a stable order.

@Seldaek Seldaek closed this in 7b1e983 Jan 9, 2025
@drupol drupol deleted the reproducible-autoloader-add-sort branch January 9, 2025 14:09
drupol added a commit to drupol/nixpkgs that referenced this pull request Jan 10, 2025
@drupol
php84Packages.composer: 2.8.1 -> 2.8.4
461cf6d 
This release includes an extra patch to fix reproducible outputs, see composer/composer#12090
drupol added a commit to drupol/nixpkgs that referenced this pull request Jan 10, 2025
@drupol
php84Packages.composer: 2.8.1 -> 2.8.4
097aafd 
This release includes an extra patch to fix reproducible outputs, see composer/composer#12090
drupol added a commit to NixOS/nixpkgs that referenced this pull request Jan 10, 2025
@drupol
php84Packages.composer: 2.8.1 -> 2.8.4
41ae527 
This release includes an extra patch to fix reproducible outputs, see composer/composer#12090

(cherry picked from commit 097aafd)
@drupol drupol mentioned this pull request Jan 12, 2025
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Seldaek Seldaek Seldaek left review comments

@piotrkwiecinski piotrkwiecinski piotrkwiecinski left review comments

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
2.8
Development

Successfully merging this pull request may close these issues.
4 participants
@drupol @stof @Seldaek @piotrkwiecinski