Skip to content

bootstrap: show available paths help text for aliased subcommands #95875

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

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

aswild
Copy link
Contributor

@aswild aswild commented Apr 10, 2022

Running ./x.py build -h -v shows a list of available build targets,
but the short alias ./x.py b -h -v does not. Fix so that the aliases
behave the same as their spelled out counterparts.

Running `./x.py build -h -v` shows a list of available build targets,
but the short alias `./x.py b -h -v` does not. Fix so that the aliases
behave the same as their spelled out counterparts.
@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 10, 2022
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I think we should consider consolidating these to be consistent at some point - there's a ton of these matches sprawled across src/bootstrap.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

Yeah, consolidating this into one place may be a little difficult but likely worthwhile.

@bors
Copy link
Collaborator

bors commented Apr 11, 2022

📌 Commit e4bbbac has been approved by Mark-Simulacrum

@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 Apr 11, 2022
@aswild
Copy link
Contributor Author

aswild commented Apr 11, 2022

Yeah, I was a bit surprised at the number of string based matches for subcommand rather than parsing into an enum early on.

The builder::Kind enum seems like it could work, but it's missing variants for Format, Clean, and Setup. flags::Subcommand is essentially what we're mapping on, but that enum has data associated with each variant that isn't built until after the subcommand type is known.

Here's a general idea for one way this could be refactored

enum SubcommandKind {
    Build,
    Check,
    ...
}
impl FromStr for SubcommandKind {...}
impl SubcommandKind {
    fn add_extra_opts(&self, opts: &mut Options) { /* flags.rs lines 279-338 */}
    fn add_extra_help(&self, subcommand_help: &mut String) { /* flags.rs lines 397-539 */ }
    fn build_subcommand(&self, paths: Vec<PathBuf>, matches: &Matches) -> Result<Subcommand> {
        /* flags.rs lines 550-624 */
    }
}

Pros of this: matching on an enum rather than string names of the commands, splitting up the very long Flags::parse() function so it's not over 500 lines long. Cons: large diff from moving so much code around may be undesirable, and now there's 3 different enums that have variants like Build, Check, Test, etc. which could be confusing.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 11, 2022
…imulacrum

bootstrap: show available paths help text for aliased subcommands

Running `./x.py build -h -v` shows a list of available build targets,
but the short alias `./x.py b -h -v` does not. Fix so that the aliases
behave the same as their spelled out counterparts.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#95743 (Update binary_search example to instead redirect to partition_point)
 - rust-lang#95771 (Update linker-plugin-lto.md to 1.60)
 - rust-lang#95861 (Note that CI tests Windows 10)
 - rust-lang#95875 (bootstrap: show available paths help text for aliased subcommands)
 - rust-lang#95876 (Add a note for unsatisfied `~const Drop` bounds)
 - rust-lang#95907 (address fixme for diagnostic variable name)
 - rust-lang#95917 (thin_box test: import from std, not alloc)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@jyn514
Copy link
Member

jyn514 commented Apr 11, 2022

The builder::Kind enum seems like it could work, but it's missing variants for Format, Clean, and Setup.

I think just adding the new variants will be simpler than trying to introduce a new type. Are you interested in working on that? I'm happy to mentor :)

@bors bors merged commit 69fb8f6 into rust-lang:master Apr 11, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 11, 2022
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants