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

win: Support Visual Studio 2019 #1715

Closed
wants to merge 70 commits into from

Conversation

Projects
None yet
@syohex
Copy link

commented Apr 9, 2019

Checklist
Description of change
  • Support Visual Studio 2019.
    Original code does not work with Visual Studio 2019 because path of VS2019's MSBuild.exe is different from VS2017's MSBuild.exe.
  • And improve error check. If msbuild.exe is not found, then throws error.

isaacs and others added some commits May 31, 2017

Upgrade to tar v3
Tar version 3 performs better and is more well tested than its
predecessor.  npm will be using this in the near future, so there is no
benefit in shipping a node-gyp that uses the slower and less reliable
fstream-based tar.

This drops support for node 0.x, and thus should be considered a
breaking semver-major change.

PR-URL: #1212
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
bump to v4.0.0
* dropping support for node < 4
* signal the CI not to test node < 4
configure: don't set ensure if tarball is set
If you're providing a path to a header tarball to install, you probably
want it to always be re-installed.

PR-URL: #1220
Fixes: #1216
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gyp: update xml string encoding conversion
* test: build simple addon in path with non-ascii characters
* test: add test-charmap.py

PR-URL: #1203
Reviewed-By: Refael Ackermann <refack@gmail.com>
doc: headerify the Install instructions
Enable linking to the platform specific installation instructions

PR-URL: #1225
Reviewed-By: Refael Ackermann <refack@gmail.com>
doc: update proposed DCO and CoC
Lifted verbatim from
https://github.com/nodejs/node/blob/master/CONTRIBUTING.md
then `s/Node.js/node-gyp/`.

PR-URL: #1229
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
doc: add github PR and Issue templates
Give users reporting bugs a clearer idea of the info that will be
helpful when reporting issues.

PR-URL: #1228
Refs: https://github.com/nodejs/node/tree/master/.github
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@SimenB

This comment has been minimized.

Copy link
Member

commented on 75cfae2 Aug 28, 2017

This isn't published, is that on purpose?

This comment has been minimized.

Copy link

replied Aug 3, 2018

Also curious if you can briefly mention what warranted the major bump?

This comment has been minimized.

Copy link

replied Sep 13, 2018

@benjamm drop node versions is a breaking change

This comment has been minimized.

Copy link

replied Apr 17, 2019

refack and others added some commits Sep 14, 2017

win: run PS with `-NoProfile`
PR-URL: #1292
Refs: #1195 (comment)
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
zos: support platform
Initial work to add z/OS support to node-gyp.


PR-URL: #1276
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fix IndexError when parsing GYP files.
GYP automatically turns variables ending in _dir, _file or _path into
absolute paths but didn't check for empty strings.

It interacted badly with variables inherited through the environment
from npm, the `scripts-prepend-node-path=false` setting in particular
because it is turned into `npm_config_script_prepend_node_path=`.

Fixes: #1217
PR-URL: #1267
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Update `--nodedir` description in README.
The description erroneously stated that it should point the node binary.
It needs to point to the node source code.

PR-URL: #1372
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Fix infinite install loop.
Retry the download+install dance only once after encountering an EACCES.

That only happens when both the devdir (usually: `$HOME/.node-gyp`) and
the current working directory aren't writable.  Users won't often hit
that except through `sudo npm install` because npm drops privileges
before executing node-gyp.

Fixes: #1383
PR-URL: #1384
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
gyp: don't print xcodebuild not found errors
As node-gyp rebuild doesn't seem to need xcodebuild, we don't need to be
printing the error every time GYP is run.

PR-URL: #1370
Fixes: #569
Refs: #1057
Refs: https://chromium-review.googlesource.com/c/492046/
@jhenkens

This comment has been minimized.

Copy link

commented on 63f43c2 May 24, 2018

I just wasted two days tracking down node-gyp issues to this exact change. Would be nice if this shipped.

josh- and others added some commits Jun 8, 2018

doc: update macOS information in README
PR-URL: #1323
Fixes: #1295
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
gyp: fix regex to match multi-digit versions
This fixes the regular expression matching in `xcode_emulation`
to also handle version numbers with multiple-digit major versions
which would otherwise break under use of XCode 10

Fixes: #1454
PR-URL: #1455
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
zos: add search locations for libnode.x
Node.js on z/OS uses shared dll (libnode.so). When linking native
addons, node-gyp needs to find the corresponding libnode.x during
the link step.

PR-URL: #1451
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
zos: don't use universal-new-lines mode
PR-URL: #1451
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gyp: add support for .mm files to msvs generator
PR-URL: #1167
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Prefix build targets with /t: on Windows
Currently, on non-Windows platforms, it is possible to have
a multi-target native module and specify building just some
of the targets using `node-gyp build my_target`.

On Windows, however, specifying the targets to build requires the `/t:`
or `/target:` flag. Without this change you will get an `MSB1008` error
because MSBuild thinks you are trying to specify multiple solutions.

PR-URL: #1164
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fix include path when pointing to Node.js source
Header files for deps are in a different location in the Node.js
source tree compared to the release tarballs.

PR-URL: #1055
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Drop dependency on minimatch.
PR-URL: #1158
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gyp: escape spaces in filenames in make generator
PR-URL: #1436
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Remove unused gyp test scripts.
Shrinks node-gyp's size by about 100 kB.

PR-URL: #1458
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
doc: lint README.md
Fixes grammar, removes extra lines and spaces, etc. Also removes a few
references to `node-waf`, which was removed ~6 years ago now. Happy to
add back if people still need that information.

PR-URL: #1498
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
bin,lib: remove extra comments/lines/spaces
- Removes "module dependencies" comments and things that, IMHO, don't add
too much value. Happy to add back if helps some people when reading
through `node-gyp`.
- DRY up `lib/process-release.js`.
- Removes a bunch of extra blank lines, as well as random spaces.

PR-URL: #1508
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Add ESLint no-unused-vars rule
- Uses `.eslintrc.yaml` for configuration
- `npm run lint` is part of `npm test`

PR-URL: #1497
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
win: improve parsing of SDK version
This makes the parsing more robust and fixes the additional issue
related to USB Device Connectivity component.

Fixes: #1466
PR-URL: #1516
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

cclauss and others added some commits Nov 14, 2017

gyp: fix target --> self.target
target is an undefined name in this context.

#1334
Reviewed-By: Refael Ackermann <refack@gmail.com>
deps: replace `osenv` dependency with native `os`
Breaking change: needs Node.js version 6 or higher

#1570
Reviewed-By: Refael Ackermann <refack@gmail.com>
bin: follow XDG OS conventions for storing data
PR-URL: #1570
Fixes: #175
Fixes: #1124
Reviewed-By: Refael Ackermann <refack@gmail.com>
lib: use print() for python version detection
PR-URL: #1534
Reviewed-By: Refael Ackermann <refack@gmail.com>
tools: fix usage of inherited -fPIC and -fPIE
PR-URL: #1340
Reviewed-By: Refael Ackermann <refack@gmail.com>
win: fix delay-load hook for electron 4
PR-URL: #1566
Reviewed-By: Refael Ackermann <refack@gmail.com>
python: more informative error
PR-URL: #1269
Refs: #1582
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
python: clean-up detection
Try everything until Python is found.

PR-URL: #1582
Reviewed-By: Rod Vagg <rod@vagg.org>
doc: python info needs own header
PR-URL: #1245
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
doc: improve issue template
Suggest using --verbose npm switch when providing logs. Hopefully,
better direct users to use backticks correctly.

PR-URL: #1618
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@marechal-p

This comment has been minimized.

Copy link
Contributor

commented on 5c2aad8 Feb 5, 2019

Why is this file here?

AFAIK, there is no way to point to it using the --format option the way it is?

Is it intentional?

marechal-p and others added some commits Feb 6, 2019

gyp: move compile_commands_json
It isn't possible to access
`tools/gyp/pylib/gyp/generator/compile_commands_json.py` using the
`--format` option.

This commit moves the file in a place where it can be accessed.

Fixes: #1526
PR-URL: #1661
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
Add ARM64 to MSBuild /Platform logic
PR-URL: #1655
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: João Reis <reis@janeasystems.com>
readme: add ARM64 info to MSVC setup instructions
Since ARM64 build dependencies are not included in the "Desktop
development with C++" workload of Visual Studio 2017, this change adds
a note explaining what additional components are required to build
native modules for the new Windows platform.

PR-URL: #1655
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: João Reis <reis@janeasystems.com>
Remove an outdated workaround for Python 2.4
PR-URL: #1650
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
win: use msbuild from the configure stage
If node-gyp configure has set up MSBuild location use it instead the
one that happens to be first on the PATH.

PR-URL: #1654
Fixes: #1653
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
@richsumn84

This comment has been minimized.

Copy link

commented on 53b6074 Mar 1, 2019

richardlau and others added some commits Apr 2, 2019

test: fix addon test for Node.js 12 and V8 7.4
V8 7.4 removes some API functions. Replace those with their NAN
counterparts.

PR-URL: #1705
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
win: Support Visual Studio 2019
Path of VS2019's MSBuild.exe is different from VS2017's MSBuild.exe.
This change supports its path. And improve error check for finding
msbuild.exe, if it is not found, then throws error.

Fixes: #1663
@Mr1008
Copy link

left a comment

Just one comment, although I think it's worth considering.

try {
// Visual Studio 2017
msbuildPath = path.join(vsSetup.path, 'MSBuild', '15.0', 'Bin', 'MSBuild.exe')
if (!fs.existsSync(msbuildPath)) {

This comment has been minimized.

Copy link
@Mr1008

Mr1008 Apr 11, 2019

How about inversing the logic here and prioritize rather VS2019 (Current) directory than 15.0. IMHO current approach would prevent users from compiling vs2019 projects when they have installed both 2017 and 2019 version of VS because msbuild 15.0 would be discovered in the first place. If it would be inverted, MSBuild 2019 would correctly detect older build tools as well. What do you think guys?

This comment has been minimized.

Copy link
@syohex

syohex Apr 12, 2019

Author

I agree and fix by 29b0c2e . Thanks for good suggestion.

@refack

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Problem is MSVS2019 comes with 14.2 toolset, so in that case it needs to set:

defaults['msbuild_toolset'] = 'v142'

But anyway, as soon as GYP3 passes all tests on 2017 & 2019, I'll PR to replace the old GYP, so we won't need these tricks (GYP3 has full support for MSVS 2017 & 2019)

@cleever

This comment has been minimized.

Copy link

commented Apr 15, 2019

Any estimate of when it will be available?

@KENNYSOFT

This comment has been minimized.

Copy link

commented Apr 16, 2019

Please accept this ASAP...

@fuyutsuki

This comment has been minimized.

Copy link

commented Apr 23, 2019

I want to use this solution ASAP…

@rvagg rvagg force-pushed the nodejs:master branch from 1456ef2 to 7a71d68 Apr 24, 2019

@bnoordhuis
Copy link
Member

left a comment

I'm not 100% convinced this is the right approach but I'm willing to be persuaded that it's better than the alternatives.

Right now this doesn't really make node-gyp support VS 2019, it just makes it look for msbuild.exe in an alternative location.

There is existing support for detecting VS 2017, see commit ae141e1, and for detecting older VS versions by scanning the registry, see doWhich() and findMsbuild() in lib/build.js. I would prefer VS 2019 support reuses/extends that rather than piling on a special case.

Aside: this PR needs a rebase against master. Someone force-pushed to the repo a while ago, invalidating the merge base. :-/

@joaocgreis

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@syohex support ended up landing from another PR, I believe we can close this one now. Thanks for working on this and opening this PR.

node-gyp v5.0.0 is out with Visual Studio 2019 support. It's not yet integrated into npm so it has to be installed manually. This should do it:

npm install --global node-gyp@latest
for /f "delims=" %P in ('npm prefix -g') do npm config set node_gyp "%P\node_modules\node-gyp\bin\node-gyp.js"

That is, install node-gyp globally and make the npm config variable node_gyp point to node-gyp.js.

How to undo this after node-gyp is integrated into npm
npm uninstall --global node-gyp
npm config delete node_gyp

@joaocgreis joaocgreis closed this Jun 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.