tools: update gyp to make xcode tools work

[ryzokuken]

A recent change must have started requiring a full XCode installation in
order to run "./configure" and pretty much everything related.
This patch ensures that a full installation isn't required and the tools
work just fine.

Credits: @bnoordhuis
Fixes: #12531

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

/cc @bnoordhuis @refack @nodejs/gyp

[nodejs-github-bot]

@ryzokuken build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/150/pipeline

[bnoordhuis]

^{Can I sign off on my own code? Of course I can!}

[Trott]

Not a blocking objection, but I think the commit message here could be revised. It seems to say (or at least imply?) that building without full XCode is broken. But if I understand correctly, it's not broken. It just shows an error message that happens to not matter. This doesn't "fix" a broken build process. It suppresses an erroneous error message. (Or am I mistaken?)

[ryzokuken]

@Trott no. The error message still keeps coming (and is very irritating), but without this patch, I found it impossible to build without full XCode, so yeah, it's probably broken.

[Trott]

@Trott no. The error message still keeps coming (and is very irritating), but without this patch, I found it impossible to build without full XCode, so yeah, it's probably broken.

Interesting. I find it nearly impossible to believe that building with full XCode could be broken without it impacting a significant percentage of core devs. But I suppose it's possible that it affects just the right version of XCode and/or OS or something like that.

[Trott]

I don't want to be too annoying about my fixation on this, and you can totally ignore me if you want. That said, some other observations:

FWIW, I don't have a full XCode installed an either of the machines I use to build Node.js and I haven't had a problem building.

The bug that this fixes is over a year old but the commit message talks of a "recent change"... I guess it's possible that the bug first manifested as an irritating-but-harmless message over a year ago and then started causing a completely build failure more recently under certain conditions.

It might be good to simply remove speculation. Maybe something more like this?

tools: patch gyp for Xcode tools

Work around errors that occur when building without a full
XCode installation. 

Maybe those errors mean you can't build and maybe they don't, but either way, let's navigate around them.

[ryzokuken]

I don't want to be too annoying about my fixation on this, and you can totally ignore me if you want.

@Trott not at all! I'd love to get to the bottom of this as well.

Interesting. I find it nearly impossible to believe that building with full XCode could be broken without it impacting a significant percentage of core devs. But I suppose it's possible that it affects just the right version of XCode and/or OS or something like that.

FWIW, I don't have a full XCode installed an either of the machines I use to build Node.js and I haven't had a problem building.

Strange, right? I should've mentioned that this happened only when setting up the project from scratch on a new machine (my home machine works just fine). So I also believe this has something to do with a specific XCode/MacOS version maybe?

[ryzokuken]

@Trott I hope this looks good to you? Planning to land this soon-ish.

[Trott]

Yup, as I wrote above, "not a blocking objection". If CI is green and no one else objects, feel free to land. I would prefer the commit message get revised (and provided my suggestion above), but it's fine if it doesn't.

[ryzokuken]

@Trott it's fine. I'll amend the commit message, run CI and land tomorrow morning.

[gibfahn]

The error message still keeps coming (and is very irritating)

FWIW applying nodejs/node-gyp#1370 to the in-tree copy of gyp should fix the other error. I haven't done it because I'm not sure how updating gyp in node works, but if someone else would like to pull that change in they're more than welcome to.

[gibfahn]

To clarify:

master:

xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance

xcrun: error: unable to lookup item 'PlatformPath' from command line tools installation
xcrun: error: unable to lookup item 'PlatformPath' in SDK '/'

This PR:

xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance

This PR + nodejs/node-gyp#1370:

I applied the patch by doing: curl https://github.com/nodejs/node-gyp/commit/3a52936d6cae0b35a4bd486017c7c7a88ae0b415.patch | git am -3 --directory=tools and then fixing the merge conflict (change GetStdout to GetStdoutQuiet).

@ryzokuken if you would be willing to apply that one (probably best left to a separate PR) then I think we could get ./configure to run cleanly, which would be amazing.

[gibfahn]

LGTM with the commit message changed

[gibfahn]

Commit message suggestion:

tools: patch gyp to avoid xcrun errors

Previously running ./configure with only the Xcode Command Line Tools
installed would give:

xcrun: error: unable to lookup item 'PlatformPath' from command line tools installation
xcrun: error: unable to lookup item 'PlatformPath' in SDK '/'

[gibfahn]

@Trott no. The error message still keeps coming (and is very irritating), but without this patch, I found it impossible to build without full XCode, so yeah, it's probably broken.

It's not really a big deal if it's fixed by this, but it sounds like something is different about your machine that makes xcode fail rather than just logging the errors and continuing, I also have no problems with the command line tools.

I would say let's debug it, but if this PR fixes the issue let's just land it.

[ryzokuken]

@gibfahn it actually does fix the issue, and I think @bnoordhuis would have much more insight over how thing even works, but I'm thinking that because they were able to hack up a solution this quick, then there must be a catch.

P.S. totally using your suggestion for the commit message, thanks.

[ryzokuken]

CI: https://ci.nodejs.org/job/node-test-pull-request/15666/

[ryzokuken]

Force pushed to update failing master, rerunning tests.

[gibfahn]

Still LGTM, but if you wanted to credit Ben you could use the Co-authored-by:.

[ryzokuken]

@gibfahn awesome! I'll add that line.

[ryzokuken]

CI: https://ci.nodejs.org/job/node-test-pull-request/15692/

[ryzokuken]

Windows Fanned: https://ci.nodejs.org/job/node-test-commit-windows-fanned/19058/

[ryzokuken]

Windows fanned passed, landing once linux passes: https://ci.nodejs.org/job/node-test-commit-linux/19881/

[ryzokuken]

CI: https://ci.nodejs.org/job/node-test-pull-request/15758/

[maclover7]

Should this patch be applied to node-gyp's version of GYP too?

[ryzokuken]

@maclover7 is node-gyp affected by this? I tried it with our version of gyp and it just won't work without a full XCode installation. Let's check if node-gyp requires this patch too?

[gibfahn]

Should this patch be applied to node-gyp's version of GYP too?

@maclover7 yep, that's a separate PR someone should raise (but it doesn't block or affect this PR).

tl;dr: 2 changes in 2 repos, once they've all landed we should be warning-free everywhere:

• xcrun error:
  • node: This PR
  • node-gyp: another PR (if someone would like to raise it then 👍 )
• xcode-select error:
  • node: another PR (if someone would like to raise it then 👍 )
  • node-gyp: gyp: don't print xcodebuild not found errors node-gyp#1370

[ryzokuken]

Replying by mail so cannot use emoji responses, but will be glad to submit the two patches in the next few hours.

…

On Tue 10 Jul, 2018, 11:55 PM Gibson Fahnestock, ***@***.***> wrote: Should this patch be applied to node-gyp's version of GYP too? @maclover7 <https://github.com/maclover7> yep, that's a separate PR someone should raise (but it doesn't block or affect this PR). tl;dr: 2 changes in 2 repos, once they've all landed we should be warning-free everywhere: - xcrun error: - node: This PR - node-gyp: another PR (if someone would like to raise it then 👍 ) - xcode-select error: - node: another PR (if someone would like to raise it then 👍 ) - node-gyp: nodejs/node-gyp#1370 <nodejs/node-gyp#1370> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21520 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMg3MsYOiipFxzzOdvFkZko7c0J8grxYks5uFPGrgaJpZM4U18HO> .

[ryzokuken]

OSX Rebuild: https://ci.nodejs.org/job/node-test-commit-osx/19776/

[ryzokuken]

Final CI run (hopefully): https://ci.nodejs.org/job/node-test-pull-request/16055/

[ryzokuken]

Windows rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/19536/

[ryzokuken]

Tests passed, landing this.

[ryzokuken]

Landed in 51812ff