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.

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

build: make node-gyp output silent #8990

Closed

Conversation

thefourtheye
Copy link
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 thefourtheye added the build Issues and PRs related to build files or the CI. label Oct 9, 2016
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 9, 2016
@gibfahn
Copy link
Member

gibfahn commented Oct 9, 2016

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

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

@gibfahn
Copy link
Member

gibfahn commented Oct 9, 2016

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).

@thefourtheye
Copy link
Contributor Author

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

@thefourtheye
Copy link
Contributor Author

cc @nodejs/collaborators

@@ -165,7 +166,8 @@ test/addons/.buildstamp: config.gypi \
# Cannot use $(wildcard test/addons/*/) here, it's evaluated before
# embedded addons have been generated from the documentation.
for dirname in test/addons/*/; do \
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@danbev
Copy link
Contributor

danbev commented Oct 31, 2016

LGTM

@rvagg
Copy link
Member

rvagg commented Oct 31, 2016

lgtm

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

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

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

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

@gibfahn
Copy link
Member

gibfahn commented Oct 31, 2016

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM if it works.

@gibfahn
Copy link
Member

gibfahn commented Nov 2, 2016

Landed in 54d3431

@gibfahn gibfahn closed this Nov 2, 2016
gibfahn pushed a commit that referenced this pull request Nov 2, 2016
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 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
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
Copy link
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
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
Copy link
Contributor Author

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

@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
@daviddias
Copy link

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
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>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
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
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
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 MylesBorins mentioned this pull request Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants