Skip to content

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

Merged
refack merged 1 commit into
nodejs:masterfrom
refack:quiet-make-by-default
Mar 22, 2019
Merged

build,meta: quiet/pretty make output by default#26740
refack merged 1 commit into
nodejs:masterfrom
refack:quiet-make-by-default

Conversation

@refack

@refack refack commented Mar 18, 2019

Copy link
Copy Markdown
Contributor

'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 BridgeAR and danbev March 18, 2019 14:50
@refack

refack commented Mar 18, 2019

Copy link
Copy Markdown
Contributor Author

/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

rvagg commented Mar 19, 2019

Copy link
Copy Markdown
Member

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

rvagg
rvagg approved these changes Mar 19, 2019
View reviewed changes

@rvagg rvagg 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.

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

refack commented Mar 19, 2019

Copy link
Copy Markdown
Contributor Author

/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

refack commented Mar 19, 2019

Copy link
Copy Markdown
Contributor Author

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

BridgeAR commented Mar 21, 2019

Copy link
Copy Markdown
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

BridgeAR commented Mar 21, 2019

Copy link
Copy Markdown
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 <rod@vagg.org>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@refack refack force-pushed the quiet-make-by-default branch from 8fd887d to ecf98b0 Compare March 22, 2019 00:21
@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

+2 more reviewers

@danbev danbev danbev approved these changes

@rvagg rvagg rvagg approved these changes

Reviewers whose approvals may not affect merge requirements

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