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

gyp: muffle xcode-select warnings #21784

Closed
wants to merge 1 commit into from

Conversation

ryzokuken
Copy link
Contributor

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 @nodejs/gyp @gibfahn

sorry for holding this off until today 😅

Do we use a different version of gyp as compared to node-gyp? I found all the right function calls but they were calling different things, so had been wondering.

@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 Jul 12, 2018
Muffle gyp from creating xcode-select related warnings, essentially
flooding the console.

Co-authored-by: Gibson Fahnestock <[email protected]>
Refs: nodejs/node-gyp#1370
Refs: nodejs#21520
@Trott
Copy link
Member

Trott commented Jul 12, 2018

Is the benefit of discarding the spurious error greater than the cost of swallowing other useful debugging information in the event of a real error? (I don't know the answer.)

@gibfahn
Copy link
Member

gibfahn commented Jul 13, 2018

Is the benefit of discarding the spurious error greater than the cost of swallowing other useful debugging information in the event of a real error? (I don't know the answer.)

See discussion in #21520 , but I'd say absolutely. This is the minimal changeset, and it shouldn't swallow any relevant errors.

See nodejs/node-gyp#569 for the pain it causes, I think we should keep the in-tree GYP build as similar as possible to the node-gyp version in any case.

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM FWIW, but would appreciate other reviews.

@richardlau
Copy link
Member

I think we should keep the in-tree GYP build as similar as possible to the node-gyp version in any case.

Radical idea: What if we dropped the in-tree gyp in favour of the one embedded in node-gyp (which we are already dependent on to build the addon tests)?

@gibfahn
Copy link
Member

gibfahn commented Jul 17, 2018

Radical idea: What if we dropped the in-tree gyp in favour of the one embedded in node-gyp (which we are already dependent on to build the addon tests)?

@nodejs/build @nodejs/node-gyp

@jasnell
Copy link
Member

jasnell commented Sep 10, 2018

What's the status on this one?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018
@ryzokuken
Copy link
Contributor Author

@richardlau has an excellent suggestion, but this PR can be landed nonetheless.

@ryzokuken
Copy link
Contributor Author

Noticed that nobody landed this, but it has enough approvals, etc. Landing this in a few hours unless someone objects.

refack
refack previously approved these changes Nov 8, 2018
@refack
Copy link
Contributor

refack commented Nov 8, 2018

@ryzokuken if you'd like to submit a PR "upstream", where we could debate the finer point of this case.

@refack refack removed the stalled Issues and PRs that are stalled. label Nov 8, 2018
@refack
Copy link
Contributor

refack commented Nov 8, 2018

wait... we already landed #21999 16cffb0
😕

@refack refack dismissed their stale review November 8, 2018 17:32

superseded by 16cffb0

@richardlau
Copy link
Member

Noticed that nobody landed this, but it has enough approvals, etc. Landing this in a few hours unless someone objects.

This has conflicts and needs an up to date CI run.

@refack
Copy link
Contributor

refack commented Nov 8, 2018

I'm going to close this as superseded. @ryzokuken feel free to reopen.

@refack refack closed this Nov 8, 2018
@ryzokuken
Copy link
Contributor Author

@refack will do.

@ryzokuken
Copy link
Contributor Author

Waiiit a minute. Why would I open two PRs with the exact same goal?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants