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: revert to pretty-print by default #10403

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Dec 22, 2016

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 nodejs-github-bot added build Issues and PRs related to build files or the CI. lts-watch-v6.x labels Dec 22, 2016
@trevnorris
Copy link
Contributor

+1 for anything that helps us see compiler warnings.

@bnoordhuis
Copy link
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
Copy link
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
Copy link
Member

gibfahn commented Dec 22, 2016

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

gibfahn commented Dec 22, 2016

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

jasnell commented Mar 24, 2017

ping. any update on this?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017
@fhinkel
Copy link
Member

fhinkel commented May 26, 2017

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

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. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants