Order of installed_paths inconsistent between runs

[kevinfodness]

Problem/Motivation

I am using this plugin to register several phpcs sniffs from different repos into my installed_paths with phpcs. I am also using Travis CI's caching directives in combination with phpcs' caching directives in order to cache the results of scans between runs for files that have not been modified. However, it appears that this plugin doesn't register installed_paths in a consistent way between CI runs, which invalidates the phpcs cache, thus negating the benefits of caching between runs.

Expected behaviour

Runs of this plugin between CI jobs should produce consistent output for the value of installed_paths (specifically, the order of the paths).

Actual behaviour

The order of the paths in installed_paths is not always the same between CI runs. This is likely due to a race condition of when certain packages finish installing.

Steps to reproduce

Use a standard in composer.json that references multiple standards, or reference multiple standards in composer.json (see example below). Create a Travis CI job to run phpcs and echo the value of installed_paths. Note that it will not always be in the same order between runs.

Example for run 1:

"installed_paths": "../../alleyinteractive/alley-coding-standards,../../wp-coding-standards/wpcs,../../sirbrillig/phpcs-variable-analysis,../../automattic/vipwpcs",

Example for run 2:

"installed_paths": "../../sirbrillig/phpcs-variable-analysis,../../automattic/vipwpcs,../../alleyinteractive/alley-coding-standards,../../wp-coding-standards/wpcs",

Contents of composer.json:

{
  "require-dev": {
    "alleyinteractive/alley-coding-standards": "^0.2.0"
},

Contents of composer.json in alley-coding-standards:

{
  "name": "alleyinteractive/alley-coding-standards",
  "description": "PHPCS sniffs for Alley Interactive",
  "type": "phpcodesniffer-standard",
  "license": "GPL-2.0-or-later",
  "require": {
    "squizlabs/php_codesniffer": "^3.5.0",
    "dealerdirect/phpcodesniffer-composer-installer": "*",
    "wp-coding-standards/wpcs": "^2.3.0",
    "automattic/vipwpcs": "^2.1.0",
    "phpcompatibility/phpcompatibility-wp": "*"
  },
  "require-dev": {}
}

Proposed changes

Make the installed_paths value consistent between runs by alphabetizing the values. This will allow repos that use this plugin to leverage Travis CI and phpcs caching for unmodified files by ensuring that the configuration values in the cache file, specifically the value for installed_paths, is consistent between runs if nothing changed.

Environment

Question	Answer
OS	Linux, Ubuntu Bionic, running on Travis CI
PHP version	7.3.14
Composer version	2.0.8
PHP_CodeSniffer version	3.5.8
Dealerdirect PHPCS plugin version	0.7.0
Install type	project local

Output of vendor/bin/phpcs --config-show:

Using config file: /home/travis/build/alleyinteractive/brookings/vendor/squizlabs/php_codesniffer/CodeSniffer.conf

Contents of CodeSniffer.conf:

<?php
 $phpCodeSnifferConfig = array (
  'installed_paths' => '../../alleyinteractive/alley-coding-standards,../../wp-coding-standards/wpcs,../../sirbrillig/phpcs-variable-analysis,../../automattic/vipwpcs',
)
?>

Tested against master branch?

• I have verified the issue still exists in the master branch.

I have not verified this against the master branch, but looking at the diff between 0.7.0 and the current HEAD, there shouldn't be anything in that diff that would affect this issue.

[Potherca]

Hi! Thank you for bringing this to our attention!

This seems to be one of these edge cases we didn't think about when the plugin was created, but it indeed seems like a valuable improvement to the current logic.

We'll have to verify that the plugin has control over setting the order of the paths.

I can't give you a time-table when this will be picked up, but I am in favor of implementing this!

[kevinfodness]

I created a PR (#126) that should fix this.

[Potherca]

@kevinfodness Thank you very much for your contribution! The changes from #126 have been merged to master and will be part of the upcoming 0.7.1 release.

[kevinfodness]

Thanks! Glad to help.

[jrfnl]

This seems to be one of these edge cases we didn't think about when the plugin was created, but it indeed seems like a valuable improvement to the current logic.

Just for context:
When this plugin was created PHPCS did not have the caching feature yet. Result caching for PHPCS was introduced in PHPCS 3.0.0, but even then, the configuration settings weren't taken into account when determining whether the cache was valid. That part was only introduced in PHPCS 3.3.0.

[jrfnl]

The order of the paths in installed_paths is not always the same between CI runs. This is likely due to a race condition of when certain packages finish installing.

@kevinfodness Thanks for bringing this to our attention and for the fix.

While it fixes the immediate issue of the cache invalidation in PHPCS, it doesn't fix the underlying problem.

I've been trying to figure out where the underlying race condition occurs, so we can report it to the Composer project.
The only thing I can currently come up with is that onDependenciesChangedEvent would be called too early, when the installation of the dependencies hasn't finished yet. That could cause the cleanInstalledPaths() method to remove paths from the originally found paths, only for the updateInstalledPaths() method to then add them again, though AFAICS this could only happen if the install finished somewhere inbetween these methods running, which would come down to microseconds and very precise timing of that.

Unfortunately I haven't been able to reproduce the issue locally which makes debugging kind of hard.

If anyone wants to look into this further - there's an easy way to see whether the race condition still exists as the plugin shows a status message if the installed_path was updated.

Basically with the race condition, the following will be displayed at the end of each install (same message each time now the paths are sorted on save):

PHP CodeSniffer Config installed_paths set to ../../automattic/vipwpcs,../../phpcompatibility/php-compatibility,../../phpcompatibility/phpcompatibility-paragonie,../../phpcompatibility/phpcompatibility-wp,../../sirbrillig/phpcs-variable-analysis,../../wp-coding-standards/wpcs

... while if the race condition would not exist, the plugin would stay silent on subsequent runs, or if running with --verbose would report Nothing to install or update.

@kevinfodness Would you mind checking if you still see the PHP CodeSniffer Config installed_paths set to... message after each composer install ?

/cc @Seldaek

[kevinfodness]

I'll do what I can. Do you mean running composer install with no changes to see if the installed_paths message pops again, even though nothing has changed?

I suspect that the issue here may be related to the fact that this is running in a Travis CI container and that we're caching Composer dependencies in Travis via caching the $HOME/.config/composer/cache directory, and we're installing phpcs to the container from the Composer cache each time the container boots up to run a job. So the container boots up, it grabs the Composer cache, we run composer install which pulls our custom standard, which installs phpcs, and then we rely on this plugin to set the installed paths locally after the install. There is only ever one composer install command run in the project during this setup, which installs phpcs as well as the sniffs.

[jrfnl]

Do you mean running composer install with no changes to see if the installed_paths message pops again, even though nothing has changed?

Yes, exactly.

I'll be sending in a PR in a moment with another tiny change as well which may or may not be related to/fix this issue.

[kevinfodness]

Interesting - it does trigger. I'm running my Travis CI job in debug mode to test it out. My before_script action runs composer install and it reports Nothing to install or update. However, when I run composer install after this, here is the output:

No lock file found. Updating dependencies instead of installing from lock file. Use composer update over composer install if you do not have a lock file.
Loading composer repositories with package information
Updating dependencies
Lock file operations: 6 installs, 0 updates, 0 removals
  - Locking alleyinteractive/alley-coding-standards (0.2.0)
  - Locking automattic/vipwpcs (2.2.0)
  - Locking dealerdirect/phpcodesniffer-composer-installer (dev-master fe39059)
  - Locking sirbrillig/phpcs-variable-analysis (v2.10.0)
  - Locking squizlabs/php_codesniffer (3.5.8)
  - Locking wp-coding-standards/wpcs (2.3.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 6 installs, 0 updates, 0 removals
  - Downloading squizlabs/php_codesniffer (3.5.8)
  - Syncing dealerdirect/phpcodesniffer-composer-installer (dev-master fe39059) into cache
  - Downloading wp-coding-standards/wpcs (2.3.0)
  - Downloading sirbrillig/phpcs-variable-analysis (v2.10.0)
  - Downloading automattic/vipwpcs (2.2.0)
  - Downloading alleyinteractive/alley-coding-standards (0.2.0)
  - Installing squizlabs/php_codesniffer (3.5.8): Extracting archive
  - Installing dealerdirect/phpcodesniffer-composer-installer (dev-master fe39059): Cloning fe390591e0 from cache
  - Installing wp-coding-standards/wpcs (2.3.0): Extracting archive
  - Installing sirbrillig/phpcs-variable-analysis (v2.10.0): Extracting archive
  - Installing automattic/vipwpcs (2.2.0): Extracting archive
  - Installing alleyinteractive/alley-coding-standards (0.2.0): Extracting archive
Generating autoload files
PHP CodeSniffer Config installed_paths set to ../../alleyinteractive/alley-coding-standards,../../automattic/vipwpcs,../../sirbrillig/phpcs-variable-analysis,../../wp-coding-standards/wpcs

Note that we aren't committing a lock file to the repo, but one was created during the initial composer install, so it's bizarre that Composer is reporting that there isn't a lock file when one was just written.

[kevinfodness]

Actually, my before_script doesn't create a lock file. I'm looking into why.

[kevinfodness]

OK, that's actually a feature of my before_script, not a bug - I'm checking to see whether there are any files that changed that would need to be run through phpcs before installing it, and in the PR I'm debugging, there aren't, so it skips the install. I've got a composer global require "phpunit/phpunit=6.1.*" earlier in the before_script which is why I'm seeing output from Composer at all. So the first time I run composer install in my project directory, it reports registering the sniffs (which I would expect, because it just set up the vendor directory for the first time on the container). Subsequent composer installs after the first one result in the nothing to update message.

[kevinfodness]

I'm noticing that the order of lock/download/install operations from Composer seems to be consistent between runs, but the order in which the standards were added (prior to my patch) did not match the order that Composer was reporting installing the standards. Is it possible that Composer is running some of these operations async?

[jrfnl]

@kevinfodness Thanks for testing.

Whether or not there is a lock file, shouldn't actually make a difference in the output for this plugin. If the installed_paths was previously set, it should still be set.

Example output of a local run I did after manually deleting the lock file from a previous install:

Note that the PHP CodeSniffer Config installed_paths set to message does not show as the paths were already set before.

[jrfnl]

Is it possible that Composer is running some of these operations async?

I honesty don't know.... anyone ?

[kevinfodness]

In my use case, since the Travis CI container is created fresh each time the job runs, there are no installed_paths set, because the config file for the installed paths is in the vendor directory, which doesn't get created on the container until composer install is run.

[kevinfodness]

I think I have an answer on the async - if I'm reading this code correctly, then Composer is running operations async, and the order in which the post-install hooks get added depends on when the packages install, which is itself an async operation:

https://github.com/composer/composer/blob/b7d770659b4e3ef21423bd67ade935572913a4c1/src/Composer/Installer/InstallationManager.php#L417-L444

So, in cases where the packages aren't installed, Composer is installing them, and creating the array of $postExecCallbacks in the order in which the async install operations complete, then it runs through them in that order. From run to run, the order in which the installs complete could change just based on when those async functions complete, which would result in the behavior I described where the installed paths value would be in a different order between runs.

[jrfnl]

@kevinfodness thanks for looking into this deeper!

From run to run, the order in which the installs complete could change just based on when those async functions complete, which would result in the behavior I described where the installed paths value would be in a different order between runs.

I must be missing something obvious, but if the onDependenciesChangedEvent() is called only after the complete install has finished, that should not influence the working of this plugin.

AFAICS, it is only if onDependenciesChangedEvent() would be trigger before all installs are finished that the async running of the installs could influence the output of this plugin and in that case, your example with the order being different is the most benign of potential bugs.

If this plugin would run before all installs are finished, it could potentially miss standards if that particular package wouldn't have finished installing, which would be the more severe error condition.

---

Edit: hang on... ok, so I think I may have it. With async running, the order of the packages as returned by $this->composer->getRepositoryManager()->getLocalRepository()->getPackages() may be different between runs - is that what you are saying ?
In that case, yes, that would explain the difference in the installed_paths value order you saw, though the order could then only ever be different on a first, fresh install, like in a CI environment.

[kevinfodness]

With async running, the order of the packages as returned by $this->composer->getRepositoryManager()->getLocalRepository()->getPackages() may be different between runs - is that what you are saying ?

Yep—I don't know all of the Composer internals, but something inherent to the async workings of Composer that would cause the order of the packages to be different on a first, fresh install, like in a CI environment, as you said.

[Seldaek]

Sooo.. yes indeed the order of packages in the local repository is not guaranteed. It is sorted by name on write though to make sure the installed.json does not change between runs/machines so when reloading it it would be guaranteed, but while it's installing packages it's not sorting them because nobody ever cared.

If you need this in a given order, I'd suggest sorting the paths or the packages yourself so you are sure the order will be what you want always.

[jrfnl]

@Seldaek Thanks for confirming and that's exactly the solution which was pulled by @kevinfodness and has been merged.

I think we can safely conclude based on the above discussion and confirmation that there is indeed no race condition, which was what the "digging deeper" above was trying to investigate/confirm/deny. So, all is good 👍

[Potherca]

Thanks for looking into this further @jrfnl and @kevinfodness! I was planning on doing a follow-up, but you all saved me a lot of time (which is still a bit scarce).

And thank you @Seldaek for your confirmation on this. I know how busy you are, your insights are much appreciated!

[jrfnl]

you guys ++you all++ (didn't you just update the CoC ?)

[Potherca]

Thanks for pointing that out! I've still not recovered the muscle-memory from my fingers 100% when typing. Fixed now. 👍

[jrfnl]

@Potherca No worries 😉