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

Download GCC from CI on test builders #138395

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Mar 12, 2025

This should reduce the duration of the x86_64-gnu-llvm-18 job, which runs on PR CI, which is currently the only one that builds GCC (outside of the x64 dist builder).

Since we handle the GCC download in the GCC step, and not eagerly in config, we can set this flag globally across all test builders, as it won't do anything unless they actually try to build GCC.

Opening as a draft to test if it works on CI, because I still need to implement logic to avoid the download if there are any local modifications to GCC (essentially the "if-unchanged" mode, although I want to try something a bit different).

r? @ghost

@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 12, 2025
@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 12, 2025

The LLVM 18 PR CI job took ~60 minutes before, let's see if this helps.

@rust-log-analyzer

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Mar 12, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 12, 2025

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 12, 2025
Download GCC from CI on test builders

This should reduce the duration of the `x86_64-gnu-llvm-18` job, which runs on PR CI, which is currently the only one that builds GCC (outside of the x64 dist builder).

Since we handle the GCC download in the GCC step, and not eagerly in config, we can set this flag globally across all test builders, as it won't do anything unless they actually try to build GCC.

Opening as a draft to test if it works on CI, because I still need to implement logic to avoid the download if there are any local modifications to GCC (essentially the "if-unchanged" mode, although I want to try something a bit different).

r? `@ghost`
@bors
Copy link
Contributor

bors commented Mar 12, 2025

⌛ Trying commit 293a246 with merge de4ba24...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 12, 2025

☀️ Try build successful - checks-actions
Build commit: de4ba24 (de4ba246353c1d095c81f3c37159eaec194f7829)

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2025
Build GCC on CI with GCC, not Clang

It seems that GCC built with Clang misbehaves.

Prerequisite for rust-lang#138395.

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2025
Build GCC on CI with GCC, not Clang

It seems that GCC built with Clang misbehaves.

Prerequisite for rust-lang#138395.

r? `@ghost`
jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 14, 2025
…meGomez

Build GCC on CI with GCC, not Clang

It seems that GCC built with Clang misbehaves. I have tested that cg_gcc tests [pass](https://github.com/rust-lang/rust/actions/runs/13842365913/job/38732750617?pr=138451) on CI with a downloaded GCC that was built in this way.

Prerequisite for rust-lang#138395.

r? `@ghost`
fmease added a commit to fmease/rust that referenced this pull request Mar 14, 2025
…meGomez

Build GCC on CI with GCC, not Clang

It seems that GCC built with Clang misbehaves. I have tested that cg_gcc tests [pass](https://github.com/rust-lang/rust/actions/runs/13842365913/job/38732750617?pr=138451) on CI with a downloaded GCC that was built in this way.

Prerequisite for rust-lang#138395.

r? ``@ghost``
fmease added a commit to fmease/rust that referenced this pull request Mar 14, 2025
…meGomez

Build GCC on CI with GCC, not Clang

It seems that GCC built with Clang misbehaves. I have tested that cg_gcc tests [pass](https://github.com/rust-lang/rust/actions/runs/13842365913/job/38732750617?pr=138451) on CI with a downloaded GCC that was built in this way.

Prerequisite for rust-lang#138395.

r? ```@ghost```
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2025
Rollup merge of rust-lang#138451 - Kobzol:gcc-ci-build-gcc, r=GuillaumeGomez

Build GCC on CI with GCC, not Clang

It seems that GCC built with Clang misbehaves. I have tested that cg_gcc tests [pass](https://github.com/rust-lang/rust/actions/runs/13842365913/job/38732750617?pr=138451) on CI with a downloaded GCC that was built in this way.

Prerequisite for rust-lang#138395.

r? ```@ghost```
@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 15, 2025

Good, the x86_64-gnu-llvm-18 is now 5 minutes quicker, because it downloads GCC rather than building it. Now I just need to implement the behavior of "if-unchanged".

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Kobzol Kobzol force-pushed the ci-download-gcc branch 2 times, most recently from 3059c1f to fc33895 Compare March 15, 2025 19:57
@rust-log-analyzer

This comment has been minimized.

@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 15, 2025

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2025
Download GCC from CI on test builders

This should reduce the duration of the `x86_64-gnu-llvm-18` job, which runs on PR CI, which is currently the only one that builds GCC (outside of the x64 dist builder).

Since we handle the GCC download in the GCC step, and not eagerly in config, we can set this flag globally across all test builders, as it won't do anything unless they actually try to build GCC.

Opening as a draft to test if it works on CI, because I still need to implement logic to avoid the download if there are any local modifications to GCC (essentially the "if-unchanged" mode, although I want to try something a bit different).

r? `@ghost`
@bors
Copy link
Contributor

bors commented Mar 15, 2025

⌛ Trying commit fc33895 with merge e3b0b1c...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 15, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2025
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#20 exporting to docker image format
#20 sending tarball 19.6s done
#20 DONE 23.2s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Listening on address 127.0.0.1:4226
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'build.print-step-timings', '--enable-verbose-tests', '--set', 'build.metrics', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--set', 'gcc.download-ci-gcc=true', '--enable-new-symbol-mangling']
configure: build.build          := x86_64-unknown-linux-gnu
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---

Merge commit4: 
HEAD: 06aa108c44b187cb3ebdf1378ad04ebd19e42540

06aa108c Jakub Beránek berykubik@gmail.com Merge f48536daec8845737f890d67c5b9637e78d8853b into 4d30011f6c616be074ba655a75e5d55441232bbb
f48536da Jakub Beránek berykubik@gmail.com WIP
4d30011f bors bors@rust-lang.org Auto merge of #138532 - matthiaskrgr:rollup-mgcynqu, r=matthiaskrgr
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Build completed unsuccessfully in 0:00:26
make: *** [Makefile:100: prepare] Error 1
Command failed. Attempt 2/5:
##[group]Building bootstrap
---

Merge commit4: 
HEAD: 06aa108c44b187cb3ebdf1378ad04ebd19e42540

06aa108c Jakub Beránek berykubik@gmail.com Merge f48536daec8845737f890d67c5b9637e78d8853b into 4d30011f6c616be074ba655a75e5d55441232bbb
f48536da Jakub Beránek berykubik@gmail.com WIP
4d30011f bors bors@rust-lang.org Auto merge of #138532 - matthiaskrgr:rollup-mgcynqu, r=matthiaskrgr
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Build completed unsuccessfully in 0:00:00
make: *** [Makefile:100: prepare] Error 1
Command failed. Attempt 3/5:
##[group]Building bootstrap
---

Merge commit4: 
HEAD: 06aa108c44b187cb3ebdf1378ad04ebd19e42540

06aa108c Jakub Beránek berykubik@gmail.com Merge f48536daec8845737f890d67c5b9637e78d8853b into 4d30011f6c616be074ba655a75e5d55441232bbb
f48536da Jakub Beránek berykubik@gmail.com WIP
4d30011f bors bors@rust-lang.org Auto merge of #138532 - matthiaskrgr:rollup-mgcynqu, r=matthiaskrgr
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Build completed unsuccessfully in 0:00:00
make: *** [Makefile:100: prepare] Error 1
Command failed. Attempt 4/5:
##[group]Building bootstrap
---

Merge commit4: 
HEAD: 06aa108c44b187cb3ebdf1378ad04ebd19e42540

06aa108c Jakub Beránek berykubik@gmail.com Merge f48536daec8845737f890d67c5b9637e78d8853b into 4d30011f6c616be074ba655a75e5d55441232bbb
f48536da Jakub Beránek berykubik@gmail.com WIP
4d30011f bors bors@rust-lang.org Auto merge of #138532 - matthiaskrgr:rollup-mgcynqu, r=matthiaskrgr
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Build completed unsuccessfully in 0:00:00
make: *** [Makefile:100: prepare] Error 1
Command failed. Attempt 5/5:
##[group]Building bootstrap
---

Merge commit4: 
HEAD: 06aa108c44b187cb3ebdf1378ad04ebd19e42540

06aa108c Jakub Beránek berykubik@gmail.com Merge f48536daec8845737f890d67c5b9637e78d8853b into 4d30011f6c616be074ba655a75e5d55441232bbb
f48536da Jakub Beránek berykubik@gmail.com WIP
4d30011f bors bors@rust-lang.org Auto merge of #138532 - matthiaskrgr:rollup-mgcynqu, r=matthiaskrgr
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Build completed unsuccessfully in 0:00:00
make: *** [Makefile:100: prepare] Error 1
The command has failed after 5 attempts.
  local time: Sat Mar 15 21:25:24 UTC 2025

@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 16, 2025

I will first work on refactoring the bootstrap git upstream fetch logic, because it is a mess.

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2025
Refactor git change detection in bootstrap

While working on rust-lang#138395, I finally found the courage to delve into the insides of git path change detection in bootstrap, which is used (amongst other things) to detect if we should rebuilt od download `[llvm|rustc|gcc]`. I found it a bit hard to understand, and given that this code was historically quite fragile, I thought that it would be better to rebuild it from scratch.

The previous approach had a bunch of limitations:
- It separated the computation of "are there local changes?" and "what upstream SHA should we use?" even though these two things are intertwined.
- It used hacks to work around what happens on CI.
- It had special cases for CI scattered throughout the codebase, rather than centralized in one place.
- It wasn't documented enough and didn't have tests for the git behavior.

The current approach should hopefully resolve all of that. I implemented a single entrypoint called `check_path_modifications` (naming bikeshed pending, half of the time I spend on this PR was thinking about names, as it's quite tricky here..) that explicitly receives a mode of operation (in CI or outside CI), and accordingly figures out that upstream SHA that we should use for downloading artifacts and it also figures out if there are any local changes. Users of this function can then use this unified output to implement `download-ci-X` and other functionality.

I also added a bunch of integration tests that literally spawn a git repository on disk and then check that the function can deal with various situations (PR CI, auto/try CI, local builds). The tests are super fast and run in parallel, as they are currently in `build_helper` and not in `bootstrap`.

After I built this inner layer, I used it for downloading GCC, LLVM and rustc. The latter two (and especially rustc) were using the `last_modified_commit` function before, but in all cases but one this function was actually only used to check if there are any local changes, which was IMO confusing. The LLVM handling would deserve a bit of refactoring, but that's a larger change that can be done as a follow-up.

In the future we could cache the results of `check_path_modifications` to reduce the number of git invocations, but I don't think that it should be excessive even now.

I hope that the implementation is now clear and easy to understand, so that in combination with the tests we can have more confidence that it does what we want. I tried to include a lot of documentation in the code, so I won't be repeating the actual implementation details here, if there are any questions, I'll add the answers to the documentation too :)

The new approach explicitly supports three scenarios:
- Running on PR CI, where we have one upstream bors parent commit and one PR merge commit made by GitHub.
- Running on try/auto CI, where we have one upstream bors parent commit and one PR merge commit made by bors.
- Running locally, where we assume that we have at least one upstream bors parent commit in our git history.

I removed the handling of upstreams on CI, as I think that it shouldn't be needed and I considered it to be a hack. However, it's possible that there are other use-cases that I haven't considered, so I want to ask around if people have other situations than the three use-cases described above. If there are other such use-cases, I would like to include them in the new centralized implementation and add them to the git test suite, rather than going back to the old ways :)

In particular, the code before relied on `git merge-base`, but I don't see why we can't just lookup the most recent bors commit and assume that is a merge commit that is also upstream? I might be running into Chesterton's Fence here :)

CC `@pietroalbini` To make sure that this won't break downstream users of Rust's CI.

Best reviewed commit by commit.

Companion PRs:
- For testing beta: rust-lang#138597

r? `@onur-ozkan`

try-job: x86_64-gnu-aux
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants