From 89c013bed803f45419847a6b8f98f4722e6607f2 Mon Sep 17 00:00:00 2001 From: Mark Lee Date: Thu, 10 Sep 2020 09:07:38 -0700 Subject: [PATCH] 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 | 2 ++ src/module-rebuilder.ts | 23 ++++++++++++++++++----- src/rebuild.ts | 2 ++ test/helpers/checksum.ts | 21 +++++++++++++++++++++ test/rebuild-yarnworkspace.ts | 7 +++++++ test/rebuild.ts | 30 ++++++++++++++++++++++++++---- 6 files changed, 76 insertions(+), 9 deletions(-) create mode 100644 test/helpers/checksum.ts diff --git a/.circleci/config.yml b/.circleci/config.yml index acce5109..2ca126ba 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -59,6 +59,8 @@ jobs: executor: name: win/vs2019 shell: bash.exe + environment: + GYP_MSVS_VERSION: '2019' <<: *steps-test release: diff --git a/src/module-rebuilder.ts b/src/module-rebuilder.ts index 2d736bbc..0f1e363e 100644 --- a/src/module-rebuilder.ts +++ b/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(); diff --git a/src/rebuild.ts b/src/rebuild.ts index c46db130..72ca053d 100644 --- a/src/rebuild.ts +++ b/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.'); diff --git a/test/helpers/checksum.ts b/test/helpers/checksum.ts new file mode 100644 index 00000000..e16af2f3 --- /dev/null +++ b/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 { + 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; +} diff --git a/test/rebuild-yarnworkspace.ts b/test/rebuild-yarnworkspace.ts index 45cc8acb..c53e99ae 100644 --- a/test/rebuild-yarnworkspace.ts +++ b/test/rebuild-yarnworkspace.ts @@ -13,6 +13,7 @@ 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 () => { @@ -20,6 +21,9 @@ describe('rebuild for yarn workspace', function() { 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; + } }); }); }); diff --git a/test/rebuild.ts b/test/rebuild.ts index 50bef38f..db2dbe16 100644 --- a/test/rebuild.ts +++ b/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 => { 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 => { + 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({