bootstrap: consolidate subcommand parsing and matching #96003

aswild · 2022-04-13T05:49:56Z

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 #95937

rust-highfive · 2022-04-13T05:49:59Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

aswild · 2022-04-13T05:52:46Z

r? @jyn514

Mark-Simulacrum · 2022-04-13T12:11:13Z

@bors delegate=jyn514

bors · 2022-04-13T12:11:15Z

✌️ @jyn514 can now approve this pull request

jyn514

This looks like the right approach, thanks so much! My only concern is that there are a lot of unrelated changes along with the fix - I'd prefer to leave things as they are unless necessary, since bootstrap is very prone to regressions.

src/bootstrap/builder.rs

jyn514 · 2022-04-13T16:01:17Z

src/bootstrap/builder.rs

+    pub fn get_help(build: &Build, kind: Kind) -> Option<String> {
+        let step_descriptions = Builder::get_step_descriptions(kind);
+        if step_descriptions.is_empty() {
+            return None;


Why did this change?

This was part of the logic for printing the hint to run with -h -v to see available paths if and only if there are paths that we could list for that subcommand.

For example, in Rust now if you run ./x.py clippy -h it will suggest Run './x.py clippy -h -v' to see a list of available paths., but when you run ./x.py clippy -v -v it doesn't actually list any paths.

My assumption was that all of these commands could have useful paths to pass to them but I might be mistaken about that. It seems like check and clippy would take paths just like build does, but it kinda seems like clippy runs for "everything" in the build graph anyway and passing paths doesn't do much.

Is this specific list of commands which could print a list of paths for help text intentional and the others (clippy, check, fix) are supposed to be excluded?

My assumption was that all of these commands could have useful paths to pass to them but I might be mistaken about that.

This is not currently true. There are several related issues:

Not all commands accept paths. x clippy, as you noticed, runs on the whole tree. x clean does the same.

Some commands that do not accept paths do not check whether a path was passed - x clippy src/bootstrap is exactly the same as an unconditional x clippy.

Some commands the do accept paths do not go through StepDescription. For example x fmt src/bootstrap is perfectly valid and limits the changes to bootstrap itself, but there is no impl Step for Format description, because Step has no way to say "all paths".

Fixing those issues should wait for follow up PRs, I think. In the meantime, can you do the following:

Remove clippy and fix from the check arm and down to the vec![] arm. clippy is certainly not supported with paths, and fix has been broken for ages.

Change the code in usage that calls get_help to panic if -h -v is called for a subcommand that does not support paths. We should give the user an idea that something is broken rather than silently ignoring the flag.

jyn514 · 2022-04-13T16:02:21Z

src/bootstrap/flags.rs

+                        "Run `./x.py {} -h -v` to see a list of available paths.",
+                        subcommand.as_str()
+                    );
+                }


Is this refactoring necessary for the fix? Unrelated changes like this make the PR harder to review.

It's related to the logic in get_help(), see my comments there. I might have had the wrong idea about the desired behavior in this area.

src/bootstrap/flags.rs

jyn514 · 2022-04-13T16:13:33Z

@rustbot label -S-waiting-on-review +S-waiting-on-author +A-rustbuild +C-cleanup

aswild · 2022-04-14T02:39:39Z

Thanks for the review and guidance! I'll get things cleaned up.

Regarding the bigger changes in get_help and usage, those were more about UX than straight refactoring, but now I'm questioning whether I'm going the right direction there.

It's not great for help text to say "run this other command for more info" when that other command doesn't actually have more info. I went the direction of providing that info (available build target paths) in more cases, but maybe not printing the -h -v hint is more appropriate. Let me know which you prefer.

jyn514 · 2022-04-14T12:51:53Z

@aswild I think you took the right approach, I misunderstood the issues you were running into. I left suggestions in #96003 (comment) :)

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

aswild · 2022-04-16T18:59:49Z

Thanks for the suggestions! I implemented them and force-pushed a new patch set.

jyn514 · 2022-04-16T19:27:25Z

@bors r+

bors · 2022-04-16T19:27:27Z

📌 Commit 870cb8e has been approved by jyn514

jyn514 · 2022-04-16T19:30:56Z

Thanks for the PR!

aswild · 2022-04-17T16:57:36Z

Thanks for the opportunity to contribute to Rust! Compilers are big and scary but I can wrap my head around tooling and build systems

…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 · 2022-04-21T10:38:49Z

⌛ Testing commit 870cb8e with merge 1dec35a...

bors · 2022-04-21T13:45:05Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 1dec35a to master...

rust-timer · 2022-04-21T15:42:15Z

Finished benchmarking commit (1dec35a): comparison url.

Summary:

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: no relevant changes found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	1	0	0	0	1
mean²	2.3%	N/A	N/A	N/A	2.3%
max	2.3%	N/A	N/A	N/A	2.3%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

rylev · 2022-04-26T08:33:39Z

This seems to be a possible case of the syn-1.0.89 opt becoming noisy since it is the only test case (in the full scenario) impacted. We should mark it as triaged for now until we figure out what's up with that benchmark.

@rustbot label: +perf-regression-triaged

rust-highfive assigned Mark-Simulacrum Apr 13, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 13, 2022

Mark-Simulacrum assigned Mark-Simulacrum and unassigned Mark-Simulacrum Apr 13, 2022

jyn514 reviewed Apr 13, 2022

View reviewed changes

jyn514 mentioned this pull request Apr 14, 2022

x setup -h -v should work #96049

Closed

aswild force-pushed the pr/bootstrap-subcommands-cleanup branch from c6d27bb to 870cb8e Compare April 16, 2022 18:57

jyn514 approved these changes Apr 16, 2022

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2022

Dylan-DPC mentioned this pull request Apr 20, 2022

Rollup of 6 pull requests #96241

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 21, 2022

bors merged commit 1dec35a into rust-lang:master Apr 21, 2022

rustbot added this to the 1.62.0 milestone Apr 21, 2022

rustbot added the perf-regression Performance regression. label Apr 21, 2022

rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 26, 2022

matthiaskrgr mentioned this pull request May 7, 2022

x.py clippy no longer works? #96826

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootstrap: consolidate subcommand parsing and matching #96003

bootstrap: consolidate subcommand parsing and matching #96003

aswild commented Apr 13, 2022

rust-highfive commented Apr 13, 2022

aswild commented Apr 13, 2022

Mark-Simulacrum commented Apr 13, 2022

bors commented Apr 13, 2022

jyn514 left a comment

jyn514 Apr 13, 2022

aswild Apr 14, 2022

jyn514 Apr 14, 2022 •

edited

Loading

jyn514 Apr 13, 2022

aswild Apr 14, 2022

jyn514 commented Apr 13, 2022

aswild commented Apr 14, 2022

jyn514 commented Apr 14, 2022

aswild commented Apr 16, 2022

jyn514 commented Apr 16, 2022

bors commented Apr 16, 2022

jyn514 commented Apr 16, 2022

aswild commented Apr 17, 2022

bors commented Apr 21, 2022

bors commented Apr 21, 2022

rust-timer commented Apr 21, 2022

rylev commented Apr 26, 2022

bootstrap: consolidate subcommand parsing and matching #96003

bootstrap: consolidate subcommand parsing and matching #96003

Conversation

aswild commented Apr 13, 2022

rust-highfive commented Apr 13, 2022

aswild commented Apr 13, 2022

Mark-Simulacrum commented Apr 13, 2022

bors commented Apr 13, 2022

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 Apr 13, 2022

Choose a reason for hiding this comment

aswild Apr 14, 2022

Choose a reason for hiding this comment

jyn514 Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

jyn514 Apr 13, 2022

Choose a reason for hiding this comment

aswild Apr 14, 2022

Choose a reason for hiding this comment

jyn514 commented Apr 13, 2022

aswild commented Apr 14, 2022

jyn514 commented Apr 14, 2022

aswild commented Apr 16, 2022

jyn514 commented Apr 16, 2022

bors commented Apr 16, 2022

jyn514 commented Apr 16, 2022

aswild commented Apr 17, 2022

bors commented Apr 21, 2022

bors commented Apr 21, 2022

rust-timer commented Apr 21, 2022

Footnotes

rylev commented Apr 26, 2022

jyn514 Apr 14, 2022 •

edited

Loading