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

build,meta: quiet/pretty make output by default #26740

Merged
merged 1 commit into from
Mar 22, 2019
Merged

build,meta: quiet/pretty make output by default #26740

merged 1 commit into from
Mar 22, 2019

Conversation

refack
Copy link
Contributor

@refack refack commented Mar 18, 2019

'cause usually you simply want the job done...
Refs: #26252

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@refack refack added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 18, 2019
@refack refack added this to the 12.0.0 milestone Mar 18, 2019
@refack refack self-assigned this Mar 18, 2019
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 18, 2019
@refack refack requested review from danbev and BridgeAR March 18, 2019 14:50
@refack
Copy link
Contributor Author

refack commented Mar 18, 2019

/CC @nodejs/build @nodejs/build-files

Semver-Major since downstream embedders (and our CI) depend on make's output.

@refack refack added the meta Issues and PRs related to the general management of the project. label Mar 18, 2019
@rvagg
Copy link
Member

rvagg commented Mar 19, 2019

previous discussion, you might want to check with people involved in that: #10403

rvagg
rvagg approved these changes Mar 19, 2019
View reviewed changes
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

fwiw I don't see this as semver-major, I'm not aware of anyone that relies on compiler output in a breaking way. In our CI we could just run make with V=1 everywhere if folks were concerned enough to see the output (I'm not, I like having less output, it makes Jenkins more manageable!).

@refack
Copy link
Contributor Author

refack commented Mar 19, 2019

/PING @trevnorris @bnoordhuis @addaleax @gibfahn

BTW: The pretty format comes from GYP

node/tools/gyp/pylib/gyp/generator/make.py

Lines 362 to 366 in ed2c696

quiet_cmd_cc = CC($(TOOLSET)) $@
cmd_cc = $(CC.$(TOOLSET)) -o $@ $< $(GYP_CFLAGS) $(DEPFLAGS) $(CFLAGS.$(TOOLSET)) -c
quiet_cmd_cxx = CXX($(TOOLSET)) $@
cmd_cxx = $(CXX.$(TOOLSET)) -o $@ $< $(GYP_CXXFLAGS) $(DEPFLAGS) $(CXXFLAGS.$(TOOLSET)) -c

So that could be easily be manipulated as well.

AS for Jenkins, my dream is nodejs/build#1517

@refack
Copy link
Contributor Author

refack commented Mar 19, 2019

fwiw I don't see this as semver-major, I'm not aware of anyone that relies on compiler output in a breaking way.

We've had complaints before 🤷‍♂️

@BridgeAR
Copy link
Member

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 21, 2019
@BridgeAR
Copy link
Member

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

@refack
build,meta: quiet/pretty make output by default
ecf98b0 
PR-URL: nodejs#26740
Refs: nodejs#26252
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@refack refack merged commit ecf98b0 into nodejs:master Mar 22, 2019
@refack refack deleted the quiet-make-by-default branch March 22, 2019 00:24
@joyeecheung joyeecheung mentioned this pull request Mar 22, 2019
Closed
@BethGriggs BethGriggs mentioned this pull request Mar 26, 2019
Merged
@refack refack removed their assignment Apr 14, 2019
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@rvagg rvagg rvagg approved these changes

@danbev danbev danbev approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
12.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@refack @rvagg @BridgeAR @danbev @nodejs-github-bot