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.

Sign up for GitHub

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

Jump to bottom

Migrate bootstrap to Clap-based argument parsing #110693

Merged
merged 1 commit into from
May 7, 2023
Merged

Migrate bootstrap to Clap-based argument parsing #110693

merged 1 commit into from
May 7, 2023

Conversation

clubby789
Copy link
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
Copy link
Collaborator

rustbot commented Apr 22, 2023

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
Copy link
Member

@albertlarsan68 albertlarsan68 left a comment

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
Copy link
Contributor Author

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

@Mark-Simulacrum
Copy link
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
Copy link
Contributor

bors commented May 6, 2023

📌 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
Copy link
Contributor

bors commented May 6, 2023

⌛ Testing commit 8b39d2d4b61b52febd7b2b15e7d1b5d6532420a5 with merge ef5365067f707662ca25b69e14c1a24d5f3ec5da...

@bors
Copy link
Contributor

bors commented May 6, 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 6, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@clubby789
Copy link
Contributor Author

Going to try and debug this now

@clubby789
Copy link
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
Copy link
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
Copy link
Member

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

@bors
Copy link
Contributor

bors commented May 6, 2023

📌 Commit 32e27cc has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 7, 2023

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

@bors
Copy link
Contributor

bors commented May 7, 2023

☀️ 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
Copy link
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
Copy link
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
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