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

@@ -21,10 +21,11 @@ export function runInstall(
   name: string,
   checkInstalled?: ?(config: Config, reporter: Reporter, install: Install) => ?Promise<void>,
   beforeInstall?: ?(cwd: string) => ?Promise<void>,
+  cleanupAfterInstall: boolean = true,
 ): Promise<void> {
   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<Lockfile> {
@@ -54,6 +55,7 @@ export async function run(
   dir: string,
   checkInstalled: ?(config: Config, reporter: Reporter, install: Install) => ?Promise<void>,
   beforeInstall: ?(cwd: string) => ?Promise<void>,
+  cleanupAfterInstall: boolean,
 ): Promise<void> {
   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}`);

__tests__/commands/add.js

@@ -26,10 +26,11 @@ function runAdd(
   name: string,
   checkInstalled?: ?(config: Config, reporter: Reporter) => ?Promise<void>,
   beforeInstall?: ?(cwd: string) => ?Promise<void>,
+  cleanupAfterInstall: boolean = true,
 ): Promise<void> {
   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<void> => {

__tests__/commands/install.js

@@ -553,6 +553,7 @@ test.concurrent('install should resolve circular dependencies 2', (): Promise<vo
   });
 });
 
+
 test('install should respect NODE_ENV=production', (): Promise<void> => {
   const env = process.env.NODE_ENV;
   process.env.NODE_ENV = 'production';
@@ -563,6 +564,33 @@ test('install should respect NODE_ENV=production', (): Promise<void> => {
   });
 });
 
+
+test.concurrent('install should resolve circular dependencies 2', (): Promise<void> => {
+  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<void> => {
+  // 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<void> => {

__tests__/fixtures/install/install-should-be-idempotent/.npmrc

@@ -0,0 +1 @@
+yarn-offline-mirror=./mirror-for-offline

__tests__/fixtures/install/install-should-be-idempotent/package.json

@@ -0,0 +1,5 @@
+{
+  "dependencies": {
+    "dep-a": "1.0.0"
+  }
+}

__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
+[email protected]:
+  version "1.0.0"
+  resolved dep-a-1.0.0.tgz#92820a28fbb001f840326a074a4047d828fe8a8d

__tests__/fixtures/install/scripts-order/yarn.lock

@@ -5,13 +5,11 @@ [email protected]:
   resolved dep-a-1.0.0.tgz
   dependencies:
     dep-b "1.0.0"
-
 [email protected]:
   version "1.0.0"
   resolved dep-b-1.0.0.tgz
   dependencies:
     dep-c "1.0.0"
-
 [email protected]:
   version "1.0.0"
   resolved dep-c-1.0.0.tgz

__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
 

src/config.js

@@ -376,6 +376,7 @@ export default class Config {
           return file;
         }
       }
+      return null;
     });
   }
 

src/fetchers/base-fetcher.js

@@ -59,6 +59,7 @@ export default class BaseFetcher {
         hash,
         dest,
         package: pkg,
+        cached: false,
       };
     });
   }

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) {

src/package-install-scripts.js

@@ -56,14 +56,12 @@ export default class PackageInstallScripts {
     return mtimes;
   }
 
-  async wrapCopyBuildArtifacts<T>(
+  async copyBuildArtifacts(
     loc: string,
     pkg: Manifest,
+    beforeFiles: Map<string, number>,
     spinner: ReporterSetSpinner,
-    factory: () => Promise<T>,
-  ): Promise<T> {
-    const beforeFiles = await this.walk(loc);
-    const res = await factory();
+  ): Promise<void> {
     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<void> {
-    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<void> => {
-          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();
   }
 }

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<string>;
   patterns: Array<string>;

src/types.js

@@ -130,6 +130,7 @@ export type FetchedMetadata = {
   resolved: ?string,
   hash: string,
   dest: string,
+  cached: boolean,
 };
 export type FetchedOverride = {
   hash: string,