build: change nosign flag to sign and flip the logic by JoeDoyle23 · Pull Request #10156 · nodejs/node · GitHub

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.

@nodejs/platform-windows @nodejs/build

Oh, yes, finally.

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

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

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

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.

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

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

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.

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

+1 to semver major to be safe.

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

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

PowerShell... yup, unintentional,reverted.

@seishun build-release turns on some flags, currently:

if defined build_release (
  set config=Release
  set package=1
  set msi=1
  set licensertf=1
  set download_arg="--download=all"
  set i18n_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.

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

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

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

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!

LGTM

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

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

@cjihrig Done!

@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!

@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.

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.

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.

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 <vvnicholas@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: João Reis <reis@janeasystems.com> PR-URL: #10156

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 <vvnicholas@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: João Reis <reis@janeasystems.com> 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

build: change nosign flag to sign and flip the logic by JoeDoyle23 · Pull Request #10156 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

build: change nosign flag to sign and flip the logic#10156JoeDoyle23 wants to merge 1 commit intonodejs:masternodejs/node:masterfrom JoeDoyle23:vcbuild-change-nosign-to-signCopy head branch name to clipboard

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!

richardlau Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

richardlau Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

JoeDoyle23 Dec 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!

build: change nosign flag to sign and flip the logic#10156
JoeDoyle23 wants to merge 1 commit into
nodejs:masterfrom
JoeDoyle23:vcbuild-change-nosign-to-sign

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