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

Update the reference #128215

Merged
merged 17 commits into from
Jul 28, 2024
Merged

Update the reference #128215

merged 17 commits into from
Jul 28, 2024

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jul 26, 2024

This updates the reference to use the new mdbook-spec preprocessor, which is a Cargo library inside the reference submodule.

Note that this PR contains a bunch of bootstrap cleanup commits to assist with making sure the submodules are working correctly. All of the cleanup PRs should have a description in their commit. I'd be happy to move those to a separate PR if that makes review easier.

The main changes for the reference are:

  • Move the doc::Reference bootstrap step out of the generic macro into a custom step.
    • This step needs to build rustdoc because the new mdbook-spec plugin uses rustdoc for generating links.
    • PATH is updated so that the rustdoc binary can be found.
  • rustbook now includes the mdbook-spec plugin as a dependency.
  • rustbook enables the mdbook-spec preprocessor.

I did a bunch of testing with the various commands and setups, such as:

  • submodules=true and submodules=false
  • having all submodules deinitialized
  • not in a git repository

However, there are probably thousands of different permutations of different commands, settings, and environments, so there is a chance I'm missing something.

ehuss added 10 commits July 25, 2024 15:39
bootstrap.py handling of submodules was removed in
rust-lang#97513.
This is not used anywhere outside this module.
This felt like an important point to me.
The argument was not necessary, since it was only ever passed one
value that exists in the config itself.
Although its origins were in bootstrap.py, that code in bootstrap.py
no longer exists since it was removed.
I put this submodule update in the entirely wrong location. I put it in
the `RustcBook` step (for generating src/doc/rustc), when it really
should exist for all steps that use the `Rustbook` tool.
This adds a new method `require_and_update_submodule` to replace
`update_submodule`. This new method will generate an error if the
submodule doesn't actually exist. This replaces some ad-hoc checks that
were performing this function. This helps ensure that a good error
message is always displayed.

This also adds require_and_update_all_submodules which does this for
all submodules.

Ideally this should not have any change other than better error messages
when submodules are missing.
If the submodule is not checked out, then these tests would fail.
This updates the reference which is now using a new mdbook plugin. This
requires a little extra work than a normal book because the plugin uses
`rustdoc` to generate links to the standard library. It also ensures
that the submodule is available for *any* command that uses rustbook,
since it is now part of the rustbook workspace.
@rustbot
Copy link
Collaborator

rustbot commented Jul 26, 2024

r? @Kobzol

rustbot has assigned @Kobzol.
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
Copy link
Collaborator

rustbot commented Jul 26, 2024

⚠️ Warning ⚠️

  • These commits modify submodules.

@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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 26, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 26, 2024

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

Copy link

@JoelMarcey JoelMarcey left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Thanks! I went through the commits and left a few comments.

I tried to run x doc reference locally, and I got this:

Finished `release` profile [optimized] target(s) in 31.00s
Rustbook (x86_64-unknown-linux-gnu) - reference
error: manifest path `mdbook-spec/Cargo.toml` does not exist

when trying to run x doc reference for the first time, not sure what to make of it. Additional executions ended successfully (or at least it seemed like so).

One thing I also noticed is that the reference does not open automatically in the browser if I do x doc reference or x doc src/doc/reference, I have to use --open (not sure if this is correct or not, from the code it looked like it might be intended to be opened by default). But this also happens on master, so nothing to do with this PR.

src/bootstrap/src/core/build_steps/compile.rs Outdated Show resolved Hide resolved
src/bootstrap/src/core/build_steps/llvm.rs Outdated Show resolved Hide resolved
src/bootstrap/src/lib.rs Outdated Show resolved Hide resolved
src/bootstrap/src/lib.rs Outdated Show resolved Hide resolved
src/bootstrap/src/lib.rs Outdated Show resolved Hide resolved
src/bootstrap/src/lib.rs Show resolved Hide resolved
src/bootstrap/src/core/build_steps/doc.rs Outdated Show resolved Hide resolved
This was a copy/paste mistake.
This makes it easier to call these functions without needing to form a
Path.
…ired

These are required 100% of the time, but they are almost always required
for any command that runs Cargo in the main workspace.

Ideally, initializing these two standard library submodules would be
lazy and only initialized when required (see
rust-lang#82653). However, it would require
updating these in almost every Step (anything that runs `cargo` in the
main workspace).
I misread this one. It is only checking if LLVM needs to be rebuilt.
There is code below that handles the case where it is unable to compute
the stamp if the source is missing.
Just trying to be a little less verbose here.
@ehuss
Copy link
Contributor Author

ehuss commented Jul 27, 2024

when trying to run x doc reference for the first time, not sure what to make of it. Additional executions ended successfully (or at least it seemed like so).

That's an unfortunate consequence of how the mdbook preprocessors are overridden. The error is ignored, and it builds successfully. I don't have a simple way to silence it right now. That's what this comment is referring to about being unable to suppress the error message. I will try to get mdbook updated in a future PR soon so that it replaces the existing preprocessor to avoid that.

One thing I also noticed is that the reference does not open automatically in the browser if I do x doc reference or x doc src/doc/reference, I have to use --open (not sure if this is correct or not, from the code it looked like it might be intended to be opened by default). But this also happens on master, so nothing to do with this PR.

That's normal. It is required to use --open to open it automatically.


BTW, while re-reviewing, I noticed a mistake in the prebuilt_llvm_config function. It should not require the submodule. There is code below when computing the stamp to handle the case when the submodule source is missing.

@Kobzol
Copy link
Contributor

Kobzol commented Jul 28, 2024

That's normal. It is required to use --open to open it automatically.

Sorry, I did not notice this condition.

Thank you!

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Jul 28, 2024

📌 Commit 1c98b8f has been approved by Kobzol

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 28, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 28, 2024
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#125889 (Add migration lint for 2024 prelude additions)
 - rust-lang#128215 (Update the reference)
 - rust-lang#128263 (rustdoc: use strategic ThinVec/Box to shrink `clean::ItemKind`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 20cf5ad into rust-lang:master Jul 28, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Jul 28, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 28, 2024
Rollup merge of rust-lang#128215 - ehuss:update-reference, r=Kobzol

Update the reference

This updates the reference to use the new mdbook-spec preprocessor, which is a Cargo library inside the reference submodule.

Note that this PR contains a bunch of bootstrap cleanup commits to assist with making sure the submodules are working correctly. All of the cleanup PRs should have a description in their commit. I'd be happy to move those to a separate PR if that makes review easier.

The main changes for the reference are:
- Move the `doc::Reference` bootstrap step out of the generic macro into a custom step.
    - This step needs to build rustdoc because the new mdbook-spec plugin uses rustdoc for generating links.
    - PATH is updated so that the rustdoc binary can be found.
- rustbook now includes the mdbook-spec plugin as a dependency.
- rustbook enables the mdbook-spec preprocessor.

I did a bunch of testing with the various commands and setups, such as:
- `submodules=true` and `submodules=false`
- having all submodules deinitialized
- not in a git repository

However, there are probably thousands of different permutations of different commands, settings, and environments, so there is a chance I'm missing something.
@bors
Copy link
Contributor

bors commented Jul 28, 2024

⌛ Testing commit 1c98b8f with merge 78c8573...

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-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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants