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

Consolidate subcommand parsing in bootstrap #95937

Closed
jyn514 opened this issue Apr 11, 2022 · 2 comments · Fixed by #96003
Closed

Consolidate subcommand parsing in bootstrap #95937

jyn514 opened this issue Apr 11, 2022 · 2 comments · Fixed by #96003
Assignees
@aswild
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@jyn514
Copy link
Member

jyn514 commented Apr 11, 2022 •
edited by rustbot
Loading

We should consolidate the subcommand parsing to be consistent - currently, there's many different places where commands are parsed, leading to inconsistencies where they don't match up (so to speak :P).

Originally posted by @jyn514 in #95875 (review)
cc @aswild

@rustbot label +A-bootstrap
@aswild
Copy link
Contributor

aswild commented Apr 11, 2022

[continued from #95875]

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 :)

Yeah, I'd love to work on this. Last night I got a proof of concept "add a new type" implementation working just to see what it'd be like, but I can change gears and look into adding the needed new variants to builder::Kind. I was a bit intimidated that there might be specific reasons not to have a Builder for the commands that are missing from it (e.g. fmt, clean, and setup).

Another thing I noticed that can be improved in this area, there's several commands that say run './x.py foo -h -v' to see a list of available paths in their help text, but then if you run -h -v there's no paths that get listed. Seems straightforward to generate that message automatically rather than having to keep two lists in sync manually.

@rustbot claim

@jyn514
Copy link
Member Author

jyn514 commented Apr 12, 2022

@rustbot label +A-rustbuild +E-easy

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Apr 12, 2022
@aswild aswild mentioned this issue Apr 13, 2022
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 20, 2022
@Dylan-DPC
Rollup merge of rust-lang#96003 - aswild:pr/bootstrap-subcommands-cle…
20b927a 
…anup, r=jyn514

bootstrap: consolidate subcommand parsing and matching

There's several places where the x.py command names are matched as
strings, leading to some inconsistencies and opportunities for cleanup.

* Add Format, Clean, and Setup variants to builder::Kind.
* Use Kind to parse the x.py subcommand name (including aliases)
* Match on the subcommand Kind rather than strings when handling
  options and help text.
* Several subcommands don't display any paths when run with `-h -v` even
  though the help text indicates that they should. Fix this and refactor
  so that manually keeping matches in sync isn't necessary.

Fixes rust-lang#95937
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 21, 2022
@bors
Auto merge of rust-lang#96003 - aswild:pr/bootstrap-subcommands-clean…
1dec35a 
…up, r=jyn514

bootstrap: consolidate subcommand parsing and matching

There's several places where the x.py command names are matched as
strings, leading to some inconsistencies and opportunities for cleanup.

* Add Format, Clean, and Setup variants to builder::Kind.
* Use Kind to parse the x.py subcommand name (including aliases)
* Match on the subcommand Kind rather than strings when handling
  options and help text.
* Several subcommands don't display any paths when run with `-h -v` even
  though the help text indicates that they should. Fix this and refactor
  so that manually keeping matches in sync isn't necessary.

Fixes rust-lang#95937
@bors bors closed this as completed in 870cb8e Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aswild aswild

Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

bootstrap: consolidate subcommand parsing and matching aswild/rust
3 participants
@aswild @jyn514 @rustbot