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: Add option to select environment for msbuild (vc2015 or vc2013) #4645

Closed
wants to merge 2 commits into from

Conversation

greenjava
Copy link
Contributor

Add option in vcbuild.bat script to select Visual Studio environment to build Node on Windows.

  • vc2013 to select Visual Studio 2013 environment
  • vc2015 to select Visual Studio 2015 environment

@joaocgreis joaocgreis added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels Jan 12, 2016
@joaocgreis
Copy link
Member

@greenjava thanks for contributing! When the option is passed, I think it should not look for any other version. Something like:

diff --git a/vcbuild.bat b/vcbuild.bat
index 924e3a3..1468ba5 100644
--- a/vcbuild.bat
+++ b/vcbuild.bat
@@ -112,10 +112,8 @@ call :getnodeversion || exit /b 1

 @rem Set environment for msbuild

-if "%target_env%"=="vc2015" goto vc-set-2015
-if "%target_env%"=="vc2013" goto vc-set-2013
-
 :vc-set-2015
+if defined target_env if "%target_env%" NEQ "vc2015" goto vc-set-2013
 @rem Look for Visual Studio 2015
 echo Looking for Visual Studio 2015
 if not defined VS140COMNTOOLS goto vc-set-2013
@@ -139,6 +137,7 @@ set PLATFORM_TOOLSET=v140
 goto msbuild-found

 :vc-set-2013
+if defined target_env if "%target_env%" NEQ "vc2013" goto msbuild-not-found
 @rem Look for Visual Studio 2013
 echo Looking for Visual Studio 2013
 if not defined VS120COMNTOOLS goto msbuild-not-found

Can you update if you agree?

@greenjava
Copy link
Contributor Author

Thanks for your comment, i'll fix that (it's a smarter solution)

@joaocgreis
Copy link
Member

LGTM, I'll land after the regulatory 48 hours pass.

The commit message has to be adjusted to obey the line sizes, when I land I'll change it to something like:

build: add option to select VS version

This changes vcbuild.bat to accept a new parameter (vc2015 or vc2013)
to select the version of Visual Studio to use.

Let me know or update if you disagree. Thanks!

@greenjava
Copy link
Contributor Author

That sounds good :)
Thanks

joaocgreis pushed a commit to JaneaSystems/node that referenced this pull request Jan 14, 2016
This changes vcbuild.bat to accept a new parameter (vc2015 or vc2013)
to select the version of Visual Studio to use.

PR-URL: nodejs#4645
Reviewed-By: João Reis <reis@janeasystems.com>
@joaocgreis
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/1233/ (failures unrelated - ref: #4679)

Landed in 8182ec0 . Thanks!

@rvagg
Copy link
Member

rvagg commented Jan 18, 2016

Nice work @greenjava. It looks like this is your first commit to core, if that's right then welcome on board! It's great to have contributors to the Windows-specific parts of the repo because our collaborator base is bit heavily skewed towards POSIX environments. I hope you stick around and find other places to contribute.

evanlucas pushed a commit that referenced this pull request Jan 18, 2016
This changes vcbuild.bat to accept a new parameter (vc2015 or vc2013)
to select the version of Visual Studio to use.

PR-URL: #4645
Reviewed-By: João Reis <reis@janeasystems.com>
@MylesBorins
Copy link
Contributor

@nodejs/build is this something we want to see in LTS?

@jasnell
Copy link
Member

jasnell commented Mar 11, 2016

hmmm.. not sure. @nodejs/lts @nodejs/build

@joaocgreis
Copy link
Member

This could help somebody out there, but I doubt it. I see this more like a new feature and I wouldn't pick it, but it's fine either way.

@rvagg
Copy link
Member

rvagg commented Mar 14, 2016

+1 for LTS from me, it might be a new feature but not in the semver-minor way since it's build-focused

MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
This changes vcbuild.bat to accept a new parameter (vc2015 or vc2013)
to select the version of Visual Studio to use.

PR-URL: #4645
Reviewed-By: João Reis <reis@janeasystems.com>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
This changes vcbuild.bat to accept a new parameter (vc2015 or vc2013)
to select the version of Visual Studio to use.

PR-URL: #4645
Reviewed-By: João Reis <reis@janeasystems.com>
@MylesBorins MylesBorins mentioned this pull request Mar 21, 2016
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
This LTS release comes with 113 commits, 56 of which are doc related,
18 of which are build / tooling related, 16 of which are test related
and 7 which are benchmark related.

Notable Changes:

* build:
  - Updated Logos for the OSX + Windows installers
    - (Rod Vagg) #5401
    - (Robert Jefe Lindstaedt) #5531
  - New option to select you VS Version in the Windows installer
    - (julien.waechter) #4645
  - Support Visual C++ Build Tools 2015
    - (João Reis) #5627
* tools:
  - Gyp now works on OSX without XCode
    - (Shigeki Ohtsu) #1325
MylesBorins pushed a commit that referenced this pull request Mar 22, 2016
This LTS release comes with 113 commits, 56 of which are doc related,
18 of which are build / tooling related, 16 of which are test related
and 7 which are benchmark related.

Notable Changes:

* build:
  - Updated Logos for the OSX + Windows installers
    - (Rod Vagg) #5401
    - (Robert Jefe Lindstaedt) #5531
  - New option to select your VS Version in the Windows installer
    - (julien.waechter) #4645
  - Support Visual C++ Build Tools 2015
    - (João Reis) #5627
* tools:
  - Gyp now works on OSX without XCode
    - (Shigeki Ohtsu) #1325
MylesBorins pushed a commit that referenced this pull request Mar 22, 2016
This LTS release comes with 113 commits, 56 of which are doc related,
18 of which are build / tooling related, 16 of which are test related
and 7 which are benchmark related.

Notable Changes:

* build:
  - Updated Logos for the OSX + Windows installers
    - (Rod Vagg) #5401
    - (Robert Jefe Lindstaedt) #5531
  - New option to select your VS Version in the Windows installer
    - (julien.waechter) #4645
  - Support Visual C++ Build Tools 2015
    - (João Reis) #5627
* tools:
  - Gyp now works on OSX without XCode
    - (Shigeki Ohtsu) #1325

PR-URL: #5835
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
This changes vcbuild.bat to accept a new parameter (vc2015 or vc2013)
to select the version of Visual Studio to use.

PR-URL: nodejs#4645
Reviewed-By: João Reis <reis@janeasystems.com>
@joaocgreis joaocgreis mentioned this pull request Mar 21, 2017
3 tasks
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.

5 participants