bootstrap: extract builder cargo to its own module #131728

jieyouxu · 2024-10-15T06:19:46Z

I was looking at our cargo rustflags/rustdocflags usages, and I found builder.rs to be a large
file which made it hard to digest. This PR tries to break out the cargo command wrapper parts to
its own submodule to make it easier to identify builder cargo-specific logic.

This PR:

Extracts the cargo command wrapper to its own module and also move Builder::{bare_,}cargo impl
to the submodule.
Reorganizes some imports in lib.rs (no functional changes).
Slightly adjusts some docs in builder.rs.

This PR is basically just moving code around, and should not contain any functional changes.

Before this PR, builder.rs was 2743 lines. After this PR, builder.rs is down to a more
manageable 1386 lines and cargo.rs is 1085 lines.

rustbot · 2024-10-15T06:19:53Z

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

jieyouxu · 2024-10-15T06:32:57Z

src/bootstrap/src/core/builder/cargo.rs

+/// `rustc_parse::parser::Parser`. See `rustc_builtin_macros::cmdline_attrs::inject` for more
+/// information.
+#[derive(Debug, Clone)]
+struct Rustflags(String, TargetSelection);


Rustflags and HostFlags apparently are only used for builder-cargo-related purposes, so I could make it default visibility here.

src/bootstrap/src/core/builder.rs

bors · 2024-10-16T18:31:56Z

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

jieyouxu · 2024-10-17T03:48:07Z

Rebased to fix merge conflicts, no functional changes.

src/bootstrap/src/lib.rs

Mark-Simulacrum · 2024-10-20T13:59:15Z

r? @onur-ozkan

I don't have a strong opinion on this sort of refactoring (file length is not really a strong indicator IMO, and in some ways I prefer fewer files over more). But leaving it to others to do fine-grained review.

onur-ozkan

LGTM, waiting for rebase

bors · 2024-10-20T19:54:37Z

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

I found builder.rs to be a massive file which made it hard to digest. To make `RUSTFLAGS` usage hardening easier later, I extracted the cargo part in `builder.rs` into its own module.

jieyouxu · 2024-10-21T02:36:34Z

I don't have a strong opinion on this sort of refactoring (file length is not really a strong indicator IMO, and in some ways I prefer fewer files over more). But leaving it to others to do fine-grained review.

This was specifically because I was trying to figure out and localize the cargo-specific parts of Builder because it's been a source of spurious rebuilds (in particular, I found out that the flags components are not used by other parts of Builder directly). I wouldn't do it just for file length purposes alone.

jieyouxu · 2024-10-21T02:37:42Z

LGTM, waiting for rebase

Rebased, no functional changes.

onur-ozkan · 2024-10-21T04:18:11Z

@bors r+

bors · 2024-10-21T04:18:14Z

📌 Commit 2b4bf2d has been approved by onur-ozkan

It is now in the queue for this repository.

…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#126588 (Added more scenarios where comma to be removed in the function arg) - rust-lang#131728 (bootstrap: extract builder cargo to its own module) - rust-lang#131968 (Rip out old effects var handling code from traits) - rust-lang#131981 (Remove the `BoundConstness::NotConst` variant) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#131728 - jieyouxu:boopstrap, r=onur-ozkan bootstrap: extract builder cargo to its own module I was looking at our cargo rustflags/rustdocflags usages, and I found `builder.rs` to be a large file which made it hard to digest. This PR tries to break out the cargo command wrapper parts to its own submodule to make it easier to identify builder cargo-specific logic. This PR: - Extracts the cargo command wrapper to its own module and also move `Builder::{bare_,}cargo` impl to the submodule. - Reorganizes some imports in `lib.rs` (no functional changes). - Slightly adjusts some docs in `builder.rs`. This PR is basically just moving code around, and should not contain any functional changes. Before this PR, `builder.rs` was 2743 lines. After this PR, `builder.rs` is down to a more manageable 1386 lines and `cargo.rs` is 1085 lines.

…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#126588 (Added more scenarios where comma to be removed in the function arg) - rust-lang#131728 (bootstrap: extract builder cargo to its own module) - rust-lang#131968 (Rip out old effects var handling code from traits) - rust-lang#131981 (Remove the `BoundConstness::NotConst` variant) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned Mark-Simulacrum Oct 15, 2024

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 Oct 15, 2024

jieyouxu force-pushed the boopstrap branch from 750936d to 3e6cbd6 Compare October 15, 2024 06:30

jieyouxu commented Oct 15, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

jieyouxu force-pushed the boopstrap branch 2 times, most recently from 63648ac to d2ad6e6 Compare October 15, 2024 07:37

onur-ozkan reviewed Oct 15, 2024

View reviewed changes

src/bootstrap/src/core/builder.rs Outdated Show resolved Hide resolved

jieyouxu force-pushed the boopstrap branch from c30d17f to 2bf27ab Compare October 17, 2024 03:04

onur-ozkan reviewed Oct 17, 2024

View reviewed changes

src/bootstrap/src/lib.rs Outdated Show resolved Hide resolved

jieyouxu force-pushed the boopstrap branch from 2bf27ab to da9b3fb Compare October 17, 2024 04:33

rustbot assigned onur-ozkan and unassigned Mark-Simulacrum Oct 20, 2024

onur-ozkan approved these changes Oct 20, 2024

View reviewed changes

jieyouxu added 3 commits October 21, 2024 10:34

bootstrap: extract builder cargo to its own module

4b28e52

I found builder.rs to be a massive file which made it hard to digest. To make `RUSTFLAGS` usage hardening easier later, I extracted the cargo part in `builder.rs` into its own module.

bootstrap: minor docs cleanup

1d77715

bootstrap: move builder.rs under builder/ directory

2b4bf2d

jieyouxu force-pushed the boopstrap branch from da9b3fb to 2b4bf2d Compare October 21, 2024 02:35

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 21, 2024

matthiaskrgr mentioned this pull request Oct 21, 2024

Rollup of 4 pull requests #131988

Merged

bors merged commit 09a22b8 into rust-lang:master Oct 21, 2024
6 checks passed

rustbot added this to the 1.84.0 milestone Oct 21, 2024

jieyouxu deleted the boopstrap branch October 21, 2024 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootstrap: extract builder cargo to its own module #131728

bootstrap: extract builder cargo to its own module #131728

jieyouxu commented Oct 15, 2024 •

edited

Loading

rustbot commented Oct 15, 2024

jieyouxu Oct 15, 2024 •

edited

Loading

This comment has been minimized.

bors commented Oct 16, 2024

jieyouxu commented Oct 17, 2024 •

edited

Loading

Mark-Simulacrum commented Oct 20, 2024

onur-ozkan left a comment

bors commented Oct 20, 2024

jieyouxu commented Oct 21, 2024 •

edited

Loading

jieyouxu commented Oct 21, 2024

onur-ozkan commented Oct 21, 2024

bors commented Oct 21, 2024

bootstrap: extract builder cargo to its own module #131728

bootstrap: extract builder cargo to its own module #131728

Conversation

jieyouxu commented Oct 15, 2024 • edited Loading

rustbot commented Oct 15, 2024

jieyouxu Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

bors commented Oct 16, 2024

jieyouxu commented Oct 17, 2024 • edited Loading

Mark-Simulacrum commented Oct 20, 2024

onur-ozkan left a comment

Choose a reason for hiding this comment

bors commented Oct 20, 2024

jieyouxu commented Oct 21, 2024 • edited Loading

jieyouxu commented Oct 21, 2024

onur-ozkan commented Oct 21, 2024

bors commented Oct 21, 2024

jieyouxu commented Oct 15, 2024 •

edited

Loading

jieyouxu Oct 15, 2024 •

edited

Loading

jieyouxu commented Oct 17, 2024 •

edited

Loading

jieyouxu commented Oct 21, 2024 •

edited

Loading