From 5e2fbe6d1d86d2f1420b66c6351f765300ddde79 Mon Sep 17 00:00:00 2001 From: Yunxing Dai Date: Wed, 26 Oct 2016 17:40:32 -0700 Subject: [PATCH] Don't re-install cached artifacts. (#805) This patch makes sure if we load a package from cache, we don't install it again. --- __tests__/commands/_install.js | 8 ++- __tests__/commands/add.js | 3 +- __tests__/commands/install.js | 28 ++++++++++ .../install-should-be-idempotent/.npmrc | 1 + .../mirror-for-offline/dep-a-1.0.0.tgz | Bin 0 -> 544 bytes .../install-should-be-idempotent/package.json | 5 ++ .../install-should-be-idempotent/yarn.lock | 5 ++ .../fixtures/install/scripts-order/yarn.lock | 2 - .../fixtures/request-cache/GET/localhost/.bin | 2 +- src/config.js | 1 + src/fetchers/base-fetcher.js | 1 + src/package-fetcher.js | 5 ++ src/package-install-scripts.js | 52 +++++++++++------- src/package-reference.js | 4 +- src/types.js | 1 + 15 files changed, 92 insertions(+), 26 deletions(-) create mode 100644 __tests__/fixtures/install/install-should-be-idempotent/.npmrc create mode 100644 __tests__/fixtures/install/install-should-be-idempotent/mirror-for-offline/dep-a-1.0.0.tgz create mode 100644 __tests__/fixtures/install/install-should-be-idempotent/package.json create mode 100644 __tests__/fixtures/install/install-should-be-idempotent/yarn.lock diff --git a/__tests__/commands/_install.js b/__tests__/commands/_install.js index 3ae383c1e1..ff38b974e8 100644 --- a/__tests__/commands/_install.js +++ b/__tests__/commands/_install.js @@ -21,10 +21,11 @@ export function runInstall( name: string, checkInstalled?: ?(config: Config, reporter: Reporter, install: Install) => ?Promise, beforeInstall?: ?(cwd: string) => ?Promise, + cleanupAfterInstall: boolean = true, ): Promise { return run((config, reporter, lockfile): Install => { return new Install(flags, config, reporter, lockfile); - }, path.join(fixturesLoc, name), checkInstalled, beforeInstall); + }, path.join(fixturesLoc, name), checkInstalled, beforeInstall, cleanupAfterInstall); } export async function createLockfile(dir: string): Promise { @@ -54,6 +55,7 @@ export async function run( dir: string, checkInstalled: ?(config: Config, reporter: Reporter, install: Install) => ?Promise, beforeInstall: ?(cwd: string) => ?Promise, + cleanupAfterInstall: boolean, ): Promise { const cwd = path.join( os.tmpdir(), @@ -110,7 +112,9 @@ export async function run( } } finally { // clean up - await fs.unlink(cwd); + if (cleanupAfterInstall) { + await fs.unlink(cwd); + } } } catch (err) { throw new Error(`${err && err.stack} \nConsole output:\n ${out}`); diff --git a/__tests__/commands/add.js b/__tests__/commands/add.js index 1e02cbca90..82a7f60670 100644 --- a/__tests__/commands/add.js +++ b/__tests__/commands/add.js @@ -26,10 +26,11 @@ function runAdd( name: string, checkInstalled?: ?(config: Config, reporter: Reporter) => ?Promise, beforeInstall?: ?(cwd: string) => ?Promise, + cleanupAfterInstall: boolean = true, ): Promise { return buildRun((config, reporter, lockfile): Install => { return new Add(args, flags, config, reporter, lockfile); - }, path.join(fixturesLoc, name), checkInstalled, beforeInstall); + }, path.join(fixturesLoc, name), checkInstalled, beforeInstall, cleanupAfterInstall); } test.concurrent('install with arg that has install scripts', (): Promise => { diff --git a/__tests__/commands/install.js b/__tests__/commands/install.js index 0c6c6d8bfa..358c4180d9 100644 --- a/__tests__/commands/install.js +++ b/__tests__/commands/install.js @@ -553,6 +553,7 @@ test.concurrent('install should resolve circular dependencies 2', (): Promise => { const env = process.env.NODE_ENV; process.env.NODE_ENV = 'production'; @@ -563,6 +564,33 @@ test('install should respect NODE_ENV=production', (): Promise => { }); }); + +test.concurrent('install should resolve circular dependencies 2', (): Promise => { + return runInstall({}, 'install-should-circumvent-circular-dependencies-2', async (config, reporter) => { + assert.equal( + await getPackageVersion(config, 'es5-ext'), + '0.10.12', + ); + }); +}); + +test.concurrent('install should be idempotent', (): Promise => { + // Install a package twice + runInstall({}, 'install-should-be-idempotent', async (config, reporter) => { + assert.equal( + await getPackageVersion(config, 'dep-a'), + '1.0.0', + ); + }, null, false); + + return runInstall({}, 'install-should-be-idempotent', async (config, reporter) => { + assert.equal( + await getPackageVersion(config, 'dep-a'), + '1.0.0', + ); + }); +}); + test.concurrent( 'install should add missing deps to yarn and mirror (PR import scenario)', (): Promise => { diff --git a/__tests__/fixtures/install/install-should-be-idempotent/.npmrc b/__tests__/fixtures/install/install-should-be-idempotent/.npmrc new file mode 100644 index 0000000000..9465b97ac3 --- /dev/null +++ b/__tests__/fixtures/install/install-should-be-idempotent/.npmrc @@ -0,0 +1 @@ +yarn-offline-mirror=./mirror-for-offline diff --git a/__tests__/fixtures/install/install-should-be-idempotent/mirror-for-offline/dep-a-1.0.0.tgz b/__tests__/fixtures/install/install-should-be-idempotent/mirror-for-offline/dep-a-1.0.0.tgz new file mode 100644 index 0000000000000000000000000000000000000000..0b4b5cf6db62f201d402e4e938438cd4d4def981 GIT binary patch literal 544 zcmV+*0^j`~iwFP!000001MQSgZqzUo#%JwQSdqG;!OzL$50V$CfLaLwqHGXi;ZEGC zNyd)iOgjZt;z(S8UZNM_2-wLa6Ep)7FbE;?ZJd1Xd$yA}zZZOUEO;iW`z4>{d@j1t ze`mCs>_vo-C=BuBqrLtV9NfC`i@rM~jQ}X%3DDSTN~Tko5Qr#5c(U5h@;}zn)V#7= z{M~ulhr#{$j|fuy?D?a|uO7biGf~zT3=j=_>s%LwzGCORFaiZp@5{N;m3SfxVSK`T zRW4fRdl_KUyUusiJWzS7futlE93K#MW(RizYFY60k*~MV1RLGf7 zMzE99BSceNa{KCc{eg&Xdh6>_F!!<$4e;sF@yXNI_Mt8rME2LA6c8kxtqQ4f`z;~H zOPR4`ngBo)5*L|U1ClrbAaR@wKlfRY8~I+akfhOI4XL!9A&$Z{v@=rNWFV!8l0mDU z<%T8bZy)GUKczoM{f&nFKUnksbI*SgI9z_q{~!*h7Nz^%$S(co{QrP3_8eHf2iR-a z>fLkJnx@WefZDnPgt|&=I&QkV4xO#LhNUgIC9E-^rc3wW^lbU2Y01U)m98?^^X6^i iv;f^KwWg*sEI2)MGva2{s8OSSQC|TO!z4oh4gdfh;t^8- literal 0 HcmV?d00001 diff --git a/__tests__/fixtures/install/install-should-be-idempotent/package.json b/__tests__/fixtures/install/install-should-be-idempotent/package.json new file mode 100644 index 0000000000..c0d4b7cf22 --- /dev/null +++ b/__tests__/fixtures/install/install-should-be-idempotent/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "dep-a": "1.0.0" + } +} diff --git a/__tests__/fixtures/install/install-should-be-idempotent/yarn.lock b/__tests__/fixtures/install/install-should-be-idempotent/yarn.lock new file mode 100644 index 0000000000..9ad95888b0 --- /dev/null +++ b/__tests__/fixtures/install/install-should-be-idempotent/yarn.lock @@ -0,0 +1,5 @@ +# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. +# yarn lockfile v1 +dep-a@1.0.0: + version "1.0.0" + resolved dep-a-1.0.0.tgz#92820a28fbb001f840326a074a4047d828fe8a8d diff --git a/__tests__/fixtures/install/scripts-order/yarn.lock b/__tests__/fixtures/install/scripts-order/yarn.lock index 4957af4416..4bf4f0cdb1 100644 --- a/__tests__/fixtures/install/scripts-order/yarn.lock +++ b/__tests__/fixtures/install/scripts-order/yarn.lock @@ -5,13 +5,11 @@ dep-a@1.0.0: resolved dep-a-1.0.0.tgz dependencies: dep-b "1.0.0" - dep-b@1.0.0: version "1.0.0" resolved dep-b-1.0.0.tgz dependencies: dep-c "1.0.0" - dep-c@1.0.0: version "1.0.0" resolved dep-c-1.0.0.tgz diff --git a/__tests__/fixtures/request-cache/GET/localhost/.bin b/__tests__/fixtures/request-cache/GET/localhost/.bin index 93408c1335..fbe8f9ebf4 100644 --- a/__tests__/fixtures/request-cache/GET/localhost/.bin +++ b/__tests__/fixtures/request-cache/GET/localhost/.bin @@ -1,5 +1,5 @@ HTTP/1.1 200 OK -Date: Mon, 24 Oct 2016 13:03:20 GMT +Date: Wed, 26 Oct 2016 22:01:42 GMT Connection: close Content-Length: 2 diff --git a/src/config.js b/src/config.js index 9307be8ccd..fc10ba6ed0 100644 --- a/src/config.js +++ b/src/config.js @@ -376,6 +376,7 @@ export default class Config { return file; } } + return null; }); } diff --git a/src/fetchers/base-fetcher.js b/src/fetchers/base-fetcher.js index ddee7fab8b..893fe42624 100644 --- a/src/fetchers/base-fetcher.js +++ b/src/fetchers/base-fetcher.js @@ -59,6 +59,7 @@ export default class BaseFetcher { hash, dest, package: pkg, + cached: false, }; }); } diff --git a/src/package-fetcher.js b/src/package-fetcher.js index 828fdf1299..443efd101d 100644 --- a/src/package-fetcher.js +++ b/src/package-fetcher.js @@ -29,6 +29,7 @@ export default class PackageFetcher { resolved: await fetcher.getResolvedFromCached(hash), hash, dest, + cached: true, }; } @@ -91,6 +92,10 @@ export default class PackageFetcher { if (res.resolved) { ref.remote.resolved = res.resolved; } + + if (res.cached) { + ref.cached = true; + } } if (newPkg) { diff --git a/src/package-install-scripts.js b/src/package-install-scripts.js index addb235dd8..f8c658f4fa 100644 --- a/src/package-install-scripts.js +++ b/src/package-install-scripts.js @@ -56,14 +56,12 @@ export default class PackageInstallScripts { return mtimes; } - async wrapCopyBuildArtifacts( + async copyBuildArtifacts( loc: string, pkg: Manifest, + beforeFiles: Map, spinner: ReporterSetSpinner, - factory: () => Promise, - ): Promise { - const beforeFiles = await this.walk(loc); - const res = await factory(); + ): Promise { const afterFiles = await this.walk(loc); // work out what files have been created/modified @@ -84,7 +82,7 @@ export default class PackageInstallScripts { if (!removedFiles.length && !buildArtifacts.length) { // nothing else to do here since we have no build side effects - return res; + return; } // if the process is killed while copying over build artifacts then we'll leave @@ -119,23 +117,21 @@ export default class PackageInstallScripts { }); await fs.writeFile(cachedMetadataLoc, cachedMetadata); } - - return res; } async install(cmds: Array<[string, string]>, pkg: Manifest, spinner: ReporterSetSpinner): Promise { - const loc = this.config.generateHardModulePath(pkg._reference); + const ref = pkg._reference; + invariant(ref, 'expected reference'); + const loc = this.config.generateHardModulePath(ref); + if (ref.cached) { + // This package is fetched directly from installed cache with build artifacts + // No need to rebuild + return; + } try { - await this.wrapCopyBuildArtifacts( - loc, - pkg, - spinner, - async (): Promise => { - for (const [stage, cmd] of cmds) { - await executeLifecycleScript(stage, this.config, loc, cmd, spinner); - } - }, - ); + for (const [stage, cmd] of cmds) { + await executeLifecycleScript(stage, this.config, loc, cmd, spinner); + } } catch (err) { err.message = `${loc}: ${err.message}`; @@ -269,8 +265,14 @@ export default class PackageInstallScripts { const installed = new Set(); const pkgs = this.resolver.getTopologicalManifests(seedPatterns); let installablePkgs = 0; + // A map to keep track of what files exist before installation + const beforeFilesMap = new Map(); for (const pkg of pkgs) { if (this.packageCanBeInstalled(pkg)) { + const ref = pkg._reference; + invariant(ref, 'expected reference'); + const loc = this.config.generateHardModulePath(ref); + beforeFilesMap.set(loc, await this.walk(loc)); installablePkgs += 1; } workQueue.add(pkg); @@ -289,6 +291,18 @@ export default class PackageInstallScripts { await Promise.all(workers); + // cache all build artifacts + for (const pkg of pkgs) { + if (this.packageCanBeInstalled(pkg)) { + const ref = pkg._reference; + invariant(ref, 'expected reference'); + const loc = this.config.generateHardModulePath(ref); + const beforeFiles = beforeFilesMap.get(loc); + invariant(beforeFiles, 'files before installation should always be recorded'); + await this.copyBuildArtifacts(loc, pkg, beforeFiles, set.spinners[0]); + } + } + set.end(); } } diff --git a/src/package-reference.js b/src/package-reference.js index f0f78c9b7c..a57da06a4d 100644 --- a/src/package-reference.js +++ b/src/package-reference.js @@ -44,7 +44,7 @@ export default class PackageReference { this.ignore = false; this.fresh = false; this.location = null; - + this.cached = false; this.addRequest(request); } @@ -59,6 +59,8 @@ export default class PackageReference { optional: ?boolean; visibility: {[action: string]: number}; ignore: boolean; + // whether or not this package is fetched from cache + cached: boolean; fresh: boolean; dependencies: Array; patterns: Array; diff --git a/src/types.js b/src/types.js index e059fe23c1..af72401330 100644 --- a/src/types.js +++ b/src/types.js @@ -130,6 +130,7 @@ export type FetchedMetadata = { resolved: ?string, hash: string, dest: string, + cached: boolean, }; export type FetchedOverride = { hash: string,