Support Visual Studio 2017 and 2019 #2
Conversation
These are supported in `node-gyp@5`, which we now use. We should be able to detect these versions to set the `--msvs_version` node-gyp flag correctly. The default install path structure is different starting with Visual Studio 2017. e.g. C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\Common7\IDE
Older versions have only one path to check, whereas starting with Visual Studio/C++ Build Tools 2017, multiple distinct install directories exist (one per edition). i.e. Community, Professional, Enterprise, Build Tools, and Express. Conditioning these path checks by version can reduce the filesystem reads (in the worst-case scenario) by 22.
node-gyp v5 refuses to use any Visual Studio older than 2013. With Node >= 8, node-gyp v5 also refuses to use Visual Studio 2013. We bundle and enforce the use of Node 10 now, eventually even newer. Therefore, with versions of Visual Studio < 2015 not usable, we shouldn't detect them or have apm/node-gyp attempt to use them. (This also saves up to 3 filesystem reads.)
In modern node-gyp, this flag only serves to limit node-gyp to using a specific version of Visual Studio. It can't make node-gyp find a version it wouldn't have found. Our checks are much less sophisticated at detecting usable installations of Visual Studio. We should let node-gyp use the install of Visual Studio/Build Tools it thinks is best. It will pick any usable Visual Studio install, preferring the newest if multiple are installed and usable. It generally will not pick one mistakenly that it actually can't use. Given that the stakes are "we might prevent a valid installation of Visual Studio from being used," and there is no benefit to be had, this flag should not be set.
|
This and the "Node 12.0.0" PR are copied exactly from the upstream PRs. |
|
As a general Note: when writing draw-backs, if it doesn't have one, it is better to write From my experience, I see that people are too picky about this part of the description. Just move the information to the description and people will feel better! |
|
Yes. I think you are right, sadly. (Or not sadly, if I think about it.) The instinct to self-review is to show I take contributing changes to upstream seriously, and have considered the ramifications. Then I am so conservative about my changes that there are usually no downsides! So I can't project all this concern and caution when there isn't much reason to feel that way from the code being suggested. I have to strike a balance, or put "none" and let reviewers see if they feel there is a downside. A middle ground option would be to hold back some of the verbosity here. |
Description of the Change
Main change:
Related changes:
--msvs_versionnode-gyp flag, which sets a specific version of Visual Studio that node-gyp must find, or else it will error out.Explanation (click to expand):
Builds on
apm's existing code to detect installs of Visual Studio, by adding the ability to detect Visual Studio 2017 and 2019.Those versions of Visual Studio are now fully supported in the version of node-gyp we use (node-gyp@5).
As of node-gyp PR nodejs/node-gyp#1762, node-gyp is much better than us at finding Visual Studio installs and identifying whether on not they can be used. This isn't a simple matter of having Visual Studio or not, but also various components must be installed.
Given that our detection is worse, I have made it so
apmno-longer sets the--msvs_versionflag. This flag forces node-gyp to use the specified major version of Visual Studio/Build Tools. It can only restrict node-gyp within usable installs. There is no scenario where setting this causes any benefit to us. But if we get it wrong (and our detection is worse, so we very likely would), this can cause node-gyp to refuse to use a valid Visual Studio/Build Tools install and error out.Alternate Designs
This, but with some of the changes reverted/taken out.
Details
All commits in this PR are designed to be logical choices that are well-backed-up by the explanation and reasoning given.
That said, the commits are in order of how radical the change is, or how controversial I thought the change might be. If any of the commits/changes are objectionable, please let me know, and the rest of the PR still makes sense without that change. This is a few related changes bundled together, but any of the three bullet points is a positive step in the right direction, and works independent of the other two bullet points.
Benefits
Possible Drawbacks
None that I am aware of. Before this PR, we have been artificially limiting how good a job node-gyp could do to find Visual Studio, in certain cases. Now, with this PR, we stop doing that.
Reaching for a hypothetical problem: I suppose if Visual Studio 2017 or 2019 are buggy, this exposes our users to that. (In my experience, VS 2017 and 2019 are not buggy.)
Verification Process
Lengthy "Verification Process" section contents (click to expand):
This un-blocks
apmfrom using a correctly-detected, working install of Visual Studio 2019 in (forked) Atom's CI.Breakdown of the "Before" case: The issue in this case was that
apmdetected an install of 2015 that turned out to be unusable. The error is: "could not find MSBuild in registry for this version". At the bottom of the error message, a message points out that 2019 would have been a valid version, had we allowed node-gyp to use it:Furthermore,
apm --versioncan now find Visual Studio 2017 and 2019.Before and after on the same machine, with Visual Studio 2019 Community installed:
Before:
After:
Note that Visual Studio 2019 is detected:
visual studio 2019(on the bottom line of output fromapm --version).Here's the before and after on another machine, this time with Visual Studio 2017:
Before
After
Note that Visual Studio 2017 is detected:
visual studio 2017(on the bottom line of output fromapm --version).Applicable Issues
Upstream PR: atom#892
Fixes atom#893