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

tools: update gyp to 25ed9ac #2074

Merged
merged 2 commits into from
Jun 30, 2015
Merged

tools: update gyp to 25ed9ac #2074

merged 2 commits into from
Jun 30, 2015

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis mentioned this pull request Jun 29, 2015
Closed
@bnoordhuis
Copy link
Member Author

@shigeki I took the liberty of uploading your patch: https://codereview.chromium.org/1213123002/

@shigeki
Copy link
Contributor

shigeki commented Jun 30, 2015

I also tested this on MacOS with XCode since CI has only the one without XCode and build is fine.
LGTM.
Thanks for submitting the patch.

@rvagg
Copy link
Member

rvagg commented Jun 30, 2015

not to be a pain but could we have a summary of what this PR is buying us? potentially useful for changelog notes but also in assessing the urgency of this going in to next prior to cutting RCs.

@shigeki
Copy link
Contributor

shigeki commented Jun 30, 2015

https://chromium.googlesource.com/external/gyp/+/25ed9ac4ac2a4d2a08909225fbb6d56e89ad38a6 fixes a link error in building iojs with ninja on linux and freebsd,. It is not serious because ninja support was removed in configure and few people is using it by tools/gyp_nod.py. But I think it is good to keep gyp updated to be less behind from the latest.

@Fishrock123
Copy link
Contributor

It is not serious because ninja support was removed in configure

Because supporting it in make was messy, hacky, and kind of broken.

and few people is using it by tools/gyp_nod.py.

Sure, but it should still work for us if possible. After all, it is much faster than make -j 8 in my experience.

@yury-s
Copy link

yury-s commented Jun 30, 2015

Is there an issue or something with a list of problems that you encountered trying to support ninja? ninja is basically default build tool for Chromium (mostly because it is a way faster on incremental builds than make) so I expected it work fine with iojs.

@bnoordhuis
Copy link
Member Author

@rvagg Would this suffice?

tools: update gyp to 25ed9ac

Includes improved support for VS 2015[0] and makes it possible to build
with ninja again[1].

[0] https://codereview.chromium.org/1112753003
[1] https://codereview.chromium.org/1209553002

Fixes: https://github.com/nodejs/io.js/pull/2065

@jbergstroem
Copy link
Member

@bnoordhuis LGTM. That's pretty much the gist of it.

bnoordhuis and others added 2 commits June 30, 2015 13:38
@bnoordhuis
tools: update gyp to 25ed9ac
99cbbc0 
Includes improved support for VS 2015[0] and makes it possible to build
with ninja again[1].

[0] https://codereview.chromium.org/1112753003
[1] https://codereview.chromium.org/1209553002

Fixes: nodejs#2065
PR-URL: nodejs#2074
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@bnoordhuis
tools: fix gyp to work on MacOSX without XCode
58e914f 
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@bnoordhuis bnoordhuis closed this Jun 30, 2015
@bnoordhuis bnoordhuis deleted the upgrade-gyp branch June 30, 2015 11:41
@bnoordhuis bnoordhuis merged commit 58e914f into nodejs:master Jun 30, 2015
@rvagg rvagg mentioned this pull request Jul 2, 2015
Closed
mscdex pushed a commit to mscdex/io.js that referenced this pull request Jul 9, 2015
@bnoordhuis @mscdex
tools: update gyp to 25ed9ac
cb6f1c9 
Includes improved support for VS 2015[0] and makes it possible to build
with ninja again[1].

[0] https://codereview.chromium.org/1112753003
[1] https://codereview.chromium.org/1209553002

Fixes: nodejs#2065
PR-URL: nodejs#2074
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bnoordhuis @shigeki @rvagg @Fishrock123 @yury-s @jbergstroem