composer complains "Skipped installation of bin bin/composer for package composer/composer: file not found in package"

What is a standard Composer build in the Drupal 8 context? I have seen interesting builds.

In this context it is when you run composer install against a git clone of the core repo.

The issue is that when the composer/composer package is installed, it makes a symlink from /vendor/composer/composer/bin/composer to /vendor/bin/composer.

Then, the vendor hardening plugin removes the '/vendor/composer/composer/bin', leaving the original symlink pointing at nothing.

That broken symlink causes a tar action to fail, and also causes composer to complain with Skipped installation of bin bin/composer for package composer/composer: file not found in package

I believe the vendor hardening plugin is doing the right thing by making sure that nobody ends up with an executable /vendor/bin/composer on a live site, but I think it might need to go a step further than the plain file removal its doing.

We have a few ways we can fix this bug:

1. Dont clean /vendor/composer/composer/bin and hope its not some RCE vector.
2. Add another event listener to composer/Plugin/VendorHardening/VendorHardeningPlugin.php for pre-package-install, that looks specifically for composer/composer and unset's the binaries key, which would prevent it from making the symlink.
3. Add another event listener to composer/Plugin/VendorHardening/VendorHardeningPlugin.php for pre-package-install, that determines if the binaries key is pointing at a file that would be removed as part of the cleanup, and if so, unsets the binaries key so that any future cleanups we might add do not cause this problem again.
4. Add another event listener to composer/Plugin/VendorHardening/VendorHardeningPlugin.php for pre-package-install, and add an additional config array for "bin removals" to composer/Plugin/VendorHardening/Config.php, and just explicitly add things we know we dont want to symlink.

Im thinking either #2 or #4, mostly because we simply dont add new, potentially threatworthy dev dependencies to our codebase very often. Though maybe #3 is better because of that reason (we'll forget to update either the config or the hardcoded check..?)

Don't mind me, just here to fix the issue linking and add the template to the IS.

#4 is close but no cigar. It's actually Drupal\Core\Composer\Composer::vendorTestCodeCleanup, not the plugin.

You can repro like this:

$ git checkout 8.8.x
$ rm -rf vendor/
$ composer install

You'll then eventually see:

    Skipped installation of bin bin/composer for package composer/composer: file not found in package

This error is a little bit of a lie because it does create the symlink even though the file doesn't exist.

I'd say the code solutions in #4 are good ideas and we should work on them. I'd also say that before we do that, we should start using the vendor hardening plugin rather than the event handler script, because I'd rather we improve the plugin than rework the event handler.

And it bears repeating: If you're packaging for production, you should say composer install --no-dev, and this problem goes away. You should be doing that anyway for production.

Updating the IS, and also...

It seems like this should also be an upstream fix. If the file doesn't exist, Composer shouldn't create a symlink.

So what you're seeing in your log is two things:

The first is:

  - Installing composer/composer (1.8.5): Loading from cache
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup

That's our event handler cleaning the suspect directories, such as bin/, out of vendor/composer/composer. This deletes vendor/composer/composer/bin/composer.

The second is:

Skipped installation of bin bin/composer for package composer/composer: file not found in package

That's Composer telling you it couldn't find vendor/composer/composer/bin/composer so it didn't symlink it to the bin directory.

So it's a mystery why the symlink to a non-existent file is there when it's done. Maybe Composer constructed it during composer/composer package installation, but before sending the event to our script. In that case, it would symlink the file just fine, and then we'd delete the target file in our event handler. Then later on it tries again and fails? This seems the likeliest explanation to me.

It also seems fair to say that Composer's designers didn't think that people would be removing the bin files during package install.

We can engineer workarounds in core, and I'll figure out if there's an actual bug in Composer tomorrow.

Here's a patch.

It goes for option #4.3 and figures out if bin configuration for the package overlaps with the cleanup directories.

It also removes the event script for vendor cleanup and uses the vendor hardening plugin. This part is kind of strange because it uses a path repo. In order to use a path repo and not get frustrated during development, the constraint is *.

It looks like this:

    Modifying bin config for composer/composer which overlaps with cleanup directories.
  - Installing composer/composer (1.8.5): Loading from cache
    Cleaning: composer/composer

And during update:

    Modifying bin config for composer/composer which overlaps with cleanup directories.
  - Updating composer/composer (1.8.5 => 1.9.0): Loading from cache
    Cleaning: composer/composer

And it prevents Composer from placing the symlinks which will later refer to deleted files.

Maybe Composer constructed it during composer/composer package installation, but before sending the event to our script.

This is exactly whats happening when I was running this through the debugger.

Because the wikimedia plugin is still kicking around in the lockfile/composer.json composer still does multiple installation passes.

1. +++ b/composer/Plugin/VendorHardening/VendorHardeningPlugin.php
   @@ -119,6 +144,63 @@ public function onPostPackageUpdate(PackageEvent $event) {
   +    $unset_these_binaries = [];
   

   For some reason, I can hear Will Ferrell dressed up as Alex Trebek, announcing this as a category for celebrity jeopardy.

2. +++ b/composer/Plugin/VendorHardening/VendorHardeningPlugin.php
   @@ -119,6 +144,63 @@ public function onPostPackageUpdate(PackageEvent $event) {
   +  protected function removeBinBeforeCleanup(CompletePackage $package) {
   

   +needs tests

3. 
   $this->io->writeError(sprintf('%sModifying bin config for %s which overlaps with cleanup directories.', str_repeat(' ', 4), $package->getName()), TRUE, IOInterface::VERBOSE);
   

   I feel like our plugins are somewhat chatty, and tell us too much what they're doing for normal situations. Maybe we should make them chatty only when we ask them to?

   Ie. perhaps add IOInterface::VERBOSE

   $this->io->writeError(sprintf('%sModifying bin config for <info>%s</info> which overlaps with cleanup directories.', str_repeat(' ', 4), $package->getName()), TRUE, IOInterface::VERBOSE);
   

4. Im trying to remember why we didnt make this a path repository in the first place.. were we hesitant to do that because of the limit on path repos? (in that case, path repo's for composer plugins seems fine to me)

   +++ b/composer.json
   @@ -67,9 +68,6 @@
   -        "post-autoload-dump": "Drupal\\Core\\Composer\\Composer::ensureHtaccess",
   -        "post-package-install": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",
   -        "post-package-update": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",
   

   I wonder if we can do #3076684: Remove deprecated vendor cleanup scripts as well ?

I think we didn't add the plugin because it would have been disruptive somehow to something, so we didn't. As they say: "It's all good, man."

Refactored for tests, added tests, set the output to verbose. Unset these binaries for $800, Alex.

Fixed typo.

Mostly here to add the "composer initiative" issue tag. :)

+1 on the approach here, although I have not tested it yet.

Very minor @covers changes.

Forgot to do the path repo shuffle and ended up with some out-of-bounds dependencies in the lock file.

This should fix that.

Also I was curious why we didn't use a path repo for this plugin initially anyway, and it's here: #3057094-15: Add Composer vendor/ hardening plugin to core and #3076684-4: Remove deprecated vendor cleanup scripts

Removing spurious empty file, core/nbproject/private/phpcsmd.xml. No other change.

Tested; this clears up the spurious warning, and removes the same set of files as the script.

Recalling what https://www.drupal.org/comment/13126115#comment-13126115 means, this improvement helps the existing plugin do binary cleaning better, and that once we have #3071726: Package tarballs using composer template if available in place, we can remove the path repo entirely from the root composer.json, because the assumption then is "no need to clean the git repo because, really, you should not try and use git clone to start a drupal site"

+++ b/composer.json
@@ -6,7 +6,8 @@
+        "drupal/core-vendor-hardening": "^8.7"

How come this is not "self.version" too?

If I apply this patch and run composer install I see

$ composer install                                                                                                                 

> Drupal\Core\Composer\Composer::ensureComposerVersion
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Package phpunit/phpunit-mock-objects is abandoned, you should avoid using it. No replacement was suggested.
Generating autoload files
> Drupal\Core\Composer\Composer::preAutoloadDump
Hardening vendor directory with .htaccess and web.config files.
    Skipped installation of bin bin/composer for package composer/composer: file not found in package
Cleaning vendor directory.

So I see the thing this is trying to fix.

@alexpott thats because the patch is to a component that hasnt been subtree split out. The patch you're applying isnt applying to the code thats running. Thats also why we have the ^8.7 in there. Because if it had to grab self.version, it could potentially fail with the same situation that grabbed whatever local branch you were working on at the time.

oh, wait, no, path repository means it *should* be executing the patch.

I cannot reproduce Alex's issue on a clean git clone/composer install.

Okay so if I do rm -rf vendor and then run composer install I can then run composer install to my hearts content and not see the message. But the rm -rf vendor step on an existing site is quite odd.

If do composer update --lock on 8.8.x HEAD this should be a no-op but at the moment it's not. And the result seems to fix this issue!?!?

#26 doesn't work for me. I still get the error and the dangling alias.

Here's a reroll of #17 because we changed composer.lock in the meantime.

#26 was just some composer.lock changes.

It looks like those lines were removed the bin/composer lines from the lockfile in #3031379: Add a new test type to do real update testing, added back in in #3086332: Explicitly require-dev symfony/browser-kit: 3.4.0 . This patch *should* remove them because after this patch is in, the next lock update will remove them.

The

+++ b/composer.lock
@@ -3652,9 +3652,6 @@
-            "bin": [
-                "bin/composer"
-            ],

Here's #27 with

git apply 3082866_27.patch
composer install
composer update --lock

It preemtively removes the bin from our lockfile to ensure cleanliness.

+1 on that.

Before #28:

$ composer install
...
    Skipped installation of bin bin/composer for package composer/composer: file not found in package

After applying #28:

$ composer install
...
    Skipped installation of bin bin/composer for package composer/composer: file not found in package

After applying #28 and

rm -rf vendor<code>

<code>$ composer install
...

Spurious error message is gone. Error message is caused by composer binary being incorrectly removed, so it is expected to need to remove the vendor directory so that composer will be re-downloaded and the composer binary restored.

RTBC.

This is definitely an 8.8.x fix.

@Mile23 I might be wrong but isn't the process usually that it's fixed in the latest branch (8.9), committed, then back-ported and committed to the up and coming release branch (8.8). I might be wrong there as it might depend on the circumstances of each issue, but regardless whether the issue is tagged 8.8 or 8.9 it should still get committed to both branches I think.

This needs a reroll.

+++ b/composer.lock
@@ -902,6 +902,36 @@
+            "version": "8.8.x-dev",

This is going to be interesting. How do we commit this to 9.0.x, 8.9.x and 8.8.x? I think this should be different on all branches.

Do we need to update committer instructions to run $ composer update drupal/core drupal/core-vendor-hardening as commit to each branch. Note we're also going to have this problem with the drupal/core version in the root composer.lock.

The annoying thing about #34 is that the version written to composer.lock for a path repository doesn't really matter. I wonder if it would be viable / valid to use a post-update hook to rewrite this value to a consistent value for all branches. The problem with this idea is that by the specification, the version value in a lock file must be a specific version or branch, i.e. if we wrote "self.version" here, that wouldn't really be a valid value.

Maybe we could submit a patch to Composer to omit the version for path repositories in the lock file, and implicitly treat it like self.version.

In the short term, though, I think we're going to have to muddle through as described in #34.

I dont think we need

$ composer update drupal/core drupal/core-vendor-hardening as commit to each branch.

We should only have to do that when a new branch is made, or, when a patch modifies composer.lock and also touches either drupal/core or drupal/core-vendor-hardening.

For this issue we should roll 3 patches, one for each version, that can be committed directly without committer friction.

We will need to update the https://www.drupal.org/core/maintainers/create-minor-branch instructions to include drupal/core-vendor-hardening

Finally, this composer update step should probably be taken when making tags/releases as well:
https://www.drupal.org/core/maintainers/create-core-release

Now that Im scratching my beard, Im pretty sure #20 brings up a valid point. Path repositories *should* be self.version. If they shouldnt for some reason, then both drupal/core and drupal/core-vendor-hardening should follow the same pattern.

But yes, the composer dance is :If you switch to a branch, and run any composer command that will update the .lock file, drupal/core and drupal/core-vendor-hardening are going to update their version to that branch name, unless we avoid self.version.

Perhaps there's a way to interfere with the lockfile creation via a plugin such that we can remove our path repos before it gets generated.

Here's a reroll.

And here are the steps I used to make the lock file (aka 'Composer dance'):

$ git checkout -b 3082866
$ git apply --rej 3082866_28.patch
[edit composer.json]
$ git checkout 8.8.x composer.lock
$ rm *.rej
$ git add [all the changed files]
$ git commit -m '28 without lock'
$ git checkout 8.8.x
$ git merge --squash 3082866
$ composer update drupal/core-vendor-hardening
$ git checkout 3082866
$ git add composer.lock
$ git commit -m '37'
$ git diff 8.8.x > 3082866_37.patch

The *only* reason we have to do this is because it's a path repo. Well, the two reasons: It's a path repo and we commit the lock file.

Now, take this patch and apply it to 8.9.x:

$ composer install
> Drupal\Core\Composer\Composer::ensureComposerVersion
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Package operations: 0 installs, 1 update, 0 removals
  - Removing drupal/core (8.8.x-dev), source is still present in core
  - Installing drupal/core (8.9.x-dev): Source already present
Package phpunit/phpunit-mock-objects is abandoned, you should avoid using it. No replacement was suggested.
Generating autoload files
> Drupal\Core\Composer\Composer::preAutoloadDump
Hardening vendor directory with .htaccess and web.config files.
    Skipped installation of bin bin/composer for package composer/composer: file not found in package
Cleaning vendor directory.

You see the "Skipped installation of bin bin/composer.." line because we have a left-over vendor directory. Remove vendor/ and reinstall and it goes away.

But the important thing is that the lock file is happy enough in 8.9.x even though the lock file says:

            "name": "drupal/core-vendor-hardening",
            "version": "8.8.x-dev",

The reason it's a path repo is because otherwise we're testing against the dependency pulled in via packagist, and we don't want that under drupalci since we're patching the dependency within the codebase.

It might be that, for drupal/drupal, we could adopt a policy of using the minor version dev as the constraint for packages in the monorepo. So for instance after we're convinced that this patch is good, we'd change it so it's not a path repo, and we'd say "drupal/core-vendor-hardening": "8.8.x-dev" instead. That way we don't have to treat it as a special case until we work on it again. Then when we need to patch it, we go back to being a path repo so it works in drupalci, until we commit it. Just a thought.

Perhaps there's a way to interfere with the lockfile creation via a plugin such that we can remove our path repos before it gets generated.

I just spent some time in xdebug/composer and theres really not a lot we can do about the lockfile having our path repos in it for now.

Im pretty sure that the 'composer dance' you're referring to is only an issue when you're working on a patch that involves something with a path repo.

None of that is necessary, afaict, unless you run a bare composer update in a branch, perhaps.

None of that is necessary, afaict, unless you run a bare composer update in a branch, perhaps.

Correct.

Now blocking #3086644: LegacyProject composer templates wrongly reference 8.x + fix test coverage

This is blocking, but nobody has answered the question from #20. Should path repos always be self.version?

The example in the composer documentation on path repositories uses "version": "*". I think this makes more sense than self.version.

Whichever one we use, we should be consistent across all path repositories.

re #42I failed to notice that you had changed #37 to be self.version, which is a tacit endorsement.

Im experimenting with all of these. Weird things are happening so far with *, things like its finding the subtree split and claiming that in the lockfile.

Per Slack conversation I think it's fair to say self.version helps us the most while tormenting us the least. So #37 makes us happy.

#37 needed some CS love, though so now we're #45.

Looking good again.

Committed dd27156 and pushed to 9.0.x. Thanks!
Committed 5238c3237e and pushed to 8.9.x. Thanks!
Committed 9a82f42ad4 and pushed to 8.8.x. Thanks!

I did $ composer update drupal/core-vendor-hardening on each brach and added the resulting lock file so each version is correct. Fun.

Sigh. The reroll in #37 lacked the changes in #28.

That's an oversight... But I think it doesn't matter that much now that the vendor hardening plugin is in use. I think the config for composer/composer ends up in the lock file, but then is successfully handled by the vendor hardening plugin whenever an install or update happens.

I'm not seeing the 'Skipped installation of bin/composer...' message locally, and it's not adding orphaned symlinks. Is the view different from infrastructure-land?

I am getting this error with Drupal 8.9.13 when I run `composer update`. I tried applying the patch, just in case there had been a regression, but it wouldn't apply, nor would this patch from a follow-up issue.

I tried manually removing this section from my composer.lock file:

"bin": [
     "bin/composer"
],

...but it returns (along with the error) if I run `composer update`.

I found that `bin/composer` code is also included in /vendor/cgweans/composer-patches/composer.lock so I removed composer-patches completely but it didn't help.

My composer.json has a long history but is currently very similar to drupal/recommended-project but with more modules, packages etc. added.

I realise this was closed due to no activity, but it seems better to add this here rather than open a new support request issue.