FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gyp: muffle xcodebuild warnings by ryzokuken · Pull Request #21999 · nodejs/node · GitHub

/ node Public

gyp: muffle xcodebuild warnings - #21999

Closed
ryzokuken wants to merge 1 commit into
nodejs:masterfrom
ryzokuken:xcode-muffle
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

ryzokuken commented Jul 27, 2018

Copy link
Copy Markdown
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 commented Jul 27, 2018

Copy link
Copy Markdown
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 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 commented Jul 27, 2018

Copy link
Copy Markdown
Member

Should this land upstream first?

gibfahn commented Jul 27, 2018

Copy link
Copy Markdown
Member

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 commented Jul 27, 2018

Copy link
Copy Markdown
Member

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

Trott commented Jul 27, 2018

Copy link
Copy Markdown
Member

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 suggested changes Jul 27, 2018
View reviewed changes

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should be function overload.

Comment thread tools/gyp/pylib/gyp/xcode_emulation.py Outdated

refack Jul 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax approved these changes Jul 27, 2018
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jul 28, 2018

Copy link
Copy Markdown
Contributor

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 commented Jul 28, 2018

Copy link
Copy Markdown
Member

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 commented Jul 29, 2018

Copy link
Copy Markdown
Contributor Author

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

refack approved these changes Aug 24, 2018
View reviewed changes

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

NM

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 commented Aug 24, 2018

Copy link
Copy Markdown
Contributor

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

ryzokuken commented Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

CI passed! Landing this.

ryzokuken commented Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 16cffb0

ryzokuken closed this Aug 24, 2018
ryzokuken added a commit that referenced this pull request Aug 24, 2018
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
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
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 mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
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 mentioned this pull request Nov 8, 2018
Closed
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
BridgeAR BridgeAR approved these changes
+2 more reviewers
refack refack approved these changes
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

Back | FazBrowse Home | New Git URL