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

fix(linking): Dont integrity check build artifacts #5470

Merged
merged 16 commits into from
Apr 5, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
34 changes: 34 additions & 0 deletions __tests__/commands/add.js
Original file line number Diff line number Diff line change
Expand Up @@ -1062,6 +1062,40 @@ test.concurrent('should retain build artifacts after add', async () => {
);
});

// If an install script changed a file that was installed with the package,
// then all subsequent yarn commands (like add) would see the file as changed, marking
// the package as "fresh" and re-running install scripts.
// The fix was to not mark a package as "fresh" if a changed file was also a build artifact.
// This test writes the current timestamp to a file tht also existed in the package.
// The timestamp generated by package A shouldn't change when adding package B.
// See: https://github.com/yarnpkg/yarn/issues/932#issuecomment-370245462
test.concurrent('should not run scripts if build artifact changed', async () => {
await buildRun(
reporters.BufferReporter,
fixturesLoc,
async (args, flags, config, reporter, lockfile): Promise<void> => {
const addA = new Add(args, flags, config, reporter, lockfile);
await addA.init();

const artifactFile = path.join(config.cwd, 'node_modules', 'a', 'foo.txt');

const beforeContents = await fs.readFileAny([artifactFile]);

// re-read lockfile after the first `add` or else it will still be empty
lockfile = await Lockfile.fromDirectory(config.cwd);

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

const afterContents = await fs.readFileAny([artifactFile]);
expect(afterContents).toEqual(beforeContents);
},
['file:a'],
{},
'retain-build-artifacts-after-add',
);
});

test.concurrent('installing with --pure-lockfile and then adding should keep build artifacts', async () => {
const fixture = 'integrity-pure-lockfile';

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
X
Original file line number Diff line number Diff line change
@@ -1 +1 @@
require('fs').writeFileSync('foo.txt', 'foobar');
require('fs').writeFileSync('foo.txt', new Date().getTime());
Binary file not shown.
1 change: 1 addition & 0 deletions src/reporters/lang/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const messages = {
verboseFileRemoveExtraneous: 'Removing extraneous file $0.',
verboseFilePhantomExtraneous:
"File $0 would be marked as extraneous but has been removed as it's listed as a phantom file.",
verboseFileSkipArtifact: 'Skipping copying of $0 as the file is marked as a built artifact and subject to change.',
verboseFileFolder: 'Creating directory $0.',

verboseRequestStart: 'Performing $0 request to $1.',
Expand Down
14 changes: 14 additions & 0 deletions src/util/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,13 @@ async function buildActionsForCopy(
} catch (err) {}
} */

if (bothFiles && artifactFiles.has(dest)) {
// this file gets changed during build, likely by a custom install script. Don't bother checking it.
onDone();
reporter.verbose(reporter.lang('verboseFileSkipArtifact', src));
return;
}

if (bothFiles && srcStat.size === destStat.size && fileDatesEqual(srcStat.mtime, destStat.mtime)) {
// we can safely assume this is the same file
onDone();
Expand Down Expand Up @@ -467,6 +474,13 @@ async function buildActionsForHardlink(
}
}

if (bothFiles && artifactFiles.has(dest)) {
// this file gets changed during build, likely by a custom install script. Don't bother checking it.
onDone();
reporter.verbose(reporter.lang('verboseFileSkipArtifact', src));
return;
}

// correct hardlink
if (bothFiles && srcStat.ino !== null && srcStat.ino === destStat.ino) {
onDone();
Expand Down