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

Bugfix 3752 - Artifacts disappearing when missing integrity file #4185

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
31 changes: 31 additions & 0 deletions __tests__/commands/add.js
Original file line number Diff line number Diff line change
Expand Up @@ -836,6 +836,37 @@ test.concurrent('should only refer to root to satisfy peer dependency', (): Prom
);
});

test.concurrent('should retain build artifacts after add when missing integrity file', (): Promise<void> => {
return buildRun(
reporters.BufferReporter,
fixturesLoc,
async (args, flags, config, reporter): Promise<void> => {
const lockfile = await createLockfile(config.cwd);

const addA = new Add(args, flags, config, reporter, lockfile);
await addA.init();

const expectedArtifacts = ['foo.txt'];
const integrityLoc = path.join(config.cwd, 'node_modules', constants.INTEGRITY_FILENAME);

const beforeIntegrity = await fs.readJson(integrityLoc);
expect(beforeIntegrity.artifacts['[email protected]']).toEqual(expectedArtifacts);

await fs.unlink(integrityLoc);

const lockfileAfterPreviousAdd = await Lockfile.fromDirectory(config.cwd);
const addB = new Add(['file:b'], flags, config, reporter, lockfileAfterPreviousAdd);
await addB.init();

const afterIntegrity = await fs.readJson(integrityLoc);
expect(afterIntegrity.artifacts['[email protected]']).toEqual(expectedArtifacts);
},
['file:a'],
{},
'retain-build-artifacts-missing-integrity',
);
});

test.concurrent('should retain build artifacts after add', (): Promise<void> => {
return buildRun(
reporters.BufferReporter,
Expand Down
18 changes: 18 additions & 0 deletions __tests__/commands/install/lockfiles.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,24 @@ test.concurrent('install should write and read integrity file based on lockfile
});
});

test.concurrent('install should retain artifacts when missing integrity file', (): Promise<void> => {
return runInstall({}, 'install-should-retain-artifacts-when-missing-integrity', async (config, reporter) => {
const expectedArtifacts = ['foo.txt'];
const integrityLoc = path.join(config.cwd, 'node_modules', constants.INTEGRITY_FILENAME);

const beforeIntegrity = await fs.readJson(integrityLoc);
expect(beforeIntegrity.artifacts['[email protected]']).toEqual(expectedArtifacts);

await fs.unlink(integrityLoc);

const reinstall = new Install({}, config, reporter, await Lockfile.fromDirectory(config.cwd));
await reinstall.init();

const afterIntegrity = await fs.readJson(integrityLoc);
expect(afterIntegrity.artifacts['[email protected]']).toEqual(expectedArtifacts);
});
});

test.concurrent('install should not continue if integrity check passes', (): Promise<void> => {
return runInstall({}, 'lockfile-stability', async (config, reporter) => {
await fs.writeFile(path.join(config.cwd, 'node_modules', 'yarn.test'), 'YARN TEST');
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require('fs').writeFileSync('foo.txt', 'foobar');
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "a",
"version": "0.0.0",
"scripts": {
"postinstall": "node install.js"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "b",
"version": "0.0.0"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require('fs').writeFileSync('foo.txt', 'foobar');
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "a",
"version": "0.0.0",
"scripts": {
"postinstall": "node install.js"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"a": "file:a"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1


"a@file:a":
version "0.0.0"
18 changes: 16 additions & 2 deletions src/cli/commands/add.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,16 @@ import type Config from '../../config.js';
import type {ListOptions} from './list.js';
import Lockfile from '../../lockfile/wrapper.js';
import PackageRequest from '../../package-request.js';
import WorkspaceLayout from '../../workspace-layout.js';
import {getExoticResolver} from '../../resolvers/index.js';
import {buildTree} from './list.js';
import {wrapLifecycle, Install} from './install.js';
import {MessageError} from '../../errors.js';
import * as constants from '../../constants.js';
import * as fs from '../../util/fs.js';

import invariant from 'invariant';
import path from 'path';
import semver from 'semver';

export class Add extends Install {
Expand Down Expand Up @@ -97,8 +101,18 @@ export class Add extends Install {
return preparedPatterns;
}

bailout(patterns: Array<string>): Promise<boolean> {
return Promise.resolve(false);
async bailout(patterns: Array<string>, workspaceLayout: ?WorkspaceLayout): Promise<boolean> {
const lockfileCache = this.lockfile.cache;
if (!lockfileCache) {
return false;
}
const match = await this.integrityChecker.check(patterns, lockfileCache, this.flags, workspaceLayout);
const haveLockfile = await fs.exists(path.join(this.config.lockfileFolder, constants.LOCKFILE_FILENAME));
if (match.integrityFileMissing && haveLockfile) {
// Integrity file missing, force script installations
this.scripts.setForce(true);
}
return false;
}

/**
Expand Down
6 changes: 6 additions & 0 deletions src/cli/commands/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,12 @@ export class Install {
return true;
}

if (match.integrityFileMissing && haveLockfile) {
// Integrity file missing, force script installations
this.scripts.setForce(true);
return false;
}

if (!patterns.length && !match.integrityFileMissing) {
this.reporter.success(this.reporter.lang('nothingToInstall'));
await this.createEmptyManifestFolders();
Expand Down
4 changes: 4 additions & 0 deletions src/package-install-scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ export default class PackageInstallScripts {
force: boolean;
artifacts: InstallArtifacts;

setForce(force: boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need these setters and getters?

Copy link
Contributor Author

@jamsinclair jamsinclair Aug 16, 2017

Choose a reason for hiding this comment

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

Good question.

The force property was formerly only set on construction of the PackageInstallScripts class:
https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/install.js#L178

When the install bailout method runs, the PackageInstallScripts instance has already been created. Adding the setter, allowing overrides, seemed like a simpler solution rather than reinstantiating. Happy to hear another approach.

Edit: Oh, is it all good to override properties directly, this.scripts.force = true? In that case could definitely remove setter 😂

Copy link
Member

Choose a reason for hiding this comment

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

I think a setter is fine, it's how we already do it (setArtifacts), and I have the feeling it makes the code a bit safer (because JS doesn't (yet) have private variables, setters are one of the only ways to clearly state that a variable might be updated from the outside).

this.force = force;
}

setArtifacts(artifacts: InstallArtifacts) {
this.artifacts = artifacts;
}
Expand Down