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

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@JoeDoyle23
Member

JoeDoyle23 commented Dec 6, 2016

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

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Dec 6, 2016

@seishun

This comment has been minimized.

Show comment
Hide comment
@seishun

seishun Dec 6, 2016

Member

Oh, yes, finally.

Member

seishun commented Dec 6, 2016

Oh, yes, finally.

@seishun

seishun approved these changes Dec 6, 2016

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

@jbergstroem

This comment has been minimized.

Show comment
Hide comment
@jbergstroem

jbergstroem Dec 6, 2016

Member

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

Member

jbergstroem commented Dec 6, 2016

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

@richardlau

This comment has been minimized.

Show comment
Hide comment
@richardlau

richardlau Dec 6, 2016

Member

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

Member

richardlau commented Dec 6, 2016

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

@seishun

This comment has been minimized.

Show comment
Hide comment
@seishun

seishun Dec 6, 2016

Member

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.

Member

seishun commented Dec 6, 2016

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

This comment has been minimized.

Show comment
Hide comment
@richardlau

richardlau Dec 6, 2016

Member

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

Member

richardlau commented Dec 6, 2016

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

@mscdex mscdex added the build label Dec 6, 2016

@mscdex mscdex referenced this pull request Dec 6, 2016

Closed

Bot failed to label PR #88

@mscdex mscdex added the windows label Dec 6, 2016

@seishun

This comment has been minimized.

Show comment
Hide comment
@seishun

seishun Dec 7, 2016

Member

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

Member

seishun commented Dec 7, 2016

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

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Dec 7, 2016

Contributor

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

+1 to semver major to be safe.

Contributor

cjihrig commented Dec 7, 2016

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

+1 to semver major to be safe.

Show outdated Hide outdated BUILDING.md
@richardlau

This comment has been minimized.

Show comment
Hide comment
@richardlau

richardlau Dec 7, 2016

Member

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

Member

richardlau commented Dec 7, 2016

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

@JoeDoyle23

This comment has been minimized.

Show comment
Hide comment
@JoeDoyle23

JoeDoyle23 Dec 7, 2016

Member

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

Member

JoeDoyle23 commented Dec 7, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@JoeDoyle23

JoeDoyle23 Dec 7, 2016

Member

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

Member

JoeDoyle23 commented Dec 7, 2016

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

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Dec 8, 2016

Member

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

Member

gibfahn commented Dec 8, 2016

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

@bzoz

bzoz approved these changes Dec 12, 2016

@cjihrig

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

@JoeDoyle23

This comment has been minimized.

Show comment
Hide comment
@JoeDoyle23
Member

JoeDoyle23 commented Dec 13, 2016

@cjihrig Done!

@joaocgreis

This comment has been minimized.

Show comment
Hide comment
@joaocgreis

joaocgreis Dec 13, 2016

Member

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

Member

joaocgreis commented Dec 13, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@JoeDoyle23

JoeDoyle23 Dec 13, 2016

Member

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

Member

JoeDoyle23 commented Dec 13, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@joaocgreis

joaocgreis Dec 14, 2016

Member

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?

Member

joaocgreis commented Dec 14, 2016

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?

build: change nosign flag to sign and flips logic
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 JoeDoyle23 reopened this Dec 14, 2016

@joaocgreis

This comment has been minimized.

Show comment
Hide comment
@joaocgreis

joaocgreis Dec 15, 2016

Member

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.

Member

joaocgreis commented Dec 15, 2016

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

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Dec 15, 2016

Member

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

Member

gibfahn commented Dec 15, 2016

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

joaocgreis added a commit that referenced this pull request Dec 18, 2016

build: change nosign flag to sign and flips logic
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
@joaocgreis

This comment has been minimized.

Show comment
Hide comment
@joaocgreis

joaocgreis Dec 18, 2016

Member

Landed in 92ed1ab

@JoeDoyle23 thanks!

Member

joaocgreis commented Dec 18, 2016

Landed in 92ed1ab

@JoeDoyle23 thanks!

@joaocgreis joaocgreis closed this Dec 18, 2016

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017

build: change nosign flag to sign and flips logic
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 seishun referenced this pull request Feb 24, 2017

Merged

build: build a x64 build by default on Windows #11537

1 of 1 task complete

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

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