When saving build artifacts, rather than copying, save list of files …

…to ignore (#1935)

* When saving build artifacts, rather than copying, save list of files to ignore

* fix lint and tests

* fix test fixture

__tests__/commands/install.js

@@ -20,6 +20,52 @@ const path = require('path');
 
 const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'install');
 
+test.concurrent('properly find and save build artifacts', async () => {
+  await runInstall({}, 'artifacts-finds-and-saves', async (config): Promise<void> => {
+    const cacheFolder = path.join(config.cacheFolder, 'npm-dummy-0.0.0');
+    assert.deepEqual(
+      (await fs.readJson(path.join(cacheFolder, constants.METADATA_FILENAME))).artifacts,
+      ['dummy', 'dummy/dummy.txt', 'dummy.txt'],
+    );
+
+    // retains artifact
+    const moduleFolder = path.join(config.cwd, 'node_modules', 'dummy');
+    assert.equal(await fs.readFile(path.join(moduleFolder, 'dummy.txt')), 'foobar');
+    assert.equal(await fs.readFile(path.join(moduleFolder, 'dummy', 'dummy.txt')), 'foobar');
+  });
+});
+
+test.concurrent("removes extraneous files that aren't in module or artifacts", async () => {
+  async function check(cwd: string): Promise<void> {
+    // retains artifact
+    const moduleFolder = path.join(cwd, 'node_modules', 'dummy');
+    assert.equal(await fs.readFile(path.join(moduleFolder, 'dummy.txt')), 'foobar');
+    assert.equal(await fs.readFile(path.join(moduleFolder, 'dummy', 'dummy.txt')), 'foobar');
+
+    // removes extraneous
+    assert.ok(!(await fs.exists(path.join(moduleFolder, 'dummy2.txt'))));
+  }
+
+  async function create(cwd: string): Promise<void> {
+    // create an extraneous file
+    const moduleFolder = path.join(cwd, 'node_modules', 'dummy');
+    await fs.mkdirp(moduleFolder);
+    await fs.writeFile(path.join(moduleFolder, 'dummy2.txt'), 'foobar');
+  }
+
+  await runInstall({}, 'artifacts-finds-and-saves', async (config): Promise<void> => {
+    await check(config.cwd);
+
+    await create(config.cwd);
+
+    // run install again
+    const install = new Install({force: true}, config, config.reporter, new Lockfile());
+    await install.init();
+
+    await check(config.cwd);
+  }, create);
+});
+
 test.concurrent('integrity hash respects flat and production flags', async () => {
   const cwd = path.join(fixturesLoc, 'noop');
   const reporter = new reporters.NoopReporter();
@@ -600,7 +646,7 @@ if (process.platform !== 'win32') {
     return runInstall({}, 'cache-symlinks', async (config, reporter) => {
       const symlink = path.resolve(config.cwd, 'node_modules', 'dep-a', 'link-index.js');
       expect(await fs.exists(symlink)).toBe(true);
-      await fs.unlink(path.resolve(config.cwd, 'node_modules'));
+      await fs.unlink(path.join(config.cwd, 'node_modules'));
 
       const lockfile = await createLockfile(config.cwd);
       const install = new Install({}, config, reporter, lockfile);

__tests__/fixtures/install/artifacts-finds-and-saves/dummy/install.js

@@ -0,0 +1,6 @@
+var fs = require('fs');
+fs.writeFileSync('dummy.txt', 'foobar');
+if (!fs.existsSync('dummy')) {
+  fs.mkdirSync('dummy');
+}
+fs.writeFileSync('dummy/dummy.txt', 'foobar');

__tests__/fixtures/install/artifacts-finds-and-saves/dummy/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "dummy",
+  "version": "0.0.0",
+  "scripts": {
+    "install": "node install.js"
+  }
+}

__tests__/fixtures/install/artifacts-finds-and-saves/package.json

@@ -0,0 +1,5 @@
+{
+  "dependencies": {
+    "dummy": "file:dummy"
+  }
+}

src/config.js

@@ -45,6 +45,7 @@ export type ConfigOptions = {
 };
 
 type PackageMetadata = {
+  artifacts: Array<string>,
   registry: RegistryNames,
   hash: string,
   remote: ?PackageRemote,
@@ -353,6 +354,7 @@ export default class Config {
 
       return {
         package: pkg,
+        artifacts: metadata.artifacts || [],
         hash: metadata.hash,
         remote: metadata.remote,
         registry: metadata.registry,

src/fetchers/base-fetcher.js

@@ -49,6 +49,7 @@ export default class BaseFetcher {
       const pkg = await this.config.readManifest(dest, this.registry);
 
       await fs.writeFile(path.join(dest, constants.METADATA_FILENAME), JSON.stringify({
+        artifacts: [],
         remote: this.remote,
         registry: this.registry,
         hash,

src/package-fetcher.js

@@ -96,10 +96,6 @@ 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,7 +56,7 @@ export default class PackageInstallScripts {
     return mtimes;
   }
 
-  async copyBuildArtifacts(
+  async saveBuildArtifacts(
     loc: string,
     pkg: Manifest,
     beforeFiles: Map<string, number>,
@@ -72,62 +72,33 @@ export default class PackageInstallScripts {
       }
     }
 
-    // install script may have removed files, remove them from the cache too
-    const removedFiles = [];
-    for (const [file] of beforeFiles) {
-      if (!afterFiles.has(file)) {
-        removedFiles.push(file);
-      }
-    }
-
-    if (!removedFiles.length && !buildArtifacts.length) {
-      // nothing else to do here since we have no build side effects
+    if (!buildArtifacts.length) {
+      // nothing else to do here since we have no build artifacts
       return;
     }
 
     // if the process is killed while copying over build artifacts then we'll leave
     // the cache in a bad state. remove the metadata file and add it back once we've
     // done our copies to ensure cache integrity.
     const cachedLoc = this.config.generateHardModulePath(pkg._reference, true);
-    const cachedMetadataLoc = path.join(cachedLoc, constants.METADATA_FILENAME);
-    const cachedMetadata = await fs.readFile(cachedMetadataLoc);
-    await fs.unlink(cachedMetadataLoc);
-
-    // remove files that install script removed
-    if (removedFiles.length) {
-      for (const file of removedFiles) {
-        await fs.unlink(path.join(cachedLoc, file));
-      }
-    }
+    const metadata = await this.config.readPackageMetadata(cachedLoc);
+    metadata.artifacts = buildArtifacts;
 
-    // copy over build artifacts to cache directory
-    if (buildArtifacts.length) {
-      const copyRequests = [];
-      for (const file of buildArtifacts) {
-        copyRequests.push({
-          src: path.join(loc, file),
-          dest: path.join(cachedLoc, file),
-          onDone() {
-            spinner.tick(`Cached build artifact ${file}`);
-          },
-        });
-      }
-      await fs.copyBulk(copyRequests, {
-        possibleExtraneous: false,
-      });
-      await fs.writeFile(cachedMetadataLoc, cachedMetadata);
-    }
+    const metadataLoc = path.join(cachedLoc, constants.METADATA_FILENAME);
+    await fs.writeFile(metadataLoc, JSON.stringify({
+      ...metadata,
+
+      // config.readPackageMetadata also returns the package manifest but that's not in the original
+      // metadata json
+      package: undefined,
+    }, null, '  '));
   }
 
   async install(cmds: Array<[string, string]>, pkg: Manifest, spinner: ReporterSetSpinner): Promise<void> {
     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 {
       for (const [stage, cmd] of cmds) {
         await executeLifecycleScript(stage, this.config, loc, cmd, spinner);
@@ -299,7 +270,7 @@ export default class PackageInstallScripts {
         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]);
+        await this.saveBuildArtifacts(loc, pkg, beforeFiles, set.spinners[0]);
       }
     }
 

src/package-linker.js

@@ -122,13 +122,23 @@ export default class PackageLinker {
       return dep1[0].localeCompare(dep2[0]);
     });
 
+    // list of artifacts in modules to remove from extraneous removal
+    const phantomFiles = [];
+
     //
     const queue: Map<string, CopyQueueItem> = new Map();
     for (const [dest, {pkg, loc: src}] of flatTree) {
       const ref = pkg._reference;
       invariant(ref, 'expected package reference');
       ref.setLocation(dest);
 
+      // get a list of build artifacts contained in this module so we can prevent them from being marked as
+      // extraneous
+      const metadata = await this.config.readPackageMetadata(src);
+      for (const file of metadata.artifacts) {
+        phantomFiles.push(path.join(dest, file));
+      }
+
       queue.set(dest, {
         src,
         dest,
@@ -166,6 +176,7 @@ export default class PackageLinker {
     let tick;
     await fs.copyBulk(Array.from(queue.values()), {
       possibleExtraneous,
+      phantomFiles,
 
       ignoreBasenames: [
         constants.METADATA_FILENAME,

src/package-reference.js

@@ -44,7 +44,6 @@ export default class PackageReference {
     this.ignore = false;
     this.fresh = false;
     this.location = null;
-    this.cached = false;
     this.addRequest(request);
   }
 
@@ -59,8 +58,6 @@ 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/util/fs.js

@@ -68,6 +68,7 @@ type CopyOptions = {
   onStart: (num: number) => void,
   possibleExtraneous: PossibleExtraneous,
   ignoreBasenames: Array<string>,
+  phantomFiles: Array<string>,
 };
 
 async function buildActionsForCopy(
@@ -76,6 +77,7 @@ async function buildActionsForCopy(
   possibleExtraneousSeed: PossibleExtraneous,
 ): Promise<CopyActions> {
   const possibleExtraneous: Set<string> = new Set(possibleExtraneousSeed || []);
+  const phantomFiles: Set<string> = new Set(events.phantomFiles || []);
   const noExtraneous = possibleExtraneousSeed === false;
   const files: Set<string> = new Set();
 
@@ -101,6 +103,11 @@ async function buildActionsForCopy(
     await Promise.all(items.map(build));
   }
 
+  // simulate the existence of some files to prevent considering them extraenous
+  for (const file of phantomFiles) {
+    possibleExtraneous.delete(file);
+  }
+
   // remove all extraneous files that weren't in the tree
   if (!noExtraneous) {
     for (const loc of possibleExtraneous) {
@@ -243,13 +250,15 @@ export async function copyBulk(
     onStart?: ?(num: number) => void,
     possibleExtraneous?: PossibleExtraneous,
     ignoreBasenames?: Array<string>,
+    phantomFiles?: Array<string>,
   },
 ): Promise<void> {
   const events: CopyOptions = {
     onStart: (_events && _events.onStart) || noop,
     onProgress: (_events && _events.onProgress) || noop,
     possibleExtraneous: _events ? _events.possibleExtraneous : [],
     ignoreBasenames: (_events && _events.ignoreBasenames) || [],
+    phantomFiles: (_events && _events.phantomFiles) || [],
   };
 
   const actions: CopyActions = await buildActionsForCopy(queue, events, events.possibleExtraneous);