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

build: make node-gyp output silent by thefourtheye · Pull Request #8990 · nodejs/node · GitHub

/ node Public

build: make node-gyp output silent - #8990

Closed
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:make-gyp-output-silent
ConversationCommits (1)ChecksFiles changed
Closed

build: make node-gyp output silent#8990
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:make-gyp-output-silent

Conversation

thefourtheye commented Oct 9, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

Description of change

As it is, node-gyp produces a lot of build related verbose messages.
Latest node-gyp upgrade allows us to specify --silent flag to suppress
those messages.

cc @nodejs/build

thefourtheye added the build Issues and PRs related to build files or the CI. label Oct 9, 2016
nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 9, 2016

gibfahn commented Oct 9, 2016

Copy link
Copy Markdown
Member

I agree that the output is a bit excessive when the addons build successfully, but when they fail it is important to get the whole output.

@thefourtheye When an addon fails to build do we still get the full verbose output with the --silent flag?

thefourtheye commented Oct 9, 2016

Copy link
Copy Markdown
Contributor Author

@gibfahn No, we will get only the failures. Would it be better if we can disabling silent output explicitly?

gibfahn commented Oct 9, 2016

Copy link
Copy Markdown
Member

I think in general we want as little output as possible when something succeeds, and plenty of info when it fails.

Ideally we'd have silent output for a passing build and verbose output for a failure (like the default output of the test runner). This may not currently be an option in node-gyp, but IMO that'd be the best thing to work towards.

If that's not possible we definitely need an option to run with verbose output, not least for CI (if something fails on the CI you don't really want to have to run it again to get more info).

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
thefourtheye force-pushed the make-gyp-output-silent branch from 9206ae1 to 9a082e6 Compare October 31, 2016 10:22

thefourtheye commented Oct 31, 2016

Copy link
Copy Markdown
Contributor Author

Now, we can pass the loglevel explicitly and by default it will be silent.

thefourtheye commented Oct 31, 2016

Copy link
Copy Markdown
Contributor Author

cc @nodejs/collaborators

danbev reviewed Oct 31, 2016
View reviewed changes
Comment thread Makefile Outdated

danbev Oct 31, 2016

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

Nit: could we make this @for instead to not have the command echoed in the console log?

thefourtheye Oct 31, 2016

Copy link
Copy Markdown
Contributor Author

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

@danbev As it is there is no indication in the command line that which command is actually being executed. I feel that it is better to have. If we feel strongly about this, this can be changed.

danbev Oct 31, 2016

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

@thefourtheye No strong feelings from my side. Was more if the goal was to reduce the amount of output.

danbev commented Oct 31, 2016

Copy link
Copy Markdown
Contributor

LGTM

rvagg commented Oct 31, 2016

Copy link
Copy Markdown
Member

lgtm

gibfahn suggested changes Oct 31, 2016
View reviewed changes

gibfahn 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

Could you change the LOGLEVEL in the test-ci targets so that it matches what we had before?

build: make node-gyp output silent
8a8b805 
As it is, node-gyp produces a lot of build related verbose messages.
Latest node-gyp upgrade allows us to specify --silent flag to suppress
those messages. Except for CI, addons build will run silently.
thefourtheye force-pushed the make-gyp-output-silent branch from 9a082e6 to 8a8b805 Compare October 31, 2016 12:23

thefourtheye commented Oct 31, 2016

Copy link
Copy Markdown
Contributor Author

@gibfahn PTAL. I made them to use info as the loglevel, which was what we had before.

gibfahn commented Oct 31, 2016

Copy link
Copy Markdown
Member

LGTM, CI: https://ci.nodejs.org/job/node-test-pull-request/4753/

imyller approved these changes Oct 31, 2016
View reviewed changes

imyller 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

indutny approved these changes Oct 31, 2016
View reviewed changes

indutny 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 if it works.

gibfahn commented Nov 2, 2016

Copy link
Copy Markdown
Member

Landed in 54d3431

gibfahn closed this Nov 2, 2016
gibfahn pushed a commit that referenced this pull request Nov 2, 2016
build: make node-gyp output silent
54d3431 
As it is, node-gyp produces a lot of build related verbose messages.
Latest node-gyp upgrade allows us to specify --silent flag to suppress
those messages. Except for CI, addons build will run silently.

PR-URL: #8990

Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
thefourtheye deleted the make-gyp-output-silent branch November 2, 2016 14:56
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
build: make node-gyp output silent
b51db71 
As it is, node-gyp produces a lot of build related verbose messages.
Latest node-gyp upgrade allows us to specify --silent flag to suppress
those messages. Except for CI, addons build will run silently.

PR-URL: #8990

Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
evanlucas mentioned this pull request Nov 3, 2016
Merged

MylesBorins commented Nov 22, 2016

Copy link
Copy Markdown
Contributor

I've landed this on v6.x-staging but not on v4.x

@thefourtheye can you confirm this is unnecessary for v4.x

MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
build: make node-gyp output silent
5c2ed2e 
As it is, node-gyp produces a lot of build related verbose messages.
Latest node-gyp upgrade allows us to specify --silent flag to suppress
those messages. Except for CI, addons build will run silently.

PR-URL: #8990

Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>

thefourtheye commented Nov 22, 2016

Copy link
Copy Markdown
Contributor Author

@thealphanerd Ya, this patch will not have any effect in v4.

MylesBorins mentioned this pull request Nov 22, 2016
Merged

daviddias commented Nov 30, 2016

Copy link
Copy Markdown

subscribing to this one, looking forward to see it released :)

thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Dec 21, 2016
build: make node-gyp output silent
db05b80 
As it is, node-gyp produces a lot of build related verbose messages.
Latest node-gyp upgrade allows us to specify --silent flag to suppress
those messages. Except for CI, addons build will run silently.

PR-URL: nodejs#8990

Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
thefourtheye mentioned this pull request Dec 21, 2016
Closed
2 tasks
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
build: make node-gyp output silent
ec74098 
As it is, node-gyp produces a lot of build related verbose messages.
Latest node-gyp upgrade allows us to specify --silent flag to suppress
those messages. Except for CI, addons build will run silently.

PR-URL: #8990

Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
build: make node-gyp output silent
d1b6407 
As it is, node-gyp produces a lot of build related verbose messages.
Latest node-gyp upgrade allows us to specify --silent flag to suppress
those messages. Except for CI, addons build will run silently.

PR-URL: #8990

Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
build: make node-gyp output silent
8bf521f 
As it is, node-gyp produces a lot of build related verbose messages.
Latest node-gyp upgrade allows us to specify --silent flag to suppress
those messages. Except for CI, addons build will run silently.

PR-URL: #8990

Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
MylesBorins mentioned this pull request Dec 21, 2016
Merged
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
+4 more reviewers
danbev danbev approved these changes
indutny indutny approved these changes
imyller imyller 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL