build,win,msi: support WiX with VS2017 by joaocgreis · Pull Request #17101 · nodejs/node · GitHub

To support releasing with VS2017, we need to be able to build the MSI with it. This PR:

Passes the Windows SDK version to the MSBuild invocation that builds the MSI, to support SDKs other than 8.1.
Adds warnings if WiX cannot be used. WiX for VS2017 requires an extension to be installed because of MSBuild changes, this adds a warning if it's missing.
Adds a note in the documentation about WiX.

Ref: #13052
Ref: #16969

cc @nodejs/platform-windows

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

build, win, msi

CI: https://ci.nodejs.org/view/All/job/node-test-pull-request/11498/

I thought the idea was to avoid calling vswhere_usability_wrapper.cmd if it's already setup? This will call it every time.

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 vcbuild.bat. I'll try to get it done by the start of next week.

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?

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

Building MSI was previously undocumented, correct?

Correct.

WiX for VS2017 requires an extension to be installed because of MSBuild changes

Do you know what changes caused this? The extension wasn't required for VS2015.

Passes the Windows SDK version to the MSBuild invocation that builds the MSI, to support SDKs other than 8.1.

How come this isn't necessary when building node itself?

Do you know what changes caused this? The extension wasn't required for VS2015.

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)

How come this isn't necessary when building node itself?

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.)

LGTM

PR-URL: #17101 Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

Landed in 04566d3

@seishun thanks for the review! I'll move forward with the releases very soon.

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>

build,win,msi: support WiX with VS2017

031a22e

joaocgreis added build Issues and PRs related to build files or the CI. dont-land-on-v4.x windows Issues and PRs related to the Windows platform. labels Nov 17, 2017

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. windows Issues and PRs related to the Windows platform. labels Nov 17, 2017

seishun reviewed Nov 17, 2017

View reviewed changes

refack mentioned this pull request Nov 18, 2017

Last builds lack Windows installers and binaries nodejs/node-v8#19

Closed

seishun reviewed Nov 20, 2017

View reviewed changes

seishun reviewed Nov 26, 2017

View reviewed changes

seishun approved these changes Nov 27, 2017

View reviewed changes

joaocgreis added a commit that referenced this pull request Nov 28, 2017

build,win,msi: support WiX with VS2017

04566d3

PR-URL: #17101 Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

joaocgreis closed this Nov 28, 2017

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017

build,win,msi: support WiX with VS2017

a441d05

PR-URL: #17101 Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017

build,win,msi: support WiX with VS2017

87c885b

PR-URL: #17101 Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn pushed a commit that referenced this pull request Feb 19, 2018

build,win,msi: support WiX with VS2017

44ab4f0

PR-URL: #17101 Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

gibfahn added land-on-v8.x and removed lts-watch-v8.x labels Feb 19, 2018

               @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

+                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

                 [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).

build,win,msi: support WiX with VS2017 by joaocgreis · Pull Request #17101 · nodejs/node · GitHub

Uh oh!

Uh oh!

build,win,msi: support WiX with VS2017 - #17101

build,win,msi: support WiX with VS2017#17101joaocgreis wants to merge 1 commit intonodejs:masternodejs/node:masterfrom JaneaSystems:joaocgreis-HBH-vs2017-wixJaneaSystems/node:joaocgreis-HBH-vs2017-wixCopy head branch name to clipboard

Conversation

joaocgreis commented Nov 17, 2017

Checklist

Affected core subsystem(s)

Uh oh!

joaocgreis commented Nov 17, 2017

Uh oh!

seishun Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

joaocgreis Nov 20, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Nov 17, 2017

Uh oh!

seishun Nov 20, 2017

Choose a reason for hiding this comment

Uh oh!

joaocgreis Nov 21, 2017

Choose a reason for hiding this comment

Uh oh!

joaocgreis commented Nov 23, 2017

Uh oh!

joaocgreis commented Nov 23, 2017

Uh oh!

seishun Nov 26, 2017

Choose a reason for hiding this comment

Uh oh!

richardlau Nov 26, 2017

Choose a reason for hiding this comment

Uh oh!

seishun commented Nov 26, 2017

Uh oh!

joaocgreis commented Nov 27, 2017

Uh oh!

seishun left a comment

Choose a reason for hiding this comment

Uh oh!

joaocgreis commented Nov 28, 2017

Uh oh!

gibfahn commented Dec 19, 2017

Uh oh!

joaocgreis commented Jan 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

build,win,msi: support WiX with VS2017#17101
joaocgreis wants to merge 1 commit into
nodejs:masterfrom
JaneaSystems:joaocgreis-HBH-vs2017-wix