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

Allow building rustdoc without first building rustc (MVP) #79540

Merged
merged 2 commits into from
Feb 9, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Nov 29, 2020

Motivation

The compile times for rustc are extremely long and a major issue for
recruiting new contributors to rustdoc. People interested in joining
often give up after running into issues with submodules or python
versions. stage1 rustdoc fundamentally doesn't care about bootstrapping
or stages, it just needs rustc_private available.

Summary of Changes

  • Add an opt-in [rust] download_rustc option
  • Determine the version of the compiler to download using log --author=bors
  • Do no work for any component other than Rustdoc for any stage. Instead, copy the CI artifacts from the downloaded sysroot stage0/ to stage0-sysroot/ or stage1/ in Sysroot. This is done with an ENABLE_DOWNLOAD_STAGE1 constant which is off by default.
  • Don't download different versions of rustfmt or cargo - those should still use the beta version (rustfmt especially).

The vast majority of work is done in bootstrap.py, which downloads the artifacts and extracts them to stage0/ in place of the beta compiler. Rustbuild just takes care of copying the artifacts to stage1 if necessary.

Future work

Some of this work has already been done in (the history for) jyn514@673476c.

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) needs-fcp This change is insta-stable, so needs a completed FCP to proceed. A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself labels Nov 29, 2020
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 29, 2020
@GuillaumeGomez
Copy link
Member

Once you'be built with cargo build, will you be able to run test using this rustdoc binary? Also, would it be possible to add a check in the global config to be able to switch between "toolchain build" (the build which doesn't require to build rustc first) and "compiler build"?

@jyn514
Copy link
Member Author

jyn514 commented Nov 29, 2020

Once you'be built with cargo build, will you be able to run test using this rustdoc binary?

I discussed this in 'Unresolved Questions'.

would it be possible to add a check in the global config to be able to switch between "toolchain build" (the build which doesn't require to build rustc first) and "compiler build"?

Hmm, you mean have x.py build sometimes use a nightly rustc, so that you use the same command for both compiling rustc from source and downloading it from CI? That seems confusing, although possibly not as confusing as having two different build commands.

@jyn514 jyn514 added the C-discussion Category: Discussion or questions that doesn't represent real issues. label Nov 29, 2020
@Mark-Simulacrum
Copy link
Member

I think rustup management is the wrong approach; we should be able to use similar logic to LLVM downloads and get a rustc from previous CI commit. That'll always work if you don't modify rustc (which we can check via git). This would still require using x.py but I think that's actually not a huge burden in practice - it should work with any python version. I think we can do more to ease the introduction to x.py, but I would rather do so for all contributors rather than piecemeal.

@jyn514
Copy link
Member Author

jyn514 commented Nov 29, 2020

Ok, I like that idea - then most of the concerns here go away because you're still going through x.py, and x.py test src/test/rustdoc might just go through a shortcut (downloading from CI) instead of building rustc from source. That seems not too hard to implement either :)

@GuillaumeGomez
Copy link
Member

That sounds like a great idea indeed!

@Mark-Simulacrum Mark-Simulacrum 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 Nov 29, 2020
@jyn514
Copy link
Member Author

jyn514 commented Nov 30, 2020

@Mark-Simulacrum ideally this would only download the rustc if you were going to use it for something (and not for, say x.py fmt). Is there a way to tell in bootstrap.py whether rustc needs to be built, or is all that logic in rust? And if it's in rust, is it ok to add download logic there? I know you've been trying to keep downloads in python.

@robinmoussu
Copy link

I'm only adding remarks. Iknow that my understanding of the big picture is flawed, so feel definitively free to ignore any of those. And this is definitively not a rant, but pure feedback. I am certain that least when those where introduced they made total sense.

I am not sure if this PR is the right place to discuss those points either. If you want to, it may be better to continue on zulip.

And I really think that this PR goes in the right direction.


Modify Cargo to understand dependencies on binaries.

Regarding this very specific sub issue, I think that adding a sysroot section in Cargo.toml would help. As far as I understand (I never used Rust 2015 so I may miss something) the sysroot is the last reason to have extern crate in Rust 2018. Having a dedicated sysroot entry would remove this, while making it easier for newcommer to discover the word sysroot.

I think rustup management is the wrong approach; we should be able to use similar logic to LLVM downloads and get a rustc from previous CI commit. That'll always work if you don't modify rustc (which we can check via git).

As an external contributor, I 100% understand the need for a tool like x.py if (and only if) I did any modification to rustc. I absolutely understand the need for a dedicated tool to be able to bootsrap the compiler. That being said, if I didn't modify it, I am not expecting, nor understanding why I need to use a bootsrapping tool and not regular cargo. This probably means that x.py is more than just a bootstraping tool. As an external contributor, it's not intuitive understand why cargo would not be sufficient.

I think that having a dedicated sysroot entry could also help to point to the right place in the documentation (via a warning) why x.py is needed when invoquing cargo directly.

Finally, rustdoc doesn't feel any more special than any other rust project from an exteral point of view. Since, the documentation for x.py is (for absolutely understandable reason) targetted towards rustc futur contributor, when working on rustdoc, I feel totally a second class citizen especially because I don't understand why x.py is needed, and what part of this complex tool I need to understand. It seems that rustdoc is in fact not just a regular rust project. This increase a lot the onboarding difficulty, because in addition to discovering the codebase, I am also required to learn new tools.

@Mark-Simulacrum
Copy link
Member

I recently found out that the downloads shell out to curl or whatever on platforms anyway, so I'm less concerned now about keeping the logic there, but I would like to avoid duplicating it if possible. I would not worry about extra downloads to start though, I expect almost everyone to need that download anyway. Note that it shouldn't be a problem to use that compiler for bootstrap as well, so you'd just be downloading a different set of libraries, not too much more (well rustc-dev would be new, but that's I think roughly 60MB overhead these days, so not horrible).

@jyn514
Copy link
Member Author

jyn514 commented Nov 30, 2020

@robinmoussu we can discuss more on Zulip if you like, but the tl;dr is that

  • Rustdoc requires extremely specific versions of the compiler to run. Forget latest nightly; sometimes it needs a commit that hasn't been published to a nightly yet. Keeping this in sync without an external tool would be very painful.
  • The test suite is completely dependent on compiletest, which only exists in-tree with rust-lang/rust. I have some ideas for making this work out of tree, but they'd still be dependent on specific versions and break frequently if used out-of-tree.

I also want to caution against trying to fix every problem at once; that's a good way to fix none of them (as I found in rust-lang/rustc-dev-guide#843).

src/librustdoc/main.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member Author

jyn514 commented Nov 30, 2020

Ok, this now downloads stage1 rustc from commit build artifacts if compiler/ hasn't been modified since the last master commit. Unfortunately, rustbuild is still rebuilding stage1 because I don't know how to tell it the artifacts are already there. I glanced at builder.ensure and it's doing crazy things with TypeId, I'm not sure where would be a good place to add caching that's not part of the type system.

@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 Nov 30, 2020
@jyn514
Copy link
Member Author

jyn514 commented Nov 30, 2020

Oh and CI is failing because it uses python2 by default and I used subprocess APIs only introduced in python3, that seems like something I could fix ~later.

src/bootstrap/bootstrap.py Outdated Show resolved Hide resolved
@jyn514
Copy link
Member Author

jyn514 commented Dec 2, 2020

Annoyingly, GitHub actions makes the .git directory read-only, so I can't run git fetch. I see two options:

  • Ignore errors from git fetch. Graceful degradation is always good, but I'm slightly concerned this will miss fixable errors and cause spurious rebuilds.
  • Find a way to determine which of the existing remotes points to rust-lang/rust. Actually maybe git remote -v can do this? Let me try that.

@bors
Copy link
Contributor

bors commented Feb 9, 2021

📌 Commit 4aec8a5 has been approved by Mark-Simulacrum

@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 Feb 9, 2021
@bors
Copy link
Contributor

bors commented Feb 9, 2021

⌛ Testing commit 4aec8a5 with merge 185de5f...

@bors
Copy link
Contributor

bors commented Feb 9, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 185de5f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 9, 2021
@bors bors merged commit 185de5f into rust-lang:master Feb 9, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 9, 2021
@jyn514 jyn514 deleted the no-xpy branch February 9, 2021 19:52
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 10, 2021
Use format string in bootstrap panic instead of a string directly

This fixes the following warning when compiling with nightly:

```
warning: panic message is not a string literal
    --> src/bootstrap/builder.rs:1515:24
     |
1515 |                 panic!(out);
     |                        ^^^
     |
     = note: `#[warn(non_fmt_panic)]` on by default
     = note: this is no longer accepted in Rust 2021
help: add a "{}" format string to Display the message
     |
1515 |                 panic!("{}", out);
     |                        ^^^^^
help: or use std::panic::panic_any instead
     |
1515 |                 std::panic::panic_any(out);
     |                 ^^^^^^^^^^^^^^^^^^^^^^
```

Found while working on rust-lang#79540. cc rust-lang#81645, which landed in 1.51.
jyn514 added a commit to jyn514/rust that referenced this pull request Feb 24, 2021
This was introduced as part of the MVP for `download-rustc`.
Unfortunately, it doesn't work very well:

- Steps are ignored by default, which makes it easy to leave out a step
that should be built. For example, the MVP forgot to enable any tests,
so it was *only* possible to build locally.
- It didn't work correctly even when it was enabled: calling
  `builder.ensure()` would completely ignore the constant and rebuild the
  step anyway. This has no obvious fix since `ensure()` has to return a
  `Step::Output`.

Instead, this handles `download-rustc` in `impl Step for Rustc` and
`impl Step for Std`, which to my knowledge are the only build steps that
don't first go through `impl Step for Sysroot` (`Rustc` is used for
the `rustc-dev` component).

See rust-lang#79540 (comment)
and rust-lang#81930 for further context.

Here are some example runs with these changes and `download-rustc`
enabled:

```
$ x.py build src/tools/clippy
Building stage1 tool clippy-driver (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 1m 09s
Building stage1 tool cargo-clippy (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.11s
$ x.py test src/tools/clippy
Updating only changed submodules
Submodules updated in 0.01 seconds
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
Building stage1 tool clippy-driver (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.09s
Building rustdoc for stage1 (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.28s
    Finished release [optimized] target(s) in 15.26s
     Running build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/deps/clippy_driver-8b407b140e0aa91c
test result: ok. 592 passed; 0 failed; 3 ignored; 0 measured; 0 filtered out
$ x.py build src/tools/rustdoc
Building rustdoc for stage1 (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 41.28s
Build completed successfully in 0:00:41
$ x.py test src/test/rustdoc-ui
Building stage0 tool compiletest (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.12s
Building rustdoc for stage1 (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.10s
test result: ok. 105 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 8.15s
$ x.py build compiler/rustc
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
Build completed successfully in 0:00:00
```

Note a few things:

- Clippy depends on stage1 rustc-dev artifacts, but rustc didn't have to
  be recompiled. Instead, the artifacts were copied automatically.
- All steps are always enabled. There is no danger of forgetting a step,
  since only the entrypoints have to handle `download-rustc`.
- Building the compiler (`compiler/rustc`) automatically does no work.
jyn514 pushed a commit to jyn514/rust that referenced this pull request Mar 1, 2021
…imulacrum

Remove `ENABLE_DOWNLOAD_RUSTC` constant

`ENABLE_DOWNLOAD_RUSTC` was introduced as part of the MVP for `download-rustc` as a way not to rebuild artifacts that have already been downloaded. Unfortunately, it doesn't work very well:

- Steps are ignored by default, which makes it easy to leave out a step
that should be built. For example, the MVP forgot to enable any tests,
so it was only possible to *build* locally.
- It didn't work correctly even when it was enabled: calling
  `builder.ensure()` would completely ignore the constant and rebuild the
  step anyway. This has no obvious fix since `ensure()` has to return a
  `Step::Output`.

Instead, this handles `download-rustc` in `impl Step for Rustc` and
`impl Step for Std`, which to my knowledge are the only build steps that
don't first go through `impl Step for Sysroot` (`Rustc` is used for
the `rustc-dev` component).

See rust-lang#79540 (comment) and rust-lang#81930 for further context.

Here are some example runs with these changes and `download-rustc`
enabled:

```
$ x.py build src/tools/clippy
Building stage1 tool clippy-driver (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 1m 09s
Building stage1 tool cargo-clippy (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.11s
$ x.py test src/tools/clippy
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
Building stage1 tool clippy-driver (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.09s
Building rustdoc for stage1 (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.28s
    Finished release [optimized] target(s) in 15.26s
     Running build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/deps/clippy_driver-8b407b140e0aa91c
test result: ok. 592 passed; 0 failed; 3 ignored; 0 measured; 0 filtered out
$ x.py build src/tools/rustdoc
Building rustdoc for stage1 (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 41.28s
Build completed successfully in 0:00:41
$ x.py test src/test/rustdoc-ui
Building stage0 tool compiletest (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.12s
Building rustdoc for stage1 (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.10s
test result: ok. 105 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 8.15s
$ x.py build compiler/rustc
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
Build completed successfully in 0:00:00
```

Note a few things:

- Clippy depends on stage1 rustc-dev artifacts, but rustc didn't have to
  be recompiled. Instead, the artifacts were copied automatically.
- All steps are always enabled. There is no danger of forgetting a step,
  since only the entrypoints have to handle `download-rustc`.
- Building the compiler (`compiler/rustc`) automatically does no work.

Helps with rust-lang#81930.

r? `@Mark-Simulacrum`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Mar 1, 2021
…lacrum

Always compile rustdoc with debug logging enabled when `download-rustc` is set

Previously, logging at DEBUG or below would always be silenced, because
rustc compiles tracing with the `static_max_level_info` feature. That
makes sense for release artifacts, but not for developing rustdoc.

Instead, this compiles two different versions of tracing: one in the
release artifacts, distributed in the sysroot, and a new version
compiled by rustdoc. Since `rustc_driver` is always linked to the
version of sysroot, this copy/pastes `init_env_logging` into rustdoc.

To avoid compiling an unnecessary version of tracing when
`download-rustc` isn't set, this adds a new `using-ci-artifacts`
feature for rustdoc and passes that feature in bootstrap.

Addresses rust-lang#81930. This builds on rust-lang#79540.

r? `@Mark-Simulacrum`
jyn514 added a commit to jyn514/rust that referenced this pull request Mar 4, 2021
On reflection on the issue in rust-lang#79540 (comment),  I think the bug was actually using the `compiler/` filter, not using `--author=bors`. rust-lang@9a1d617 has no CI artifacts because it was merged as part of a rollup:
```
$ curl -I https://ci-artifacts.rust-lang.org/rustc-builds/96e843ce6ae42e0aa519ba45e148269de347fd84/rust-std-nightly-x86_64-unknown-linux-gnu.tar.xz
HTTP/2 404
```
So 9a1d617 is the correct commit to download, and that's what `--author=bors` does:

$ git log --author=bors 4aec8a5
commit 9a1d617

Ideally it would look for "the most recent bors commit not followed by a change to `compiler/`", which would exclude things like documentation changes and avoid redownloading more than necessary, but
- Redownloading isn't the end of the world,
- That metric is hard to implement, and
- Documentation-only or library-only changes are very rare anyway since they're usually rolled up with changes to the compiler.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 4, 2021
…acrum

Fix commit detected when using `download-rustc`

On reflection on the issue in rust-lang#79540 (comment), I think the bug was actually using the `compiler/` filter, not using `--author=bors`. rust-lang@9a1d617 has no CI artifacts because it was merged as part of a rollup:
```
$ curl -I https://ci-artifacts.rust-lang.org/rustc-builds/96e843ce6ae42e0aa519ba45e148269de347fd84/rust-std-nightly-x86_64-unknown-linux-gnu.tar.xz
HTTP/2 404
```
So 9a1d617 is the correct commit to download, and that's what `--author=bors` does:

$ git log --author=bors 4aec8a5
commit 9a1d617

Ideally it would look for "the most recent bors commit not followed by a change to `compiler/`", which would exclude things like documentation changes and avoid redownloading more than necessary, but
- Redownloading isn't the end of the world,
- That metric is hard to implement, and
- Documentation-only or library-only changes are very rare anyway since they're usually rolled up with changes to the compiler.

Helps with rust-lang#81930.

r? `@Mark-Simulacrum`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 4, 2021
…acrum

Fix commit detected when using `download-rustc`

On reflection on the issue in rust-lang#79540 (comment), I think the bug was actually using the `compiler/` filter, not using `--author=bors`. rust-lang@9a1d617 has no CI artifacts because it was merged as part of a rollup:
```
$ curl -I https://ci-artifacts.rust-lang.org/rustc-builds/96e843ce6ae42e0aa519ba45e148269de347fd84/rust-std-nightly-x86_64-unknown-linux-gnu.tar.xz
HTTP/2 404
```
So 9a1d617 is the correct commit to download, and that's what `--author=bors` does:

$ git log --author=bors 4aec8a5
commit 9a1d617

Ideally it would look for "the most recent bors commit not followed by a change to `compiler/`", which would exclude things like documentation changes and avoid redownloading more than necessary, but
- Redownloading isn't the end of the world,
- That metric is hard to implement, and
- Documentation-only or library-only changes are very rare anyway since they're usually rolled up with changes to the compiler.

Helps with rust-lang#81930.

r? `@Mark-Simulacrum`
@jyn514 jyn514 added the A-download-rustc Area: Related to the `rust.download-rustc` build option label Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-download-rustc Area: Related to the `rust.download-rustc` build option 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-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants