build: change nosign flag to sign and flip the logic#10156

JoeDoyle23 · 2016-12-06T21:06:01Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build

Description of change

Makes the default build on Windows not try to sign the node.exe
binary after a build. Instead the 'sign' flag now indicates that the
binary should be signed. The 'nosign' flag is left as a noop.

This is a common stumbling block for new users compiling Node on Windows. If the nosign flag is left off any vcbuild.bat command, they see what appears to be an error with the build. Seeing as the only time node.exe gets signed is for official builds, it makes more sense to default to not trying to sign the binary and to flip the flag to indicate when it should be attempted.

If accepted, this change will require the Windows build process to be adjusted to include the sign flag when calling vcbuild.bat.

jasnell · 2016-12-06T21:10:55Z

@nodejs/platform-windows @nodejs/build

seishun · 2016-12-06T21:14:57Z

Oh, yes, finally.

seishun

LGTM, but need input from someone more familiar with batch scripts.

jbergstroem · 2016-12-06T21:16:01Z

Makes sense. Is this major seeing how it changes default behavior? This is usually when @bnoordhuis joins and sets me straight.

richardlau · 2016-12-06T21:39:03Z

Perhaps sign should be enabled for build-release (is that what the release builds on the CI use)?

seishun · 2016-12-06T21:40:27Z

Perhaps sign should be enabled for build-release (is that what the release builds on the CI use)?

I don't think so, contributors need to build release for tests.

richardlau · 2016-12-06T21:41:47Z

I specifically meant the build-release argument, not the release argument.

seishun · 2016-12-07T07:29:11Z

Ah, sorry about it. I don't see where build-release is used in the batch script though.

bnoordhuis

Can you also update BUILDING.md?

Regarding semver-major: grey area. This change would break third-party build farms but we don't make any real commitments w.r.t. the stability of our build tools.

If I had to make the call, I'd play it safe and make it semver-major as it's not a critical bug fix.

cjihrig · 2016-12-07T17:41:02Z

It looks like the pull request template needs to be updated too.

+1 to semver major to be safe.

richardlau · 2016-12-07T20:30:29Z

BUILDING.md

Removing .\ is undoing part of #8704 -- Is this intentional?

From memory I think the .\ was added for compatibility with PowerShell.

PowerShell... yup, unintentional,reverted.

richardlau · 2016-12-07T20:41:27Z

@seishunbuild-release turns on some flags, currently:

ifdefined build_release ( setconfig=Release setpackage=1setmsi=1setlicensertf=1setdownload_arg="--download=all"seti18n_arg=small-icu )

(Note this block is only executed for vcbuild build-release and not for vcbuild release). I'm suggesting sign should be set in this block, as I think (need someone from the build/release team to confirm) that build-release is used in the release jobs on Jenkins.

JoeDoyle23 · 2016-12-07T22:36:58Z

@richardlau Yes, I think I missed adding it there. Updated.

bnoordhuis

LGTM with the final nit that the status line of the commit log is just over the 50 character limit.

JoeDoyle23 · 2016-12-07T22:44:35Z

Sorry about that. I tried to trim it up to be under 50. I guess I could have dropped the last the.

bzoz

As already mentioned by @cjihrig https://github.com/nodejs/node/blob/master/.github/PULL_REQUEST_TEMPLATE.md should be updated to.

Other than that LGTM! Can't wait for this to land!

mhdawson

LGTM

gibfahn · 2016-12-08T22:37:31Z

I guess it'd be good to get a review from someone in @nodejs/release , particularly for #10156 (comment)

cjihrig

LGTM, but this needs to be rebased. Could you squash the commits while you're at it?

JoeDoyle23 · 2016-12-13T17:59:24Z

@cjihrig Done!

joaocgreis · 2016-12-13T18:34:20Z

@JoeDoyle23 sorry for joining the party so late. Your change to set sign by default in build-release looks good, it should not break our release process this way. I would still like vcbuild build-release nosign to work, as it's useful to test locally. Something like:

diff --git a/vcbuild.bat b/vcbuild.bat index 3a6827c..bc578c8 100644 --- a/vcbuild.bat+++ b/vcbuild.bat@@ -51,7 +51,7 @@ if /i "%1"=="x64" set target_arch=x64&goto arg-ok if /i "%1"=="vc2015" set target_env=vc2015&goto arg-ok if /i "%1"=="noprojgen" set noprojgen=1&goto arg-ok if /i "%1"=="nobuild" set nobuild=1&goto arg-ok -if /i "%1"=="nosign" goto arg-ok+if /i "%1"=="nosign" set "sign="&goto arg-ok if /i "%1"=="sign" set sign=1&goto arg-ok if /i "%1"=="nosnapshot" set nosnapshot=1&goto arg-ok if /i "%1"=="noetw" set noetw=1&goto arg-ok @@ -73,7 +73,7 @@ if /i "%1"=="jslint" set jslint=1&goto arg-ok if /i "%1"=="jslint-ci" set jslint_ci=1&goto arg-ok if /i "%1"=="package" set package=1&goto arg-ok if /i "%1"=="msi" set msi=1&set licensertf=1&set download_arg="--download=all"&set i18n_arg=small-icu&goto arg-ok -if /i "%1"=="build-release" set build_release=1&goto arg-ok+if /i "%1"=="build-release" set build_release=1&set sign=1&goto arg-ok if /i "%1"=="upload" set upload=1&goto arg-ok if /i "%1"=="small-icu" set i18n_arg=%1&goto arg-ok if /i "%1"=="full-icu" set i18n_arg=%1&goto arg-ok @@ -97,7 +97,6 @@ goto next-arg if defined build_release ( set config=Release set package=1 - set sign=1 set msi=1 set licensertf=1 set download_arg="--download=all"

That requires nosign to be after build-release but we already live with that for other parameters.

To be careful, I'd like to make a test build before this lands. I'll start it when my comment above is included or rejected if that's the case. Thanks!

JoeDoyle23 · 2016-12-13T22:07:49Z

@joaocgreis I didn't have much insight into all the ways build-release was used. I think your use case makes perfect sense to add back in.

joaocgreis · 2016-12-14T01:15:15Z

Test build: https://nodejs.org/download/test/v8.0.0-test201612141edf886649/ (should be exactly the same as without the changes here, it just proves that there is no problem with the release scripts).

LGTM

@JoeDoyle23 Looks like there are still conflicts, can you please rebase again so we can start a CI run?

Makes the default build on Windows not try to sign the node.exe binary after a build. Instead the 'sign' flag now indicates that the binary should be signed. The 'nosign' flag is left as a noop.

joaocgreis · 2016-12-15T01:35:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/5407/ (freebsd failure unrelated to this)

Regarding semver, an ill configured 3rd party release farm could indeed start releasing unsigned binaries because of this. It pains me, but I'll add the label. Why did the @nodejs-github-bot add the dont-land-on-v7.x and watch labels? (cc @nodejs/github-bot )

I can land this tomorrow if there are no objections.

gibfahn · 2016-12-15T03:44:16Z

Regarding semver, an ill configured 3rd party release farm could indeed start releasing unsigned binaries because of this. It pains me, but I'll add the label.

😭 , but agreed

Makes the default build on Windows not try to sign the node.exe binary after a build. Instead the 'sign' flag now indicates that the binary should be signed. The 'nosign' flag is left as a noop. Reviewed-By: Nikolai Vavilov <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: João Reis <[email protected]> PR-URL: #10156

joaocgreis · 2016-12-18T01:22:27Z

Landed in 92ed1ab

@JoeDoyle23 thanks!

Makes the default build on Windows not try to sign the node.exe binary after a build. Instead the 'sign' flag now indicates that the binary should be signed. The 'nosign' flag is left as a noop. Reviewed-By: Nikolai Vavilov <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: João Reis <[email protected]> PR-URL: nodejs#10156

seishun approved these changes Dec 6, 2016
View reviewed changes

mscdex added the build Issues and PRs related to build files or the CI. label Dec 6, 2016

mscdex mentioned this pull request Dec 6, 2016
Bot failed to label PR nodejs/github-bot#88
Closed

mscdex added the windows Issues and PRs related to the Windows platform. label Dec 6, 2016

bnoordhuis requested changes Dec 7, 2016
View reviewed changes

richardlau reviewed Dec 7, 2016
View reviewed changes

bnoordhuis approved these changes Dec 7, 2016
View reviewed changes

bzoz suggested changes Dec 8, 2016
View reviewed changes

mhdawson approved these changes Dec 8, 2016
View reviewed changes

nodejs-github-bot added the dont-land-on-v7.x label Dec 9, 2016

gibfahn mentioned this pull request Dec 9, 2016
doc: corrected vcbuild parameters for testing on windows #10112
Closed
3 tasks

bzoz approved these changes Dec 12, 2016
View reviewed changes

cjihrig approved these changes Dec 13, 2016
View reviewed changes

nodejs-github-bot added dont-land-on-v7.x labels Dec 13, 2016

nodejs-github-bot added the dont-land-on-v7.x label Dec 13, 2016

nodejs-github-bot added the lts-watch-v6.x label Dec 13, 2016

nodejs-github-bot added dont-land-on-v7.x labels Dec 14, 2016

JoeDoyle23 closed this Dec 14, 2016

nodejs-github-bot added lts-watch-v6.x labels Dec 14, 2016

build: change nosign flag to sign and flips logic
9ad2ed7
Makes the default build on Windows not try to sign the node.exe binary after a build. Instead the 'sign' flag now indicates that the binary should be signed. The 'nosign' flag is left as a noop.

JoeDoyle23 reopened this Dec 14, 2016

joaocgreis added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 15, 2016

mscdex removed dont-land-on-v7.x labels Dec 15, 2016

joaocgreis mentioned this pull request Dec 15, 2016
attempt-backport still does not work Edit: now working?! nodejs/github-bot#100
Closed

joaocgreis closed this Dec 18, 2016

This was referenced Jan 16, 2017
v7.x backport - doc: correct vcbuild options for windows testing #10686
Closed
Revert "doc: correct vcbuild options for windows testing" #10839
Merged

seishun mentioned this pull request Feb 24, 2017
build: build a x64 build by default on Windows #11537
Merged
1 task

jasnell mentioned this pull request Apr 4, 2017
8.0.0 Release Proposal #12220
Closed

sam-github mentioned this pull request Apr 25, 2017
(v7.x backport) src: allow CLI args in env with NODE_OPTIONS #12648
Merged
4 tasks

Uh oh!

build: change nosign flag to sign and flip the logic#10156

build: change nosign flag to sign and flip the logic #10156

Uh oh!

Conversation

JoeDoyle23 commented Dec 6, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell commented Dec 6, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seishun commented Dec 6, 2016

Uh oh!

seishun left a comment

Choose a reason for hiding this comment

Uh oh!

jbergstroem commented Dec 6, 2016

Uh oh!

richardlau commented Dec 6, 2016

Uh oh!

seishun commented Dec 6, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Dec 6, 2016

Uh oh!

seishun commented Dec 7, 2016

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Dec 7, 2016

Uh oh!

richardlauDec 7, 2016

Choose a reason for hiding this comment

Uh oh!

richardlauDec 7, 2016

Choose a reason for hiding this comment

Uh oh!

JoeDoyle23Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

richardlau commented Dec 7, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoeDoyle23 commented Dec 7, 2016

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

JoeDoyle23 commented Dec 7, 2016

Uh oh!

bzoz left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Dec 8, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

JoeDoyle23 commented Dec 13, 2016

Uh oh!

joaocgreis commented Dec 13, 2016

Uh oh!

JoeDoyle23 commented Dec 13, 2016

Uh oh!

joaocgreis commented Dec 14, 2016

Uh oh!

joaocgreis commented Dec 15, 2016

Uh oh!

gibfahn commented Dec 15, 2016

Uh oh!

joaocgreis commented Dec 18, 2016

Uh oh!

Reviewers

jasnell commented Dec 6, 2016•
edited
Loading

seishun commented Dec 6, 2016•
edited
Loading

richardlau commented Dec 7, 2016•
edited
Loading

gibfahn commented Dec 8, 2016•
edited
Loading