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.

Sign up for GitHub

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

Jump to bottom

Greatly reduce GCC build logs #122496

Merged
merged 2 commits into from
Mar 15, 2024
Merged

Greatly reduce GCC build logs #122496

merged 2 commits into from
Mar 15, 2024

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Mar 14, 2024 •
edited
Loading

@rustbot
Copy link
Collaborator

rustbot commented Mar 14, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 14, 2024
@GuillaumeGomez
Copy link
Member Author

The --quiet option isn't enough apparently... Let's do the good old > /dev/null.

@RalfJung
Copy link
Member

RalfJung commented Mar 14, 2024 via email

@antoyo
Copy link
Contributor

antoyo commented Mar 14, 2024

That will be annoying if it ever fails though... Is there an upstream issue tracking "please provide a build option that outputs less than 100 lines on success"?

The errors are printed on stderr, so it's not an issue imo.

@RalfJung
Copy link
Member

RalfJung commented Mar 14, 2024 via email

@TimNN
Copy link
Contributor

TimNN commented Mar 14, 2024 •
edited
Loading

I don't know if it's easily installable in whatever OS the CI Docker container uses, but there's a tool called chronic that

runs a command, and arranges for its standard out and standard error to only be displayed if the command fails (exits nonzero or crashes). If the command succeeds, any extraneous output will be hidden.

Which seems like it would be useful here.

https://manpages.ubuntu.com/manpages/noble/man1/chronic.1.html

edit: Looks like chronic is part of moreutils on CentOS, which requires epel-release to be installed first, but the Docker image already does that.

@GuillaumeGomez GuillaumeGomez force-pushed the reduce-gcc-build-logs branch 2 times, most recently from 0028fb7 to 6d312a2 Compare March 14, 2024 20:21
@klensy
Copy link
Contributor

klensy commented Mar 14, 2024

Well, you can tune log analyzer to skip these build logs instead turning off logs.

@RalfJung
Copy link
Member

Sometimes one has to look at the logs though. I don't think it is reasonable to have 80% of them filled with the GCC build when that's a tiny part of what we are testing.

@klensy
Copy link
Contributor

klensy commented Mar 14, 2024

It wasn't an issue before, so there something changed in pipeline?

@RalfJung
Copy link
Member

It's not been that long since GCC was added to the build. I think it was a problem ever since, or do you have evidence to the contrary?

Mark-Simulacrum
Mark-Simulacrum reviewed Mar 15, 2024
View reviewed changes
src/ci/docker/host-x86_64/dist-x86_64-linux/build-gccjit.sh Outdated
# of output. Unfortunately, even when using `--quiet` option for all commands,
# the output still thousands of lines of output, forcing us to use this solution.
make > /dev/null
make install > /dev/null
Copy link
Member

Choose a reason for hiding this comment

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

Please use hide_output from src/ci/docker/scripts/shared.sh (feel free to grep for other usages). That makes sure there's some progress indication and we do the right thing on failure.

@RalfJung
Copy link
Member

RalfJung commented Mar 15, 2024 •
edited
Loading

Btw is it expected that the GCC build takes about 30 minutes? (If I understand this log correctly.) That's 30% of the entire CI job! And in particular this happens before the rustc test suite is run, greatly increasing the latency until regular test failures are discovered.

@RalfJung
Copy link
Member

Also why do both the gnu-llvm-16 and the gnu-tools runner build GCC...?

@GuillaumeGomez
Copy link
Member Author

Also why do both the gnu-llvm-16 and the gnu-tools runner build GCC...?

It is run it 3 CIs: gnu-tools, llvm-16 and llvm-17. When I made the last sync of rustc_codegen_gcc, I removed the installations of libgccjit through the system and replaced it with the build of GCC (you can see that in the last commits of #122042, like c5c6729).

If people agree on reducing the number of CIs building GCC, I can remove some of them.

@GuillaumeGomez
Copy link
Member Author

Btw is it expected that the GCC build takes about 30 minutes? (If I understand this log correctly.) That's 30% of the entire CI job! And in particular this happens before the rustc test suite is run, greatly increasing the latency until regular test failures are discovered.

For now it's built using only one thread. I don't know if I can get the number of CPUs available in the CI and pass it to the make scripts. That would likely greatly improve the time it takes in the CI.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Mark-Simulacrum
Copy link
Member

https://github.com/rust-lang/rust/blob/master/src/ci/docker/host-x86_64/dist-x86_64-linux/build-clang.sh#L42 is an example. It also demonstrates the use of hide_output.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung RalfJung mentioned this pull request Mar 15, 2024
Open
RalfJung
RalfJung reviewed Mar 15, 2024
View reviewed changes
src/ci/docker/host-x86_64/dist-x86_64-linux/build-gccjit.sh
make install
--prefix=$(pwd)/../gcc-install \

hide_output make -j$(nproc)
Copy link
Member

Choose a reason for hiding this comment

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

This seems to speed up the build by a factor of 4.^^ It was 30min before, now it's...

2024-03-15T12:47:03.8776557Z #14 DONE 446.8s

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice. :)

RalfJung
RalfJung reviewed Mar 15, 2024
View reviewed changes
src/ci/docker/host-x86_64/dist-x86_64-linux/build-gccjit.sh Outdated

set -ex

source /tmp/shared.sh
Copy link
Member

Choose a reason for hiding this comment

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

Why didn't you put this in the same folder as the build-gccjit script?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly because I was first trying to make it work. Cleanup time!

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

Clean up done. Nice speed up in any case. :)

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 15, 2024

📌 Commit c4ece1f has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2024
@GuillaumeGomez GuillaumeGomez mentioned this pull request Mar 15, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2024
@bors
Auto merge of rust-lang#122555 - GuillaumeGomez:rollup-tr6wu54, r=Gui…
72d7897 
…llaumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#114651 (rustdoc: add `--test-builder-wrapper` arg to support wrappers such as RUSTC_WRAPPER when building doctests)
 - rust-lang#122468 (Cleanup `MirBorrowckCtxt::prefixes`)
 - rust-lang#122496 (Greatly reduce GCC build logs)
 - rust-lang#122512 (Cursor.rs documentation fix)
 - rust-lang#122513 (hir: Remove `opt_local_def_id_to_hir_id` and `opt_hir_node_by_def_id`)
 - rust-lang#122530 (less symbol interner locks)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5325c2b into rust-lang:master Mar 15, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2024
@rust-timer
Unrolled build for rust-lang#122496
ece41f3 
Rollup merge of rust-lang#122496 - GuillaumeGomez:reduce-gcc-build-logs, r=Mark-Simulacrum

Greatly reduce GCC build logs

Fixes rust-lang/rust-log-analyzer#80.

Based on [makefile documentation](https://www.gnu.org/software/make/manual/html_node/Options-Summary.html#index-_002d_002dquiet-1) and [configure documentation](https://www.gnu.org/software/autoconf/manual/autoconf-2.68/html_node/configure-Invocation.html).

cc `@RalfJung` `@antoyo`
@GuillaumeGomez GuillaumeGomez deleted the reduce-gcc-build-logs branch March 15, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@antoyo antoyo antoyo approved these changes

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.78.0
Development

Successfully merging this pull request may close these issues.

Log often shows lots of irrelevant build messages
9 participants
@GuillaumeGomez @rustbot @RalfJung @antoyo @TimNN @klensy @rust-log-analyzer @Mark-Simulacrum @bors