fix(nm): Reinstall removed module directories #3467
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
larixer
changed the title
larixer
force-pushed
the
larixer/nm-sync-with-disk
branch
from
e801566 to
f0a9c08
Compare
1 task
1 task
merceyz
requested changes
Mar 22, 2022
merceyz
approved these changes
Mar 22, 2022
3 tasks
1 task
merceyz
pushed a commit
that referenced
this pull request
May 12, 2022
3 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 28, 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](https://github.com/yarnpkg/berry/blob/master/CHANGELOG.md#321), I suspect it's [this](#3467) 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](https://github.com/yarnpkg/berry/blob/master/packages/plugin-nm/sources/NodeModulesLinker.ts#L1361)) 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 <!--- Don't worry if you miss something, chores are automatically tested. --> <!--- This checklist exists to help you remember doing the chores when you submit a PR. --> <!--- Put an `x` in all the boxes that apply. --> - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). <!-- See https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released for more details. --> <!-- Check with `yarn version check` and fix with `yarn version check -i` --> - [x] I have set the packages that need to be released for my changes to be effective. <!-- The "Testing chores" workflow validates that your PR follows our guidelines. --> <!-- If it doesn't pass, click on it to see details as to what your PR might be missing. --> - [x] I will check that all automated PR checks pass before the PR gets reviewed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's the problem this PR addresses?
When you remove unplugged directory with pnp linker, it reinstalls the directory again. This behaviour is currently different for the nm linker, it will perform reinstall only when top-level nm folder is removed. The PR changes this for the nm linker, so that both linkers behave the same - when you remove directory deep inside
node_modulesthe nm linker reinstalls only this directory and rebuilds the module if needed. The same logic applies to the bin symlinks - removed bin symlinks gets recreated by the nm linker.Closes #3214
How did you fix it?
I use the fact that the nm linker writes
node_modules/.yarn-state.ymlas the last operation of the link step, hence it must have themtimewhich is greater than all the files and folders insidenode_modules. To detect whether the user has removed any module directory insidenode_modules, it is enough to check whether each parentnode_modulesfolder hasmtime >= state_file_mtimeand if it does - the user has modified the directory, we then performreaddirand find entries deleted by the user, if any, and record these modifications in the preinstall state. The algorithm that diffs install and preinstall state later does the rest of the job of reinstall deleted modules and directories.Checklist