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

Support excluding the generation of the standalone docs #102706

Merged
merged 4 commits into from
Oct 26, 2022

Conversation

pietroalbini
Copy link
Member

@pietroalbini pietroalbini commented Oct 5, 2022

For Ferrocene we need to exclude the generation of the standalone docs (which include the index page, which we want to replace with our own), but with the way bootstrap is currently implemented that proved not possible. This PR aims to support that.

The first problem is that the doc::Standalone step did two things: it generated the "standalone" documentation (which includes the index page and all the pages at the root of the documentation tree), but it also generated some files like rust.css and version_info.html that other step like doc::TheBook required. This meant generating the book required generating the index page, which made disabling the index page generation problematic.

The approach I took to fix the first problem is to split the step into doc::Standalone and doc::SharedAssets, with doc::TheBook now depending on doc::SharedAssets.

The second problem is that disabling the doc::Standalone proved to be tricky due to its path, src/doc. The path is accurate, as the source files for that step are src/doc/*.md. The problem is, bootstrap treats --exclude as a suffix, and so it also excluded the Cargo book whose source lives at src/tools/cargo/src/doc.

The approach I took to fix the second problem is to add the standalone path in addition to src/doc, so that you can pass --exclude standalone. I'm not fully happy with the solution, and the other idea I had was to just move the standalone docs source code to src/doc/standalone. I feel that second approach is cleaner, but also requires more changes and might require more consensus.

This PR is best reviewed commit-by-commit.

r? @jyn514

Before this commit, the step to generate the standalone docs (which
included the index page and other HTML files at the root of the
documentation) was bundled with the code copying files needed by
multiple pieces of documentation. This means it wasn't possible to avoid
generating the standalone docs.

This commit splits the step into two, allowing the standalone docs
generation to be excluded while still building the rest of the docs.
Before this commit, the path for the doc::Standalone step was "src/doc",
which is accurate as the standalone docs source files live at the root
of the "src/doc" directory tree.

Unfortunately, that caused bad interactions when trying to exclude it
with `--exclude src/doc`. When an exclusion is passed to bootstrap, it
will exclude all steps whose path *ends with* the exclusion, which
results in the Cargo book (src/tools/cargo/src/doc) to also be excluded.

To work around this problem, this commit adds the "standalone" path as
an alternate path for doc::Standalone, allowing `--exclude standalone`
to work without side effects.
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Oct 5, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2022
@joshtriplett
Copy link
Member

Could we teach bootstrap to support anchoring paths, like .gitignore does?

@pietroalbini
Copy link
Member Author

I guess? That sounds like a way larger change than this (as we'd need to change how bootstrap handles paths), and it would retain the weird behavior if someone doesn't know how to add anchoring paths.

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! r=me with the nits fixed.

@joshtriplett I think we should have a holistic conversation about what excludes should look like. The current syntax is currently quite baroque and AFAIK Pietro's CI scripts are the only ones using most of the features. Slapping git-style ignores on top seems like it will make things more confusing, not less.

That said, I agree the behavior is confusing and there's room for improvement; I'd love to get rid of the doc:: prefixes somehow and make the various paths unambiguous.

src/bootstrap/doc.rs Outdated Show resolved Hide resolved
Co-authored-by: Joshua Nelson <[email protected]>
@pietroalbini
Copy link
Member Author

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Oct 18, 2022

📌 Commit f134370 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 Oct 18, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 20, 2022
…yn514

Support excluding the generation of the standalone docs

For Ferrocene we need to exclude the generation of the standalone docs (which include the index page, which we want to replace with our own), but with the way bootstrap is currently implemented that proved not possible. This PR aims to support that.

The first problem is that the `doc::Standalone` step did two things: it generated the "standalone" documentation (which includes the index page and all the pages at the root of the documentation tree), but it also generated some files like `rust.css` and `version_info.html` that other step like `doc::TheBook` required. This meant generating the book required generating the index page, which made disabling the index page generation problematic.

The approach I took to fix the first problem is to split the step into `doc::Standalone` and `doc::SharedAssets`, with `doc::TheBook` now depending on `doc::SharedAssets`.

The second problem is that disabling the `doc::Standalone` proved to be tricky due to its path, `src/doc`. The path is accurate, as the source files for that step are `src/doc/*.md`. The problem is, bootstrap treats `--exclude` as a *suffix*, and so it also excluded the Cargo book whose source lives at `src/tools/cargo/src/doc`.

The approach I took to fix the second problem is to add the `standalone` path in addition to `src/doc`, so that you can pass `--exclude standalone`. I'm not fully happy with the solution, and the other idea I had was to just move the standalone docs source code to `src/doc/standalone`. I feel that second approach is cleaner, but also requires more changes and might require more consensus.

This PR is best reviewed commit-by-commit.

r? `@jyn514`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 20, 2022
…yn514

Support excluding the generation of the standalone docs

For Ferrocene we need to exclude the generation of the standalone docs (which include the index page, which we want to replace with our own), but with the way bootstrap is currently implemented that proved not possible. This PR aims to support that.

The first problem is that the `doc::Standalone` step did two things: it generated the "standalone" documentation (which includes the index page and all the pages at the root of the documentation tree), but it also generated some files like `rust.css` and `version_info.html` that other step like `doc::TheBook` required. This meant generating the book required generating the index page, which made disabling the index page generation problematic.

The approach I took to fix the first problem is to split the step into `doc::Standalone` and `doc::SharedAssets`, with `doc::TheBook` now depending on `doc::SharedAssets`.

The second problem is that disabling the `doc::Standalone` proved to be tricky due to its path, `src/doc`. The path is accurate, as the source files for that step are `src/doc/*.md`. The problem is, bootstrap treats `--exclude` as a *suffix*, and so it also excluded the Cargo book whose source lives at `src/tools/cargo/src/doc`.

The approach I took to fix the second problem is to add the `standalone` path in addition to `src/doc`, so that you can pass `--exclude standalone`. I'm not fully happy with the solution, and the other idea I had was to just move the standalone docs source code to `src/doc/standalone`. I feel that second approach is cleaner, but also requires more changes and might require more consensus.

This PR is best reviewed commit-by-commit.

r? ``@jyn514``
@matthiaskrgr
Copy link
Member

@bors r- failed in a rollup it seems
#103314 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 20, 2022
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Oct 24, 2022
@pietroalbini
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Oct 24, 2022

⌛ Trying commit 039a0f008cb5b4be922fafc74162ee65a2bd8b26 with merge 8f7d9c41d4f4a79d510dd11f125d677cd03291c4...

@pietroalbini
Copy link
Member Author

Ok the last commit fixed the dist-i686-msvc (tested through the bors try).

@rustbot review

@rustbot rustbot 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 Oct 24, 2022
@jyn514
Copy link
Member

jyn514 commented Oct 24, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Oct 24, 2022

📌 Commit 91c09d4 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 Oct 24, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 26, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#95710 (Stabilize arbitrary_enum_discriminant, take 2)
 - rust-lang#102706 (Support excluding the generation of the standalone docs)
 - rust-lang#103428 (Removed verbose printing from the `PrettyPrinter` when printing constants)
 - rust-lang#103543 (Update books)
 - rust-lang#103546 (interpret: a bit of cast cleanup)
 - rust-lang#103554 (rustdoc: add visible focus outline to rustdoc-toggle)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b03fa1a into rust-lang:master Oct 26, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 26, 2022
@pietroalbini pietroalbini deleted the pa-ignore-doc-index branch October 26, 2022 21:10
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) 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.

7 participants