build,windows: fix vcbuild.bat#13969
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added build 
/cc @joaocgreis @nodejs/release |
tniessen commented Jun 28, 2017
refack changed the title 
joaocgreis left a comment
joaocgreis left a comment There was a problem hiding this comment.
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 commented Jun 29, 2017
lgtm, thanks for getting onto it so quick @refack |
vcbuild.bat Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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]>
* 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]>
* 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]>
* 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]>
* 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]>
* 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]>
* 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]>
9 participants
restore DISTTYPEDIR
Ref: #13900 (review)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build,windows