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

build,windows: fix vcbuild.bat #13969

Merged
merged 1 commit into from
Jun 30, 2017
Merged

Conversation

refack
Copy link
Contributor

@refack refack commented Jun 28, 2017

restore DISTTYPEDIR
Ref: #13900 (review)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build,windows

@nodejs-github-bot 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 Jun 28, 2017
@refack
Copy link
Contributor Author

refack commented Jun 28, 2017

/cc @joaocgreis @nodejs/release
Apparently 614dbbd broke the release script. We need to fast track this
[edit]
No need for fast track since 614dbbd is not part of any release branch yet.

@refack refack mentioned this pull request Jun 28, 2017
@tniessen
Copy link
Member

@refack refack changed the title build,windows: partial revert of 614dbbd build,windows: fix vcbuild.bat Jun 28, 2017
Copy link
Member

@joaocgreis joaocgreis left a comment

Choose a reason for hiding this comment

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

I also added a fix to the CI script (nodejs/build#776) because the nightlies would break. There should be no issue with having both, so I believe this should land.

@rvagg
Copy link
Member

rvagg commented Jun 29, 2017

lgtm, thanks for getting onto it so quick @refack

@@ -530,7 +530,7 @@ if not defined NODE_VERSION (
if not defined DISTTYPE set DISTTYPE=release
if "%DISTTYPE%"=="release" (
set FULLVERSION=%NODE_VERSION%
exit /b 0
goto distexit
Copy link
Member

@gibfahn gibfahn Jun 29, 2017

Choose a reason for hiding this comment

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

Is there a reason the goto is needed? If it's only used in this one function can't you just do:

 if "%DISTTYPE%"=="release" (
     set FULLVERSION=%NODE_VERSION%
+   if not defined DISTTYPEDIR set DISTTYPEDIR=%DISTTYPE%
   exit /b 0
)
set FULLVERSION=%NODE_VERSION%-%TAG%
+if not defined DISTTYPEDIR set DISTTYPEDIR=%DISTTYPE%

It's possible probable I'm misunderstanding some of the subtleties of this script.

Copy link
Member

Choose a reason for hiding this comment

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

The two implementations are functionally the same.

Here the idea is to have two parts: first do checks and set variables depending on the DISTTYPE, then goto a common block for all DISTTYPEs that may depend on the variables set above. I prefer the current change as it avoids duplicating the same set, but no strong feelings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think at some point the subroutine was used more than once... Now it's a degenerate label, but I as well would rather keep it this way...

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that the set repetition is also not great. If you guys prefer it this way then let's do that! I just found the goto :EOF as the last line confusing, but hopefully long-term we can move to the Powershell promised land anyway.

* rename :exit to :distexit

PR-URL: nodejs#13969
Refs: nodejs#13900 (review)
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@refack refack force-pushed the restore-DISTTYPEDIR branch from 0c7fc82 to 9e5a211 Compare June 30, 2017 20:14
@refack refack merged commit 9e5a211 into nodejs:master Jun 30, 2017
@refack refack deleted the restore-DISTTYPEDIR branch July 2, 2017 02:33
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 3, 2017
* rename :exit to :distexit

PR-URL: nodejs#13969
Refs: nodejs#13900 (review)
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@addaleax addaleax mentioned this pull request Jul 3, 2017
addaleax pushed a commit that referenced this pull request Jul 11, 2017
* rename :exit to :distexit

PR-URL: #13969
Refs: #13900 (review)
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
* rename :exit to :distexit

PR-URL: #13969
Refs: #13900 (review)
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
refack added a commit to refack/node that referenced this pull request Aug 15, 2017
* rename :exit to :distexit

PR-URL: nodejs#13969
Refs: nodejs#13900 (review)
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
* rename :exit to :distexit

Backport-PR-URL: #14842
PR-URL: #13969
Refs: #13900 (review)
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants