Skip to content

Conversation

Shourya742
Copy link
Contributor

@Shourya742 Shourya742 commented Aug 13, 2025

This PR removes the default config initialization from parse_inner, as it introduced many assumptions during config setup. Instead, each variable is now manually initialized to eliminate certain invariants in parse_inner and streamline the process.

r? @Kobzol

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 13, 2025
@rust-log-analyzer

This comment has been minimized.

@Shourya742 Shourya742 force-pushed the 2025-08-12-remove-default-config branch from e1f4e5c to 9662ff8 Compare August 13, 2025 13:04
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Heh, I had feedback for you, but I see that Clippy did it for me in one of the commits xD Let me know once it's ready and I'll check the changes locally, but in general looks great.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Shourya742 Shourya742 force-pushed the 2025-08-12-remove-default-config branch from a53f060 to d22b2a4 Compare August 15, 2025 02:34
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Aug 15, 2025

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

@Shourya742 Shourya742 force-pushed the 2025-08-12-remove-default-config branch from fdcd154 to 77c3d6e Compare August 18, 2025 04:16
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Let me know if it's ready for another round of review :)

Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Great work! Left a bunch of comments.

In general, I'm not happy about DownloadContext. Even with the macro I proposed, it's still too easy to use values before they are initialized, for example rust_info is used in download context before it is initialized :( And that's just one thing I noticed, possibly there may be more.

Before we can remove more I/O from parse_inner, I would suggest creating more granular versions of DownloadContext. For example, download_beta_toolchain requires exec_ctx, stage0_metadata, host_target, out, patch_binaries_for_nix, bootstrap_cache_path, llvm_assertions and is_running_on_ci, so approx. 50% of DownloadContext. If we created a separate ctx for this function, we could reduce the likelihood of similar errors.

};

Config {
change_id: toml.change_id.inner,
Copy link
Member

Choose a reason for hiding this comment

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

Could you please sort the fields in this constructor, and apply // tidy-alphabetical-start + // tidy-alphabetical-end to make sure that they stay sorted? Currently some relates values are far from one another.

@rust-log-analyzer

This comment has been minimized.

@Shourya742 Shourya742 force-pushed the 2025-08-12-remove-default-config branch from 83d5b07 to e261e25 Compare August 21, 2025 02:37
@Shourya742 Shourya742 marked this pull request as ready for review August 22, 2025 02:40
@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 Aug 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2025

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@Kobzol
Copy link
Member

Kobzol commented Aug 22, 2025

Thank you! I really like how it looks like, good work on trimming down the download context.

While reviewing, I added the tidy markers and sorted the Config fields and did a few small changes. I'll approve the PR if CI is green.

@Kobzol
Copy link
Member

Kobzol commented Aug 22, 2025

@bors r+ rollup=iffy

@bors
Copy link
Collaborator

bors commented Aug 22, 2025

📌 Commit c058ce5 has been approved by Kobzol

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 Aug 22, 2025
@bors
Copy link
Collaborator

bors commented Aug 22, 2025

⌛ Testing commit c058ce5 with merge 46c219b...

@bors
Copy link
Collaborator

bors commented Aug 22, 2025

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing 46c219b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 22, 2025
@bors bors merged commit 46c219b into rust-lang:master Aug 22, 2025
11 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Aug 22, 2025
Copy link
Contributor

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing d20509c (parent) -> 46c219b (this PR)

Test differences

Show 4 test diffs

4 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 46c219bd24862c0a87f0299570bb37f2d5ecf6ce --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. dist-x86_64-apple: 8069.0s -> 6391.6s (-20.8%)
  2. dist-apple-various: 4958.8s -> 4373.0s (-11.8%)
  3. dist-various-1: 4257.2s -> 3807.2s (-10.6%)
  4. aarch64-gnu-llvm-19-2: 2190.1s -> 2379.5s (8.6%)
  5. aarch64-gnu-llvm-19-1: 3301.5s -> 3566.2s (8.0%)
  6. arm-android: 5821.6s -> 6224.2s (6.9%)
  7. pr-check-2: 2140.6s -> 2276.4s (6.3%)
  8. dist-x86_64-msvc: 6868.8s -> 6448.8s (-6.1%)
  9. x86_64-mingw-2: 7555.7s -> 7947.8s (5.2%)
  10. dist-x86_64-msvc-alt: 9723.1s -> 9222.7s (-5.1%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (46c219b): 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 (secondary 3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Cycles

Results (secondary 3.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

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

Bootstrap: 468.202s -> 465.73s (-0.53%)
Artifact size: 378.26 MiB -> 378.26 MiB (-0.00%)

samueltardieu added a commit to samueltardieu/rust that referenced this pull request Aug 23, 2025
…lt-opts-method, r=Kobzol

Remove default opts from config

We forgot to remove this method in rust-lang#145352. This PR removes that.

r? `@Kobzol`
samueltardieu added a commit to samueltardieu/rust that referenced this pull request Aug 23, 2025
…lt-opts-method, r=Kobzol

Remove default opts from config

We forgot to remove this method in rust-lang#145352. This PR removes that.

r? ``@Kobzol``
samueltardieu added a commit to samueltardieu/rust that referenced this pull request Aug 23, 2025
…lt-opts-method, r=Kobzol

Remove default opts from config

We forgot to remove this method in rust-lang#145352. This PR removes that.

r? ```@Kobzol```
rust-timer added a commit that referenced this pull request Aug 24, 2025
Rollup merge of #145774 - Shourya742:2025-08-23-remove-default-opts-method, r=Kobzol

Remove default opts from config

We forgot to remove this method in #145352. This PR removes that.

r? ```@Kobzol```
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Aug 24, 2025
…ethod, r=Kobzol

Remove default opts from config

We forgot to remove this method in rust-lang/rust#145352. This PR removes that.

r? ```@Kobzol```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants