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

Distribute bootstrap in CI #98483

Merged
merged 9 commits into from
Sep 24, 2022
Merged

Distribute bootstrap in CI #98483

merged 9 commits into from
Sep 24, 2022

Conversation

dvtkrlbs
Copy link
Contributor

@dvtkrlbs dvtkrlbs commented Jun 25, 2022

This pre-compiles bootstrap from source and adds it to the existing rust-dev component. There are two main goals here:

  1. Make it faster to build rust from source, both the first time and incrementally
  2. Make it easier to add non-python entrypoints, since they can call out to bootstrap directly rather than having to figure out the right flags to pre-compile it. This second part is still in a bit of flux, see the tracking issue below for more information.

There are also several changes to make bootstrap able to run on a machine other than the one it was built (particularly around config.src and config.out detection). I (@jyn514) am slightly concerned these will regress unless tested - maybe we should add an automated test that runs bootstrap in a chroot or something? Unclear whether the effort is worth the test coverage.

Helps with #94829.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2022
@dvtkrlbs
Copy link
Contributor Author

r? @jyn514

@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 added 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. labels Jun 25, 2022
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Thanks! This looks like roughly the right approach - I left a couple comments, but I'll go ahead and do a try run to confirm this works in CI.

src/bootstrap/dist.rs Outdated Show resolved Hide resolved
src/bootstrap/dist.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Jun 25, 2022

@bors try

@bors
Copy link
Contributor

bors commented Jun 25, 2022

⌛ Trying commit 57612869a0995361c9d9bbac8014b449608369d5 with merge 374196694bb6c1d561fe1ee38206a7e5a13bd4f7...

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Jun 25, 2022

@bors try

@bors
Copy link
Contributor

bors commented Jun 25, 2022

⌛ Trying commit b648737caf442c51ffe61a942774e68f6b82ffbb with merge e9b1f9380fec42aa93b6998a1e1a1dc2ae9adaff...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 25, 2022

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

@jyn514 jyn514 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 Jun 26, 2022
@dvtkrlbs dvtkrlbs force-pushed the bootstrap-dist branch 2 times, most recently from 643f118 to 4afd45d Compare June 26, 2022 18:06
@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 26, 2022
@jyn514 jyn514 assigned Mark-Simulacrum and unassigned jyn514 Jun 26, 2022
@jyn514
Copy link
Member

jyn514 commented Jun 26, 2022

This is looking pretty good to me except for the defaults. @Mark-Simulacrum I'd appreciate if you could take a look at #98483 (comment).

@jyn514
Copy link
Member

jyn514 commented Sep 24, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Sep 24, 2022

📌 Commit 2ef3d17 has been approved by jyn514

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 Sep 24, 2022
@bors
Copy link
Contributor

bors commented Sep 24, 2022

⌛ Testing commit 2ef3d17 with merge 3f83906...

@bors
Copy link
Contributor

bors commented Sep 24, 2022

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 3f83906 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 24, 2022
@bors bors merged commit 3f83906 into rust-lang:master Sep 24, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 24, 2022
@bors bors mentioned this pull request Sep 24, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3f83906): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Warning ⚠: The following benchmark(s) failed to build:

  • rustc

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.1% [2.1%, 2.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.9% [-3.9%, -3.9%] 1
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
2.4% [2.4%, 2.4%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.4% [2.4%, 2.4%] 1

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

bors pushed a commit to rust-lang-ci/rust that referenced this pull request Sep 25, 2022
This restores functionality broken by rust-lang#98483. Unfortunately, it doesn't
add a test to verify this works, but in this case we notice pretty
quickly as perf uses this functionality and so reports breakage
immediately after merging.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 25, 2022
…jyn514

Support overriding initial rustc and cargo paths

This restores functionality broken by rust-lang#98483. Unfortunately, it doesn't add a test to verify this works, but in this case we notice pretty quickly as perf uses this functionality and so reports breakage immediately after merging.

r? `@jyn514`

cc https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/rustc.20and.20cargo.20option.20broken.20in.20config.2Etoml, https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/Rustc.20benchmark.20broken
@kleisauke
Copy link
Contributor

This PR seems to break my workflow that configures the Rust nightly tarball inside a Git-tracked directory. For example:

$ git init my-project
$ mkdir my-project/rust-nightly
$ curl -sL https://static.rust-lang.org/dist/2022-09-28/rustc-nightly-src.tar.xz | tar xJC my-project/rust-nightly --strip-components=1
$ cd my-project/rust-nightly
$ git rev-parse --show-toplevel
/home/kleisauke/my-project
$ cp config.toml.example config.toml
$ ./x.py install --stage 1 -v
running: /home/kleisauke/my-project/rust-nightly/build/bootstrap/debug/bootstrap install --stage 1 -v
thread 'main' panicked at 'std::fs::read(&config.src.join("src").join("stage0.json")) failed with No such file or directory (os error 2)', config.rs:848:27
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Traceback (most recent call last):
  File "./x.py", line 28, in <module>
    bootstrap.main()
  File "/home/kleisauke/my-project/rust-nightly/src/bootstrap/bootstrap.py", line 936, in main
    bootstrap(help_triggered)
  File "/home/kleisauke/my-project/rust-nightly/src/bootstrap/bootstrap.py", line 922, in bootstrap
    run(args, env=env, verbose=build.verbose, is_bootstrap=True)
  File "/home/kleisauke/my-project/rust-nightly/src/bootstrap/bootstrap.py", line 166, in run
    raise RuntimeError(err)
RuntimeError: failed to run: /home/kleisauke/my-project/rust-nightly/build/bootstrap/debug/bootstrap install --stage 1 -v

(this is a simplified example from our Rust build script based on MXE)

Is there a way to skip this logic?

@jyn514
Copy link
Member

jyn514 commented Sep 28, 2022

@kleisauke we can fix your issue by using current_dir(std::env::current_exe().parent(). But in general running bootstrap outside the source directory has very limited support, bugs are fixed as they arise but we don't test it in CI.

@kleisauke
Copy link
Contributor

@jyn514 IIUC, that wouldn't fix the above issue (+ I don't run the build script outside the source directory). The thing is that my parent directory is a Git-tracked directory and git rev-parse --show-toplevel would resolve to that, causing a build error since my-project/src/stage0.json doesn't exist in that case.

Perhaps git_root needs to be checked for the presence of that file before setting config.src?

@jyn514
Copy link
Member

jyn514 commented Sep 29, 2022

Oh I see, you've unpacked a source tarball in a subdirectory of another git project. I highly suspect there's several other things that will be broken for you with that setup (like the commit hash we detect for the compiler version), it's not something we've ever considered or tested.

I'm willing to take a PR that falls back to the tarball behavior when stage0.json is missing, but this is very likely not the only thing that's broken and I'd suggest unpacking the tarball in a temporary directory instead.

kleisauke added a commit to libvips/build-win64-mxe that referenced this pull request Sep 30, 2022
@kleisauke
Copy link
Contributor

Sounds reasonable. Fixed with commit libvips/build-win64-mxe@9b293bc instead, thanks!

@cr1901
Copy link
Contributor

cr1901 commented Oct 1, 2022

it's not something we've ever considered or tested.

I appear to be in the same boat as @kleisauke. I build rust in a directory called build-rust, which happens to be an (ignored) subdirectory of a git repo where I keep my scripts for building projects using LLVM. The Rust source git repo can be anywhere on the filesystem and is not a subdirectory of any other git repo in my case.

However, right now, my scripts for building Rust/LLVM only support building in the immediate subdirectory of where my scripts live. Because the bootstrap logic doesn't know were in the subdirectory of a different repo, this commit makes it impossible for me to build Rust using my workflow unless I do mv .git .git-bak for my scripts repo every time I build Rust. Alternatively, I could just not version-control my scripts, but I don't want to do that :P.

Perhaps a PR for overriding the git check when using a separate build directory that is a subdir of a git repo may be accepted?

@jyn514
Copy link
Member

jyn514 commented Oct 1, 2022

@cr1901

I'm willing to take a PR that falls back to the tarball behavior when stage0.json is missing, but this is very likely not the only thing that's broken and I'd suggest unpacking the tarball in a temporary directory instead.

@cr1901
Copy link
Contributor

cr1901 commented Oct 1, 2022

this is very likely not the only thing that's broken

FWIW, doing mv .git .git-bak got me a successful compile. I've been using a separate build dir for several years to build Rust. So I'm willing to bet fallback logic will work at least for now.

I'd suggest unpacking the tarball in a temporary directory instead.

If by "tarball" you mean "the Rust source" and not "the downloaded bootstrap compiler", I don't think this applies to me since I'm building Rust from the git source.

In other words, I think a PR that falls back to the tarball behavior is the only option I have unless you can think of a method to support my use case w/ a downloaded bootstrap.

That said, I use sccache for building Rust, so I don't know how much time I will lose having to rebuild the bootstrap artifacts when the bootstrap compiler version hasn't changed.

I'll make a PR when I get the chance.

@jyn514
Copy link
Member

jyn514 commented Oct 2, 2022

Ok, you have a slightly different case: the build directory is within an unrelated git repo, not the source directory. I agree we should support that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.