build,win: restore vcbuild TAG functionality by rvagg · Pull Request #18031 · nodejs/node

rvagg · 2018-01-08T05:08:26Z

Ref: #17299
Ref: nodejs/abi-stable-node#289

Would someone mind testing this on a Windows machine please?

something like this (off the top of my head, I don't have a Windows machine to tinker with right at this moment):

set DISTTYPE=nightly
set DATESTRING=20180108
set COMMIT=abc123
vcbuild.bat build x64

gibfahn · 2018-01-08T11:54:10Z

@rvagg could you add an explanation to the commit message? I'm not following how this would fix the problem.

richardlau · 2018-01-08T17:04:04Z

@gibfahn Probably that TAG is defined in the :getnodeversion subroutine and therefore should be added to configure_flags after the call :getnodeversion.

digitalinfinity

Can confirm that it works (though I was very confused for a bit because TAG is one of the environment variables that gets leaked to the environment so if you do vcbuild clean followed by vcbuild x64 release, it sets the right versions because the clean set up the TAG variable for the following invocation of vcbuild. Sigh.

However, this patch appears to work correctly on my machine- much appreciated, @rvagg - thanks for fixing!

digitalinfinity · 2018-01-09T01:28:50Z

P.S: +1 to adding a comment to this change indicating that TAG needs to be checked after getnodeversion is called.

--tag needs to be set after `getnodeversion` because TAG is defined in there when DISTTYPE is not "release", setting it before `getnodeversion` leads to --tag not being passed down in to `configure` and src/node_version.h setting it as `-pre` by default. This change restores the functionality that properly sets the TAG for nightlies, rc builds and other custom build types. Ref: nodejs#17299 Ref: nodejs/abi-stable-node#289

rvagg · 2018-01-09T02:44:24Z

Thanks for testing @digitalinfinity! I've updated the commit message with much more detail explaining what's going on.

This variable leakage is one of the most painful things about vcbuild.bat, it gets very confusing and can ruin subsequent runs. Do you have any thoughts on what we should be doing here to improve the situation? Should we just be planning on converting this whole thing to PowerShell or is there some other build scripting language that we should be aiming for these days? vcbuild.bat is a complicated mess that very few people can understand but it's actually pretty simple (way too simple compared to what we do in Makefile) if you were to lay the basic logic out.

richardlau · 2018-01-09T10:43:33Z

This variable leakage is one of the most painful things about vcbuild.bat, it gets very confusing and can ruin subsequent runs. Do you have any thoughts on what we should be doing here to improve the situation?

setlocal / endlocal?

edit We've tried this and reverted: #16270

gibfahn · 2018-01-09T10:47:05Z

Should we just be planning on converting this whole thing to PowerShell or is there some other build scripting language that we should be aiming for these days?

Discussed in #12310, I think PowerShell had the most support. @refack actually did a lot of the porting work already: https://gist.github.com/refack/969ecc6642981bfd7caef7d948d78076

joaocgreis

Built two test releases: before this change and after this change. The second has correct process.release information, confirming this PR fixes the issue.

mhdawson

LGTM

mhdawson · 2018-01-11T18:44:22Z

CI run: https://ci.nodejs.org/job/node-test-commit/15353/

mhdawson · 2018-01-11T20:32:58Z

Pretty sure ARM failure was not related, but new CI just in case: https://ci.nodejs.org/job/node-test-pull-request/12508/

mhdawson · 2018-01-11T21:47:31Z

ARM failure was unrelated, will land.

mhdawson · 2018-01-11T21:51:21Z

Landed as b225970

--tag needs to be set after `getnodeversion` because TAG is defined in there when DISTTYPE is not "release", setting it before `getnodeversion` leads to --tag not being passed down in to `configure` and src/node_version.h setting it as `-pre` by default. This change restores the functionality that properly sets the TAG for nightlies, rc builds and other custom build types. Ref: #17299 Ref: nodejs/abi-stable-node#289 PR-URL: #18031 Ref: #17299 Ref: nodejs/abi-stable-node#289 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: JoãReis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>

kfarnung · 2018-01-11T22:10:04Z

@mhdawson It looks like this fix needs a backport to v8.x, what's the process for that?

--tag needs to be set after `getnodeversion` because TAG is defined in there when DISTTYPE is not "release", setting it before `getnodeversion` leads to --tag not being passed down in to `configure` and src/node_version.h setting it as `-pre` by default. This change restores the functionality that properly sets the TAG for nightlies, rc builds and other custom build types. Ref: nodejs/node#17299 Ref: nodejs/abi-stable-node#289 PR-URL: nodejs/node#18031 Ref: nodejs/node#17299 Ref: nodejs/abi-stable-node#289 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: JoãReis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>

gibfahn · 2018-01-12T09:39:19Z

It looks like this fix needs a backport to v8.x, what's the process for that?

Given that this is build script regression fix, I'm inclined to bring it back in the next v8.x releases to fix our nightly builds.

Thoughts @nodejs/lts?

It doesn't need to go back to v6.x as #17299 didn't.

richardlau · 2018-01-12T12:02:16Z

Given that this is build script regression fix, I'm inclined to bring it back in the next v8.x releases to fix our nightly builds.

No objections here.

richardlau · 2018-01-12T14:02:17Z

Given that this is build script regression fix, I'm inclined to bring it back in the next v8.x releases to fix our nightly builds.

Are we supposed to be producing nightly builds on 8.x? We appear to have stopped for 8.x since last November (8.9.1): https://nodejs.org/download/nightly/

cc @nodejs/build

joaocgreis · 2018-01-15T12:53:37Z

We are currently producing nightlies from 9.x-staging and master. In nodejs/Release#229 we decided not to produce nightlies for LTS, but we can easily start if anyone finds them useful.

--tag needs to be set after `getnodeversion` because TAG is defined in there when DISTTYPE is not "release", setting it before `getnodeversion` leads to --tag not being passed down in to `configure` and src/node_version.h setting it as `-pre` by default. This change restores the functionality that properly sets the TAG for nightlies, rc builds and other custom build types. Ref: #17299 Ref: nodejs/abi-stable-node#289 PR-URL: #18031 Ref: #17299 Ref: nodejs/abi-stable-node#289 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: JoãReis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Jan 8, 2018

rvagg mentioned this pull request Jan 8, 2018
Get node-test-node-addon-api green on windows nodejs/abi-stable-node#289
Closed

digitalinfinity approved these changes Jan 9, 2018
View reviewed changes

rvagg force-pushed the rvagg/vcbuild-tag-restore branch from 217e689 to c1b106aCompare January 9, 2018 03:35

richardlau approved these changes Jan 9, 2018
View reviewed changes

joaocgreis approved these changes Jan 11, 2018
View reviewed changes

jasnell approved these changes Jan 11, 2018
View reviewed changes

gibfahn approved these changes Jan 11, 2018
View reviewed changes

mhdawson approved these changes Jan 11, 2018
View reviewed changes

kfarnung approved these changes Jan 11, 2018
View reviewed changes

mhdawson closed this Jan 11, 2018

kfarnung mentioned this pull request Jan 12, 2018
build,win: restore vcbuild TAG functionality nodejs/node-chakracore#447
Merged
2 tasks

gibfahn added dont-land-on-v6.x labels Jan 12, 2018

evanlucas mentioned this pull request Jan 30, 2018
v9.5.0 release proposal #18464
Merged

joaocgreis mentioned this pull request May 9, 2018
v8.11.2 proposal #20478
Merged

MylesBorins added land-on-v8.x and removed lts-watch-v8.x labels May 15, 2018

Uh oh!

Uh oh!

Conversation

rvagg commented Jan 8, 2018

Uh oh!

gibfahn commented Jan 8, 2018

Uh oh!

richardlau commented Jan 8, 2018

Uh oh!

digitalinfinity left a comment

Choose a reason for hiding this comment

Uh oh!

digitalinfinity commented Jan 9, 2018

Uh oh!

rvagg commented Jan 9, 2018

Uh oh!

richardlau commented Jan 9, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented Jan 9, 2018

Uh oh!

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

mhdawson commented Jan 11, 2018

Uh oh!

mhdawson commented Jan 11, 2018

Uh oh!

mhdawson commented Jan 11, 2018

Uh oh!

mhdawson commented Jan 11, 2018

Uh oh!

kfarnung commented Jan 11, 2018

Uh oh!

gibfahn commented Jan 12, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Jan 12, 2018

Uh oh!

richardlau commented Jan 12, 2018

Uh oh!

joaocgreis commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

richardlau commented Jan 9, 2018•
edited
Loading

gibfahn commented Jan 12, 2018•
edited
Loading