Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--link-duplicates flag. Creates hardlinks to duplicate files in node_modules #2620

Merged
merged 7 commits into from
Feb 9, 2017
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions __tests__/commands/install/integration-deduping.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
/* @flow */

import {getPackageVersion, runInstall} from '../_helpers.js';
import * as fs from '../../../src/util/fs.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need to do * as here?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean I could only import stat instead of * as fs?
Not sure if that would be better when the number of imports from fs grow.
Everything grouped into one import makes sense rather than a lot functions that need sorting and formatting.
What should be the guideline here?


const assert = require('assert');
const path = require('path');

jasmine.DEFAULT_TIMEOUT_INTERVAL = 120000;

Expand Down Expand Up @@ -201,3 +203,37 @@ test.concurrent('install should dedupe dependencies avoiding conflicts 9', (): P
assert.equal(await getPackageVersion(config, 'run-async'), '0.1.0');
});
});

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(path.join(
config.cwd,
'node_modules/b/node_modules/a/package.json',
));
const c_a = await fs.stat(path.join(
config.cwd,
'node_modules/c/node_modules/a/package.json',
));
assert.equal(b_a.ino, 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(path.join(
config.cwd,
'node_modules/b/node_modules/a/package.json',
));
const c_a = await fs.stat(path.join(
config.cwd,
'node_modules/c/node_modules/a/package.json',
));
assert(b_a.ino != c_a.ino);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "a",
"version": "1.0.0"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "a",
"version": "2.0.0"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "b",
"version": "1.0.0",
"dependencies": {
"a": "file:../a-2"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "c",
"version": "1.0.0",
"dependencies": {
"a": "file:../a-2"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"dependencies": {
"a": "file:deps/a-1",
"b": "file:deps/b-1",
"c": "file:deps/c-1"
}
}
6 changes: 4 additions & 2 deletions src/cli/commands/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,12 @@ export type IntegrityMatch = {

type Flags = {
// install
har: boolean,
ignorePlatform: boolean,
ignoreEngines: boolean,
ignoreScripts: boolean,
ignoreOptional: boolean,
har: boolean,
linkDuplicates: boolean,
force: boolean,
flat: boolean,
lockfile: boolean,
Expand Down Expand Up @@ -128,6 +129,7 @@ function normalizeFlags(config: Config, rawFlags: Object): Flags {
pureLockfile: !!rawFlags.pureLockfile,
skipIntegrity: !!rawFlags.skipIntegrity,
frozenLockfile: !!rawFlags.frozenLockfile,
linkDuplicates: !!rawFlags.linkDuplicates,

// add
peer: !!rawFlags.peer,
Expand Down Expand Up @@ -397,7 +399,7 @@ export class Install {
const loc = await this.getIntegrityHashLocation();
await fs.unlink(loc);
this.reporter.step(curr, total, this.reporter.lang('linkingDependencies'), emoji.get('link'));
await this.linker.init(patterns);
await this.linker.init(patterns, this.flags.linkDuplicates);
});

steps.push(async (curr: number, total: number) => {
Expand Down
1 change: 1 addition & 0 deletions src/cli/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ commander.option('--prod, --production [prod]', '');
commander.option('--no-lockfile', "don't read or generate a lockfile");
commander.option('--pure-lockfile', "don't generate a lockfile");
commander.option('--frozen-lockfile', "don't generate a lockfile and fail if an update is needed");
commander.option('--link-duplicates', 'create hardlinks to the repeated modules in node_modules');
commander.option('--global-folder <path>', '');
commander.option(
'--modules-folder <path>',
Expand Down
79 changes: 60 additions & 19 deletions src/package-linker.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ export default class PackageLinker {
return Promise.resolve(hoister.init());
}

async copyModules(patterns: Array<string>): Promise<void> {
async copyModules(patterns: Array<string>, linkDuplicates: boolean): Promise<void> {
let flatTree = await this.getFlatHoistedTree(patterns);

// sorted tree makes file creation and copying not to interfere with each other
Expand All @@ -121,10 +121,13 @@ export default class PackageLinker {
});

// list of artifacts in modules to remove from extraneous removal
const phantomFiles = [];
const artifactFiles = [];

//
const queue: Map<string, CopyQueueItem> = new Map();
const copyQueue: Map<string, CopyQueueItem> = new Map();
const hardlinkQueue: Map<string, CopyQueueItem> = new Map();
const hardlinksEnabled = linkDuplicates && await fs.hardlinksWork(this.config.cwd);

const copiedSrcs: Map<string, string> = new Map();
for (const [dest, {pkg, loc: src}] of flatTree) {
const ref = pkg._reference;
invariant(ref, 'expected package reference');
Expand All @@ -134,18 +137,34 @@ export default class PackageLinker {
// extraneous
const metadata = await this.config.readPackageMetadata(src);
for (const file of metadata.artifacts) {
phantomFiles.push(path.join(dest, file));
artifactFiles.push(path.join(dest, file));
}

queue.set(dest, {
src,
dest,
onFresh() {
if (ref) {
ref.setFresh(true);
}
},
});
const copiedDest = copiedSrcs.get(src);
if (!copiedDest) {
if (hardlinksEnabled) {
copiedSrcs.set(src, dest);
}
copyQueue.set(dest, {
src,
dest,
onFresh() {
if (ref) {
ref.setFresh(true);
}
},
});
} else {
hardlinkQueue.set(dest, {
src: copiedDest,
dest,
onFresh() {
if (ref) {
ref.setFresh(true);
}
},
});
}
}

// keep track of all scoped paths to remove empty scopes after copy
Expand Down Expand Up @@ -179,15 +198,15 @@ export default class PackageLinker {
const stat = await fs.lstat(loc);
if (stat.isSymbolicLink()) {
possibleExtraneous.delete(loc);
queue.delete(loc);
copyQueue.delete(loc);
}
}

//
let tick;
await fs.copyBulk(Array.from(queue.values()), this.reporter, {
await fs.copyBulk(Array.from(copyQueue.values()), this.reporter, {
possibleExtraneous,
phantomFiles,
artifactFiles,

ignoreBasenames: [
constants.METADATA_FILENAME,
Expand All @@ -204,6 +223,28 @@ export default class PackageLinker {
}
},
});
await fs.hardlinkBulk(Array.from(hardlinkQueue.values()), this.reporter, {
possibleExtraneous,
artifactFiles,

onStart: (num: number) => {
tick = this.reporter.progress(num);
},

onProgress(src: string) {
if (tick) {
tick(src);
}
},
});

// remove all extraneous files that weren't in the tree
for (const loc of possibleExtraneous) {
this.reporter.verbose(
this.reporter.lang('verboseFileRemoveExtraneous', loc),
);
await fs.unlink(loc);
}

// remove any empty scoped directories
for (const scopedPath of scopedPaths) {
Expand Down Expand Up @@ -287,9 +328,9 @@ export default class PackageLinker {
return range === '*' || semver.satisfies(version, range, this.config.looseSemver);
}

async init(patterns: Array<string>): Promise<void> {
async init(patterns: Array<string>, linkDuplicates: boolean): Promise<void> {
this.resolvePeerModules();
await this.copyModules(patterns);
await this.copyModules(patterns, linkDuplicates);
await this.saveAll(patterns);
}

Expand Down
2 changes: 2 additions & 0 deletions src/reporters/lang/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,11 @@ const messages = {
manifestDirectoryNotFound: 'Unable to read $0 directory of module $1',

verboseFileCopy: 'Copying $0 to $1.',
verboseFileLink: 'Creating hardlink at $0 to $1.',
verboseFileSymlink: 'Creating symlink at $0 to $1.',
verboseFileSkip: 'Skipping copying of file $0 as the file at $1 is the same size ($2) and mtime ($3).',
verboseFileSkipSymlink: 'Skipping copying of $0 as the file at $1 is the same symlink ($2).',
verboseFileSkipHardlink: 'Skipping copying of $0 as the file at $1 is the same hardlink ($2).',
verboseFileRemoveExtraneous: 'Removing extraneous file $0.',
verboseFilePhantomExtraneous: "File $0 would be marked as extraneous but has been removed as it's listed as a phantom file.",
verboseFileFolder: 'Creating directory $0.',
Expand Down
Loading