Skip to content

Migrate bootstrap to Clap-based argument parsing#110693

Merged
bors merged 1 commit into
rust-lang:masterfrom
clubby789:x-clap-take-2
May 7, 2023
Merged

Migrate bootstrap to Clap-based argument parsing#110693
bors merged 1 commit into
rust-lang:masterfrom
clubby789:x-clap-take-2

Conversation

@clubby789

@clubby789 clubby789 commented Apr 22, 2023

Copy link
Copy Markdown
Contributor

Supercedes #108083

I chose to re-do the work rather than rebase the onto the large changes since the original PR. If it's preferred I can instead force-push the original PR to this version.

cc @jyn514 @albertlarsan68

@rustbot

rustbot commented Apr 22, 2023

Copy link
Copy Markdown
Collaborator

r? @albertlarsan68

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 22, 2023
@clubby789 clubby789 changed the title Migrate bootstrap to Clap-based arguments Migrate bootstrap to Clap-based argument parsing Apr 22, 2023
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Apr 22, 2023
@clubby789 clubby789 mentioned this pull request Apr 22, 2023
Closed
albertlarsan68
albertlarsan68 reviewed Apr 22, 2023
View reviewed changes

@albertlarsan68 albertlarsan68 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, but I would like to have another pair of eyes.
r? @Mark-Simulacrum

@albertlarsan68 albertlarsan68 self-assigned this Apr 22, 2023
@jyn514 jyn514 mentioned this pull request Apr 26, 2023
Closed
@clubby789

clubby789 commented May 5, 2023

Copy link
Copy Markdown
Contributor Author

Rebased and preserved the behaviour of --warnings warn, which is not currently documented

@Mark-Simulacrum

Mark-Simulacrum commented May 6, 2023

Copy link
Copy Markdown
Member

@bors r+ rollup=never

This seems broadly OK to me. I think the compile-time impact is minimal enough for now, we can iterate further if needed. clap is pretty minimal in terms of additional compile times that it itself contributes.

@bors

bors commented May 6, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit 8b39d2d4b61b52febd7b2b15e7d1b5d6532420a5 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 May 6, 2023
@bors

bors commented May 6, 2023

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 8b39d2d4b61b52febd7b2b15e7d1b5d6532420a5 with merge ef5365067f707662ca25b69e14c1a24d5f3ec5da...

@bors

bors commented May 6, 2023

Copy link
Copy Markdown
Collaborator

💔 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 6, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@clubby789

clubby789 commented May 6, 2023

Copy link
Copy Markdown
Contributor Author

Going to try and debug this now

@clubby789

clubby789 commented May 6, 2023

Copy link
Copy Markdown
Contributor Author

Clap was parsing --host="" as a single TargetSelection with empty contents. Added some logic to instead parse this as an empty list

@Noratrieb

Noratrieb commented May 6, 2023

Copy link
Copy Markdown
Member

@bors retry

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 6, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 6, 2023
@Noratrieb

Noratrieb commented May 6, 2023

Copy link
Copy Markdown
Member

Wait actually I think
@bors r=Mark-Simulacrum
is more correct here, i dont know what retry does here

@bors

bors commented May 6, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit 32e27cc has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors

bors commented May 7, 2023

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 32e27cc with merge 70a779c...

@bors

bors commented May 7, 2023

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 70a779c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 7, 2023
@bors bors merged commit 70a779c into rust-lang:master May 7, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 7, 2023
@bors bors mentioned this pull request May 7, 2023
Merged
@rust-timer

rust-timer commented May 7, 2023

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (70a779c): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -1.5% [-2.8%, -0.5%] 5
Improvements ✅
(secondary)		 -5.2% [-7.5%, -0.2%] 7
All ❌✅ (primary) -1.5% [-2.8%, -0.5%] 5

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -4.4% [-4.8%, -4.1%] 3
Improvements ✅
(secondary)		 -2.3% [-3.3%, -1.0%] 3
All ❌✅ (primary) -4.4% [-4.8%, -4.1%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -4.7% [-5.3%, -4.5%] 6
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 654.308s -> 653.509s (-0.12%)

@albertlarsan68

albertlarsan68 commented May 9, 2023

Copy link
Copy Markdown
Member

This is some big noise, this PR only touches bootstrap, not Rustc.

@clubby789 clubby789 mentioned this pull request May 9, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 14, 2023
@bors
Auto merge of rust-lang#111388 - clubby789:clap-complete, r=jyn514
2e18605 
Generate shell completions for bootstrap with Clap

Now that rust-lang#110693 has been merged, we can look at generating shell completions for x.py with `clap_complete`. Leaving this as draft for now as I'm not sure of the best way to integration the completion generator. Additionally, the generated completions for zsh are completely broken (will need to be resolved upstream, it doesn't seem to handle subcommands + global arguments well).
I don't have Fish installed and would be interested to know how well completions work there.

Alternative to rust-lang#107827
@jyn514 jyn514 mentioned this pull request May 29, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albertlarsan68 albertlarsan68 albertlarsan68 left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

@albertlarsan68 albertlarsan68

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

Milestone

1.71.0

Development

Successfully merging this pull request may close these issues.

8 participants

@clubby789 @rustbot @Mark-Simulacrum @bors @rust-log-analyzer @Noratrieb @rust-timer @albertlarsan68