Unlink destination before copy-on-write (#34852) (#35884)

* Unlink destination before copy-on-write * Catch unlink exception if destination file doesn't exist * Add link to upstream node issue * Add .node_binaries to kbn cleanup * Remove unlink call * Remove fs.copy() * Fix extract_node_builds_task test * Remove copy from build/lib exports
elastic · May 2, 2019 · 15549e3 · 15549e3
1 parent ae4fa1d
commit 15549e3
Show file tree

Hide file tree

Showing 6 changed files with 29 additions and 72 deletions.
diff --git a/package.json b/package.json
@@ -29,7 +29,8 @@
         "build",
         "optimize",
         "built_assets",
-        ".eslintcache"
+        ".eslintcache",
+        ".node_binaries"
       ]
     }
   },

diff --git a/src/dev/build/lib/__tests__/fs.js b/src/dev/build/lib/__tests__/fs.js
@@ -23,7 +23,7 @@ import { chmodSync, statSync } from 'fs';
 import del from 'del';
 import expect from '@kbn/expect';
 
-import { mkdirp, write, read, getChildPaths, copy, copyAll, getFileHash, untar } from '../fs';
+import { mkdirp, write, read, getChildPaths, copyAll, getFileHash, untar } from '../fs';
 
 const TMP = resolve(__dirname, '__tmp__');
 const FIXTURES = resolve(__dirname, 'fixtures');
@@ -149,48 +149,6 @@ describe('dev/build/lib/fs', () => {
     });
   });
 
-  describe('copy()', () => {
-    it('rejects if source path is not absolute', async () => {
-      try {
-        await copy('foo/bar', __dirname);
-        throw new Error('Expected getChildPaths() to reject');
-      } catch (error) {
-        assertNonAbsoluteError(error);
-      }
-    });
-
-    it('rejects if destination path is not absolute', async () => {
-      try {
-        await copy(__dirname, 'foo/bar');
-        throw new Error('Expected getChildPaths() to reject');
-      } catch (error) {
-        assertNonAbsoluteError(error);
-      }
-    });
-
-    it('rejects if neither path is not absolute', async () => {
-      try {
-        await copy('foo/bar', 'foo/bar');
-        throw new Error('Expected getChildPaths() to reject');
-      } catch (error) {
-        assertNonAbsoluteError(error);
-      }
-    });
-
-    it('copies the contents of one file to another', async () => {
-      const destination = resolve(TMP, 'bar.txt');
-      await copy(BAR_TXT_PATH, destination);
-      expect(await read(destination)).to.be('bar\n');
-    });
-
-    it('copies the mode of the source file', async () => {
-      const destination = resolve(TMP, 'dest.txt');
-      await copy(WORLD_EXECUTABLE, destination);
-
-      expect(getCommonMode(destination)).to.be(isWindows ? '666' : '777');
-    });
-  });
-
   describe('copyAll()', () => {
     it('rejects if source path is not absolute', async () => {
       try {

diff --git a/src/dev/build/lib/fs.js b/src/dev/build/lib/fs.js
@@ -34,12 +34,10 @@ import { createPromiseFromStreams, createMapStream } from '../../../legacy/utils
 import { Extract } from 'tar';
 
 const mkdirpAsync = promisify(mkdirpCb);
-const statAsync = promisify(fs.stat);
 const writeFileAsync = promisify(fs.writeFile);
 const readFileAsync = promisify(fs.readFile);
 const readdirAsync = promisify(fs.readdir);
 const utimesAsync = promisify(fs.utimes);
-const copyFileAsync = promisify(fs.copyFile);
 
 export function assertAbsolute(path) {
   if (!isAbsolute(path)) {
@@ -77,16 +75,6 @@ export async function getChildPaths(path) {
   return childNames.map(name => resolve(path, name));
 }
 
-export async function copy(source, destination) {
-  assertAbsolute(source);
-  assertAbsolute(destination);
-
-  // do a stat call to make sure the source exists before creating the destination directory
-  await statAsync(source);
-  await mkdirp(dirname(destination));
-  await copyFileAsync(source, destination, fs.constants.COPYFILE_FICLONE);
-}
-
 export async function deleteAll(patterns, log) {
   if (!Array.isArray(patterns)) {
     throw new TypeError('Expected patterns to be an array');

diff --git a/src/dev/build/lib/index.js b/src/dev/build/lib/index.js
@@ -25,7 +25,6 @@ export {
   read,
   write,
   mkdirp,
-  copy,
   copyAll,
   getFileHash,
   untar,

diff --git a/src/dev/build/tasks/nodejs/__tests__/extract_node_builds_task.js b/src/dev/build/tasks/nodejs/__tests__/extract_node_builds_task.js
@@ -19,7 +19,6 @@
 
 import sinon from 'sinon';
 import { resolve } from 'path';
-
 import * as NodeDownloadInfoNS from '../node_download_info';
 import * as FsNS from '../../../lib/fs';
 import { ExtractNodeBuildsTask } from '../extract_node_builds_task';
@@ -37,7 +36,7 @@ describe('src/dev/build/tasks/node_extract_node_builds_task', () => {
       extractDir: 'extractDir',
     });
 
-    sandbox.stub(FsNS, 'copy');
+    sandbox.stub(ExtractNodeBuildsTask, 'copyWindows');
     sandbox.stub(FsNS, 'untar');
 
     const platform = {
@@ -53,8 +52,8 @@ describe('src/dev/build/tasks/node_extract_node_builds_task', () => {
     sinon.assert.calledOnce(NodeDownloadInfoNS.getNodeDownloadInfo);
     sinon.assert.calledWithExactly(NodeDownloadInfoNS.getNodeDownloadInfo, config, platform);
 
-    sinon.assert.calledOnce(FsNS.copy);
-    sinon.assert.calledWithExactly(FsNS.copy, 'downloadPath', resolve('extractDir/node.exe'));
+    sinon.assert.calledOnce(ExtractNodeBuildsTask.copyWindows);
+    sinon.assert.calledWithExactly(ExtractNodeBuildsTask.copyWindows, 'downloadPath', resolve('extractDir/node.exe'));
 
     sinon.assert.notCalled(FsNS.untar);
   });
@@ -65,7 +64,7 @@ describe('src/dev/build/tasks/node_extract_node_builds_task', () => {
       extractDir: 'extractDir',
     });
 
-    sandbox.stub(FsNS, 'copy');
+    sandbox.stub(ExtractNodeBuildsTask, 'copyWindows');
     sandbox.stub(FsNS, 'untar');
 
     const platform = {
@@ -81,7 +80,7 @@ describe('src/dev/build/tasks/node_extract_node_builds_task', () => {
     sinon.assert.calledOnce(NodeDownloadInfoNS.getNodeDownloadInfo);
     sinon.assert.calledWithExactly(NodeDownloadInfoNS.getNodeDownloadInfo, config, platform);
 
-    sinon.assert.notCalled(FsNS.copy);
+    sinon.assert.notCalled(ExtractNodeBuildsTask.copyWindows);
 
     sinon.assert.calledOnce(FsNS.untar);
     sinon.assert.calledWithExactly(FsNS.untar, 'downloadPath', 'extractDir', {

diff --git a/src/dev/build/tasks/nodejs/extract_node_builds_task.js b/src/dev/build/tasks/nodejs/extract_node_builds_task.js
@@ -17,29 +17,41 @@
  * under the License.
  */
 
-import { resolve } from 'path';
+import { dirname, resolve } from 'path';
+import fs from 'fs';
+import { promisify } from 'bluebird';
+import mkdirp from 'mkdirp';
 
-import { copy, untar } from '../../lib';
+import { untar } from '../../lib';
 import { getNodeDownloadInfo } from './node_download_info';
 
+const statAsync = promisify(fs.stat);
+const mkdirpAsync = promisify(mkdirp);
+const copyFileAsync = promisify(fs.copyFile);
+
 export const ExtractNodeBuildsTask = {
   global: true,
   description: 'Extracting node.js builds for all platforms',
   async run(config) {
     await Promise.all(
       config.getNodePlatforms().map(async platform => {
         const { downloadPath, extractDir } = getNodeDownloadInfo(config, platform);
-
-        // windows executable is not extractable, it's just a .exe file
+        // windows executable is not extractable, it's just an .exe file
         if (platform.isWindows()) {
-          return await copy(downloadPath, resolve(extractDir, 'node.exe'));
+          const destination = resolve(extractDir, 'node.exe');
+          return this.copyWindows(downloadPath, destination);
         }
 
         // all other downloads are tarballs
-        await untar(downloadPath, extractDir, {
-          strip: 1
-        });
-      })
+        return untar(downloadPath, extractDir, { strip: 1 });
+      }),
     );
   },
+  async copyWindows(source, destination) {
+    // ensure source exists before creating destination directory
+    await statAsync(source);
+    await mkdirpAsync(dirname(destination));
+    // for performance reasons, do a copy-on-write by using the fs.constants.COPYFILE_FICLONE flag
+    return await copyFileAsync(source, destination, fs.constants.COPYFILE_FICLONE);
+  },
 };