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

build: revert to pretty-print by default by rvagg · Pull Request #10403 · nodejs/node · GitHub

/ node Public

build: revert to pretty-print by default - #10403

Closed
rvagg wants to merge 1 commit into
nodejs:masterfrom
rvagg:pretty-print-gyp
ConversationCommits (1)ChecksFiles changed
Closed

build: revert to pretty-print by default#10403
rvagg wants to merge 1 commit into
nodejs:masterfrom
rvagg:pretty-print-gyp

Conversation

rvagg commented Dec 22, 2016

Copy link
Copy Markdown
Member

This essentially reverts 8656c26 which arose as a result of nodejs/node-v0.x-archive#3761, which says:

The practice of hiding command execution during the build process frustrates debugging and provides no meaningful benefit. A quick survey of existing GH issues shows several incidents in which submitters who provided build output were asked to take another lap with V=1. We should save everyone a lot of time -- including people who are debugging the build themselves in development or automated build environments -- and just make that behavior the default. If for some reason there is someone relying on not having any usable information about the build and wants the existing behavior, it could be preserved if a similar make variable is set (e.g., SILENT_BUILD=1 or similar).

This is what you get when you're not running verbose:

  CXX(target) /home/rvagg/git/iojs/io.js/out/Release/obj.target/v8_libbase/deps/v8/src/base/logging.o
  CXX(target) /home/rvagg/git/iojs/io.js/out/Release/obj.target/v8_libbase/deps/v8/src/base/once.o
  CXX(target) /home/rvagg/git/iojs/io.js/out/Release/obj.target/v8_libbase/deps/v8/src/base/platform/time.o
  CXX(target) /home/rvagg/git/iojs/io.js/out/Release/obj.target/v8_libbase/deps/v8/src/base/platform/condition-variable.o

And this is what you get when you run verbose as we do now:

  g++ '-DV8_TARGET_ARCH_X64' '-DENABLE_DISASSEMBLER' '-DV8_I18N_SUPPORT' -I../deps/v8  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -fno-strict-aliasing -m64 -fdata-sections -ffunction-sections -O3 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/rvagg/git/iojs/io.js/out/Release/.deps//home/rvagg/git/iojs/io.js/out/Release/obj.target/v8_libbase/deps/v8/src/base/logging.o.d.raw   -c -o /home/rvagg/git/iojs/io.js/out/Release/obj.target/v8_libbase/deps/v8/src/base/logging.o ../deps/v8/src/base/logging.cc
  g++ '-DV8_TARGET_ARCH_X64' '-DENABLE_DISASSEMBLER' '-DV8_I18N_SUPPORT' -I../deps/v8  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -fno-strict-aliasing -m64 -fdata-sections -ffunction-sections -O3 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/rvagg/git/iojs/io.js/out/Release/.deps//home/rvagg/git/iojs/io.js/out/Release/obj.target/v8_libbase/deps/v8/src/base/once.o.d.raw   -c -o /home/rvagg/git/iojs/io.js/out/Release/obj.target/v8_libbase/deps/v8/src/base/once.o ../deps/v8/src/base/once.cc
  g++ '-DV8_TARGET_ARCH_X64' '-DENABLE_DISASSEMBLER' '-DV8_I18N_SUPPORT' -I../deps/v8  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -fno-strict-aliasing -m64 -fdata-sections -ffunction-sections -O3 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/rvagg/git/iojs/io.js/out/Release/.deps//home/rvagg/git/iojs/io.js/out/Release/obj.target/v8_libbase/deps/v8/src/base/platform/time.o.d.raw   -c -o /home/rvagg/git/iojs/io.js/out/Release/obj.target/v8_libbase/deps/v8/src/base/platform/time.o ../deps/v8/src/base/platform/time.cc
  g++ '-DV8_TARGET_ARCH_X64' '-DENABLE_DISASSEMBLER' '-DV8_I18N_SUPPORT' -I../deps/v8  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -fno-strict-aliasing -m64 -fdata-sections -ffunction-sections -O3 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/rvagg/git/iojs/io.js/out/Release/.deps//home/rvagg/git/iojs/io.js/out/Release/obj.target/v8_libbase/deps/v8/src/base/platform/condition-variable.o.d.raw   -c -o /home/rvagg/git/iojs/io.js/out/Release/obj.target/v8_libbase/deps/v8/src/base/platform/condition-variable.o ../deps/v8/src/base/platform/condition-variable.cc

I take the point of the original issue poster and I wasn't around in core before we turned V=1 on so I can't speak to the experience of having to go back and ask people to turn it on in order to get bug reports so I'd be happy to hear from more experienced folks, like @bnoordhuis and @indutny.

Here's the case for going back to a cleaner build though:

  1. Our builds have double the number of steps than they did when they were first made silent (833 then vs 1798 now), they have become extremely noisy for everyone
  2. I don't see people being asked to provide snippets of their build output these days, I suspect that we've reached a new level of maturity where our problems are elsewhere
  3. Further to point 2, we have very broad CI support, our CI system covers all of our primary targets and most of our secondary, it's really only the exotic cases where we're likely to run into problems where compiler options that users are using are interesting.
  4. It's slowing down Jenkins (I believe), all of this output gets stored for each run and it's rarely looked it. It's at least slowing down loading of console output from Jenkins (I'm regularly doing this and the majority is this compile output, not even the test output!).
  5. It's hiding compiler warnings, they get hidden in the output and we mostly ignore them.
  6. It's pretty and shiny, and who doesn't love a bit of pretty and shiny?

nodejs-github-bot added build Issues and PRs related to build files or the CI. lts-watch-v6.x labels Dec 22, 2016

trevnorris commented Dec 22, 2016

Copy link
Copy Markdown
Contributor

+1 for anything that helps us see compiler warnings.

bnoordhuis commented Dec 22, 2016

Copy link
Copy Markdown
Member

I take the point of the original issue poster and I wasn't around in core before we turned V=1 on so I can't speak to the experience of having to go back and ask people to turn it on in order to get bug reports so I'd be happy to hear from more experienced folks, like @bnoordhuis and @indutny.

I thought the verbosity was annoying at first but it did streamline bug reporting.

I don't see people being asked to provide snippets of their build output these days, I suspect that we've reached a new level of maturity where our problems are elsewhere

I still do from time to time. For example, there was a mips build issue last week that was caused by conflicting compiler flags.

It's hiding compiler warnings, they get hidden in the output and we mostly ignore them.

Yes, that's a legitimate drawback.

Can I suggest doing export V= on the CI machines but keep V=1 the default?

addaleax commented Dec 22, 2016

Copy link
Copy Markdown
Member

It's hiding compiler warnings, they get hidden in the output and we mostly ignore them.

We do occasionally receive bug reports from people having legitimate worries about compiler warnings.

gibfahn commented Dec 22, 2016

Copy link
Copy Markdown
Member

So the difference is whether you get the CC/CXX binary being run, and whether you get the compiler flags used? Am I right in thinking that these are pretty much the same for most of the compiler commands? In which case it might be possible to print them once at the top, and then not have them repeat for every command.

I'm certainly a fan of the ninja build output, which is pretty spartan except for the compiler warnings.

bnoordhuis commented Dec 22, 2016

Copy link
Copy Markdown
Member

Am I right in thinking that these are pretty much the same for most of the compiler commands?

Not really, I'm afraid. Dependencies each have their own set of flags and sometimes even different flags for different files.

gibfahn commented Dec 22, 2016

Copy link
Copy Markdown
Member

Not really, I'm afraid.

😭

Can I suggest doing export V= on the CI machines but keep V=1 the default?

We could do this and also explain that you can do make -j4 V= in the docs, which should make things easier for people who read the docs.

jasnell commented Mar 24, 2017

Copy link
Copy Markdown
Member

ping. any update on this?

jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017

fhinkel commented May 26, 2017

Copy link
Copy Markdown
Contributor

There hasn't been any activity here. I'm closing this. Feel free to reopen if I closed this in error.

fhinkel closed this May 26, 2017
rvagg mentioned this pull request Mar 19, 2019
Merged
2 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

No reviews

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. stalled Issues and PRs that are stalled.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL