Skip to content

Commit 7553478

Browse files
oskarolsoskarols
authored
fix(plugin-nm): set binary permissions for partial installs (#6807)
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.
1 parent 8661aac commit 7553478

File tree

5 files changed

+131
-0
lines changed

5 files changed

+131
-0
lines changed

‎.yarn/versions/944c0a3d.yml

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
releases:
2+
"@yarnpkg/cli": patch
3+
"@yarnpkg/plugin-nm": patch
4+
5+
declined:
6+
- "@yarnpkg/plugin-compat"
7+
- "@yarnpkg/plugin-constraints"
8+
- "@yarnpkg/plugin-dlx"
9+
- "@yarnpkg/plugin-essentials"
10+
- "@yarnpkg/plugin-init"
11+
- "@yarnpkg/plugin-interactive-tools"
12+
- "@yarnpkg/plugin-npm-cli"
13+
- "@yarnpkg/plugin-pack"
14+
- "@yarnpkg/plugin-patch"
15+
- "@yarnpkg/plugin-pnp"
16+
- "@yarnpkg/plugin-pnpm"
17+
- "@yarnpkg/plugin-stage"
18+
- "@yarnpkg/plugin-typescript"
19+
- "@yarnpkg/plugin-version"
20+
- "@yarnpkg/plugin-workspace-tools"
21+
- "@yarnpkg/builder"
22+
- "@yarnpkg/core"
23+
- "@yarnpkg/doctor"

‎packages/acceptance-tests/pkg-tests-fixtures/packages/@scoped__has-bin-entry-2.0.0/bin.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
#!/usr/bin/env node
2+
3+
console.log(require(`./package.json`).version);

‎packages/acceptance-tests/pkg-tests-fixtures/packages/@scoped__has-bin-entry-2.0.0/package.json

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"name": "@scoped/has-bin-entry",
3+
"version": "2.0.0",
4+
"bin": {
5+
"@scoped/has-bin-entry": "./bin.js"
6+
}
7+
}

‎packages/acceptance-tests/pkg-tests-specs/sources/node-modules.test.ts

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1780,6 +1780,81 @@ describe(`Node_Modules`, () => {
17801780
),
17811781
);
17821782

1783+
it(`should set correct permissions on binaries in installed packages`,
1784+
makeTemporaryEnv(
1785+
{
1786+
dependencies: {
1787+
[`has-bin-entries`]: `1.0.0`,
1788+
},
1789+
},
1790+
{
1791+
nodeLinker: `node-modules`,
1792+
},
1793+
async ({path, run}) => {
1794+
await run(`install`);
1795+
const {mode} = await xfs.lstatPromise(npath.toPortablePath(`${path}/node_modules/has-bin-entries/bin.js`));
1796+
const permissions = (mode & 0o777).toString(8);
1797+
expect(permissions).toBe(process.platform === `win32` ? `666` : `755`);
1798+
},
1799+
),
1800+
);
1801+
1802+
it(`should set correct permissions on binaries in packages that were upgraded`,
1803+
makeTemporaryEnv(
1804+
{
1805+
dependencies: {
1806+
[`has-bin-entries`]: `1.0.0`,
1807+
[`scoped/has-bin-entry`]: `1.0.0`,
1808+
},
1809+
},
1810+
{
1811+
nodeLinker: `node-modules`,
1812+
},
1813+
async ({path, run}) => {
1814+
await run(`install`);
1815+
await xfs.writeJsonPromise(`${path}/package.json` as PortablePath, {
1816+
dependencies: {
1817+
'has-bin-entries': `2.0.0`,
1818+
'@scoped/has-bin-entry': `2.0.0`,
1819+
},
1820+
});
1821+
await run(`install`);
1822+
1823+
const binaries = [
1824+
`has-bin-entries/bin.js`,
1825+
`@scoped/has-bin-entry/bin.js`,
1826+
];
1827+
1828+
for (const binary of binaries) {
1829+
const {mode} = await xfs.lstatPromise(npath.toPortablePath(`${path}/node_modules/${binary}`));
1830+
const permissions = (mode & 0o777).toString(8);
1831+
expect(permissions).toBe(process.platform === `win32` ? `666` : `755`);
1832+
}
1833+
},
1834+
),
1835+
);
1836+
1837+
it(`should reinstall and set correct permissions on next install when packages have been deleted by the user`,
1838+
makeTemporaryEnv(
1839+
{
1840+
dependencies: {
1841+
[`has-bin-entries`]: `1.0.0`,
1842+
},
1843+
},
1844+
{
1845+
nodeLinker: `node-modules`,
1846+
},
1847+
async ({path, run}) => {
1848+
await run(`install`);
1849+
await xfs.removePromise(`${path}/node_modules/has-bin-entries` as PortablePath);
1850+
await run(`install`);
1851+
const {mode} = await xfs.lstatPromise(npath.toPortablePath(`${path}/node_modules/has-bin-entries/bin.js`));
1852+
const permissions = (mode & 0o777).toString(8);
1853+
expect(permissions).toBe(process.platform === `win32` ? `666` : `755`);
1854+
},
1855+
),
1856+
);
1857+
17831858
it(`should only reinstall scoped dependencies deleted by the user on the next install`,
17841859
makeTemporaryEnv(
17851860
{

‎packages/plugin-nm/sources/NodeModulesLinker.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1045,6 +1045,28 @@ export function getGlobalHardlinksStore(configuration: Configuration): PortableP
10451045
return ppath.join(configuration.get(`globalFolder`), `store` as Filename);
10461046
}
10471047

1048+
/**
1049+
* Mutate binSymlinks by removing binaries related to the changedLocations.
1050+
*/
1051+
function invalidateBinSymlinks(binSymlinks: BinSymlinkMap, changedLocations: Set<PortablePath>): void {
1052+
const getLocationPackageRoot = (targetPath: PortablePath): PortablePath => {
1053+
const parts = targetPath.split(ppath.sep);
1054+
const nmIndex = parts.lastIndexOf(NODE_MODULES);
1055+
if (nmIndex < 0 || nmIndex == parts.length - 1)
1056+
throw new Error(`Assertion failed. Path is outside of any node_modules package ${targetPath}`);
1057+
1058+
return parts.slice(0, nmIndex + (parts[nmIndex + 1].startsWith(`@`) ? 3 : 2)).join(ppath.sep) as PortablePath;
1059+
};
1060+
1061+
for (const binSymlinkMap of binSymlinks.values()) {
1062+
for (const [binFile, binLocation] of binSymlinkMap) {
1063+
if (changedLocations.has(getLocationPackageRoot(binLocation))) {
1064+
binSymlinkMap.delete(binFile);
1065+
}
1066+
}
1067+
}
1068+
}
1069+
10481070
async function persistNodeModules(preinstallState: InstallState, installState: NodeModulesLocatorMap, {baseFs, project, report, loadManifest, realLocatorChecksums}: {project: Project, baseFs: FakeFS<PortablePath>, report: Report, loadManifest: LoadManifest, realLocatorChecksums: Map<LocatorHash, string | null>}) {
10491071
const rootNmDirPath = ppath.join(project.cwd, NODE_MODULES);
10501072

@@ -1314,6 +1336,7 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
13141336

13151337
await xfs.mkdirPromise(rootNmDirPath, {recursive: true});
13161338

1339+
invalidateBinSymlinks(prevBinSymlinks, new Set(addList.map(l => l.dstDir)));
13171340
const binSymlinks = await createBinSymlinkMap(installState, locationTree, project.cwd, {loadManifest});
13181341
await persistBinSymlinks(prevBinSymlinks, binSymlinks, project.cwd, windowsLinkType);
13191342

0 commit comments

Comments
 (0)