Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Fix msi installer for Visual Studio versions != 2013 #25569

Closed
wants to merge 4 commits into from

Conversation

misterdjules
Copy link

After landing e7c84f8, the release process on the Windows build machine was broken. The problem was that the following Visual Studio versions and tools had been installed on that machine, in the following order:

  1. Visual Studio 2010
  2. WiX
  3. Visual Studio 2013

As a result, The WiX SDK was not installed for Visual Studio 2013, because that's taken care of during the WiX installation process. At that time, Visual Studio 2013 was not installed on that machine.

However, vcbuild.bat would use Visual Studio 2013 anyway because it could find the corresponding vcvarsall.bat file. So it would detect that it can run the custom_actions project (which has a requirement on the platform toolset that corresponds to Visual Studio 2013), but when it would be time to include the WiX SDK's header for Visual Studio 2013, it would not find it and the Windows installer generation process would fail.

This commit makes the custom_actions project not use any hardcoded value related to the Visual Studio version used to generate the Windows installer. Instead, msbuild passes the Visual Studio version number that vcbuild.bat detects to the custom_actions project.

In my case, I had to re-run the WiX installer process to install the WiX SDK for Visual Studio 2013, because I wanted to release the next v0.10.x release with the same compiler than the one that had been used for a while now (since September 2014, when Visual Studio 2013 was installed on that build machine). However this PR fixes the problem for users who don't have access to Visual Studio 2013 for some reason, and I think it just improves the user experience in general (better error messages, etc.). It will come in handy when/if we update the compiler used to build official releases to Visual Studio 2015.

This PR also backports 3122052 and a58b174 as they deal and fix issues about generating installers and binaries on Windows machines with different Visual Studio versions.

/cc @joyent/node-collaborators @joyent/node-tsc

@joaocgreis
Copy link
Member

LGTM with one suggestion: I don't have an older version of VS here to reproduce the problem @richardlau had with WiX 3.6, but if WiX 3.9 is required for any version of VS we can add RequiredVersion="3.9.1208.0" in the WiX element of product.wxs.

@misterdjules
Copy link
Author

@orangemocha @piscisaureus @joaocgreis Do we actually support building node on Windows with Visual Studio < 2013? I get the following error when building it with Visual Studio 2010:

C:\Users\jgilli\dev\node>vcbuild.bat nosign
ctrpp not found in WinSDK path--using pre-gen files from tools/msvs/genfiles.
{ 'target_defaults': { 'cflags': [],
                       'default_configuration': 'Release',
                       'defines': [],
                       'include_dirs': [],
                       'libraries': []},
  'variables': { 'clang': 0,
                 'host_arch': 'x64',
                 'node_install_npm': 'true',
                 'node_prefix': '',
                 'node_shared_cares': 'false',
                 'node_shared_http_parser': 'false',
                 'node_shared_libuv': 'false',
                 'node_shared_openssl': 'false',
                 'node_shared_v8': 'false',
                 'node_shared_zlib': 'false',
                 'node_tag': '',
                 'node_unsafe_optimizations': 0,
                 'node_use_dtrace': 'false',
                 'node_use_etw': 'true',
                 'node_use_openssl': 'true',
                 'node_use_perfctr': 'true',
                 'node_use_systemtap': 'false',
                 'openssl_no_asm': 0,
                 'python': 'C:\\Python27\\python.exe',
                 'target_arch': 'ia32',
                 'v8_enable_gdbjit': 0,
                 'v8_no_strict_aliasing': 1,
                 'v8_use_snapshot': 'true',
                 'visibility': '',
                 'want_separate_host_toolset': 1}}
creating  config.gypi
creating  config.mk
Project files generated.
Setting environment for using Microsoft Visual Studio 2010 x86 tools.
Project file contains ToolsVersion="12.0". This toolset may be unknown or missi
ng, in which case you may be able to resolve this by installing the appropriate
 version of MSBuild, or the build may have been forced to a particular ToolsVer
sion for policy reasons. Treating the project as if it had ToolsVersion="4.0".
For more information, please see http://go.microsoft.com/fwlink/?LinkId=291333.
C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\Platforms\Win32\Microsoft.Cpp
.Win32.Targets(511,5): error MSB8008: Specified platform toolset (v120) is not
installed or invalid. Please make sure that a supported PlatformToolset value i
s selected. [C:\Users\jgilli\dev\node\deps\cares\cares.vcxproj]

@richardlau
Copy link
Member

@misterdjules That output looks like gyp has decided to generate project files for a later version (2013?). On our systems that only have Visual Studio 2010 the generated cares.vcxproj file has ToolsVersion="4.0".

Looking at vcbuild.bat in the v0.10 branch it does seem odd that the batch file attempts to set the GYP_MSVS_VERSION env variable but it looks like it's doing this after gyp has already run (and thus wouldn't affect it) so gyp will attempt to autodetect the Visual Studio version to target.

@misterdjules
Copy link
Author

@richardlau Right, this has been fixed by @orangemocha with a74425b. I was just confused, thank you for pointing me again in the right direction 👍

@misterdjules
Copy link
Author

@orangemocha @joaocgreis Added more info output in misterdjules@84cd9a7.

@joaocgreis I could not find a way to make WiX's RequiredVersion attribute error out or warn me when using a different version than the required one. It seems it would just be ignored by WiX. Did you manage to get it working?

@misterdjules
Copy link
Author

@orangemocha Changed VStudio to Visual Studio according to your latest comment, thank you!

@joaocgreis
Copy link
Member

@misterdjules Sorry for the delay, I wanted to try it with older versions of WiX to be sure. On my system it works:

Building node-0.12.5
C:\Users\Administrator\Desktop\joyent_v0.12\tools\msvs\msi\product.wxs(2): error CNDL0007: The current version of the toolset is 3.8.1128.0, but version 3.9.1208.0 is required. [C:\Users\Administrator\Desktop\joyent_v0.12\tools\msvs\msi\nodemsi.wixproj]

Just to be sure we are on the same page:

--- a/tools/msvs/msi/product.wxs
+++ b/tools/msvs/msi/product.wxs
@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <Wix xmlns="http://schemas.microsoft.com/wix/2006/wi"
-     xmlns:util="http://schemas.microsoft.com/wix/UtilExtension">
+     xmlns:util="http://schemas.microsoft.com/wix/UtilExtension" RequiredVersion="3.9.1208.0">

   <?define ProductName = "Node.js" ?>
   <?define ProductDescription = "Node.js" ?>

@orangemocha
Copy link
Contributor

Aside from the Wix RequiredVersion pointed out by @joaocgreis , the output changes LGTM.

@misterdjules
Copy link
Author

@joaocgreis Yes, I had the same change in mind. Since then I found out that this works correctly in v0.12, but fails to display any diagnostic in v0.10. I don't know what the reason for that difference in behavior is.

Anyway, if we can't figure it out, we can still add this change so that when it's merged in v0.12 we'll get a better diagnostic for such errors.

Thoughts?

@joaocgreis
Copy link
Member

@misterdjules The error messages were not very clear, but it seems that supporting older versions of WiX is simple. I just renamed the custom action source file to be C++ and it worked, seems that older versions of Wix include a header with extern "C" on it. I based this on your last commit, feel free to include: JaneaSystems/node@4342efb

I tested with WiX 3.8 and was able to install the MSI. According to the installation log, the custom action ran fine.

@misterdjules
Copy link
Author

@joaocgreis Excellent, thank you very much for investigating this! I merged your commit in this PR.

@joaocgreis @orangemocha LGTY?

@misterdjules
Copy link
Author

@orangemocha @joaocgreis Also, I squashed two of my commits into one, but the others should be kept separate in my opinion.

@joaocgreis
Copy link
Member

@misterdjules Only one suggestion (sorry for noticing this only now): To determine what version of VS to pick from the WiX folder, you use a property called VisualStudioVersion (takes values like 2012, 2013). However, there is an environment variable with the exact same name set by vcvarsall.bat, that has values like 11.0 or 12.0. This might create some confusion so I suggest changing the name of the property to GypMsvsVersion like the variable that it's taken from in vcbuild.bat or something else that you like. Something like:

sed s/VisualStudioVersion/GypMsvsVersion/g -i vcbuild.bat tools/msvs/msi/custom_actions.vcxproj

This works as it is now, so either way, with or without this change, LGTM.

Julien Gilli and others added 2 commits July 23, 2015 10:39
The original change that added support for running custom actions during
the install process (e7c84f8) assumed
that Visual Studio 2013 is used to generate the installer file.

However, that is not always the case, and older versions of Visual
Studio should allow users to generate Windows installer files. This
change makes the custom actions visual studio project use the visual
studio version that is found by vcbuild.bat.
Older WiX versions included a header with extern "C" declaration,
hence the custom action source must be C++.
@misterdjules
Copy link
Author

@joaocgreis Thanks again, updated 👍

orangemocha added a commit that referenced this pull request Jul 23, 2015
Backport commit a58b174 from branch
v0.12.

Original commit message:

  vcbuild.bat calls python configure before setting GYP_MSVS_VERSION,
  so SelectVisualStudioVersion (tools\gyp\pylib\gyp\MSVSVersion.py)
  defaults to 'auto' and selects VS 2005.

  vcbuild sets the environment in the current shell, so this issue
  would manifest itself only on the first invocation of the script
  in any given shell windows.

  Reviewed-By: Julien Gilli <[email protected]>
  PR-URL: #20109

Conflicts:
	vcbuild.bat

Reviewed-By: João Reis <[email protected]>
PR-URL: #25569
misterdjules pushed a commit that referenced this pull request Jul 23, 2015
Issue: #25348
The gyp/project files don't explicitly specify a subsystem version,
which results in the default being used. The default changed from
VS 2010 to VS 2012 and later.

Backport 3122052, which itself
backports e8d0850 from io.js.
Original commit message follows:

  Chrome still runs on Windows XP, so there is no reason that iojs
  couldn't.

  PR: nodejs/node#512
  (cherry picked from commit e8d0850)

  Reviewed-By: Julien Gilli <[email protected]>
  PR-URL: #25367

Conflicts:
	node.gyp

Reviewed-By: João Reis <[email protected]>
PR-URL: #25569
misterdjules pushed a commit that referenced this pull request Jul 23, 2015
The original change that added support for running custom actions during
the install process (e7c84f8) assumed
that Visual Studio 2013 is used to generate the installer file.

However, that is not always the case, and older versions of Visual
Studio should allow users to generate Windows installer files. This
change makes the custom actions visual studio project use the visual
studio version that is found by vcbuild.bat.

Reviewed-By: João Reis <[email protected]>
PR-URL: #25569
joaocgreis added a commit that referenced this pull request Jul 23, 2015
Older WiX versions included a header with extern "C" declaration,
hence the custom action source must be C++.

Reviewed-By: João Reis <[email protected]>
PR-URL: #25569
@misterdjules
Copy link
Author

Landed in 10f251e, 517986c, 16bcd68 and e192f61.

joaocgreis pushed a commit to JaneaSystems/node that referenced this pull request Aug 24, 2015
This is a port of nodejs/node-v0.x-archive@16bcd68 .

Original commit message:

  The original change that added support for running custom actions
  during the install process
  (nodejs/node-v0.x-archive@e7c84f8) assumed that
  Visual Studio 2013 is used to generate the installer file.

  However, that is not always the case, and older versions of Visual
  Studio should allow users to generate Windows installer files. This
  change makes the custom actions visual studio project use the visual
  studio version that is found by vcbuild.bat.

  Reviewed-By: João Reis <[email protected]>
  PR-URL: nodejs/node-v0.x-archive#25569

PR-URL: nodejs#2365
Reviewed-By: Alexis Campailla <[email protected]>
joaocgreis added a commit to JaneaSystems/node that referenced this pull request Aug 24, 2015
This is a port of nodejs/node-v0.x-archive@e192f61 .

Original commit message:

  Older WiX versions included a header with extern "C" declaration,
  hence the custom action source must be C++.

  Reviewed-By: João Reis <[email protected]>
  PR-URL: nodejs/node-v0.x-archive#25569

PR-URL: nodejs#2365
Reviewed-By: Alexis Campailla <[email protected]>
misterdjules pushed a commit to nodejs/node that referenced this pull request Aug 24, 2015
This is a port of nodejs/node-v0.x-archive@16bcd68 .

Original commit message:

  The original change that added support for running custom actions
  during the install process
  (nodejs/node-v0.x-archive@e7c84f8) assumed that
  Visual Studio 2013 is used to generate the installer file.

  However, that is not always the case, and older versions of Visual
  Studio should allow users to generate Windows installer files. This
  change makes the custom actions visual studio project use the visual
  studio version that is found by vcbuild.bat.

  Reviewed-By: João Reis <[email protected]>
  PR-URL: nodejs/node-v0.x-archive#25569

PR-URL: #2365
Reviewed-By: Alexis Campailla <[email protected]>
joaocgreis added a commit to nodejs/node that referenced this pull request Aug 24, 2015
This is a port of nodejs/node-v0.x-archive@e192f61 .

Original commit message:

  Older WiX versions included a header with extern "C" declaration,
  hence the custom action source must be C++.

  Reviewed-By: João Reis <[email protected]>
  PR-URL: nodejs/node-v0.x-archive#25569

PR-URL: #2365
Reviewed-By: Alexis Campailla <[email protected]>
joaocgreis pushed a commit to JaneaSystems/node that referenced this pull request Sep 12, 2015
This is a port of 16bcd68 .

Original commit message:

  The original change that added support for running custom actions
  during the install process (e7c84f8)
  assumed that Visual Studio 2013 is used to generate the installer
  file.

  However, that is not always the case, and older versions of Visual
  Studio should allow users to generate Windows installer files. This
  change makes the custom actions visual studio project use the visual
  studio version that is found by vcbuild.bat.

  Reviewed-By: João Reis <[email protected]>
  PR-URL: nodejs/node-v0.x-archive#25569
joaocgreis added a commit to JaneaSystems/node that referenced this pull request Sep 12, 2015
This is a port of e192f61 .

Original commit message:

  Older WiX versions included a header with extern "C" declaration,
  hence the custom action source must be C++.

  Reviewed-By: João Reis <[email protected]>
  PR-URL: nodejs/node-v0.x-archive#25569
misterdjules pushed a commit to nodejs/node that referenced this pull request Sep 16, 2015
This is a port of 16bcd68 .

Original commit message:

  The original change that added support for running custom actions
  during the install process (e7c84f8)
  assumed that Visual Studio 2013 is used to generate the installer
  file.

  However, that is not always the case, and older versions of Visual
  Studio should allow users to generate Windows installer files. This
  change makes the custom actions visual studio project use the visual
  studio version that is found by vcbuild.bat.

  Reviewed-By: João Reis <[email protected]>
  PR-URL: nodejs/node-v0.x-archive#25569

PR-URL: #2843
Reviewed-By: rvagg - Rod Vagg <[email protected]>
Reviewed-By: orangemocha - Alexis Campailla <[email protected]>
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
joaocgreis added a commit to nodejs/node that referenced this pull request Sep 16, 2015
This is a port of e192f61 .

Original commit message:

  Older WiX versions included a header with extern "C" declaration,
  hence the custom action source must be C++.

  Reviewed-By: João Reis <[email protected]>
  PR-URL: nodejs/node-v0.x-archive#25569

PR-URL: #2843
Reviewed-By: rvagg - Rod Vagg <[email protected]>
Reviewed-By: orangemocha - Alexis Campailla <[email protected]>
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
This is a port of 16bcd68 .

Original commit message:

  The original change that added support for running custom actions
  during the install process (e7c84f8)
  assumed that Visual Studio 2013 is used to generate the installer
  file.

  However, that is not always the case, and older versions of Visual
  Studio should allow users to generate Windows installer files. This
  change makes the custom actions visual studio project use the visual
  studio version that is found by vcbuild.bat.

  Reviewed-By: João Reis <[email protected]>
  PR-URL: nodejs#25569

PR-URL: nodejs/node#2843
Reviewed-By: rvagg - Rod Vagg <[email protected]>
Reviewed-By: orangemocha - Alexis Campailla <[email protected]>
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
This is a port of e192f61 .

Original commit message:

  Older WiX versions included a header with extern "C" declaration,
  hence the custom action source must be C++.

  Reviewed-By: João Reis <[email protected]>
  PR-URL: nodejs#25569

PR-URL: nodejs/node#2843
Reviewed-By: rvagg - Rod Vagg <[email protected]>
Reviewed-By: orangemocha - Alexis Campailla <[email protected]>
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants