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

win: rename platform to msbplatform in vcbuild #868

Closed

Conversation

joaocgreis
Copy link
Contributor

@joaocgreis joaocgreis commented May 12, 2016

This variable is used to select 32 or 64 bit builds when invoking MSBuild.

The Visual C++ Build Tools also use the platform variable name, resulting in a conflict when vcvarsall.bat is called. Thus, it is necessary to rename it. This is similar to what was done in node in nodejs/node#5627 .

This enables building with Visual C++ Build Tools without any change.

Ref: #839

@bnoordhuis
Copy link
Member

Maybe call it msbuild_platform for clarity. LGTM apart from that.

This variable is used to select 32 or 64 bit builds when invoking
MSBuild. The Visual C++ Build Tools also use the `platform` variable
name, resulting in a conflict when vcvarsall.bat is called. Thus, it
is necessary to rename it.

Ref: libuv#839
@joaocgreis joaocgreis force-pushed the joaocgreis-G5C-rename-platform branch from c0386ed to f9e5a9f Compare May 12, 2016 09:56
@joaocgreis
Copy link
Contributor Author

Updated to msbuild_platform.

@saghul
Copy link
Member

saghul commented May 12, 2016

LGTM

saghul pushed a commit that referenced this pull request May 12, 2016
This variable is used to select 32 or 64 bit builds when invoking
MSBuild. The Visual C++ Build Tools also use the `platform` variable
name, resulting in a conflict when vcvarsall.bat is called. Thus, it
is necessary to rename it.

Ref: #839
PR-URL: #868
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@saghul
Copy link
Member

saghul commented May 12, 2016

Landed in 11e93aa, thanks Joao!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants