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

Move rustbook to its own workspace. #127786

Merged
merged 1 commit into from
Jul 22, 2024
Merged

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jul 15, 2024

This moves rustbook (the wrapper around mdbook) to its own Cargo workspace. This is done for two reasons:

  • Some users want to avoid having to check out documentation submodules if they are not working on documentation. These submodules are required for submodules that have Cargo dependencies in the tree (such as mdbook preprocessors).
  • The pinned memchr is causing problems with updating. That pin is only necessary for the standard library, but unfortunately it is affecting all other crates.

This will have some drawbacks:

  • A slight increase in the vendor directory size. My measurement shows about a 14M increase (0.7%), but somehow the compressed filesize is smaller.
  • The dependencies for rustbook now need to be managed separately. I have updated the cron job to try to mitigate this.
  • There will be a slight dist build time penalty. I'm not sure what it will be, since it heavily depends on the machine, but I suspect in the 30-45s range.
  • Adds more complexity to things like bootstrap and tidy.

There are a few other alternatives considered:

  • Publish preprocessors on crates.io. This adds the burden of publishing every change, and ensuring those publishes happen and the sources don't get out of sync, and somehow syncing those updates back to rust-lang/rust during the automatic updates. This is also more work.
  • Move the submodules to subtrees. These have the added burden of doing updates in a way that is more difficult than submodules. I believe it also causes problems with GitHub's #NNNN tagging and closing issues. This is also more work.

The only thing I haven't tested here is the cron job. However, there's a pretty decent chance this won't pass CI, or that I missed something.

@rustbot
Copy link
Collaborator

rustbot commented Jul 15, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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 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 15, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 15, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

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

@rust-log-analyzer

This comment has been minimized.

@ehuss ehuss force-pushed the rustbook-workspace branch from 57b5f1d to 590f58f Compare July 15, 2024 23:31
@bors
Copy link
Contributor

bors commented Jul 17, 2024

☔ The latest upstream changes (presumably #127831) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss ehuss force-pushed the rustbook-workspace branch from 590f58f to eff8d03 Compare July 17, 2024 01:17
@Mark-Simulacrum
Copy link
Member

r=me

@bors
Copy link
Contributor

bors commented Jul 22, 2024

☔ The latest upstream changes (presumably #128056) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss ehuss force-pushed the rustbook-workspace branch from eff8d03 to 5dfa062 Compare July 22, 2024 14:21
@ehuss
Copy link
Contributor Author

ehuss commented Jul 22, 2024

Thanks for the review. Not sure if you wanted to wait for something instead of issuing the approval directly? Regardless, I think this should be ready.

@bors r=Mark-Simulacrum rollup=never

@bors
Copy link
Contributor

bors commented Jul 22, 2024

📌 Commit 5dfa062 has been approved by Mark-Simulacrum

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 Jul 22, 2024
@ehuss
Copy link
Contributor Author

ehuss commented Jul 22, 2024

@bors p=1

This is blocking some reference changes.

@bors
Copy link
Contributor

bors commented Jul 22, 2024

⌛ Testing commit 5dfa062 with merge cefe1dc...

@bors
Copy link
Contributor

bors commented Jul 22, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 22, 2024
@bors bors merged commit cefe1dc into rust-lang:master Jul 22, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Jul 22, 2024
This was referenced Jul 22, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cefe1dc): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary -4.2%)

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-4.2% [-4.2%, -4.2%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -4.2% [-4.2%, -4.2%] 1

Cycles

Results (secondary -3.6%)

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.6% [-3.6%, -3.6%] 1
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 772.148s -> 770.458s (-0.22%)
Artifact size: 328.82 MiB -> 328.76 MiB (-0.02%)

surechen pushed a commit to surechen/rust that referenced this pull request Jul 23, 2024
…ulacrum

Move rustbook to its own workspace.

This moves rustbook (the wrapper around mdbook) to its own Cargo workspace. This is done for two reasons:

- Some users want to avoid having to check out documentation submodules if they are not working on documentation. These submodules are required for submodules that have Cargo dependencies in the tree (such as mdbook preprocessors).
- The [pinned `memchr`](https://github.com/rust-lang/rust/blob/eb72697e41b00e5d8723f14c64a969d59d9b9474/compiler/rustc_ast/Cargo.toml#L10) is causing problems with updating. That pin is only necessary for the standard library, but unfortunately it is affecting all other crates.

This will have some drawbacks:

- A slight increase in the vendor directory size. My measurement shows about a 14M increase (0.7%), but somehow the compressed filesize is smaller.
- The dependencies for rustbook now need to be managed separately. I have updated the cron job to try to mitigate this.
- There will be a slight dist build time penalty. I'm not sure what it will be, since it heavily depends on the machine, but I suspect in the 30-45s range.
- Adds more complexity to things like bootstrap and tidy.

There are a few other alternatives considered:

- Publish preprocessors on crates.io. This adds the burden of publishing every change, and ensuring those publishes happen and the sources don't get out of sync, and somehow syncing those updates back to rust-lang/rust during the automatic updates. This is also more work.
- Move the submodules to subtrees. These have the added burden of doing updates in a way that is more difficult than submodules. I believe it also causes problems with GitHub's `#NNNN` tagging and closing issues. This is also more work.

The only thing I haven't tested here is the cron job. However, there's a pretty decent chance this won't pass CI, or that I missed something.
@compiler-errors
Copy link
Member

Did this PR break tidy? I can't run tidy and this seems relevant:

thread 'deps (.)' panicked at src/tools/tidy/src/deps.rs:557:24:
cmd.exec() failed with `cargo metadata` exited with an error: error: failed to load manifest for dependency `mdbook-trpl-listing`

Caused by:
  failed to read `/home/michael/programming/rust2/src/doc/book/packages/mdbook-trpl-listing/Cargo.toml`

Caused by:
  No such file or directory (os error 2)

stack backtrace:
   0: rust_begin_unwind
             at /rustc/75ac3b6331873133c4f7a10f2252afd6f3906c6a/library/std/src/panicking.rs:652:5
   1: core::panicking::panic_fmt
             at /rustc/75ac3b6331873133c4f7a10f2252afd6f3906c6a/library/core/src/panicking.rs:72:14
   2: check
             at ./src/tools/tidy/src/lib.rs:32:23
   3: {closure#1}
             at ./src/tools/tidy/src/main.rs:87:21
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread 'main' panicked at src/tools/tidy/src/main.rs:61:49:
called `Result::unwrap()` on an `Err` value: Any { .. }

Because my src/doc/book submodule is empty... 🤔

@ehuss
Copy link
Contributor Author

ehuss commented Jul 24, 2024

Yea, I'll post a PR.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 24, 2024
…k-Simulacrum

Fix tidy check if book submodule is not checked out

This fixes tidy in a checkout without submodules. rust-lang#127786 added a new cargo workspace, and the corresponding checks in tidy. There is code in tidy to skip those checks if the submodule is checked out, but that code assumed the root of the workspace was also the root of the submodule. With `rustbook`, the root is `src/tools/rustbook`, but the submodules it needs are in the `src/doc` directory.

The solution here is to explicitly list which submodules are needed instead of assuming the root is also the submodule.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 14, 2024
…obzol

Fix dependencies cron job

This fixes a mistake in rust-lang#127786 that broke the dependencies update cron job. I accidentally set the wrong path in the cargo command.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2024
Rollup merge of rust-lang#129100 - ehuss:fix-dependencies-update, r=Kobzol

Fix dependencies cron job

This fixes a mistake in rust-lang#127786 that broke the dependencies update cron job. I accidentally set the wrong path in the cargo command.
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 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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants