Skip to content

Conversation

oskarols
Copy link
Contributor

@oskarols oskarols commented May 25, 2025

Hi! 👋

What's the problem this PR addresses?

Resolves #5344 #6551

When using nodeLinker: node-modules there's an issue where bin files sometimes don't have the correct permissions (755) after installation. Some examples of scenarios where this can occur:

  1. Deleting a package directory containing a bin file in node_modules and then running yarn install
  2. Upgrading a package containing a bin file
  3. Using a stale node_modules (e.g. when using a CI cache) and then running yarn install

I ran a bisect and at least scenario 1 above seems to have started in 3.2.1, I suspect it's this change specifically although previous to this change the deleted directory was not reinstalled at all.

Digging into the issue I found that the general issue stems from reliance on presence of symlinks inside node_modules/.bin/ (here) to check whether permissions should be re-applied. The flaw with this approach is that for stale installation the old symlink is still there e.g. if a new version of a package is installed.

How did you fix it?

This changes the prevBinSymlinks to remove all symlinks that were related to added/changed packages during the installation. The comparison between previous and current symlinks inside persistBinSymlinks will now recreate the symlinks for these updated packages and also set the correct permissions.

I've added two tests which were previously failing to cover this behavior, and a third which was already succeeding since it seemed sensible to explicitly test for permissions in a basic test as well.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@oskarols oskarols force-pushed the nm-linker-fix-binary-permissions-on-partial-installs branch from cb72ab0 to 5220052 Compare May 25, 2025 16:44
@oskarols oskarols force-pushed the nm-linker-fix-binary-permissions-on-partial-installs branch from 5220052 to 2e850a7 Compare May 25, 2025 17:00
@oskarols
Copy link
Contributor Author

General question: browsing the commit history it seems that the PRs description seem to be used for the commits. Is this done automatically upon merge? Or do you want me to write a detailed commit description by hand?

@oskarols oskarols changed the title fix(plugin-nm): ensure binary permissions are set on partial installs fix(plugin-nm): ensure binary permissions are set for partial installs May 25, 2025
@oskarols oskarols changed the title fix(plugin-nm): ensure binary permissions are set for partial installs fix(plugin-nm): set binary permissions for partial installs May 25, 2025
@oskarols oskarols force-pushed the nm-linker-fix-binary-permissions-on-partial-installs branch from 7ac999d to f479e67 Compare May 25, 2025 19:39
@larixer
Copy link
Member

larixer commented May 27, 2025

General question: browsing the commit history it seems that the PRs description seem to be used for the commits. Is this done automatically upon merge? Or do you want me to write a detailed commit description by hand?

I think it's done automatically. We just squash-merge PRs

@oskarols oskarols force-pushed the nm-linker-fix-binary-permissions-on-partial-installs branch from f479e67 to b8866bf Compare May 27, 2025 19:36
@oskarols oskarols requested a review from larixer May 27, 2025 19:44
Copy link
Member

@larixer larixer left a comment

Choose a reason for hiding this comment

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

Thanks, look good to me!

@larixer larixer added this pull request to the merge queue May 28, 2025
Merged via the queue into yarnpkg:master with commit 7553478 May 28, 2025
32 checks passed
@oskarols
Copy link
Contributor Author

oskarols commented Jun 2, 2025

@larixer Thanks for the reviews and help!

Any clue on when this would be released? Is there anything I can do to get this out the door?

@arcanis
Copy link
Member

arcanis commented Jun 2, 2025

I'll review a couple of other PRs later today and make a release

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

Successfully merging this pull request may close these issues.

[Bug?]: yarn install does not set executable bit on bin files when folder has been manually removed
3 participants