From 5ef113f7db1201f48e22498374023ebe1e61b069 Mon Sep 17 00:00:00 2001 From: ConnectDotz Date: Thu, 8 Mar 2018 10:40:45 +0800 Subject: [PATCH] fixed linkDuplicates for symlinks (#5442) --- __tests__/commands/_helpers.js | 3 + .../commands/install/integration-deduping.js | 282 ++++++++++++------ .../deps/a-1/package.json | 4 + .../deps/b-1/package.json | 7 + .../deps/c-1/package.json | 4 + .../hardlink-scoped-workspaces/package.json | 9 + .../packages/w1/package.json | 9 + .../packages/w2/package.json | 8 + src/config.js | 20 +- src/package-linker.js | 3 +- src/reporters/lang/en.js | 2 +- 11 files changed, 248 insertions(+), 103 deletions(-) create mode 100644 __tests__/fixtures/install/hardlink-scoped-workspaces/deps/a-1/package.json create mode 100644 __tests__/fixtures/install/hardlink-scoped-workspaces/deps/b-1/package.json create mode 100644 __tests__/fixtures/install/hardlink-scoped-workspaces/deps/c-1/package.json create mode 100644 __tests__/fixtures/install/hardlink-scoped-workspaces/package.json create mode 100644 __tests__/fixtures/install/hardlink-scoped-workspaces/packages/w1/package.json create mode 100644 __tests__/fixtures/install/hardlink-scoped-workspaces/packages/w2/package.json diff --git a/__tests__/commands/_helpers.js b/__tests__/commands/_helpers.js index 73b3c8c257..6b82806565 100644 --- a/__tests__/commands/_helpers.js +++ b/__tests__/commands/_helpers.js @@ -170,6 +170,9 @@ export async function run( try { const config = await makeConfigFromDirectory(cwd, reporter, flags); + if (typeof flags.workspacesNohoistEnabled === 'boolean') { + config.workspacesNohoistEnabled = flags.workspacesNohoistEnabled; + } const install = await factory(args, flags, config, reporter, lockfile, () => out); if (checkInstalled) { diff --git a/__tests__/commands/install/integration-deduping.js b/__tests__/commands/install/integration-deduping.js index 06a9d2b93b..3f5415636d 100644 --- a/__tests__/commands/install/integration-deduping.js +++ b/__tests__/commands/install/integration-deduping.js @@ -199,107 +199,205 @@ test.concurrent('install should dedupe dependencies avoiding conflicts 9', (): P }); }); -test.concurrent('install should hardlink repeated dependencies', (): Promise => { - // A@1 - // B@1 -> A@2 - // C@1 -> A@2 (this is hardlink to B@1->A@2) - return runInstall({linkDuplicates: true}, 'hardlink-repeated-dependencies', async config => { - const b_a = await fs.stat(getPackageManifestPath(config, 'b/a')); - const c_a = await fs.stat(getPackageManifestPath(config, 'c/a')); - expect(b_a.ino).toEqual(c_a.ino); +describe('hardlink', () => { + test.concurrent('install should hardlink repeated dependencies', (): Promise => { + // A@1 + // B@1 -> A@2 + // C@1 -> A@2 (this is hardlink to B@1->A@2) + return runInstall({linkDuplicates: true}, 'hardlink-repeated-dependencies', async config => { + const b_a = await fs.stat(getPackageManifestPath(config, 'b/a')); + const c_a = await fs.stat(getPackageManifestPath(config, 'c/a')); + expect(b_a.ino).toEqual(c_a.ino); + }); }); -}); -test.concurrent('install should not hardlink repeated dependencies if linkDuplicates=false', (): Promise => { - // A@1 - // B@1 -> A@2 - // C@1 -> A@2 - return runInstall({linkDuplicates: false}, 'hardlink-repeated-dependencies', async config => { - const b_a = await fs.stat(getPackageManifestPath(config, 'b/a')); - const c_a = await fs.stat(getPackageManifestPath(config, 'c/a')); - expect(b_a.ino).not.toEqual(c_a.ino); + test.concurrent('install should not hardlink repeated dependencies if linkDuplicates=false', (): Promise => { + // A@1 + // B@1 -> A@2 + // C@1 -> A@2 + return runInstall({linkDuplicates: false}, 'hardlink-repeated-dependencies', async config => { + const b_a = await fs.stat(getPackageManifestPath(config, 'b/a')); + const c_a = await fs.stat(getPackageManifestPath(config, 'c/a')); + expect(b_a.ino).not.toEqual(c_a.ino); + }); }); -}); -test.concurrent('install should not crash when hardlinking deep structures', (): Promise => { - // https://github.com/yarnpkg/yarn/issues/2734 - // A@1 -> B@1 -> C@1 - // -> C@2 - // B@2 - // C@3 - // D@1 -> B@1 (hardlink) -> C@1 (hardlink) - // -> C@2 - return runInstall({linkDuplicates: true}, 'hardlink-collision', async config => { - let a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/package.json')); - let d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/package.json')); - expect(a_1.ino).toEqual(d_1.ino); - a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/node_modules/c/package.json')); - d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/node_modules/c/package.json')); - expect(a_1.ino).toEqual(d_1.ino); + test.concurrent('install should not crash when hardlinking deep structures', (): Promise => { + // https://github.com/yarnpkg/yarn/issues/2734 + // A@1 -> B@1 -> C@1 + // -> C@2 + // B@2 + // C@3 + // D@1 -> B@1 (hardlink) -> C@1 (hardlink) + // -> C@2 + return runInstall({linkDuplicates: true}, 'hardlink-collision', async config => { + let a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/package.json')); + let d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/package.json')); + expect(a_1.ino).toEqual(d_1.ino); + a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/node_modules/c/package.json')); + d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/node_modules/c/package.json')); + expect(a_1.ino).toEqual(d_1.ino); + }); }); -}); -test.concurrent('install should consider different hoisting with --link-duplicate', (): Promise => { - // https://github.com/yarnpkg/yarn/issues/2734 - // A@1 -> B@1 -> C@1 - // -> C@2 - // B@2 - // C@3 - // D@1 -> B@1 (hardlink) -> *C@1* (redundant) - // -> C@1 (hardlink) - return runInstall({linkDuplicates: true}, 'hardlink-collision-2', async config => { - let a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/package.json')); - let d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/package.json')); - expect(a_1.ino).toEqual(d_1.ino); - a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/node_modules/c/package.json')); - d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/c/package.json')); - expect(a_1.ino).toEqual(d_1.ino); - // this is redundant but we are ok with it - expect(await fs.exists(path.join(config.cwd, 'node_modules/d/node_modules/b/node_modules/c/package.json'))).toBe( - true, - ); + test.concurrent('install should consider different hoisting with --link-duplicate', (): Promise => { + // https://github.com/yarnpkg/yarn/issues/2734 + // A@1 -> B@1 -> C@1 + // -> C@2 + // B@2 + // C@3 + // D@1 -> B@1 (hardlink) -> *C@1* (redundant) + // -> C@1 (hardlink) + return runInstall({linkDuplicates: true}, 'hardlink-collision-2', async config => { + let a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/package.json')); + let d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/package.json')); + expect(a_1.ino).toEqual(d_1.ino); + a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/node_modules/c/package.json')); + d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/c/package.json')); + expect(a_1.ino).toEqual(d_1.ino); + // this is redundant but we are ok with it + expect(await fs.exists(path.join(config.cwd, 'node_modules/d/node_modules/b/node_modules/c/package.json'))).toBe( + true, + ); + }); }); -}); -test.concurrent('install should consider different hoisting with --link-duplicate 2', (): Promise => { - // https://github.com/yarnpkg/yarn/issues/2734 - // A@1 -> B@1 - // -> C@1 - // B@2 - // C@3 - // D@1 -> B@1 (hardlink) -> C@1 (hardlink) - // -> C@2 - return runInstall({linkDuplicates: true}, 'hardlink-collision-3', async config => { - let a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/package.json')); - let d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/package.json')); - expect(a_1.ino).toEqual(d_1.ino); - a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/c/package.json')); - d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/node_modules/c/package.json')); - expect(a_1.ino).toEqual(d_1.ino); + test.concurrent('install should consider different hoisting with --link-duplicate 2', (): Promise => { + // https://github.com/yarnpkg/yarn/issues/2734 + // A@1 -> B@1 + // -> C@1 + // B@2 + // C@3 + // D@1 -> B@1 (hardlink) -> C@1 (hardlink) + // -> C@2 + return runInstall({linkDuplicates: true}, 'hardlink-collision-3', async config => { + let a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/package.json')); + let d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/package.json')); + expect(a_1.ino).toEqual(d_1.ino); + a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/c/package.json')); + d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/node_modules/c/package.json')); + expect(a_1.ino).toEqual(d_1.ino); + }); }); -}); -test.concurrent('install should not hardlink full package structure', (): Promise => { - // https://github.com/yarnpkg/yarn/issues/2734 - // A@1 -> B@1 -> C@1 -> (bundle leftpad) - // -> C@2 - // B@2 - // C@3 - // D@1 -> B@1 (hardlink) -> C@1 (hardlink) -> (bundle leftpad) (hardlink) - // -> C@2 - return runInstall({linkDuplicates: true}, 'hardlink-collision-with-bundled', async config => { - let a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/package.json')); - let d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/package.json')); - expect(a_1.ino).toEqual(d_1.ino); - a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/node_modules/c/package.json')); - d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/node_modules/c/package.json')); - expect(a_1.ino).toEqual(d_1.ino); - a_1 = await fs.stat( - path.join(config.cwd, 'node_modules/a/node_modules/b/node_modules/c/node_modules/left-pad/package.json'), - ); - d_1 = await fs.stat( - path.join(config.cwd, 'node_modules/d/node_modules/b/node_modules/c/node_modules/left-pad/package.json'), - ); - expect(a_1.ino).toEqual(d_1.ino); + test.concurrent('install should not hardlink full package structure', (): Promise => { + // https://github.com/yarnpkg/yarn/issues/2734 + // A@1 -> B@1 -> C@1 -> (bundle leftpad) + // -> C@2 + // B@2 + // C@3 + // D@1 -> B@1 (hardlink) -> C@1 (hardlink) -> (bundle leftpad) (hardlink) + // -> C@2 + return runInstall({linkDuplicates: true}, 'hardlink-collision-with-bundled', async config => { + let a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/package.json')); + let d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/package.json')); + expect(a_1.ino).toEqual(d_1.ino); + a_1 = await fs.stat(path.join(config.cwd, 'node_modules/a/node_modules/b/node_modules/c/package.json')); + d_1 = await fs.stat(path.join(config.cwd, 'node_modules/d/node_modules/b/node_modules/c/package.json')); + expect(a_1.ino).toEqual(d_1.ino); + a_1 = await fs.stat( + path.join(config.cwd, 'node_modules/a/node_modules/b/node_modules/c/node_modules/left-pad/package.json'), + ); + d_1 = await fs.stat( + path.join(config.cwd, 'node_modules/d/node_modules/b/node_modules/c/node_modules/left-pad/package.json'), + ); + expect(a_1.ino).toEqual(d_1.ino); + }); + }); + + describe('in workspaces', () => { + // https://github.com/yarnpkg/yarn/issues/5421 + // test hardlink can work within workspaces containing 1) scoped 2) linked dependency + // 3) internal workspace reference + // M -> w1, w2 + // w1 -> a-1, c-1(link), w2 + // w2 -> b-1, c-1(link) + // a-1 + // b-1 -> a-1 + // c-1 + test.concurrent('no hardlink for workspace references and linked dependencies', (): Promise => { + // when no conflict, everything should be hoisted to the top without exception + return runInstall( + {linkDuplicates: true, workspacesNohoistEnabled: false}, + 'hardlink-scoped-workspaces', + async config => { + await Promise.all( + //verify the expected modules created under root node_modules + [['a'], ['b'], ['c'], ['@sub', 'w1'], ['@sub', 'w2']].map(async m => { + const p = path.join(config.cwd, 'node_modules', ...m, 'package.json'); + const existed = await fs.exists(p); + expect(existed).toEqual(true); + }), + ); + //verify the c, w1 and w2 are created as symlink + await Promise.all( + [['c'], ['@sub', 'w1'], ['@sub', 'w2']].map(async m => { + const p = path.join(config.cwd, 'node_modules', ...m); + const f = await fs.lstat(p); + expect(f.isSymbolicLink()).toEqual(true); + + //we can follow the link to find the package.json + const existed = await fs.exists(path.join(p, 'package.json')); + expect(existed).toEqual(true); + }), + ); + + //verify there is nothing under any workspaces' node_modules + await Promise.all( + [['packages', 'w1'], ['packages', 'w2']].map(async m => { + const existed = await fs.exists(path.join(config.cwd, ...m, 'node_modules')); + expect(existed).toEqual(false); + }), + ); + }, + ); + }); + test.concurrent('should work with nohoist', (): Promise => { + // https://github.com/yarnpkg/yarn/issues/5421 + // nohoist everything should put all dependencies under each workspaces's + // local node_modules and hardlink accordingly + return runInstall( + {linkDuplicates: true, workspacesNohoistEnabled: true}, + 'hardlink-scoped-workspaces', + async config => { + //verify the expected modules created under root node_modules + await Promise.all( + [['@sub', 'w1'], ['@sub', 'w2']].map(async m => { + const p = path.join(config.cwd, 'node_modules', ...m); + const f = await fs.lstat(p); + expect(f.isSymbolicLink()).toEqual(true); + }), + ); + + //verify w1 and w2 node_modules hardlinked modules: a + const a1 = await fs.stat(path.join(config.cwd, 'packages', 'w1', 'node_modules', 'a', 'package.json')); + const a2 = await fs.stat(path.join(config.cwd, 'packages', 'w2', 'node_modules', 'a', 'package.json')); + expect(a1.ino).toEqual(a2.ino); + + //verify b is created under w2 but without anything under its modules + let existed = await fs.exists(path.join(config.cwd, 'packages', 'w2', 'node_modules', 'b', 'package.json')); + expect(existed).toEqual(true); + existed = await fs.exists(path.join(config.cwd, 'packages', 'w2', 'node_modules', 'b', 'node_modules')); + expect(existed).toEqual(false); + + //verify the c, w1 are created as symlink under the workspaces + await Promise.all( + [ + ['packages', 'w1', 'node_modules', 'c'], + ['packages', 'w2', 'node_modules', 'c'], + ['packages', 'w1', 'node_modules', '@sub', 'w2'], + ].map(async m => { + const p = path.join(config.cwd, ...m); + const f = await fs.lstat(p); + expect(f.isSymbolicLink()).toEqual(true); + + //we can follow the link to find the package.json + const existed = await fs.exists(path.join(p, 'package.json')); + expect(existed).toEqual(true); + }), + ); + }, + ); + }); }); }); diff --git a/__tests__/fixtures/install/hardlink-scoped-workspaces/deps/a-1/package.json b/__tests__/fixtures/install/hardlink-scoped-workspaces/deps/a-1/package.json new file mode 100644 index 0000000000..9113c2528e --- /dev/null +++ b/__tests__/fixtures/install/hardlink-scoped-workspaces/deps/a-1/package.json @@ -0,0 +1,4 @@ +{ + "name": "a", + "version": "1.0.0" +} diff --git a/__tests__/fixtures/install/hardlink-scoped-workspaces/deps/b-1/package.json b/__tests__/fixtures/install/hardlink-scoped-workspaces/deps/b-1/package.json new file mode 100644 index 0000000000..f7dcf4ac07 --- /dev/null +++ b/__tests__/fixtures/install/hardlink-scoped-workspaces/deps/b-1/package.json @@ -0,0 +1,7 @@ +{ + "name": "b", + "version": "1.0.0", + "dependencies": { + "a": "file:../a-1" + } +} diff --git a/__tests__/fixtures/install/hardlink-scoped-workspaces/deps/c-1/package.json b/__tests__/fixtures/install/hardlink-scoped-workspaces/deps/c-1/package.json new file mode 100644 index 0000000000..abd3384930 --- /dev/null +++ b/__tests__/fixtures/install/hardlink-scoped-workspaces/deps/c-1/package.json @@ -0,0 +1,4 @@ +{ + "name": "c", + "version": "1.0.0" +} diff --git a/__tests__/fixtures/install/hardlink-scoped-workspaces/package.json b/__tests__/fixtures/install/hardlink-scoped-workspaces/package.json new file mode 100644 index 0000000000..d6eb145c1b --- /dev/null +++ b/__tests__/fixtures/install/hardlink-scoped-workspaces/package.json @@ -0,0 +1,9 @@ +{ + "name": "hardlink-workspaces", + "version": "1.0.0", + "private": true, + "workspaces": { + "packages": ["packages/*"], + "nohoist": ["**"] + } +} diff --git a/__tests__/fixtures/install/hardlink-scoped-workspaces/packages/w1/package.json b/__tests__/fixtures/install/hardlink-scoped-workspaces/packages/w1/package.json new file mode 100644 index 0000000000..ceb24282b5 --- /dev/null +++ b/__tests__/fixtures/install/hardlink-scoped-workspaces/packages/w1/package.json @@ -0,0 +1,9 @@ +{ + "name": "@sub/w1", + "version": "1.0.0", + "dependencies": { + "a": "file:../../deps/a-1", + "c": "link:../../deps/c-1", + "@sub/w2": "*" + } +} diff --git a/__tests__/fixtures/install/hardlink-scoped-workspaces/packages/w2/package.json b/__tests__/fixtures/install/hardlink-scoped-workspaces/packages/w2/package.json new file mode 100644 index 0000000000..456a9c46b0 --- /dev/null +++ b/__tests__/fixtures/install/hardlink-scoped-workspaces/packages/w2/package.json @@ -0,0 +1,8 @@ +{ + "name": "@sub/w2", + "version": "1.0.0", + "dependencies": { + "b": "file:../../deps/b-1", + "c": "link:../../deps/c-1" + } +} diff --git a/src/config.js b/src/config.js index a37eaee047..12646e8a6a 100644 --- a/src/config.js +++ b/src/config.js @@ -713,10 +713,11 @@ export default class Config { // validate eligibility let wsCopy = {...ws}; const warnings: Array = []; + const errors: Array = []; // packages if (wsCopy.packages && wsCopy.packages.length > 0 && !manifest.private) { - warnings.push(this.reporter.lang('workspacesRequirePrivateProjects')); + errors.push(this.reporter.lang('workspacesRequirePrivateProjects')); wsCopy = undefined; } // nohoist @@ -725,19 +726,20 @@ export default class Config { warnings.push(this.reporter.lang('workspacesNohoistDisabled', manifest.name)); wsCopy.nohoist = undefined; } else if (!manifest.private) { - warnings.push(this.reporter.lang('workspacesNohoistRequirePrivatePackages', manifest.name)); + errors.push(this.reporter.lang('workspacesNohoistRequirePrivatePackages', manifest.name)); wsCopy.nohoist = undefined; } } - if (warnings.length > 0) { - const msg = warnings.join('\n'); - if (shouldThrow) { - throw new MessageError(msg); - } else { - this.reporter.warn(msg); - } + if (errors.length > 0 && shouldThrow) { + throw new MessageError(errors.join('\n')); } + + const msg = errors.concat(warnings).join('\n'); + if (msg.length > 0) { + this.reporter.warn(msg); + } + return wsCopy; } diff --git a/src/package-linker.js b/src/package-linker.js index 34269f1bd5..83a6ca8a78 100644 --- a/src/package-linker.js +++ b/src/package-linker.js @@ -218,7 +218,8 @@ export default class PackageLinker { const copiedDest = copiedSrcs.get(src); if (!copiedDest) { - if (hardlinksEnabled) { + // no point to hardlink to a symlink + if (hardlinksEnabled && type !== 'symlink') { copiedSrcs.set(src, dest); } copyQueue.set(dest, { diff --git a/src/reporters/lang/en.js b/src/reporters/lang/en.js index aa89ceffba..d11fd1e9ec 100644 --- a/src/reporters/lang/en.js +++ b/src/reporters/lang/en.js @@ -190,7 +190,7 @@ const messages = { workspacesNohoistRequirePrivatePackages: 'nohoist config is ignored in $0 because it is not a private package. If you think nohoist should be allowed in public packages, please submit an issue for your use case.', - workspacesNohoistDisabled: `$0 defines nohoist but the feature is disabled in your Yarn config. Please check "workspaces-nohoist-experimental" in your .yarnrc file.`, + workspacesNohoistDisabled: `$0 defines nohoist but the feature is disabled in your Yarn config ("workspaces-nohoist-experimental" in .yarnrc file)`, workspaceRootNotFound: "Cannot find the root of your workspace - are you sure you're currently in a workspace?", workspaceMissingWorkspace: 'Missing workspace name.',