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

* fix(linking): Dont integrity check build artifacts

If an install script modified a file that existed in the package, then yarn would view the file as
stale on every command, mark the package as "fresh", and re-run the install scripts.  With this
change, known build artifacts from the integrity file are checked, and if this package file is a
build atrifact, it's size and timestamp are not checked.

#932

* WIP log message for debugging on CI

* WIP adding logging for debugging CI

* WIP adding logging for debugging CI

* remove debugging console statements.

* fix file timestamps for node 8 copyFile. fixes issue on linux.

* change file mode to what works on windows

* add file timestamp comparison and skip if correction not needed

* fix lint errors

* add error handling to fixTimes for some edge cases

* pass undefined instead of 0 as a file id

* refactor fs code, move OS specific edge cases to fs-normalized.js

* empty commit to rerun CI

* fix logic error when closing file, and fix Flow error

__tests__/commands/add.js

@@ -1063,6 +1063,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('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';
 

__tests__/fixtures/add/retain-build-artifacts-after-add/a/foo.txt

@@ -0,0 +1 @@
+X

__tests__/fixtures/add/retain-build-artifacts-after-add/a/install.js

@@ -1 +1 @@
-require('fs').writeFileSync('foo.txt', 'foobar');
+require('fs').writeFileSync('foo.txt', new Date().getTime());

__tests__/util/fs.js → __tests__/util/fs-normalized.js

@@ -1,6 +1,6 @@
 /* @flow */
 
-import {fileDatesEqual} from '../../src/util/fs.js';
+import {fileDatesEqual} from '../../src/util/fs-normalized.js';
 
 describe('fileDatesEqual', () => {
   describe('!win32', () => {

src/reporters/lang/en.js

@@ -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.',

src/util/fs-normalized.js

@@ -0,0 +1,170 @@
+/* @flow */
+
+// This module serves as a wrapper for file operations that are inconsistant across node and OS versions.
+
+import fs from 'fs';
+import {promisify} from './promise.js';
+
+export type CopyFileAction = {
+  src: string,
+  dest: string,
+  atime: Date,
+  mtime: Date,
+  mode: number,
+};
+
+let disableTimestampCorrection: ?boolean = undefined; // OS dependent. will be detected on first file copy.
+
+const readFileBuffer = promisify(fs.readFile);
+const close: (fd: number) => Promise<void> = promisify(fs.close);
+const lstat: (path: string) => Promise<fs.Stats> = promisify(fs.lstat);
+const open: (path: string, flags: string | number, mode: number) => Promise<number> = promisify(fs.open);
+const futimes: (fd: number, atime: number, mtime: number) => Promise<void> = promisify(fs.futimes);
+
+const write: (
+  fd: number,
+  buffer: Buffer,
+  offset: ?number,
+  length: ?number,
+  position: ?number,
+) => Promise<void> = promisify(fs.write);
+
+export const unlink: (path: string) => Promise<void> = promisify(require('rimraf'));
+
+/**
+ * Unlinks the destination to force a recreation. This is needed on case-insensitive file systems
+ * to force the correct naming when the filename has changed only in character-casing. (Jest -> jest).
+ */
+export const copyFile = async function(data: CopyFileAction, cleanup: () => mixed): Promise<void> {
+  try {
+    await unlink(data.dest);
+    await copyFilePoly(data.src, data.dest, 0, data);
+  } finally {
+    if (cleanup) {
+      cleanup();
+    }
+  }
+};
+
+// Node 8.5.0 introduced `fs.copyFile` which is much faster, so use that when available.
+// Otherwise we fall back to reading and writing files as buffers.
+const copyFilePoly: (src: string, dest: string, flags: number, data: CopyFileAction) => Promise<void> = (
+  src,
+  dest,
+  flags,
+  data,
+) => {
+  if (fs.copyFile) {
+    return new Promise((resolve, reject) =>
+      fs.copyFile(src, dest, flags, err => {
+        if (err) {
+          reject(err);
+        } else {
+          fixTimes(undefined, dest, data).then(() => resolve()).catch(ex => reject(ex));
+        }
+      }),
+    );
+  } else {
+    return copyWithBuffer(src, dest, flags, data);
+  }
+};
+
+const copyWithBuffer: (src: string, dest: string, flags: number, data: CopyFileAction) => Promise<void> = async (
+  src,
+  dest,
+  flags,
+  data,
+) => {
+  // Use open -> write -> futimes -> close sequence to avoid opening the file twice:
+  // one with writeFile and one with utimes
+  const fd = await open(dest, 'w', data.mode);
+  try {
+    const buffer = await readFileBuffer(src);
+    await write(fd, buffer, 0, buffer.length);
+    await fixTimes(fd, dest, data);
+  } finally {
+    await close(fd);
+  }
+};
+
+// We want to preserve file timestamps when copying a file, since yarn uses them to decide if a file has
+// changed compared to the cache.
+// There are some OS specific cases here:
+// * On linux, fs.copyFile does not preserve timestamps, but does on OSX and Win.
+// * On windows, you must open a file with write permissions to call `fs.futimes`.
+// * On OSX you can open with read permissions and still call `fs.futimes`.
+async function fixTimes(fd: ?number, dest: string, data: CopyFileAction): Promise<void> {
+  const doOpen = fd === undefined;
+  let openfd: number = fd ? fd : -1;
+
+  if (disableTimestampCorrection === undefined) {
+    // if timestamps match already, no correction is needed.
+    // the need to correct timestamps varies based on OS and node versions.
+    const destStat = await lstat(dest);
+    disableTimestampCorrection = fileDatesEqual(destStat.mtime, data.mtime);
+  }
+
+  if (disableTimestampCorrection) {
+    return;
+  }
+
+  if (doOpen) {
+    try {
+      openfd = await open(dest, 'a', data.mode);
+    } catch (er) {
+      // file is likely read-only
+      try {
+        openfd = await open(dest, 'r', data.mode);
+      } catch (err) {
+        // We can't even open this file for reading.
+        return;
+      }
+    }
+  }
+
+  try {
+    if (openfd) {
+      await futimes(openfd, data.atime, data.mtime);
+    }
+  } catch (er) {
+    // If `futimes` throws an exception, we probably have a case of a read-only file on Windows.
+    // In this case we can just return. The incorrect timestamp will just cause that file to be recopied
+    // on subsequent installs, which will effect yarn performance but not break anything.
+  } finally {
+    if (doOpen && openfd) {
+      await close(openfd);
+    }
+  }
+}
+
+// Compare file timestamps.
+// Some versions of Node on windows zero the milliseconds when utime is used.
+export const fileDatesEqual = (a: Date, b: Date) => {
+  const aTime = a.getTime();
+  const bTime = b.getTime();
+
+  if (process.platform !== 'win32') {
+    return aTime === bTime;
+  }
+
+  // See https://github.com/nodejs/node/pull/12607
+  // Submillisecond times from stat and utimes are truncated on Windows,
+  // causing a file with mtime 8.0079998 and 8.0081144 to become 8.007 and 8.008
+  // and making it impossible to update these files to their correct timestamps.
+  if (Math.abs(aTime - bTime) <= 1) {
+    return true;
+  }
+
+  const aTimeSec = Math.floor(aTime / 1000);
+  const bTimeSec = Math.floor(bTime / 1000);
+
+  // See https://github.com/nodejs/node/issues/2069
+  // Some versions of Node on windows zero the milliseconds when utime is used
+  // So if any of the time has a milliseconds part of zero we suspect that the
+  // bug is present and compare only seconds.
+  if (aTime - aTimeSec * 1000 === 0 || bTime - bTimeSec * 1000 === 0) {
+    return aTimeSec === bTimeSec;
+  }
+
+  return aTime === bTime;
+};