-
Notifications
You must be signed in to change notification settings - Fork 30k
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
gyp: get ninja generator compatible with VS2017 (bump to a478c1ab51) #12632
Conversation
423dc7d
to
a37d7a8
Compare
OSX was flaky. |
a37d7a8
to
565867a
Compare
565867a
to
145ebef
Compare
145ebef
to
ac3afc7
Compare
ac3afc7
to
6c8d141
Compare
/cc @nodejs/build @bnoordhuis |
Can you use a commit message format like 23498f2 uses? That makes it a lot more obvious that it’s an upstream backport. |
6c8d141
to
9c0ae89
Compare
@addaleax is this what you had in mind? |
@refack Yeah, it’s a lot better, thank you. I’d still include the metadata in the indented original commit messages, though (you can grep our commit log for |
9c0ae89
to
8ed72fe
Compare
@addaleax Done. Only thing out of the ordinary is I'm backporting 4 commits at once, since they are logically one atomic fix, so I explicitly added the hashes of each. |
8ed72fe
to
e461ea1
Compare
this is a backport of 4 gyp commits since last bump, syncing us to https://chromium.googlesource.com/external/gyp/+/a478c1ab51ea3e04e79791ac3d1dad01b3f57434 this is instead of a bump and rebase of floating patches the goal is to fix the ninja generator on Windows also includes: * windows: use "mkdir" even when copying directory Original commit messages: a478c1ab51ea3e04e79791ac3d1dad01b3f57434: win: mkdir even when copying directory * also "fix" the paths in the message * un-skip test/copies/gyptest-all.py BUG=gyp:536 Change-Id: Id8ff7941b995c25d68d454138cd8b04940fdd82b Reviewed-on: https://chromium-review.googlesource.com/487521 Commit-Queue: Dirk Pranke <dpranke@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> ffd524cefaad622e72995e852ffb0b18e83f8054 win ninja/make: Always use a native compiler executable with MSVS 2017 A host-native executable will always be used, and it will be a cross compiler if the target architecture differs from the host architecture. BUG=683729 Change-Id: I02a09e1755dd2ab7eca5c9d1957d7aeb56db6af6 Reviewed-on: https://chromium-review.googlesource.com/486400 Commit-Queue: Mark Mentovai <mark@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> e8850240a433259052705fb8c56e51795b7dc9c3 Fix MSVC++ 32-on-32 builds after b62d04ff85e6 BUG=683729 Change-Id: Ic8c227960b859ddc3c19fce0e98144510f5e74bf Reviewed-on: https://chromium-review.googlesource.com/486380 Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Mark Mentovai <mark@chromium.org> b62d04ff85e6234e4fec7fff9377dd96c09d41a7 win,ninja: ninja generator better on windows * add compatibility with VS2017 * adjust `_TargetConfig` and `/FS` for VS2017 compat * find new place of `vcvarsall.bat` in VS2017 * normalize "path like" arguments of actions * better check for `.lib` and `.def` file names BUG=683729 Change-Id: I123bff7bd8a0011cf65d27a62b5267ba884e3b42 Reviewed-on: https://chromium-review.googlesource.com/482580 Reviewed-by: Dirk Pranke <dpranke@chromium.org> Reviewed-by: Mark Mentovai <mark@chromium.org> Commit-Queue: Dirk Pranke <dpranke@chromium.org> Ref: nodejs#12281 PR-URL: nodejs#12632
@jasnell @addaleax @gibfahn @bnoordhuis @joaocgreis |
@refack This would be a bit easier to review if the patches were kept in separate commits so that they can easily be matched against the upstream ones (or if Node didn’t float patches and one could just compare the files, but unfortunately we do) |
/cc @nodejs/build @bnoordhuis |
I guess that because it's a muti-commit PR we don't get CI report :( |
1cdbaa6
to
7be09a7
Compare
New new CI (with auto-status): https://ci.nodejs.org/job/node-test-commit/9929/ |
@sam-github is this worked for you could you put a ✔️ on it |
@refack This cleared away my problems. I don't have any comment on how it did that, but without rebasing this onto master, and then my local branches onto this, I can't generate ninja build files on Windows. |
This issue has already submitted to the upstream in https://code.google.com/p/gyp/issues/detail?id=477 Use this commit until the upstream is to be fixed. PR-URL: nodejs#1325 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Gyp defaults to gcc/g++ if CC.host/CXX.host is unset. This is not suitable for environments that only uses the clang toolchain. Since we already assume that the user will provide clang/clang++ through CC/CXX, lean against it (then drop to gcc/g++). Also apply the same logic for link/ar for consistency although it doesn't affect us. PR-URL: nodejs#6173 Fixes: nodejs#6152 Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
this is a re-base of the gyp part of 3c46bb9 after bumping GYP version to https://chromium.googlesource.com/external/gyp/+/eb296f67da078ec01f5e3a9ea9cdc6d26d680161 Original-Review-By: James M Snell <jasnell@gmail.com> Ref: nodejs#7986 PR-URL: nodejs#12450 Reviewed-By: João Reis <reis@janeasystems.com>
this is a re-base of the gyp part of 6a09a69 after bumping GYP version to https://chromium.googlesource.com/external/gyp/+/eb296f67da078ec01f5e3a9ea9cdc6d26d680161 Original-PR-URL: nodejs#11956 Original-Ref: nodejs#9163 Original-Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: nodejs#12450 Reviewed-By: João Reis <reis@janeasystems.com>
Currently the files specified in libraries in node.gyp `cctest` target are getting a '.lib' extension on windows when generated with ninja. This commit adds a check to see if a file has a '.obj' extension and in that case no '.lib' extension will be added. Also, the LIBS specified in the 'libraries' section are not being included in the --start-group --end-group section which means that these libraries will not be searched causing issue with linkers where the order matters. PR-URL: nodejs#12484 Fixes: nodejs#12448 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
7be09a7
to
2ee6704
Compare
ping @nodejs/build @nodejs/platform-windows @nodejs/python
|
The ability to set the link rule is used for FIPS, and needs to set both the `ld =` and `ldxx =` variables in the ninja build file to link c++ (node) and c (openssl-cli, etc.) executables. PR-URL: nodejs#14227 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
2ee6704
to
ce4f27c
Compare
@bnoordhuis PTAL |
Dies of old age. |
this is a bump of
GYP
to refack/GYP3@d61a939Then refloating our 6 patches on top
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
tools, gyp, build