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: muffle xcodebuild warnings #21999

Closed
wants to merge 1 commit into from
Closed

gyp: muffle xcodebuild warnings #21999

wants to merge 1 commit into from

Conversation

ryzokuken
Copy link
Contributor

Muffle xcodebuild warnings by introducing an alternative quieter
alternative to GetStdout, called GetStdoutQuiet, and call it selectively
in particularly noisy xcodebuild commands.

Co-authored-by: Gibson Fahnestock gibfahn@gmail.com

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

@nodejs-github-bot
Copy link
Collaborator

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

@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 27, 2018
@addaleax
Copy link
Member

Should this land upstream first?

@gibfahn
Copy link
Member

gibfahn commented Jul 27, 2018

Should this land upstream first?

Question for @nodejs/gyp , but I don't think we're doing that any more.

cc/ @rvagg @gdams @bnoordhuis (who reviewed nodejs/node-gyp#1370, the equivalent patch in node-gyp).

@jasnell
Copy link
Member

jasnell commented Jul 27, 2018

I believe at this point in time we effectively own the gyp stuff so I don't believe landing upstream in necessary.

@Trott
Copy link
Member

Trott commented Jul 27, 2018

Nit on the commit message: suppress would be better/more-precise than muffle, I think.

(muffle makes it sound like we make it quieter and/or distort it. suppress makes it sound like we stop it from appearing.)

refack
refack suggested changes Jul 27, 2018
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Should be function overload.

tools/gyp/pylib/gyp/xcode_emulation.py
@@ -1442,6 +1442,18 @@ def CLTVersion():
continue


def GetStdoutQuiet(cmdlist):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the method duplication.
This should be an overload.

addaleax
addaleax approved these changes Jul 27, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

It sounds like maybe we should create a nodejs/gyp repo? IIUC addressing @refack’s nit would lead to divergence between our copy and node-gyp’s one, and I don’t think we want that…

@refack
Copy link
Contributor

refack commented Jul 28, 2018

It sounds like maybe we should create a nodejs/gyp repo?

👍

IIUC addressing @refack’s nit would lead to divergence between our copy and node-gyp’s one, and I don’t think we want that…

They are already not the same 😢, but I get your point.

@gibfahn
Copy link
Member

gibfahn commented Jul 28, 2018

I don't like the method duplication.
This should be an overload.

This seems like the most minimal diff, I didn't want to touch existing code more than absolutely necessary. There are very few people with any knowledge of gyp's internals to review the node-gyp change, so ease of review was more important than writing nice code, especially as gyp is basically deprecated at this point.

I agree overloading is a slightly nicer code style, but I don't think it actually makes any difference beyond that.

@ryzokuken
Copy link
Contributor Author

@refack are you convinced and okay with dismissing your review?

refack
refack approved these changes Aug 24, 2018
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

NM

@ryzokuken @refack
gyp: muffle xcodebuild warnings
ad7b648 
Muffle xcodebuild warnings by introducing an alternative quieter
alternative to GetStdout, called GetStdoutQuiet, and call it selectively
in particularly noisy xcodebuild commands.

Co-authored-by: Gibson Fahnestock <gibfahn@gmail.com>

PR-URL: nodejs#21999
Original-PR-URL: nodejs/node-gyp#1370
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@refack
Copy link
Contributor

refack commented Aug 24, 2018

macOS CI: https://ci.nodejs.org/job/node-test-commit-osx/20760/

@ryzokuken
Copy link
Contributor Author

CI passed! Landing this.

@ryzokuken
Copy link
Contributor Author

Landed in 16cffb0

@ryzokuken ryzokuken closed this Aug 24, 2018
ryzokuken added a commit that referenced this pull request Aug 24, 2018
@ryzokuken
gyp: muffle xcodebuild warnings
16cffb0 
Muffle xcodebuild warnings by introducing an alternative quieter
alternative to GetStdout, called GetStdoutQuiet, and call it selectively
in particularly noisy xcodebuild commands.

Co-authored-by: Gibson Fahnestock <gibfahn@gmail.com>

PR-URL: #21999
Original-PR-URL: nodejs/node-gyp#1370
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit that referenced this pull request Aug 27, 2018
@ryzokuken @addaleax
gyp: muffle xcodebuild warnings
d7c110a 
Muffle xcodebuild warnings by introducing an alternative quieter
alternative to GetStdout, called GetStdoutQuiet, and call it selectively
in particularly noisy xcodebuild commands.

Co-authored-by: Gibson Fahnestock <gibfahn@gmail.com>

PR-URL: #21999
Original-PR-URL: nodejs/node-gyp#1370
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 3, 2018
@ryzokuken @targos
gyp: muffle xcodebuild warnings
1e9d3e6 
Muffle xcodebuild warnings by introducing an alternative quieter
alternative to GetStdout, called GetStdoutQuiet, and call it selectively
in particularly noisy xcodebuild commands.

Co-authored-by: Gibson Fahnestock <gibfahn@gmail.com>

PR-URL: #21999
Original-PR-URL: nodejs/node-gyp#1370
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@ryzokuken @targos
gyp: muffle xcodebuild warnings
cd5c672 
Muffle xcodebuild warnings by introducing an alternative quieter
alternative to GetStdout, called GetStdoutQuiet, and call it selectively
in particularly noisy xcodebuild commands.

Co-authored-by: Gibson Fahnestock <gibfahn@gmail.com>

PR-URL: #21999
Original-PR-URL: nodejs/node-gyp#1370
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This was referenced Sep 6, 2018
Closed
Closed
@refack refack mentioned this pull request Nov 8, 2018
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

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
@ryzokuken @nodejs-github-bot @addaleax @gibfahn @jasnell @Trott @refack @BridgeAR