Skip to content

Commit

Permalink
fixed linkDuplicates for symlinks (#5442)
Browse files Browse the repository at this point in the history
  • Loading branch information
@connectdotz @bestander
connectdotz authored and bestander committed Mar 8, 2018
1 parent 95b3dfe commit 5ef113f
Show file tree
Hide file tree
Showing 11 changed files with 248 additions and 103 deletions.
3 changes: 3 additions & 0 deletions __tests__/commands/_helpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,9 @@ export async function run<T, R>(

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) {
Expand Down
282 changes: 190 additions & 92 deletions __tests__/commands/install/integration-deduping.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,107 +199,205 @@ test.concurrent('install should dedupe dependencies avoiding conflicts 9', (): P
});
});

test.concurrent('install should hardlink repeated dependencies', (): Promise<void> => {
// 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<void> => {
// 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<void> => {
// 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<void> => {
// 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<void> => {
// 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<void> => {
// 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<void> => {
// 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<void> => {
// 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<void> => {
// 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<void> => {
// 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<void> => {
// 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<void> => {
// 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<void> => {
// 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<void> => {
// 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);
}),
);
},
);
});
});
});
4 changes: 4 additions & 0 deletions __tests__/fixtures/install/hardlink-scoped-workspaces/deps/a-1/package.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "a",
"version": "1.0.0"
}
7 changes: 7 additions & 0 deletions __tests__/fixtures/install/hardlink-scoped-workspaces/deps/b-1/package.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "b",
"version": "1.0.0",
"dependencies": {
"a": "file:../a-1"
}
}
4 changes: 4 additions & 0 deletions __tests__/fixtures/install/hardlink-scoped-workspaces/deps/c-1/package.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "c",
"version": "1.0.0"
}
9 changes: 9 additions & 0 deletions __tests__/fixtures/install/hardlink-scoped-workspaces/package.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "hardlink-workspaces",
"version": "1.0.0",
"private": true,
"workspaces": {
"packages": ["packages/*"],
"nohoist": ["**"]
}
}
9 changes: 9 additions & 0 deletions __tests__/fixtures/install/hardlink-scoped-workspaces/packages/w1/package.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "@sub/w1",
"version": "1.0.0",
"dependencies": {
"a": "file:../../deps/a-1",
"c": "link:../../deps/c-1",
"@sub/w2": "*"
}
}
8 changes: 8 additions & 0 deletions __tests__/fixtures/install/hardlink-scoped-workspaces/packages/w2/package.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "@sub/w2",
"version": "1.0.0",
"dependencies": {
"b": "file:../../deps/b-1",
"c": "link:../../deps/c-1"
}
}
20 changes: 11 additions & 9 deletions src/config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -713,10 +713,11 @@ export default class Config {
// validate eligibility
let wsCopy = {...ws};
const warnings: Array<string> = [];
const errors: Array<string> = [];

// 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
Expand All @@ -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;
}

Expand Down

0 comments on commit 5ef113f

Please sign in to comment.