diff --git a/__tests__/commands/install.js b/__tests__/commands/install.js index 19a657bc27..8d65354ec1 100644 --- a/__tests__/commands/install.js +++ b/__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 => { + 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 { + // 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 { + // 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 => { + 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); diff --git a/__tests__/fixtures/install/artifacts-finds-and-saves/dummy/install.js b/__tests__/fixtures/install/artifacts-finds-and-saves/dummy/install.js new file mode 100644 index 0000000000..cfb5611833 --- /dev/null +++ b/__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'); diff --git a/__tests__/fixtures/install/artifacts-finds-and-saves/dummy/package.json b/__tests__/fixtures/install/artifacts-finds-and-saves/dummy/package.json new file mode 100644 index 0000000000..76d1d32c28 --- /dev/null +++ b/__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" + } +} diff --git a/__tests__/fixtures/install/artifacts-finds-and-saves/package.json b/__tests__/fixtures/install/artifacts-finds-and-saves/package.json new file mode 100644 index 0000000000..1f2473da39 --- /dev/null +++ b/__tests__/fixtures/install/artifacts-finds-and-saves/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "dummy": "file:dummy" + } +} diff --git a/src/config.js b/src/config.js index e0c5ff57ab..426653b1d1 100644 --- a/src/config.js +++ b/src/config.js @@ -45,6 +45,7 @@ export type ConfigOptions = { }; type PackageMetadata = { + artifacts: Array, 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, diff --git a/src/fetchers/base-fetcher.js b/src/fetchers/base-fetcher.js index 893fe42624..388622550d 100644 --- a/src/fetchers/base-fetcher.js +++ b/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, diff --git a/src/package-fetcher.js b/src/package-fetcher.js index a8c1c88514..60cdf23dbb 100644 --- a/src/package-fetcher.js +++ b/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) { diff --git a/src/package-install-scripts.js b/src/package-install-scripts.js index f8c658f4fa..ad516af71b 100644 --- a/src/package-install-scripts.js +++ b/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, @@ -72,16 +72,8 @@ 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; } @@ -89,45 +81,24 @@ export default class PackageInstallScripts { // 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 { 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]); } } diff --git a/src/package-linker.js b/src/package-linker.js index 2201d0da57..346ff6fd7b 100644 --- a/src/package-linker.js +++ b/src/package-linker.js @@ -122,6 +122,9 @@ 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 = new Map(); for (const [dest, {pkg, loc: src}] of flatTree) { @@ -129,6 +132,13 @@ export default class PackageLinker { 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, diff --git a/src/package-reference.js b/src/package-reference.js index a57da06a4d..a7568378b7 100644 --- a/src/package-reference.js +++ b/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; patterns: Array; diff --git a/src/util/fs.js b/src/util/fs.js index b5e9d25ed8..b00d023708 100644 --- a/src/util/fs.js +++ b/src/util/fs.js @@ -68,6 +68,7 @@ type CopyOptions = { onStart: (num: number) => void, possibleExtraneous: PossibleExtraneous, ignoreBasenames: Array, + phantomFiles: Array, }; async function buildActionsForCopy( @@ -76,6 +77,7 @@ async function buildActionsForCopy( possibleExtraneousSeed: PossibleExtraneous, ): Promise { const possibleExtraneous: Set = new Set(possibleExtraneousSeed || []); + const phantomFiles: Set = new Set(events.phantomFiles || []); const noExtraneous = possibleExtraneousSeed === false; const files: Set = 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,6 +250,7 @@ export async function copyBulk( onStart?: ?(num: number) => void, possibleExtraneous?: PossibleExtraneous, ignoreBasenames?: Array, + phantomFiles?: Array, }, ): Promise { const events: CopyOptions = { @@ -250,6 +258,7 @@ export async function copyBulk( 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);