build,win,msi: support WiX with VS2017 #17101
Conversation
joaocgreis
added
build
nodejs-github-bot
added
build
| @rem check if VS2017 is already setup, and for the requested arch | ||
| if "_%VisualStudioVersion%_" == "_15.0_" if "_%VSCMD_ARG_TGT_ARCH%_"=="_%target_arch%_" goto found_vs2017 | ||
| @rem need to clear VSINSTALLDIR for vcvarsall to work as expected | ||
| set "VSINSTALLDIR=" | ||
| call tools\msvs\vswhere_usability_wrapper.cmd | ||
| if "_%VCINSTALLDIR%_" == "__" goto vs-set-2015 |
There was a problem hiding this comment.
I thought the idea was to avoid calling vswhere_usability_wrapper.cmd if it's already setup? This will call it every time.
There was a problem hiding this comment.
vcvarsall.bat below is the one to avoid. vswhere is not a problem and is needed for the WiX checks.
|
I would actually like to take this opportunity to refactor out all the release-only parts from |
| if not exist "%WIX%\SDK\VS2017" ( | ||
| echo Failed to find WiX install for Visual Studio 2017 | ||
| echo VS2017 support for WiX is only present starting at version 3.11 | ||
| goto vs-set-2015 |
There was a problem hiding this comment.
Should we really fall back to VS2015 (or exit once #16969 lands) if the extension is not installed? Perhaps msi should just be skipped instead?
There was a problem hiding this comment.
MSI is not enabled by default, and this only applies if vcbuild was explicitly called with msi. So, skipping the msi here would be ignoring a parameter. Given that this is for releases, not just a nice to have, I don't think we should skip.
|
@refack can we go ahead with this here and do the refactor separately? I'd like to move ahead with the VS2017 transition for v10. |
|
@nodejs/platform-windows @nodejs/build PTAL |
| @@ -209,6 +209,8 @@ Prerequisites: | |||
| * Basic Unix tools required for some tests, | |||
| [Git for Windows](http://git-scm.com/download/win) includes Git Bash | |||
| and tools which can be included in the global `PATH`. | |||
| * **Optional** (to build the MSI): the [WiX Toolset v3.11](http://wixtoolset.org/releases/) | |||
| and the [Wix Toolset Visual Studio 2017 Extension](https://marketplace.visualstudio.com/items?itemName=RobMensching.WixToolsetVisualStudio2017Extension). | |||
There was a problem hiding this comment.
Building MSI was previously undocumented, correct?
Do you know what changes caused this? The extension wasn't required for VS2015.
How come this isn't necessary when building node itself? |
VS2017 introduced the ability to install different editions side-by-side. MSBuild was changed to look for files in the directory relative to the installation in use, and ignore the global directory that was used for VS2015. The extension needs to be installed in the edition of VS2017 that will be used to build the MSI, it installs an extra copy of the targets files in the correct directory for MSBuild to find. Ref: wixtoolset/issues#5525 (comment)
It is, but Gyp handles that. The project files used to build the MSI are not generated by Gyp, so the SDK version needs to be passed explicitly. (IIRC there were attempts to generate with Gyp in the past, but it's not straightforward because of the WiX specific parts. Since the files are quite simple as they are, the effort is hard to justify.) |
PR-URL: #17101 Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
PR-URL: #17101 Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
PR-URL: #17101 Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
|
Any reason not to include this in the next v8.x release? |
|
@gibfahn no, but it's not very important either. Feel free to include it if it lands cleanly. |
PR-URL: #17101 Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
To support releasing with VS2017, we need to be able to build the MSI with it. This PR:
Ref: #13052
Ref: #16969
cc @nodejs/platform-windows
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build, win, msi