Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

misc(build): set correct exit code when build scripts fail #13459

Merged
merged 8 commits into from
Jan 11, 2022

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Dec 3, 2021

These scripts don't exit with 1 as expected when they fail. They should else they be footguns.

@connorjclark connorjclark requested a review from a team as a code owner December 3, 2021 01:13
@connorjclark connorjclark requested review from adamraine and removed request for a team December 3, 2021 01:13
@google-cla google-cla bot added the cla: yes label Dec 3, 2021
@brendankenny
Copy link
Member

These scripts don't exit with 1 as expected when they fail. They should else they be footguns.

FWIW that's only true in Node 14. In Node 15+ unhandled rejections throw and exit with status code 1.

@@ -32,7 +32,7 @@
* @property {Run[]} runs
*/

import {strict as assert} from 'assert';
import assert from 'assert/strict';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'assert/strict' also added in Node 15 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Y'all could maybe beef up the PR descriptions because it took me a bit of debugging to figure out what was going wrong for me is apparently what is being fixed here :P

This is from yarn build-smokehouse-bundle failing on polyfilling assert.strict, but we only yarn build-all in ci.yml, which is Node 14 only, so the action wasn't failing?

Ideally we could get their polyfill updated if we're going to keep using it (even browserify's polyfill has strict), but for now we could switch to just assert but manually use assert.strictEqual and assert.deepStrictEqual instead of assert.equal and assert.deepEqual. Not a huge deal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is from yarn build-smokehouse-bundle failing on polyfilling assert.strict

yup

but we only yarn build-all in ci.yml, which is Node 14 only, so the action wasn't failing?

The GH action wasn't (because of wrong exit codes), correct. The import into google3 (see import_tool) was spitting out errors while bundling smokehouse but still chugging along.

@brendankenny
Copy link
Member

Hmm, this doesn't appear to fix the yarn build-all exit code problem. See the CI/basics log before the assert/strict change. The error shows up correctly on the subcommand (error Command failed with exit code 1) but the overall build-all script still exits with 0 and so the CI check passes.

This matches what happens locally for me in Node 16 (where unhandled rejections throw so build-smokehouse-bundle fails but the overall build-all is good).

Is it the build-all script? & wait or whatever eating errors?

"build-all:task": "yarn build-report && yarn build-cdt-lib && yarn build-devtools && (yarn build-extension & yarn build-lr & yarn build-viewer & yarn build-treemap & yarn build-smokehouse-bundle & wait) && yarn build-pack",

@connorjclark
Copy link
Collaborator Author

Is it the build-all script? & wait or whatever eating errors?

yup

image

@connorjclark
Copy link
Collaborator Author

should we just add concurrently from npm?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants