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.

Sign up for GitHub

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

Jump to bottom

gyp: get ninja generator compatible with VS2017 (bump to a478c1ab51) #12632

Closed
wants to merge 7 commits into from
Closed

gyp: get ninja generator compatible with VS2017 (bump to a478c1ab51) #12632

wants to merge 7 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Apr 24, 2017 •
edited
Loading

this is a bump of GYP to refack/GYP3@d61a939
Then refloating our 6 patches on top

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools, gyp, build

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Apr 24, 2017
@refack
Copy link
Contributor Author

refack commented Apr 24, 2017

CI: https://ci.nodejs.org/job/node-test-commit/9403/

@refack
Copy link
Contributor Author

refack commented Apr 25, 2017

OSX was flaky.
Another CI: https://ci.nodejs.org/job/node-test-commit/9407/

@jasnell
Copy link
Member

jasnell commented Apr 25, 2017

/cc @nodejs/build @bnoordhuis

@refack refack self-assigned this Apr 25, 2017
@refack refack requested a review from bnoordhuis April 25, 2017 22:04
@addaleax
Copy link
Member

Can you use a commit message format like 23498f2 uses? That makes it a lot more obvious that it’s an upstream backport.

@refack
Copy link
Contributor Author

refack commented Apr 27, 2017

@addaleax is this what you had in mind?

@refack
Copy link
Contributor Author

refack commented Apr 27, 2017

New CI: https://ci.nodejs.org/job/node-test-commit/9460/

@addaleax
Copy link
Member

@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 Original commit message: – it’s a pretty standard format by now).

@refack
Copy link
Contributor Author

refack commented Apr 27, 2017

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

refack added a commit to refack/node that referenced this pull request Apr 30, 2017
@refack
tools: backport patches from upstream gyp
e461ea1 
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
@refack
Copy link
Contributor Author

refack commented Apr 30, 2017

@jasnell @addaleax @gibfahn @bnoordhuis @joaocgreis
Can anyone review this? It's blocking #12425

@addaleax
Copy link
Member

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

@refack refack changed the title gyp: get ninja generator compatible with VS2017 gyp: get ninja generator compatible with VS2017 (bump to a478c1ab51) May 16, 2017
@refack refack requested a review from joaocgreis May 16, 2017 12:51
@refack
Copy link
Contributor Author

refack commented May 16, 2017

/cc @nodejs/build @bnoordhuis
I turned this PR to the more common bump+refloat. PTAL

@refack refack mentioned this pull request May 16, 2017
Closed
10 tasks
@refack
Copy link
Contributor Author

refack commented May 16, 2017

I guess that because it's a muti-commit PR we don't get CI report :(
New CI: https://ci.nodejs.org/job/node-test-pull-request/8106/

@refack refack force-pushed the gyp-b62d04f-patch branch 2 times, most recently from 1cdbaa6 to 7be09a7 Compare May 16, 2017 20:48
@refack
Copy link
Contributor Author

refack commented May 16, 2017

New new CI (with auto-status): https://ci.nodejs.org/job/node-test-commit/9929/

@refack
Copy link
Contributor Author

refack commented Jul 11, 2017

@sam-github is this worked for you could you put a ✔️ on it

@refack refack mentioned this pull request Jul 18, 2017
Closed
@sam-github
Copy link
Contributor

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

refack and others added 6 commits July 18, 2017 15:42
@refack
build: bump GYP to d61a9397e6
2a48f8f 
https://chromium.googlesource.com/external/gyp/+/d61a9397e668fa9843c4aa7da9e79460fe590bfb
@refack
tools: fix gyp to work on MacOSX without XCode
ebb2b04 
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>
@jbergstroem @refack
gyp: inherit parent for *.host
4713f8e 
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>
@bnoordhuis @refack
tools: add compile_commands.json gyp generator
ec105b6 
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>
@danbev @refack
build: enable cctest to use objects (gyp part)
81c0dfa 
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>
@danbev @refack
build: fix ninja build failure (GYP patch)
a9180ea 
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>
@refack
Copy link
Contributor Author

refack commented Jul 18, 2017

ping @nodejs/build @nodejs/platform-windows @nodejs/python

@sam-github @refack
gyp: implement LD/LDXX for ninja and FIPS
ce4f27c 
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>
@refack refack mentioned this pull request Jul 18, 2017
Merged
@sam-github
Copy link
Contributor

@bnoordhuis PTAL

@refack refack mentioned this pull request Aug 9, 2017
Closed
2 tasks
@refack
Copy link
Contributor Author

refack commented Aug 9, 2017

Dies of old age.

@refack refack closed this Aug 9, 2017
@refack refack deleted the gyp-b62d04f-patch branch August 9, 2017 19:03
@refack refack removed their assignment Oct 20, 2018
@snyk-bot snyk-bot mentioned this pull request Mar 18, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 1, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis Awaiting requested review from bnoordhuis

@joaocgreis joaocgreis Awaiting requested review from joaocgreis

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@refack @jasnell @addaleax @bnoordhuis @sam-github @nodejs-github-bot @jbergstroem @danbev