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

make Step doc-comments more clear #130965

Merged
merged 1 commit into from
Oct 13, 2024

Conversation

onur-ozkan
Copy link
Member

Aiming to improve complicated Step documentation. Once we merge this, I will update this page too.

@rustbot
Copy link
Collaborator

rustbot commented Sep 28, 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 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 Sep 28, 2024
@onur-ozkan onur-ozkan force-pushed the bootstrap-step-trait-doc branch 4 times, most recently from d5bdc05 to 08d3378 Compare September 28, 2024 08:12
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thank you 💚! This already makes it much easier to understand for me. I left a few more questions because I think other people also run into those quite often.

src/bootstrap/src/core/builder.rs Show resolved Hide resolved
src/bootstrap/src/core/builder.rs Outdated Show resolved Hide resolved
src/bootstrap/src/core/builder.rs Outdated Show resolved Hide resolved
src/bootstrap/src/core/builder.rs Outdated Show resolved Hide resolved
Comment on lines +93 to +97
/// - First in "dry-run" mode to validate certain things (like cyclic Step invocations,
/// directory creation, etc) super quickly.
/// - Then it's called again to run the actual, very expensive process.
Copy link
Member

Choose a reason for hiding this comment

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

Remark: I almost wonder if this wants to be split into dry_run and run instead of always having to remember checking if dry_run { ... } lol. Implementation details shared between dry_run and run can always be shared by delegating repeated parts to other methods on the impl Step for $StepName { } impl block.

src/bootstrap/src/core/builder.rs Outdated Show resolved Hide resolved
src/bootstrap/src/core/builder.rs Outdated Show resolved Hide resolved
@onur-ozkan onur-ozkan force-pushed the bootstrap-step-trait-doc branch 3 times, most recently from 33614fd to 8d92b44 Compare September 28, 2024 10:39
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thank you, this looks very clear to me now 👍

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 12, 2024

📌 Commit 8ef0ba2 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 Oct 12, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 12, 2024
…c, r=Mark-Simulacrum

make `Step` doc-comments more clear

Aiming to improve complicated `Step` documentation. Once we merge this, I will update [this page](https://rustc-dev-guide.rust-lang.org/building/bootstrapping/how-bootstrap-does-it.html?highlight=Step#synopsis-of--step) too.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 12, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#128784 (Check ABI target compatibility for function pointers)
 - rust-lang#130965 (make `Step` doc-comments more clear)
 - rust-lang#131239 (Don't assume traits used as type are trait objs in 2021 edition)
 - rust-lang#131277 (Handle `clippy` cases of `rustc::potential_query_instability` lint)
 - rust-lang#131503 (More clearly document Stdin::read_line)
 - rust-lang#131567 (Emit an error for unstable attributes that reference already stable features)
 - rust-lang#131599 (Shallowly match opaque key in storage)
 - rust-lang#131617 (remove const_cow_is_borrowed feature gate)

Failed merges:

 - rust-lang#131616 (merge const_ipv4 / const_ipv6 feature gate into 'ip' feature gate)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ba75f24 into rust-lang:master Oct 13, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 13, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 13, 2024
Rollup merge of rust-lang#130965 - onur-ozkan:bootstrap-step-trait-doc, r=Mark-Simulacrum

make `Step` doc-comments more clear

Aiming to improve complicated `Step` documentation. Once we merge this, I will update [this page](https://rustc-dev-guide.rust-lang.org/building/bootstrapping/how-bootstrap-does-it.html?highlight=Step#synopsis-of--step) too.
@onur-ozkan onur-ozkan deleted the bootstrap-step-trait-doc branch October 13, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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