fix: run node-gyp rebuild correctly (#409)

* fix(module-rebuilder): iterate through all of the node-gyp commands
* fix(module-rebuilder): set node-gyp's working directory to the module being rebuilt & improve errors
* test: add test to ensure that the module actually got rebuilt
* build(ci): set GYP_MSVS_VERSION in CircleCI
* fix(rebuilder): cache the MSVS version when the rebuilder is created
* test: reset MSVS version after rebuild tests

.circleci/config.yml

@@ -59,6 +59,8 @@ jobs:
     executor:
       name: win/vs2019
       shell: bash.exe
+    environment:
+      GYP_MSVS_VERSION: '2019'
     <<: *steps-test
 
   release:

src/module-rebuilder.ts

@@ -93,8 +93,8 @@ export class ModuleRebuilder {
 
     args.push(...(await this.buildNodeGypArgsFromBinaryField()));
 
-    if (process.env.GYP_MSVS_VERSION) {
-      args.push(`--msvs_version=${process.env.GYP_MSVS_VERSION}`);
+    if (this.rebuilder.msvsVersion) {
+      args.push(`--msvs_version=${this.rebuilder.msvsVersion}`);
     }
 
     return args;
@@ -168,12 +168,25 @@ export class ModuleRebuilder {
     }
 
     const nodeGypArgs = await this.buildNodeGypArgs();
+    d('rebuilding', this.moduleName, 'with args', nodeGypArgs);
 
     const nodeGyp = NodeGyp();
     nodeGyp.parseArgv(nodeGypArgs);
-    const rebuildArgs = nodeGyp.todo[0].args;
-    d('rebuilding', this.moduleName, 'with args', rebuildArgs);
-    await promisify(nodeGyp.commands.rebuild)(rebuildArgs);
+    let command = nodeGyp.todo.shift();
+    const originalWorkingDir = process.cwd();
+    try {
+      process.chdir(this.modulePath);
+      while (command) {
+        await promisify(nodeGyp.commands[command.name])(command.args);
+        command = nodeGyp.todo.shift();
+      }
+    } catch (err) {
+      let errorMessage = `node-gyp failed to rebuild '${this.modulePath}'.\n`;
+      errorMessage += `Error: ${err.message || err}\n\n`;
+      throw new Error(errorMessage);
+    } finally {
+      process.chdir(originalWorkingDir);
+    }
 
     d('built:', this.moduleName);
     await this.writeMetadata();

src/rebuild.ts

@@ -67,6 +67,7 @@ export class Rebuilder {
   public cachePath: string;
   public prebuildTagPrefix: string;
   public projectRootPath?: string;
+  public msvsVersion?: string;
 
   constructor(options: RebuilderOptions) {
     this.lifecycle = options.lifecycle;
@@ -83,6 +84,7 @@ export class Rebuilder {
     this.useCache = options.useCache || false;
     this.cachePath = options.cachePath || path.resolve(os.homedir(), '.electron-rebuild-cache');
     this.prebuildTagPrefix = options.prebuildTagPrefix || 'v';
+    this.msvsVersion = process.env.GYP_MSVS_VERSION;
 
     if (this.useCache && this.force) {
       console.warn('[WARNING]: Electron Rebuild has force enabled and cache enabled, force take precedence and the cache will not be used.');

test/helpers/checksum.ts

@@ -0,0 +1,21 @@
+import * as crypto from 'crypto';
+import * as fs from 'fs-extra';
+import { promisify } from 'util';
+import * as stream from 'stream';
+
+const pipeline = promisify(stream.pipeline);
+
+export async function determineChecksum(filename: string): Promise<string> {
+  let calculated = '';
+  const file = fs.createReadStream(filename, { encoding: 'binary' });
+  const hasher = crypto.createHash('sha256', { defaultEncoding: 'binary' });
+  hasher.on('readable', () => {
+    const data = hasher.read();
+    if (data) {
+      calculated = data.toString('hex');
+    }
+  });
+  await pipeline(file, hasher);
+
+  return calculated;
+}

test/rebuild-yarnworkspace.ts

@@ -13,13 +13,17 @@ const testElectronVersion = getExactElectronVersionSync();
 describe('rebuild for yarn workspace', function() {
   this.timeout(2 * 60 * 1000);
   const testModulePath = path.resolve(os.tmpdir(), 'electron-rebuild-test');
+  const msvs_version: string | undefined = process.env.GYP_MSVS_VERSION;
 
   describe('core behavior', () => {
     before(async () => {
       await fs.remove(testModulePath);
       await fs.copy(path.resolve(__dirname, 'fixture/workspace-test'), testModulePath);
 
       await spawn('yarn', [], { cwd: testModulePath });
+      if (msvs_version) {
+        process.env.GYP_MSVS_VERSION = msvs_version;
+      }
 
       const projectRootPath = await getProjectRootPath(path.join(testModulePath, 'workspace-test', 'child-workspace'));
 
@@ -41,6 +45,9 @@ describe('rebuild for yarn workspace', function() {
 
     after(async () => {
       await fs.remove(testModulePath);
+      if (msvs_version) {
+        process.env.GYP_MSVS_VERSION = msvs_version;
+      }
     });
   });
 });

test/rebuild.ts

@@ -4,6 +4,7 @@ import * as path from 'path';
 import * as os from 'os';
 import { spawn } from '@malept/cross-spawn-promise';
 
+import { determineChecksum } from './helpers/checksum';
 import { expectNativeModuleToBeRebuilt, expectNativeModuleToNotBeRebuilt } from './helpers/rebuild';
 import { getExactElectronVersionSync } from './helpers/electron-version';
 import { rebuild, RebuildOptions } from '../src/rebuild';
@@ -13,7 +14,13 @@ const testElectronVersion = getExactElectronVersionSync();
 describe('rebuilder', () => {
   const testModulePath = path.resolve(os.tmpdir(), 'electron-rebuild-test');
   const timeoutSeconds = process.platform === 'win32' ? 5 : 2;
+  const msvs_version: string | undefined = process.env.GYP_MSVS_VERSION;
 
+  const resetMSVSVersion = () => {
+    if (msvs_version) {
+      process.env.GYP_MSVS_VERSION = msvs_version;
+    }
+  };
   const resetTestModule = async (): Promise<void> => {
     await fs.remove(testModulePath);
     await fs.mkdirs(testModulePath);
@@ -22,8 +29,14 @@ describe('rebuilder', () => {
       path.resolve(testModulePath, 'package.json')
     );
     await spawn('npm', ['install'], { cwd: testModulePath });
+    resetMSVSVersion();
   };
 
+  const cleanupTestModule = async (): Promise<void> => {
+    await fs.remove(testModulePath);
+    resetMSVSVersion();
+  }
+
   const optionSets: {
     name: string;
     args: RebuildOptions | string[];
@@ -81,7 +94,7 @@ describe('rebuilder', () => {
 
       after(async () => {
         delete process.env.ELECTRON_REBUILD_TESTS;
-        await fs.remove(testModulePath);
+        await cleanupTestModule();
       });
     });
   }
@@ -90,9 +103,12 @@ describe('rebuilder', () => {
     this.timeout(timeoutSeconds * 60 * 1000);
 
     before(resetTestModule);
+    after(cleanupTestModule);
+    afterEach(resetMSVSVersion);
 
     it('should skip the rebuild step when disabled', async () => {
       await rebuild(testModulePath, testElectronVersion, process.arch);
+      resetMSVSVersion();
       const rebuilder = rebuild(testModulePath, testElectronVersion, process.arch, [], false);
       let skipped = 0;
       rebuilder.lifecycle.on('module-skip', () => {
@@ -104,6 +120,7 @@ describe('rebuilder', () => {
 
     it('should rebuild all modules again when disabled but the electron ABI bumped', async () => {
       await rebuild(testModulePath, testElectronVersion, process.arch);
+      resetMSVSVersion();
       const rebuilder = rebuild(testModulePath, '3.0.0', process.arch, [], false);
       let skipped = 0;
       rebuilder.lifecycle.on('module-skip', () => {
@@ -115,6 +132,7 @@ describe('rebuilder', () => {
 
     it('should rebuild all modules again when enabled', async () => {
       await rebuild(testModulePath, testElectronVersion, process.arch);
+      resetMSVSVersion();
       const rebuilder = rebuild(testModulePath, testElectronVersion, process.arch, [], true);
       let skipped = 0;
       rebuilder.lifecycle.on('module-skip', () => {
@@ -129,20 +147,24 @@ describe('rebuilder', () => {
     this.timeout(2 * 60 * 1000);
 
     beforeEach(resetTestModule);
-    afterEach(async () => await fs.remove(testModulePath));
+    afterEach(cleanupTestModule);
 
     it('should rebuild only specified modules', async () => {
+      const nativeModuleBinary = path.join(testModulePath, 'node_modules', 'farmhash', 'build', 'Release', 'farmhash.node');
+      const nodeModuleChecksum = await determineChecksum(nativeModuleBinary);
       const rebuilder = rebuild({
         buildPath: testModulePath,
         electronVersion: testElectronVersion,
         arch: process.arch,
-        onlyModules: ['ffi-napi'],
+        onlyModules: ['farmhash'],
         force: true
       });
       let built = 0;
       rebuilder.lifecycle.on('module-done', () => built++);
       await rebuilder;
       expect(built).to.equal(1);
+      const electronModuleChecksum = await determineChecksum(nativeModuleBinary);
+      expect(electronModuleChecksum).to.not.equal(nodeModuleChecksum);
     });
 
     it('should rebuild multiple specified modules via --only option', async () => {
@@ -164,7 +186,7 @@ describe('rebuilder', () => {
     this.timeout(10 * 60 * 1000);
 
     before(resetTestModule);
-    afterEach(async () => await fs.remove(testModulePath));
+    after(cleanupTestModule);
 
     it('should have rebuilt ffi-napi module in Debug mode', async () => {
       await rebuild({