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.

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

[2.2.2+] $this->composer->getConfig()->get('bin-dir') returns incorrect path on Windows #10504

Closed
jrfnl opened this issue Feb 2, 2022 · 11 comments
Labels
Milestone

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Feb 2, 2022

Context

I'm currently in the process of writing End-to-End tests for the DealerDirect PHPCS Composer Installer plugin and discovered the following due to tests failing.

Problem description

In the plugin we call PHPCS using the following code, which should be executed either in the Composer global or the project local directory, depending on the context in which a Composer install/update is being run:

$this->processExecutor->execute(
    'phpcs --config-show',
    $output,
    $this->composer->getConfig()->get('bin-dir')
);

This used to work correctly in all cases, but since Composer 2.2.2, it appears to be broken on Windows (server).

Based on my debugging:

  • Ubuntu is not affected.
  • The issue exists independently of the PHP version used to run Composer (tested with PHP 5.4, 7.2 and 8.1).

Output of $this->composer->getConfig()->get('bin-dir') for Composer < 2.2.2 on Windows (via GitHub actions)

This output is correct and as expected:

  • Global install:
    • Ubuntu: /tmp/PHPCSPluginTest/PreexistingPHPCSInstalledPathsConfigTest_61fa7843f015f2.37374271/global/vendor/bin
    • Windows: C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\PHPCSPluginTest\\PreexistingPHPCSInstalledPathsConfigTest_61fa786d7645e1.82846304\\global/vendor/bin
  • Project local install:
    • Ubuntu: /tmp/PHPCSPluginTest/PreexistingPHPCSInstalledPathsConfigTest_61fa784784d707.70495208/local/vendor/bin
    • Windows: C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\PHPCSPluginTest\\PreexistingPHPCSInstalledPathsConfigTest_61fa787cb36404.36256677\\local/vendor/bin

Output of $this->composer->getConfig()->get('bin-dir') for Composer 2.2.2 up to snapshot on Windows (via GitHub actions)

This output is wrong for Windows, correct for Ubuntu:

  • Global install:
    • Ubuntu: /tmp/PHPCSPluginTest/PreexistingPHPCSInstalledPathsConfigTest_61fa783fcdb903.30230172/global/vendor/bin
    • Windows: D:\\a\\phpcodesniffer-composer-installer\\phpcodesniffer-composer-installer\\vendor\\bin
  • Project local install:
    • Ubuntu: /tmp/PHPCSPluginTest/PreexistingPHPCSInstalledPathsConfigTest_61fa7842c99229.60312001/local/vendor/bin
    • Windows: D:\\a\\phpcodesniffer-composer-installer\\phpcodesniffer-composer-installer\\vendor\\bin
@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 3, 2022

Just in case anyone is wondering how I got the output I'm showing:

  • In the tests setup two temporary directories are made for testing (global and local), including setting putenv('COMPOSER_HOME=' . $tempGlobalPath);.
  • Then for global, Composer is triggered via an exec() call doing composer global install.
  • Similarly for local, Composer is triggered via an exec() call doing composer install -v --working-dir=$tempLocalPath.
  • Those calls should (and do) trigger the plugin I'm testing and in the plugin code I added a var_dump($this->composer->getConfig()->get('bin-dir')); just above the code snippet I posted above to see what was happening.

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 3, 2022

Based on the changelog I suspect this is related to #10402

@Seldaek
Copy link
Member

Seldaek commented Feb 3, 2022

Oh I guess COMPOSER_BIN_DIR as defined in the .bat file is then read by Composer inside your process somehow? Depends how you run the test suite but if you start the test suite via a Composer script probably this might happen.

So I guess that name was poorly chosen as it clashes with the existing COMPOSER_BIN_DIR env var that Composer reads.

Probably should rename that to something else (but what?).. But I am unsure why this would only break on Windows.. Kinda would like to understand this more before messing with things.

Do you have a way to access the build system, to perhaps change the .bat file so COMPOSER_BIN_DIR is called something else see if that fixes?

@Seldaek Seldaek added the Bug label Feb 3, 2022
@Seldaek Seldaek added this to the 2.2 milestone Feb 3, 2022
@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 3, 2022

@Seldaek I'm not actually calling Composer via the bat file, but calling it via php.exe composer.phar (both on Ubuntu as well as Windows)

@Seldaek
Copy link
Member

Seldaek commented Feb 3, 2022

No I don't mean composer I mean phpunit or whatever test runner.

@Seldaek
Copy link
Member

Seldaek commented Feb 3, 2022

The only way I see this could corrupt things is if you have a workflow like:

  • run composer test
  • composer runs the test script, which is defined as "vendor/bin/foo"
  • on linux, vendor/bin/foo would be a php binary proxy with #!/bin/env php to run with PHP (that sets _composer_bin_dir global but NO env var), but on windows it'd perhaps execute vendor/bin/foo.bat which does set the COMPOSER_BIN_DIR env.
  • then within the test suite, you run composer again, and at that point you have a polluted env with COMPOSER_BIN_DIR set on windows, and that messes up with Composer's Config class which assumes it should override the default bin-dir with the env var

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 3, 2022

Ah, I see. Yes, calling PHPUnit via vendor/bin/phpunit.

You can see the basic principle of the setup I'm using here: PHPCSStandards/composer-installer#153

This doesn't include the specific test through which I found this issue yet, but I can put that online in a follow-up branch (can't pull it yet until the above mentioned PR has been merged).

@Seldaek
Copy link
Member

Seldaek commented Feb 3, 2022

OK so if you run vendor/bin/phpunit directly.. I guess this is also the same mechanism as above, the first two steps are actually not needed I don't know what I was thinking :) All that's needed is that you run a .bat file first, then run Composer within your tests.

To confirm this suspicion you could try to change your CI run to use php vendor/bin/phpunit instead of simply vendor/bin/phpunit, that should make windows use php and the bin proxy directly instead of using vendor/bin/phpunit.bat. If that fixes it then at least we know it's that and the only challenge left is finding a better new name for this env var ;)

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 3, 2022

@Seldaek I'll get a test running when I finish what I'm currently working on and will report back.

FYI: for this particular plugin I have actually lined up a work-around in the plugin already, but yes, that doesn't negate the underlying issue.

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 3, 2022

Test run showing the problem using vendor/bin/phpunit: https://github.com/Dealerdirect/phpcodesniffer-composer-installer/runs/5054319281?check_suite_focus=true

Test run using php vendor/bin/phpunit confirming that that makes the problem go away: https://github.com/Dealerdirect/phpcodesniffer-composer-installer/actions/runs/1790431136

Both runs are selective, only running the specific test via which I discovered the issue, and both contain debug logging output in the test run output (for both for the successful as well as the failed test runs) which shows the bin-dir (and more).

@Seldaek
Copy link
Member

Seldaek commented Feb 3, 2022

OK great, thanks!

Seldaek added a commit to Seldaek/composer that referenced this issue Feb 3, 2022
…s as env var since Composer 2.2.2) to COMPOSER_RUNTIME_BIN_DIR to fix a regression due to a name clash, fixes composer#10504
@Seldaek Seldaek closed this as completed in 7c2954d Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants