Skip to content

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

Merged
bors merged 2 commits intorust-lang:masterfrom
jyn514:no-xpy
Feb 9, 2021
Merged

Allow building rustdoc without first building rustc (MVP)#79540
bors merged 2 commits intorust-lang:masterfrom
jyn514:no-xpy

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, or significant enough to need a team FCP to proceed. A-contributor-roadblock Area: Makes things more difficult for new or seasoned contributors to Rust 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).

@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.

@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.

@jyn514 jyn514 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 25, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 force-pushed the no-xpy branch 2 times, most recently from 9906e34 to fe8bd9f Compare January 25, 2021 01:39
@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 Jan 27, 2021
@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 Jan 29, 2021
@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

Okay - I've finally had some time to review this, sorry for the long wait.

I think I am prepared to r+ this after a rebase to squash out the iterative fixes. Please also update the top level description to drop the strike outs - if you'd like to document future work, that's great, but a separate section is better.

That said, I do have some concerns, but not ones that make me want to block this:

  • I am unconvinced that the new Step constant is actually useful in practice and will work well, but it shouldn't affect people not enabling this option and we can iterate on that over time as needed. In particular, I am worried it excludes e.g. clippy or cargo work today; it seems like both of those should in theory support download-rustc as the interaction mode, but with this PR I believe wouldn't.
  • I am a bit worried that we'll eventually end up with download-X-from-CI list (e.g., one could imagine wanting to do so for std or rustdoc); that seems OK to deal with in the future in some way. Perhaps rustbuild can have two modes: local building and eager downloads, the latter of which detects what you have changed and downloads things you have not intelligently. Seems hard, but not implausible, with some coarse granularity.

I also note you've (or someone) has marked this as needing FCP, but I do not think that is the case, so curious to hear more.

@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 Feb 8, 2021
@jyn514
Copy link
Member Author

jyn514 commented Feb 9, 2021

I am unconvinced that the new Step constant is actually useful in practice and will work well, but it shouldn't affect people not enabling this option and we can iterate on that over time as needed. In particular, I am worried it excludes e.g. clippy or cargo work today; it seems like both of those should in theory support download-rustc as the interaction mode, but with this PR I believe wouldn't.

I agree I don't think Step is a great fit for clippy, miri, and rustfmt - or at least, it should be on for them, instead of off like it is now. Working on fixing that now. I'm surprised to hear you mention cargo though, AFAIK it doesn't depend on the compiler at all.

I am a bit worried that we'll eventually end up with download-X-from-CI list (e.g., one could imagine wanting to do so for std or rustdoc)

Hmm, the idea is that you choose in config.toml exactly what components you want to build from source? That seems useful if e.g. you want to build clippy from source but not rustdoc, but I don't know how it would work for std since the compiler depends on the master version of libstd.

Perhaps rustbuild can have two modes: local building and eager downloads, the latter of which detects what you have changed and downloads things you have not intelligently. Seems hard, but not implausible, with some coarse granularity.

This sounds really awesome :D

I also note you've (or someone) has marked this as needing FCP, but I do not think that is the case, so curious to hear more.

Oh well, that probably isn't necessary. I think I had in my head "this should get feedback from the rustdoc team" but since then there have been a lot of hearts (which I assume means people are in favor :P) and I'm also now the team lead, so I don't know if we need to go throguh the formality of checkboxes.

@jyn514 jyn514 closed this Feb 9, 2021
@jyn514
Copy link
Member Author

jyn514 commented Feb 9, 2021

(sorry, I hate github's UI sometimes)

Copy link
Member Author

@jyn514 jyn514 Feb 9, 2021

Choose a reason for hiding this comment

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

This already went wrong :/ I just rebased over 9a1d617, which is a bors commit, but git doesn't recognize as modifying any code:

$ git show 9a1d6174c925f54c923599e29b09d6855e6b3a78
commit 9a1d6174c925f54c923599e29b09d6855e6b3a78 (origin/master, origin/HEAD, master)
Merge: 37d067f5d71 11e7897eae7
Author: bors <bors@rust-lang.org>
Date:   Sat Feb 6 18:03:37 2021 +0000

    Auto merge of #81832 - jonas-schievink:rollup-3nw53p0, r=jonas-schievink
    
    Rollup of 7 pull requests
    
    Successful merges:
    
     - #81402 (tidy: Run tidy style against markdown files.)
     - #81434 (BTree: fix documentation of unstable public members)
     - #81680 (Refactor `PrimitiveTypeTable` for Clippy)
     - #81737 (typeck: Emit structured suggestions for tuple struct syntax)
     - #81738 (Miscellaneous small diagnostics cleanup)
     - #81766 (Enable 'task list' markdown extension)
     - #81812 (Add a test for escaping LLVMisms in inline asm)
    
    Failed merges:
    
    r? `@ghost`
    `@rustbot` modify labels: rollup
$

merge-base correctly says 9a1d617 was most recent change, but log --author=bors shows cfba499 instead, which is wrong, because compiler/ was modified since then (specifically in 7acf9ec, 85fb5cd, f631410, and 96e843c). In this case it didn't hurt anything, but if any of the APIs used by rustdoc had changed, it would have caused a build failure.

I really do think merge-base is more correct here. But I'm ok with fixing it in a follow-up if you like.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Feb 9, 2021

Choose a reason for hiding this comment

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

It should be possible to pass -m1 or something to log to treat merge commits as modification; it shouldn't be a fundamental limitation. But maybe I need to read and understand merge base. Future PR, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried -m and it didn't help. But I agree this can wait for a follow-up :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Another alternative is to run git log --author=bors without adding compiler/ to the end, and then run git log <output of previous command> compiler/ on top of that. But that doesn't seem any simpler than merge-base.

@jyn514
Copy link
Member Author

jyn514 commented Feb 9, 2021

This is ready to merge if you think it is :) I didn't change anything since the last review other than fixing conflicts and squashing the commits. If you like you can compare the changes with

$ git range-diff 65767e56537e20903b54ecde7c371cbfb1b201b0..459fb247f885b0082a04d58d3e9cfe088d26e19c 9a1d6174c925f54c923599e29b09d6855e6b3a78..f459dfa415640b309de5213773a2b6d0345fba18

- Use the same compiler for stage0 and stage1. This should be fixed at
  some point (so bootstrap isn't constantly rebuilt).
- Make sure `x.py build` and `x.py check` work.
- Use `git merge-base` to determine the most recent commit to download.
- Copy stage0 to the various sysroots in `Sysroot`, and delegate to
  Sysroot in Assemble. Leave all other code unchanged.
- Rename date -> key

  This can also be a commit hash, so 'date' is no longer a good name.

- Add the commented-out option to config.toml.example
- Disable all steps by default when `download-rustc` is enabled

  Most steps don't make sense when downloading a compiler, because they'll
  be pre-built in the sysroot. Only enable the ones that might be useful,
  in particular Rustdoc and all `check` steps.

  At some point, this should probably enable other tools, but rustdoc is
  enough to test out `download-rustc`.

- Don't print 'Skipping' twice in a row

  Bootstrap forcibly enables a dry run if it isn't already set, so
  previously it would print the message twice:

  ```
  Skipping bootstrap::compile::Std because it is not enabled for `download-rustc`
  Skipping bootstrap::compile::Std because it is not enabled for `download-rustc`
  ```

  Now it correctly only prints once.

 ## Future work

- Add FIXME about supporting beta commits
- Debug logging will never work. This should be fixed.
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

I think this is good to go. I'm not sure if the ergonomics are quite right but we'll find out through experimentation :)

@bors
Copy link
Collaborator

bors commented Feb 9, 2021

📌 Commit 4aec8a5 has been approved by Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Feb 9, 2021

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

@bors
Copy link
Collaborator

bors commented Feb 9, 2021

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

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 or seasoned contributors to Rust A-download-rustc Area: 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