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

update gyp to b3cef02 #781

Closed
wants to merge 4 commits into from
Closed

update gyp to b3cef02 #781

wants to merge 4 commits into from

Conversation

imran-iq
Copy link

Includes two patches for AIX. Adds support for both 32-bit and 64-bit
files[0] and uses -RPf for cp instead of -af (which is unsupported)[1]

[0] https://codereview.chromium.org/1319663007
[1] https://codereview.chromium.org/1368133002

CI run here https://ci.nodejs.org/job/node-test-commit/882/
Intermittent failure that passed on rerun:
https://ci.nodejs.org/job/node-test-commit-arm/958/

@imran-iq
Copy link
Author

Note this also removes support for android as gyp has abandoned it. If you still want to keep android support in the code I can just pull the two AIX changes

@mhdawson
Copy link
Member

@bnoordhuis can you take a look at this one

@mhdawson
Copy link
Member

@bnoordhuis, there were 2 PRs to update gyp if you can also look at this one it would be great.

@shigeki, since you commented on the other one giving you an FYI here as well

@shigeki
Copy link

shigeki commented Oct 27, 2015

FYI: Currently, a floating patch of nodejs/node@58e914f is applied to tools/gyp in node so as to avoid a build error occured in MacOS without XCode. Ben has already submitted the patch to upstream in https://codereview.chromium.org/1213123002/ but it's not fixed yet.
The patch is needed to work in CI where only command line tools are installed but XCode is not.
I think it is a very limited environments and I'm not sure how much it is affected in using node-gyp.

@bnoordhuis
Copy link
Member

@iwuzhere Before I start reviewing this: node-gyp's copy of gyp was floating a few patches of its own. Did you preserve those?

@imran-iq
Copy link
Author

I have added the floating patches.

@bnoordhuis
Copy link
Member

I think you're missing 39a06bc.

@imran-iq
Copy link
Author

I shouldn't be as easy_xml.py hasnt changed in https://github.com/iWuzHere/node-gyp/commit/2f1d5efa1c747b1c0c7d5d721b25942d283606f9 and the code shows up in the repo

@bnoordhuis
Copy link
Member

LGTM but can you update the commit log with a quick overview of changes/additions?

@imran-iq
Copy link
Author

imran-iq commented Nov 5, 2015

I have updated the commit message

bnoordhuis pushed a commit that referenced this pull request Nov 6, 2015
Includes two patches for AIX. Adds support for both 32-bit and 64-bit
files[0] and uses -RPf for cp instead of -af (which is unsupported)[1]

[0] https://codereview.chromium.org/1319663007
[1] https://codereview.chromium.org/1368133002

PR-URL: #781
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis
Copy link
Member

Thanks, landed in f5d86eb...9049241.

@rvagg @TooTallNate Maybe one of you can do a release for this?

@bnoordhuis bnoordhuis closed this Nov 6, 2015
@mhdawson
Copy link
Member

mhdawson commented Nov 6, 2015

@bnoordhuis when you say to a release is that how it gets pulled into the dependencies within Node itself ?

@bnoordhuis
Copy link
Member

Sadly, no. The chain of releases is node-gyp -> npm -> node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants