Skip to content

gyp: muffle xcodebuild warnings#21999

Closed
ryzokuken wants to merge 1 commit intonodejs:masterfrom
ryzokuken:xcode-muffle
Closed

gyp: muffle xcodebuild warnings#21999
ryzokuken wants to merge 1 commit intonodejs:masterfrom
ryzokuken:xcode-muffle

Conversation

@ryzokuken
Copy link
Contributor

@ryzokuken ryzokuken commented Jul 27, 2018

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

nodejs-github-bot commented Jul 27, 2018

@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

addaleax commented Jul 27, 2018

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 Outdated
Copy link
Contributor

@refack refack Jul 27, 2018

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

ryzokuken commented Jul 29, 2018

@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

ryzokuken commented Aug 24, 2018

CI passed! Landing this.

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Aug 24, 2018

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

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+2 more reviewers

@refack refack refack approved these changes

@gibfahn gibfahn gibfahn approved these changes

Reviewers whose approvals may not affect merge requirements

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