Conversation
|
/cc @joaocgreis @nodejs/release |
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.
|
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 <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
0c7fc82 to
9e5a211
Compare
* rename :exit to :distexit PR-URL: nodejs#13969 Refs: nodejs#13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
* rename :exit to :distexit PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
* rename :exit to :distexit PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
* rename :exit to :distexit PR-URL: nodejs#13969 Refs: nodejs#13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
* rename :exit to :distexit Backport-PR-URL: #14842 PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
* rename :exit to :distexit Backport-PR-URL: #14842 PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
restore DISTTYPEDIR
Ref: #13900 (review)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build,windows