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

Make COMPOSER_BIN_DIR env or _composer_bin_dir global available to binaries #10402

Merged
merged 1 commit into from Dec 28, 2021

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Dec 28, 2021

Fixes #10389 - or at least adds a utility for packages with problems to fix it by doing something like this:

if [[ -z "$COMPOSER_BIN_DIR" ]]; then
  DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
else
  DIR="$COMPOSER_BIN_DIR"
fi

Instead of simply DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" to find the Composer bin dir.

The problem is before Composer 2.2, when a binary was symlinked into bin-dir, it got executed as vendor/bin/foo which ran the script really stored in vendor/foo/bar/bin/foo, but $BASH_SOURCE[0] was set to vendor/bin/foo still, so that allowed people to discover the bin dir. Composer 2.2 always creates proxies instead of symlinking, so now the proxy runs vendor/foo/bar/bin/foo directly and so the resolved dir is vendor/foo/bar/bin instead of bin-dir, which breaks some assumptions.

These scripts were kinda broken anyway as they relied on symlinks which was just one way binaries could be installed, and so they were working on some envs but not all. The current 2.2 model is simpler as it's always working the same, but it will require some adjustments.

@Seldaek Seldaek added the Bug label Dec 28, 2021
@Seldaek Seldaek added this to the 2.2 milestone Dec 28, 2021
"{$caller} \"%BIN_TARGET%\" %*\r\n";
}

return "@ECHO OFF\r\n".
"setlocal DISABLEDELAYEDEXPANSION\r\n".
"SET BIN_TARGET=%~dp0/".trim(ProcessExecutor::escape($binPath), '"\'')."\r\n".
"SET COMPOSER_BIN_DIR=%~dp0\r\n".
Copy link
Member Author

Choose a reason for hiding this comment

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

@johnstevenson do you see any problem with this?

Copy link
Member

Choose a reason for hiding this comment

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

No, except that COMPOSER_BIN_DIR there will include a trailing \, if that matters.

I'm curious why this %~dp0 stuff is needed, when you know the location anyway - or will it move?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea why it's there anymore :) I saw the trailing slash, but i am assuming this will safely be ignored by most usages.

Copy link
Member Author

@Seldaek Seldaek Dec 28, 2021

Choose a reason for hiding this comment

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

It's been in since the very beginning https://github.com/composer/composer/pull/140/files#diff-254824db37e271a109993b63f65d2072eaeeaf7b89996c8ccc0f937cb81ed2d0R173-R195 but perhaps it's not needed at all. Perhaps it was needed to create an absolute path to help with cygwin and such crappy envs. Anyway unless we have a good reason to get rid of it I'd probably leave it in.

Copy link
Member

Choose a reason for hiding this comment

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

I think it was just copied from the way Composer-Setup.exe does it. Whatever, it is certainly not a crucial enough issue to be worrying about at the moment, so I'll shut up!

Copy link
Member

Choose a reason for hiding this comment

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

Actually, in case of spaces in directory names it should probably be:
"SET COMPOSER_BIN_DIR=\"%~dp0\"\r\n".

Copy link
Member Author

Choose a reason for hiding this comment

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

You sure? Because the one above BIN_TARGET does not have it, it only uses quotes when using the variable, not setting it.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, you're correct. I had assumed that because BIN_TARGET was being escaped, this would also need it. But it would appear that even though SET is a command, it uses its full command-line and ignores any whitespace.

Note entirely sure why BIN_TARGET is escaped and then has any surrounding quotes removed, but I'm not going to worry about that.

@naderman naderman merged commit 6f5baab into composer:main Dec 28, 2021
@Seldaek Seldaek deleted the add_bin_dir_for_binaries branch December 28, 2021 21:04
jakzal added a commit to jakzal/toolbox that referenced this pull request Dec 30, 2021
Until there is support in composer-bin-plugin for composer/composer#10402
newmedia pushed a commit to newmedia/toolbox that referenced this pull request Jan 4, 2022
Until there is support in composer-bin-plugin for composer/composer#10402
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 this pull request may close these issues.

[2.2.0] Scripts from "bin" are executed not in "bin" directory.
3 participants