From 201ea6b2bb1781130036636e1c5cc61b98be0c40 Mon Sep 17 00:00:00 2001 From: onurtemizkan Date: Sun, 23 Jul 2017 11:51:30 +0300 Subject: [PATCH 1/3] Handle scripts failing with signals (fixes:#3954) --- .../lifecycle-scripts/script-fail/app.js | 1 + .../script-fail/package.json | 5 +++++ .../script-segfault/package.json | 9 +++++++++ .../script-segfault/segfault.c | 5 +++++ .../lifecycle-scripts/script-success/app.js | 1 + .../script-success/package.json | 5 +++++ __tests__/lifecycle-scripts.js | 20 +++++++++++++++++++ src/errors.js | 3 ++- src/reporters/lang/en.js | 3 ++- src/util/child.js | 16 +++++++++++++-- src/util/execute-lifecycle-script.js | 6 +++++- 11 files changed, 69 insertions(+), 5 deletions(-) create mode 100644 __tests__/fixtures/lifecycle-scripts/script-fail/app.js create mode 100644 __tests__/fixtures/lifecycle-scripts/script-fail/package.json create mode 100644 __tests__/fixtures/lifecycle-scripts/script-segfault/package.json create mode 100644 __tests__/fixtures/lifecycle-scripts/script-segfault/segfault.c create mode 100644 __tests__/fixtures/lifecycle-scripts/script-success/app.js create mode 100644 __tests__/fixtures/lifecycle-scripts/script-success/package.json diff --git a/__tests__/fixtures/lifecycle-scripts/script-fail/app.js b/__tests__/fixtures/lifecycle-scripts/script-fail/app.js new file mode 100644 index 0000000000..6cee2e1e79 --- /dev/null +++ b/__tests__/fixtures/lifecycle-scripts/script-fail/app.js @@ -0,0 +1 @@ +process.exit(1); diff --git a/__tests__/fixtures/lifecycle-scripts/script-fail/package.json b/__tests__/fixtures/lifecycle-scripts/script-fail/package.json new file mode 100644 index 0000000000..ca471576c0 --- /dev/null +++ b/__tests__/fixtures/lifecycle-scripts/script-fail/package.json @@ -0,0 +1,5 @@ +{ + "scripts": { + "test": "node app.js" + } +} diff --git a/__tests__/fixtures/lifecycle-scripts/script-segfault/package.json b/__tests__/fixtures/lifecycle-scripts/script-segfault/package.json new file mode 100644 index 0000000000..ea0c6c1b2a --- /dev/null +++ b/__tests__/fixtures/lifecycle-scripts/script-segfault/package.json @@ -0,0 +1,9 @@ +{ + "scripts": { + "compile": "gcc segfault.c -g -o segfault", + "pretest": "yarn run compile", + "test": "./segfault", + "pretest-alt": "yarn run compile", + "test-alt": "./segfault && true" + } +} diff --git a/__tests__/fixtures/lifecycle-scripts/script-segfault/segfault.c b/__tests__/fixtures/lifecycle-scripts/script-segfault/segfault.c new file mode 100644 index 0000000000..3d6d511929 --- /dev/null +++ b/__tests__/fixtures/lifecycle-scripts/script-segfault/segfault.c @@ -0,0 +1,5 @@ +int main(void) +{ + char *s = "hello world"; + *s = 'H'; +} diff --git a/__tests__/fixtures/lifecycle-scripts/script-success/app.js b/__tests__/fixtures/lifecycle-scripts/script-success/app.js new file mode 100644 index 0000000000..dcbbff6c93 --- /dev/null +++ b/__tests__/fixtures/lifecycle-scripts/script-success/app.js @@ -0,0 +1 @@ +process.exit(0); diff --git a/__tests__/fixtures/lifecycle-scripts/script-success/package.json b/__tests__/fixtures/lifecycle-scripts/script-success/package.json new file mode 100644 index 0000000000..ca471576c0 --- /dev/null +++ b/__tests__/fixtures/lifecycle-scripts/script-success/package.json @@ -0,0 +1,5 @@ +{ + "scripts": { + "test": "node app.js" + } +} diff --git a/__tests__/lifecycle-scripts.js b/__tests__/lifecycle-scripts.js index d0a0255eda..b4bc1fd3b8 100644 --- a/__tests__/lifecycle-scripts.js +++ b/__tests__/lifecycle-scripts.js @@ -142,3 +142,23 @@ test('should inherit existing environment variables when setting via yarnrc', as expect(stdout).toMatch(/^RAB$/m); expect(stdout).toMatch(/^FOO$/m); }); + +test('should not show any error messages when script ends successfully', async () => { + const stdout = await execCommand('test', 'script-success'); + + expect(stdout).toMatch(/Done in /); +}); + +test('should show correct error message when the script ends with an exit code', async () => { + await execCommand('test', 'script-fail').catch(error => { + expect(error.message).toMatch(/Command failed with exit code /); + expect(error.code).not.toBe(0); + }); +}); + +test('should show correct error message when the script ends an exit signal', async () => { + await execCommand('test', 'script-segfault').catch(error => { + expect(error.message).toMatch(/Command failed with signal /); + expect(error.code).not.toBe(0); + }); +}); diff --git a/src/errors.js b/src/errors.js index 3516ed4eb0..816eebd350 100644 --- a/src/errors.js +++ b/src/errors.js @@ -12,7 +12,8 @@ export class MessageError extends Error { export class SecurityError extends MessageError {} export class SpawnError extends MessageError { - EXIT_CODE: number; + EXIT_CODE: ?number; + EXIT_SIGNAL: ?string; } export class ResponseError extends Error { diff --git a/src/reporters/lang/en.js b/src/reporters/lang/en.js index 08a3bcf219..3e47665b4f 100644 --- a/src/reporters/lang/en.js +++ b/src/reporters/lang/en.js @@ -175,7 +175,8 @@ const messages = { binCommands: 'Commands available from binary scripts: ', possibleCommands: 'Project commands', commandQuestion: 'Which command would you like to run?', - commandFailed: 'Command failed with exit code $0.', + commandFailedWithCode: 'Command failed with exit code $0.', + commandFailedWithSignal: 'Command failed with signal $0.', packageRequiresNodeGyp: 'This package requires node-gyp, which is not currently installed. Yarn will attempt to automatically install it. If this fails, you can run "yarn global add node-gyp" to manually install it.', nodeGypAutoInstallFailed: diff --git a/src/util/child.js b/src/util/child.js index 559b0049bf..d99754db33 100644 --- a/src/util/child.js +++ b/src/util/child.js @@ -94,8 +94,20 @@ export function spawn( processingDone = true; } - proc.on('close', (code: number) => { - if (code >= 1) { + proc.on('close', (code: number, signal: string) => { + if (signal) { + err = new SpawnError( + [ + 'Command failed.', + `Exit signal: ${signal}`, + `Command: ${program}`, + `Arguments: ${args.join(' ')}`, + `Directory: ${opts.cwd || process.cwd()}`, + `Output:\n${stdout.trim()}`, + ].join('\n'), + ); + err.EXIT_SIGNAL = signal; + } else if (code >= 1) { // TODO make this output nicer err = new SpawnError( [ diff --git a/src/util/execute-lifecycle-script.js b/src/util/execute-lifecycle-script.js index 8f05b5faba..e20194365d 100644 --- a/src/util/execute-lifecycle-script.js +++ b/src/util/execute-lifecycle-script.js @@ -254,7 +254,11 @@ export async function execCommand(stage: string, config: Config, cmd: string, cw return Promise.resolve(); } catch (err) { if (err instanceof SpawnError) { - throw new MessageError(reporter.lang('commandFailed', err.EXIT_CODE)); + if (err.EXIT_SIGNAL) { + throw new MessageError(reporter.lang('commandFailedWithSignal', err.EXIT_SIGNAL)); + } else { + throw new MessageError(reporter.lang('commandFailedWithCode', err.EXIT_CODE)); + } } else { throw err; } From 96431755501812cb377a1793ea86b5e701a81224 Mon Sep 17 00:00:00 2001 From: onurtemizkan Date: Sun, 23 Jul 2017 23:03:37 +0300 Subject: [PATCH 2/3] Review updates --- .../lifecycle-scripts/script-segfault/app.js | 1 + .../script-segfault/package.json | 6 +----- .../lifecycle-scripts/script-segfault/segfault.c | 5 ----- __tests__/lifecycle-scripts.js | 2 +- src/util/child.js | 16 ++-------------- 5 files changed, 5 insertions(+), 25 deletions(-) create mode 100644 __tests__/fixtures/lifecycle-scripts/script-segfault/app.js delete mode 100644 __tests__/fixtures/lifecycle-scripts/script-segfault/segfault.c diff --git a/__tests__/fixtures/lifecycle-scripts/script-segfault/app.js b/__tests__/fixtures/lifecycle-scripts/script-segfault/app.js new file mode 100644 index 0000000000..ce079e4e19 --- /dev/null +++ b/__tests__/fixtures/lifecycle-scripts/script-segfault/app.js @@ -0,0 +1 @@ +process.kill(process.pid, "SIGSEGV"); diff --git a/__tests__/fixtures/lifecycle-scripts/script-segfault/package.json b/__tests__/fixtures/lifecycle-scripts/script-segfault/package.json index ea0c6c1b2a..ca471576c0 100644 --- a/__tests__/fixtures/lifecycle-scripts/script-segfault/package.json +++ b/__tests__/fixtures/lifecycle-scripts/script-segfault/package.json @@ -1,9 +1,5 @@ { "scripts": { - "compile": "gcc segfault.c -g -o segfault", - "pretest": "yarn run compile", - "test": "./segfault", - "pretest-alt": "yarn run compile", - "test-alt": "./segfault && true" + "test": "node app.js" } } diff --git a/__tests__/fixtures/lifecycle-scripts/script-segfault/segfault.c b/__tests__/fixtures/lifecycle-scripts/script-segfault/segfault.c deleted file mode 100644 index 3d6d511929..0000000000 --- a/__tests__/fixtures/lifecycle-scripts/script-segfault/segfault.c +++ /dev/null @@ -1,5 +0,0 @@ -int main(void) -{ - char *s = "hello world"; - *s = 'H'; -} diff --git a/__tests__/lifecycle-scripts.js b/__tests__/lifecycle-scripts.js index b4bc1fd3b8..401b309621 100644 --- a/__tests__/lifecycle-scripts.js +++ b/__tests__/lifecycle-scripts.js @@ -158,7 +158,7 @@ test('should show correct error message when the script ends with an exit code', test('should show correct error message when the script ends an exit signal', async () => { await execCommand('test', 'script-segfault').catch(error => { - expect(error.message).toMatch(/Command failed with signal /); + expect(error.message).toMatch(/Command failed with signal "SIGSEGV"/); expect(error.code).not.toBe(0); }); }); diff --git a/src/util/child.js b/src/util/child.js index d99754db33..9ef5582852 100644 --- a/src/util/child.js +++ b/src/util/child.js @@ -95,11 +95,11 @@ export function spawn( } proc.on('close', (code: number, signal: string) => { - if (signal) { + if (signal || code >= 1) { err = new SpawnError( [ 'Command failed.', - `Exit signal: ${signal}`, + signal ? `Exit signal: ${signal}` : `Exit code: ${code}`, `Command: ${program}`, `Arguments: ${args.join(' ')}`, `Directory: ${opts.cwd || process.cwd()}`, @@ -107,18 +107,6 @@ export function spawn( ].join('\n'), ); err.EXIT_SIGNAL = signal; - } else if (code >= 1) { - // TODO make this output nicer - err = new SpawnError( - [ - 'Command failed.', - `Exit code: ${code}`, - `Command: ${program}`, - `Arguments: ${args.join(' ')}`, - `Directory: ${opts.cwd || process.cwd()}`, - `Output:\n${stdout.trim()}`, - ].join('\n'), - ); err.EXIT_CODE = code; } From 70e161c7f77b195ed2469ce138060a79e1662e04 Mon Sep 17 00:00:00 2001 From: onurtemizkan Date: Wed, 2 Aug 2017 01:00:06 +0300 Subject: [PATCH 3/3] Review updates round #2 --- __tests__/lifecycle-scripts.js | 20 +++++++------------- src/util/execute-lifecycle-script.js | 10 +++++----- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/__tests__/lifecycle-scripts.js b/__tests__/lifecycle-scripts.js index 401b309621..29e1d7bee5 100644 --- a/__tests__/lifecycle-scripts.js +++ b/__tests__/lifecycle-scripts.js @@ -144,21 +144,15 @@ test('should inherit existing environment variables when setting via yarnrc', as }); test('should not show any error messages when script ends successfully', async () => { - const stdout = await execCommand('test', 'script-success'); - - expect(stdout).toMatch(/Done in /); + await expect(execCommand('test', 'script-success')).resolves.toBeDefined(); }); -test('should show correct error message when the script ends with an exit code', async () => { - await execCommand('test', 'script-fail').catch(error => { - expect(error.message).toMatch(/Command failed with exit code /); - expect(error.code).not.toBe(0); - }); +test('should throw error when the script ends with an exit code', async () => { + await expect(execCommand('test', 'script-fail')).rejects.toBeDefined(); }); -test('should show correct error message when the script ends an exit signal', async () => { - await execCommand('test', 'script-segfault').catch(error => { - expect(error.message).toMatch(/Command failed with signal "SIGSEGV"/); - expect(error.code).not.toBe(0); +if (process.platform === 'darwin') { + test('should throw error when the script ends with an exit signal', async () => { + await expect(execCommand('test', 'script-segfault')).rejects.toBeDefined(); }); -}); +} diff --git a/src/util/execute-lifecycle-script.js b/src/util/execute-lifecycle-script.js index e20194365d..0a3ec1ac86 100644 --- a/src/util/execute-lifecycle-script.js +++ b/src/util/execute-lifecycle-script.js @@ -254,11 +254,11 @@ export async function execCommand(stage: string, config: Config, cmd: string, cw return Promise.resolve(); } catch (err) { if (err instanceof SpawnError) { - if (err.EXIT_SIGNAL) { - throw new MessageError(reporter.lang('commandFailedWithSignal', err.EXIT_SIGNAL)); - } else { - throw new MessageError(reporter.lang('commandFailedWithCode', err.EXIT_CODE)); - } + throw new MessageError( + err.EXIT_SIGNAL + ? reporter.lang('commandFailedWithSignal', err.EXIT_SIGNAL) + : reporter.lang('commandFailedWithCode', err.EXIT_CODE), + ); } else { throw err; }