Navigation Menu

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.0] Scripts from "bin" are executed not in "bin" directory. #10389

Closed
biozshock opened this issue Dec 22, 2021 · 22 comments · Fixed by #10402
Closed

[2.2.0] Scripts from "bin" are executed not in "bin" directory. #10389

biozshock opened this issue Dec 22, 2021 · 22 comments · Fixed by #10402
Labels
Milestone

Comments

@biozshock
Copy link
Contributor

Create a composer json that require any package, which contains a bash/sh script in bin, which executes another bin file from a require.

My composer.json:

{
    "require": {
        "ninjify/qa": "^0.12.1"
    },
    "scripts": {
        "codesniffer": "codesniffer"
    },
    "config": {
        "allow-plugins": {
            "dealerdirect/phpcodesniffer-composer-installer": true
        }
    }
}

Output of composer diagnose:

Checking composer.json: WARNING
No license specified, it is recommended to do so. For closed-source software you may use "proprietary" as license.
Checking platform settings: OK
Checking git settings: OK
Checking http connectivity to packagist: OK
Checking https connectivity to packagist: OK
Checking github.com rate limit: OK
Checking disk free space: OK
Checking pubkeys: FAIL
Missing pubkey for tags verification
Missing pubkey for dev verification
Run composer self-update --update-keys to set them up
Checking composer version: You are not running the latest stable version, run `composer self-update` to update (226689b90cbe0a4692a047df19539d4d3e959d79 => 2.2.0)
Composer version: 2.2-dev+226689b90cbe0a4692a047df19539d4d3e959d79
PHP version: 7.4.26
PHP binary path: /usr/bin/php7.4
OpenSSL version: OpenSSL 1.1.1f  31 Mar 2020
cURL version: 7.68.0 libz 1.2.11 ssl OpenSSL/1.1.1f
zip: extension present, unzip present, 7-Zip not available

When I run this command:

composer codesniffer

I get the following output:

Reading ./composer.json (/tmp/comtest/composer.json)
Loading config file ./composer.json (/tmp/comtest/composer.json)
Checked CA file /etc/pki/tls/certs/ca-bundle.crt does not exist or it is not a file.
Checked directory /etc/pki/tls/certs/ca-bundle.crt does not exist or it is not a directory.
Checked CA file /etc/ssl/certs/ca-certificates.crt: valid
Executing command (/tmp/comtest): git branch -a --no-color --no-abbrev -v
Executing command (/tmp/comtest): git describe --exact-match --tags
Executing command (CWD): git --version
Executing command (/tmp/comtest): git log --pretty="%H" -n1 HEAD --no-show-signature
Executing command (/tmp/comtest): hg branch
Executing command (/tmp/comtest): fossil branch list
Executing command (/tmp/comtest): fossil tag list
Executing command (/tmp/comtest): svn info --xml
Failed to initialize global composer: Composer could not find the config file: /home/home/.config/composer/composer.json

Reading /tmp/comtest/vendor/composer/installed.json
Loading plugin Dealerdirect\Composer\Plugin\Installers\PHPCodeSniffer\Plugin (from dealerdirect/phpcodesniffer-composer-installer)
Running 2.2-dev+226689b90cbe0a4692a047df19539d4d3e959d79 (2021-12-22 15:04:34) with PHP 7.4.26 on Linux / 5.4.0-91-generic
> codesniffer: codesniffer
Executing command (CWD): codesniffer
/tmp/comtest/vendor/ninjify/qa/bin/codesniffer: line 31: /tmp/comtest/vendor/ninjify/qa/bin/phpcs: No such file or directory
/tmp/comtest/vendor/ninjify/qa/bin/codesniffer: line 39: /tmp/comtest/vendor/ninjify/qa/bin/phpcs: No such file or directory
Script codesniffer handling the codesniffer event returned with error code 127

And I expected this to happen: Command is executed.

@Seldaek
Copy link
Member

Seldaek commented Dec 23, 2021

Did this really work before? I suppose it should have as we push the bin-dir on to the $PATH env var, but I'm not sure what would cause this to break now.. Anyway will try and repro later thanks.

@Seldaek Seldaek added this to the 2.2 milestone Dec 23, 2021
@cmuench
Copy link

cmuench commented Dec 23, 2021

@Seldaek I am not sure if it's related. Have also an issue in my Magerun builds with Composer 2.2.1 with a bin execute.
See: https://github.com/netz98/n98-magerun2/actions/runs/1614552858

In 2.1.14 it's fine.
Reproduced that on my local machine. A upgrade to 2.2.1 with a fresh composer install let my tests fail.
I get PHPUnit\Framework\Exception: PHP Fatal error: strict_types declaration must be the very first statement in the script.
The first lines in the phpunit file are:

#!/usr/bin/env php
<?php declare(strict_types=1);

I start vendor/bin/phpunit but it executes vendor/phpunit/phpunit/phpunit. Maybe a realpath topic?

@herndlm
Copy link
Contributor

herndlm commented Dec 23, 2021

@cmuench what you're describing is different but #10387 exists for that already

@Seldaek fyi git bisect tells me that f12a5b8 / #10137 is the bad commit here too. so those 2 issues are actually related indeed. tested with 7.4 btw

@cmuench
Copy link

cmuench commented Dec 23, 2021

@cmuench what you're describing is different but #10387 exists for that already

@Seldaek fyi git bisect tells me that f12a5b8 / #10137 is the bad commit here too. so those 2 issues are actually related indeed. tested with 7.4 btw

Thanks for the context. In my case I run the tests with process isolation.

@Seldaek
Copy link
Member

Seldaek commented Dec 23, 2021

@biozshock I can reproduce this but I can also reproduce it with Composer 2.1.14 so it doesn't seem to be a new problem?

if I run env in your codesniffer script I see PATH=/path/to/vendor/bin:[...] and just running phpcs -h from the codesniffer script also seems to work.. so I am not sure what you are doing wrong but I don't think you need the ${DIR}/ prefix to call phpcs

@biozshock
Copy link
Contributor Author

biozshock commented Dec 23, 2021

The issue with using phpcs from $PATH is that the system may have already installed a different phpcs globally. And there is no guarantee that the phpcs from require will be used.

EDIT: also i can see the issue with adding vendor to $PATH is that on my development machine i will end up with all vendor/bin from all projects.

You need to remove vendor directory before testing with 2.1.14

con@dace779ff42b:/tmp/compotest$ php composer2_2_1.phar -V
Composer version 2.2.1 2021-12-22 22:21:31

con@dace779ff42b:/tmp/compotest$ php composer2_2_1.phar install
Installing dependencies from lock file (including require-dev)
Verifying lock file contents can be installed on current platform.
Package operations: 7 installs, 0 updates, 0 removals
  - Installing squizlabs/php_codesniffer (3.6.2): Extracting archive
  - Installing dealerdirect/phpcodesniffer-composer-installer (v0.7.1): Extracting archive
  - Installing php-parallel-lint/php-parallel-lint (v1.3.1): Extracting archive
  - Installing phpstan/phpdoc-parser (0.4.9): Extracting archive
  - Installing slevomat/coding-standard (6.4.1): Extracting archive
  - Installing ninjify/coding-standard (v0.11.2): Extracting archive
  - Installing ninjify/qa (v0.12.1): Extracting archive
Generating autoload files
3 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
PHP CodeSniffer Config installed_paths set to ../../ninjify,../../slevomat/coding-standard

con@dace779ff42b:/tmp/compotest$ php composer2_2_1.phar codes  
> codesniffer
/tmp/compotest/vendor/ninjify/qa/bin/codesniffer: line 31: /tmp/compotest/vendor/ninjify/qa/bin/phpcs: No such file or directory
/tmp/compotest/vendor/ninjify/qa/bin/codesniffer: line 39: /tmp/compotest/vendor/ninjify/qa/bin/phpcs: No such file or directory
Script codesniffer handling the codesniffer event returned with error code 127

con@dace779ff42b:/tmp/compotest$ rm -fr vendor

con@dace779ff42b:/tmp/compotest$ php composer2_1_14.phar -V   
Composer version 2.1.14 2021-11-30 10:51:43

con@dace779ff42b:/tmp/compotest$ php composer2_1_14.phar install
Installing dependencies from lock file (including require-dev)
Verifying lock file contents can be installed on current platform.
Package operations: 7 installs, 0 updates, 0 removals
  - Installing squizlabs/php_codesniffer (3.6.2): Extracting archive
  - Installing dealerdirect/phpcodesniffer-composer-installer (v0.7.1): Extracting archive
  - Installing php-parallel-lint/php-parallel-lint (v1.3.1): Extracting archive
  - Installing phpstan/phpdoc-parser (0.4.9): Extracting archive
  - Installing slevomat/coding-standard (6.4.1): Extracting archive
  - Installing ninjify/coding-standard (v0.11.2): Extracting archive
  - Installing ninjify/qa (v0.12.1): Extracting archive
Generating autoload files
3 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
PHP CodeSniffer Config installed_paths set to ../../ninjify,../../slevomat/coding-standard

con@dace779ff42b:/tmp/compotest$ php composer2_1_14.phar codes  
> codesniffer
The installed coding standards are Squiz, PEAR, PSR1, MySource, PSR12, Zend, PSR2, coding-standard and SlevomatCodingStandard
ERROR: You must supply at least one file or directory to process.

Run "phpcs --help" for usage information

Script codesniffer handling the codesniffer event returned with error code 3

@Seldaek
Copy link
Member

Seldaek commented Dec 23, 2021

The issue with using phpcs from $PATH is that the system may have already installed a different phpcs globally. And there is no guarantee that the phpcs from require will be used.

Yes it is guaranteed as the bin-dir is prepended to the path. This is done by composer BTW you don't have to add anything to your path manually. But it only works if running the binary via a composer script.

If you call vendor/bin/codesniffer directly then the path wouldn't be setup.

While it worked for you in 2.1 it didn't work equally in all platforms/envs, which is why your script also didn't work for me with 2.1. 2.2 normalizes things so that we have bin proxies everywhere.

It may require that you adjust your path computation a bit to figure out where the other binaries are.

@Seldaek
Copy link
Member

Seldaek commented Dec 23, 2021

I guess the main question here is why did DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" resolve correctly before when it was executing your script as a symlink directly from vendor/ninjify/qa/bin/codesniffer and it doesn't work anymore now that it runs as vendor/bin/codesniffer which then runs the source one. The added indirection seems to cause this to change but I probably wont have time to investigate this further now so sharing my current understanding in the hope it'll help you figure it out.

@danshumaker
Copy link

danshumaker commented Dec 23, 2021

@Seldaek I am not sure if it's related. Have also an issue in my Magerun builds with Composer 2.2.1 with a bin execute. See: https://github.com/netz98/n98-magerun2/actions/runs/1614552858

In 2.1.14 it's fine. Reproduced that on my local machine. A upgrade to 2.2.1 with a fresh composer install let my tests fail. I get PHPUnit\Framework\Exception: PHP Fatal error: strict_types declaration must be the very first statement in the script. The first lines in the phpunit file are:

#!/usr/bin/env php
<?php declare(strict_types=1);

I start vendor/bin/phpunit but it executes vendor/phpunit/phpunit/phpunit. Maybe a realpath topic?

We are also experiencing this error for vendor/bin/dep and specifically for our mage deployer ( https://github.com/unleashedtech/deployer-recipes/blob/magento2/cms/magento/magento2.php#L333 )

vendor/bin/dep tree deploy
PHP Fatal error:  Namespace declaration statement has to be the very first statement or after any declare call in the script in /srv/www/client-name.com/releases/2021-12-23-11-29-37/vendor/bin/dep on line 3

Tested with both composer 2.2.0 and 2.2.1 . Reverting to composer 2.1.14 then command executes fine.

@hopeseekr
Copy link
Contributor

hopeseekr commented Dec 25, 2021

This is SERIOUSLY affecting my package which has several thousand users. https://packagist.org/packages/phpexperts/dockerize

vendor/bin/php in composer v2.1.14 and prior:

#!/bin/bash

ROOT="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../.." && pwd )"

# ...

ORIG_PHP_VERSION=$PHP_VERSION
if [ -f "${ROOT}/.env" ]; then
    source "${ROOT}/.env"
    if [ ! -z "$ORIG_PHP_VERSION" ]; then
        PHP_VERSION="$ORIG_PHP_VERSION"
    fi
fi

In composer 2.2.0-rc1 and above:

#!/usr/bin/env sh

dir=$(cd "${0%[/\\]*}" > /dev/null; cd '../phpexperts/dockerize/bin' && pwd)

if [ -d /proc/cygdrive ]; then
    case $(which php) in
        $(readlink -n /proc/cygdrive)/*)
            # We are in Cygwin using Windows php, so the path must be translated
            dir=$(cygpath -m "$dir");
            ;;
    esac
fi

"${dir}/php" "$@"

The working dir for composer vendor scripts has changed from vendor/bin to vendor/<vendor>/<project>/<bin-dir>, causing scripts to break.

@hopeseekr
Copy link
Contributor

hopeseekr commented Dec 25, 2021

To fix:

In vendor/phpexperts/dockerize/bin/php, I changed the top of the script to:

# @see https://linuxize.com/post/how-to-check-if-string-contains-substring-in-bash/
# @see https://github.com/composer/composer/issues/10389
SUB="/vendor/"
if [[ "$0" == *"$SUB"* ]]; then
  ROOT="$(readlink -f /proc/$PPID/cwd)"
else
  ROOT="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../.." && pwd )"
fi

This seems to work for both pre-2.2.0 and 2.2.0+.

hopeseekr added a commit to PHPExpertsInc/dockerize that referenced this issue Dec 25, 2021
The problem is that composer v2.2.0+ changed the way that vendor bin scripts are executed.
Now they are executed by an intermediary shell script in a way that causes their PWD to
change to the project's bin-dir, breaking project root detection.

See composer/composer#10389
hopeseekr added a commit to PHPExpertsInc/dockerize that referenced this issue Dec 25, 2021
The problem is that composer v2.2.0+ changed the way that vendor bin scripts are executed. Now they are executed by an intermediary shell script in a way that causes their PWD to change to the project's bin-dir, breaking project root detection.

See composer/composer#10389
Seldaek added a commit to Seldaek/composer that referenced this issue Dec 28, 2021
@Seldaek
Copy link
Member

Seldaek commented Dec 28, 2021

OK with a bit more free time now I had another look here and got a better understanding. I'd say #10402 is probably the best we can do, to allow packages with problems to solve it cleanly. Would that work for everyone?

@Seldaek
Copy link
Member

Seldaek commented Dec 29, 2021

2.2.2 is now out with the new env var available https://github.com/composer/composer/releases/tag/2.2.2

@convenient
Copy link

Hello @Seldaek

I have found another issue with this I think 🙈 . I do not believe I can any longer source scripts from vendor/bin into my current shell. If you would like this in a separate issue please let me know.

Context
I have a tool which allows us to install vanilla magento inside travis infrastructure.

Some parts of this process take a lot of time and travis exits early if no output is received for a while to stop builds hanging. To ensure the builds do not exit early I use the travis_wait bash function which is only available inside the default travis shell.

Problem

The changes introduced means that I cannot source the script from vendor/bin/example.sh into the current shell, which I need to do to access functions defined only in the default travis shell.

I get an error like

$ if [[ $TEST_GROUP = latest ]];                then  NAME=$TEST_GROUP WITH_SAMPLE_DATA=1                   . ./vendor/bin/travis-install-magento.sh; fi
./vendor/bin/travis-install-magento.sh: line 9: cd: ../ampersand/travis-vanilla-magento: No such file or directory
./vendor/bin/travis-install-magento.sh: line 22: /travis-install-magento.sh: No such file or directory
The command "if [[ $TEST_GROUP = latest ]];                then  NAME=$TEST_GROUP WITH_SAMPLE_DATA=1                   . ./vendor/bin/travis-install-magento.sh; fi" exited with 127.

These errors seem to come from the wrapper which s now introduced by composer, specifically this line

#!/usr/bin/env sh

self=$(realpath $0 >/dev/null 2>&1)
if [ -z "$self" ]
then
    self="$0"
fi

dir=$(cd "${self%[/\\]*}" > /dev/null; cd '../ampersand/travis-vanilla-magento' && pwd)

Replication

I have distilled this issue down into a (hopefully) easily reproducible script which removes basically all the dependencies.

Please see https://github.com/convenient/composer-2-2-3-issue-10389-debug-b

Essentially I can get this to the point that it operates like so for me

$ ./vendor/bin/script.sh
This script is running
$ source ./vendor/bin/script.sh 
bash: cd: ../convenient/composer-2-2-3-issue-10389-debug-a: No such file or directory
bash: /script.sh: No such file or directory

And I cannot see any way that altering COMPOSER_BIN_DIR works as this happens in the wrapper within vendor/bin which I believe is a fully composer concern?

Please let me know if you have any questions or if I need to clarify anything.

@Seldaek
Copy link
Member

Seldaek commented Jan 7, 2022

@convenient IMO this strictly speaking misuse and thus not a bug from Composer's point of view. source only happened to work because symlinks were the default, but if you put bin-compat:full in your config even in older composer versions that use case would have been broken anyway.

The use case for binaries is to execute them. symlinking stuff from the vendor dir was just an accident of history and not something you should rely on.

That said it appears to be fixable reasonably easily by using $BASH_SOURCE instead of $0, I did not initially want to do that to avoid messing things up with non-bash shells, but the fallback I put in there will hopefully be enough.

And for the record yes please do open a new issue next time, you are welcome to reference back to this one but it'd make it easier to have a proper changelog entry and so on if things are separated.

Anyway see 6dea58c for the fix and feel free to give it a shot with composer self-update --snapshot (it'll require a rm vendor/bin && composer install though)

@naderman
Copy link
Member

naderman commented Jan 7, 2022

@Seldaek That will execute the right script, but it will still execute it in a subshell, so the source will not have any effect. The bin proxy would have to detect this situation and use source itself to invoke the bin file.

@Seldaek
Copy link
Member

Seldaek commented Jan 7, 2022

Ahh good point, should be easy enough checking that $BASH_SOURCE != $0

@Seldaek
Copy link
Member

Seldaek commented Jan 7, 2022

OK that should seems to do the job for me 24b62a1 - I checked with the target binary adding a bash alias and it gets added properly.

@convenient
Copy link

@naderman @Seldaek Thank you both so much 🙌 Really appreciate this, using the snapshot seems to fix the issue for me.

I feel a little like this XKCD comic 😬

I'm not trying to pester please forgive me if it comes across that way, but what is your release cycle like / when would you expect this to go into a stable release? Days/weeks/months? Just trying to work out my next steps for internally fixing all our repositories affected, so that I know whether I force a 2.1 version of composer or whether I wait until another 2.2 release.

Thanks again

@Seldaek
Copy link
Member

Seldaek commented Jan 7, 2022

Probably can do a patch release next week as we've got a few fixes lined up.

@Seldaek
Copy link
Member

Seldaek commented Jan 8, 2022

https://github.com/composer/composer/releases/tag/2.2.4 is out now

@convenient
Copy link

Thanks @Seldaek all my builds have gone green this morning. Very much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants