Skip to content

build,windows: fix vcbuild.bat #13969

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 30, 2017
Merged

build,windows: fix vcbuild.bat #13969

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions vcbuild.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,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 •
edited
Loading

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.

)
if "%DISTTYPE%"=="custom" (
if not defined CUSTOMTAG (
Expand All @@ -561,4 +561,7 @@ if not "%DISTTYPE%"=="custom" (
set TAG=%DISTTYPE%%DATESTRING%%COMMIT%
)
set FULLVERSION=%NODE_VERSION%-%TAG%
exit /b 0

:distexit
if not defined DISTTYPEDIR set DISTTYPEDIR=%DISTTYPE%
goto :EOF