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

bootstrap: Various Step refactors #111955

Merged
merged 8 commits into from
May 30, 2023
Merged

bootstrap: Various Step refactors #111955

merged 8 commits into from
May 30, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented May 25, 2023

This ended up being a bunch of only somewhat related changes, sorry 😓 on the bright side, they're all fairly small and well-commented in the commit messages.

  • Document ShouldRun::crates

  • Switch Steps from crates to crate_or_deps where possible

    and document why the single remaining place can't switch

  • Switch doc::{Std, Rustc} to crate_or_deps

    Previously they were using all_krates and various hacks to determine
    which crates to document. Switch them to crate_or_deps so ShouldRun
    tells them which crate to document instead of having to guess.

    This also makes a few other refactors:

    • Remove the now unused all_krates; new code should only use
      crate_or_deps.
    • Add tests for documenting Std
    • Remove the unnecessary run_cargo_rustdoc_for closure so that we only
      run cargo once
    • Give a more helpful error message when documenting a no_std target
    • Use builder.msg in the Steps instead of builder.info
  • Extend msg and description to work with any subcommand

    Previously description only supported Testing and Benchmarking,
    and msg gave weird results for doc (it would say Docing).

  • Add a make_run_crates function and use it Rustc and Std

    This fixes the panic from the previous commit.

  • Allow checking individual crates

    This is useful for profiling metadata generation.

    This comes very close to removing all_krates, but doesn't quite -
    there's one last usage left in doc. This is fixed in a later commit.

  • Give a more helpful error when calling cargo_crates_in_set for an alias

    Before:

    thread 'main' panicked at 'no entry found for key', builder.rs:110:30
    

    After:

    thread 'main' panicked at 'missing crate for path library', check.rs:89:26
    

@jyn514 jyn514 added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label May 25, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 25, 2023

r? @clubby789

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added 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 May 25, 2023
Copy link
Contributor

@clubby789 clubby789 left a comment

Choose a reason for hiding this comment

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

Not super familiar with the code here but looks broadly good and (aside from a couple of nitpicks) a lot cleaner

src/bootstrap/builder.rs Outdated Show resolved Hide resolved
src/bootstrap/builder.rs Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@clubby789
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 25, 2023

📌 Commit e72ad594bfd2965bd8cc9774a64d5ba4fa23e53d has been approved by clubby789

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 May 25, 2023
@jyn514
Copy link
Member Author

jyn514 commented May 25, 2023

Just for later reference - Mark and I discussed about a year ago that this is the eventual direction we want to move bootstrap towards: #95503 (comment), https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/bootstrap.20.2B.20building.20individual.20crates.20.2395503/near/279768255

This keeps the design mentioned there, where ensure(doc::Std) and ensure(check::Std) always checks all crates, and only an explicit x check core is allowed to build a subset of crates. It actually goes a little further in that direction since x test core linkchecker would previously have only documented core, which seems wrong.

I think I've been the reviewer for all PRs involving builder.msg, but I do feel pretty strongly that we should switch to msg wherever possible, I'll open a tracking issue for that in a second.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 26, 2023
Generate docs for bootstrap itself

This verifies the intra-doc links are correct, and hopefully makes things easier for new contributors.

Note that this will conflict with rust-lang#111955, i'm pretty sure i typo-ed some of the intra-doc links lol
compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 26, 2023
Generate docs for bootstrap itself

This verifies the intra-doc links are correct, and hopefully makes things easier for new contributors.

Note that this will conflict with rust-lang#111955, i'm pretty sure i typo-ed some of the intra-doc links lol
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 26, 2023
Generate docs for bootstrap itself

This verifies the intra-doc links are correct, and hopefully makes things easier for new contributors.

Note that this will conflict with rust-lang#111955, i'm pretty sure i typo-ed some of the intra-doc links lol
@bors
Copy link
Contributor

bors commented May 28, 2023

⌛ Testing commit e72ad594bfd2965bd8cc9774a64d5ba4fa23e53d with merge 3bd00e870384ff8b0547fbea6c0c38b8ebc6d850...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 28, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 28, 2023
jyn514 added 7 commits May 29, 2023 13:24
…lias

Before:
```
thread 'main' panicked at 'no entry found for key', builder.rs:110:30
```

After:
```
thread 'main' panicked at 'missing crate for path library', check.rs:89:26
```
This is useful for profiling metadata generation.

This comes very close to removing all_krates, but doesn't quite -
there's one last usage left in `doc`.
This fixes the panic from the previous commit.
Previously `description` only supported `Testing` and `Benchmarking`,
and `msg` gave weird results for `doc` (it would say `Docing`).
Previously they were using `all_krates` and various hacks to determine
which crates to document. Switch them to `crate_or_deps` so `ShouldRun`
tells them which crate to document instead of having to guess.

This also makes a few other refactors:
- Remove the now unused `all_krates`; new code should only use
  `crate_or_deps`.
- Add tests for documenting Std
- Remove the unnecessary `run_cargo_rustdoc_for` closure so that we only
  run cargo once
- Give a more helpful error message when documenting a no_std target
- Use `builder.msg` in the Steps instead of `builder.info`
and document why the single remaining place can't switch
- Switch from `cargo rustdoc` to `cargo doc`

  This allows passing `-p` to multiple packages.

- Remove `OsStr` support

  It doesn't work with RUSTDOCFLAGS, and we don't support non-utf8 paths
  anyway.

- Pass `-p std` for each crate in the standard library

  By default cargo only documents the top-level crate, which is
  `sysroot` and has no docs.
@jyn514
Copy link
Member Author

jyn514 commented May 29, 2023

@bors r=clubby789

@bors
Copy link
Contributor

bors commented May 29, 2023

📌 Commit c28ee60 has been approved by clubby789

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 May 29, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 30, 2023
Rollup of 7 pull requests

Successful merges:

 - rust-lang#107916 (fix comment on Allocator trait)
 - rust-lang#111543 (Uplift `clippy::invalid_utf8_in_unchecked` lint)
 - rust-lang#111872 (fix: dedup `static_candidates` before report)
 - rust-lang#111955 (bootstrap: Various Step refactors)
 - rust-lang#112060 (`EarlyBinder::new` -> `EarlyBinder::bind`)
 - rust-lang#112064 (Migrate GUI colors test to original CSS color format)
 - rust-lang#112100 (Don't typecheck recovered method call from suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 65833ed into rust-lang:master May 30, 2023
@rustbot rustbot added this to the 1.72.0 milestone May 30, 2023
@jyn514 jyn514 deleted the step-refactors branch May 30, 2023 16:13
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 13, 2023
…-crates, r=albertlarsan68

bootstrap: update defaults for `compiler` and `library` aliases

* `x doc compiler` now documents all of compiler, not just `rustc_driver`.
* `x doc` with compiler docs enabled now includes `rustc-main` and `rustc_smir`. `rustc_codegen_llvm` is only included if the LLVM backend is enabled, which is the default.
* `x doc library` now excludes `sysroot`.
* `x check compiler` and `x check library` now properly check tests/benches/examples of all compiler or library crates, respectively. Note that `x check compiler` will check the library artifacts, but not tests.

fixes the fallout from rust-lang#111955, cc `@jyn514`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 13, 2023
…-crates, r=albertlarsan68

bootstrap: update defaults for `compiler` and `library` aliases

* `x doc compiler` now documents all of compiler, not just `rustc_driver`.
* `x doc` with compiler docs enabled now includes `rustc-main` and `rustc_smir`. `rustc_codegen_llvm` is only included if the LLVM backend is enabled, which is the default.
* `x doc library` now excludes `sysroot`.
* `x check compiler` and `x check library` now properly check tests/benches/examples of all compiler or library crates, respectively. Note that `x check compiler` will check the library artifacts, but not tests.

fixes the fallout from rust-lang#111955, cc ``@jyn514``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 13, 2023
…-crates, r=albertlarsan68

bootstrap: update defaults for `compiler` and `library` aliases

* `x doc compiler` now documents all of compiler, not just `rustc_driver`.
* `x doc` with compiler docs enabled now includes `rustc-main` and `rustc_smir`. `rustc_codegen_llvm` is only included if the LLVM backend is enabled, which is the default.
* `x doc library` now excludes `sysroot`.
* `x check compiler` and `x check library` now properly check tests/benches/examples of all compiler or library crates, respectively. Note that `x check compiler` will check the library artifacts, but not tests.

fixes the fallout from rust-lang#111955, cc ```@jyn514```
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2023
…rates, r=albertlarsan68

bootstrap: update defaults for `compiler` and `library` aliases

* `x doc compiler` now documents all of compiler, not just `rustc_driver`.
* `x doc` with compiler docs enabled now includes `rustc-main` and `rustc_smir`. `rustc_codegen_llvm` is only included if the LLVM backend is enabled, which is the default.
* `x doc library` now excludes `sysroot`.
* `x check compiler` and `x check library` now properly check tests/benches/examples of all compiler or library crates, respectively. Note that `x check compiler` will check the library artifacts, but not tests.

fixes the fallout from rust-lang#111955, cc `@jyn514`
Comment on lines +479 to +480
/// multiple times, whereas a single call to `paths` will only ever generate a single call to
/// `paths`.
Copy link
Member

Choose a reason for hiding this comment

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

"a single call to paths will only ever generate a single call to paths" eh... what does that mean?^^

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, this is a blast from the past. probably i meant "a single call to make_run", by analogy with the docs above? i honestly don't remember though, it's been ages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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.

6 participants