Create "suggested tests" tool in `rustbuild` #106249

Ezrashaw · 2022-12-29T09:18:04Z

Not the claimed person in #97339 but:
I've done a very rough implementation of this feature in-tree. I'm very new to rustc development (outside of docs) so some help would be greatly appreciated. The UI of this new subcommand obviously will change and I need some mentoring with the --run flag.

r? @jyn514

jyn514

Thanks for starting on this! It looks like this is just the scaffolding so far - adding a new subcommand seems reasonable, but I would ideally like this to be a separate tool, not part of bootstrap, so that the compile times are better.

Does that make sense, do you know how to get started with the suggestions part of it?

src/bootstrap/flags.rs

Ezrashaw · 2022-12-29T19:16:16Z

Ok, just a few things for clarification:

What do you mean by as a subcommand for x.py but also an external tool? I think that the tool existing inside rustbuild allows it to better be able to control the build process (and avoid all the complexities of invoking shell commands).
Obviously you probably have a better understanding of this than me, but why might my 50-odd line suggest.rs file have an adverse impact on compile times compared to the massive other rustbuild files?
Assuming that my current code is not completely different from what you had in mind, what should I do now? How could I implement the --run flag?

Thanks for your help.

EDIT: yeah, this is just scaffolding, just a rough idea. Also, I have started working on suggestions, it provides them to you based on modified files that match a glob (obviously more suggestions need to be added but that can be done over time)?

jyn514 · 2022-12-29T19:54:30Z

What do you mean by as a subcommand for x.py but also an external tool? I think that the tool existing inside rustbuild allows it to better be able to control the build process (and avoid all the complexities of invoking shell commands).

See how e.g. tidy is invoked. We have lots of existing libraries in bootstrap for running commands, actually building the separate tool won't be too much work in comparison.

Obviously you probably have a better understanding of this than me, but why might my 50-odd line suggest.rs file have an adverse impact on compile times compared to the massive other rustbuild files?

It means you have to rebuild rustbuild every time you change the suggestions, i.e. you have to recompile those massive other files. If you put it in a separate tool, you only have to recompile that tool itself, which makes it faster to iterate.

I'm not sure what you mean by a --run flag. Do you mean running the suggested tests first, before other tests? That's a stretch goal, I think we should just print the list of paths first before doing that.

Ezrashaw · 2022-12-29T20:03:53Z

Ok, moving the tool now makes a lot of sense, do you think I should put it in src/tools or an external repo? Also do you have any suggestions for my actual suggestions code, maybe it should load suggestions from a file?

Thanks again for your patience.

jyn514 · 2022-12-29T20:44:26Z

src/tools seems fine :) I don't have strong opinions on how the tool itself should be designed - either a configuration file or hard-coding the patterns seems fine since it's going to be quick to compile.

Ezrashaw · 2022-12-29T20:48:42Z

Ok, could you also give some feedback on the suggest.rs file which contains the actual stuff. Also if I load suggestions from a file, would you be happy with the tool being directly in src/bootstrap for simplicity?

jyn514

No, please put it in src/tools, src/bootstrap is specifically for rustbuild.

src/bootstrap/suggest.rs

Ezrashaw · 2022-12-30T00:30:14Z

@jyn514 I've applied most of your suggestions and I've written some (hacky) code for the --run flag. As far as I can tell, it is integrating seamlessly with the rest of the build process. Do you think I'm on the right track with this? Or is it not very good? I'll also implement dynamically loaded suggestions soon.

Ezrashaw · 2022-12-30T02:17:48Z

@rustbot ready

jyn514

Sorry I should have been more clear: I'm not particularly keen on moving it into its own tool now. If the tool itself stays quite small and static but suggestions are dynamically loaded then it kind of seems unnecessarily complex but I'll move it into src/tools if you want.

ah interesting! ok, that seems reasonable now that you're using reflection in bootstrap itself :) looks like you haven't started working on the dynamic suggestions yet - I was imaging that as a JSON file, something like this:

$ cat src/bootstrap/suggestions.json
{
  "path": "compiler/rustc_data_structures/*",
  "suggestions": [
    {"cmd": "check"},
    {"cmd": "test", "stage": 0, "paths": ["tidy", "compiler/rustc_data_structures"] }
  ]
}

jyn514 · 2022-12-30T19:00:34Z

src/bootstrap/builder.rs

+    /// Creates a new standalone builder for use outside of the normal process
+    pub fn new_standalone(
+        build: &mut Build,
+        kind: Kind,
+        paths: Vec<PathBuf>,
+        stage: Option<u32>,
+    ) -> Builder<'_> {
+        // FIXME: don't mutate `build`
+        if let Some(stage) = stage {
+            build.config.stage = stage;
+        }
+
+        Self::new_internal(build, kind, paths.to_owned())


I think you can avoid needing this new function by modifying build.config.cmd and using Builder::new instead. You could change sug! to generate the command for you so it's not too much of a pain:

(test, $stage:literal, $paths:expr) => Suggestion { stage: $stage, cmd: Subcommand::Test { paths: $paths } },

Thanks for the idea, I think I also need to modify build.config.stage. Sorry I'm on holiday right now so the progress is slow.

No need to apologize! You're responding quite fast actually 😄

Hmm, using serde_json means that we can't directly create Subcommands. Honestly, the solution is good enough as is for now.

I'm not sure I understand the problem, sorry - how is serde_json related here?

@Ezrashaw I don't think I ever got an answer here - how is serde related?

Hmm, using serde_json means that we can't directly create Subcommands. Honestly, the solution is good enough as is for now.

Sorry about that. I have no idea what I was thinking lol. I think that this isn't that relevant anymore since suggestions are implemented in code.

Ezrashaw · 2022-12-30T20:34:30Z

Seems quite complex :P. I was thinking more like plain text if that's ok, because then it becomes easier to write tidy checks, etc. Assuming JSON, is there a library preference?

jyn514 · 2022-12-30T20:46:31Z

Bootstrap already uses serde_json, you can reuse that :)

What would a text format look like? The fields you're parsing are pretty nested, I worry that using a non-structured format would be pretty ambiguous.

Ezrashaw · 2022-12-30T21:18:12Z

Yes, that's a good point. Also it's probably easier to #[derive(Deserialize)] a bunch than write a parser for plain text.

How should we go about compiling a list of suggestions? Obviously it can be expanded over time but some more now might be good. Maybe also a tidy check for this because if you have bad JSON then it'll only get picked up on at runtime (not tested)? And some kind of check to make sure that files matching the globs do exist, to keep the suggestions in date?

src/bootstrap/suggest.rs

Ezrashaw · 2022-12-31T03:54:38Z

@jyn514 I've implemented the things we've talked about. I think we should just wait a bit while the reverts happen. It might turn out that this PR brings in the build_helper crate with the git functions. Also see my last comment about the list of suggestions.

EDIT: I've fixed the merge conflict, basically I cherry-picked the "create build_helper + my modified files stuff" commit from the reverted #105058.

EDIT 2: yet another merge conflict 🙄

bors · 2022-12-31T05:37:26Z

☔ The latest upstream changes (presumably #106320) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2022-12-31T09:24:58Z

☔ The latest upstream changes (presumably #106324) made this pull request unmergeable. Please resolve the merge conflicts.

Create "suggested tests" tool in `rustbuild` Not the claimed person in rust-lang#97339 but: I've done a very rough implementation of this feature in-tree. I'm very new to `rustc` development (outside of docs) so some help would be greatly appreciated. The UI of this new subcommand obviously will change and I need some mentoring with the `--run` flag. r? ``@jyn514``

compiler-errors · 2023-04-11T18:40:51Z

This failed in a rollup: https://github.com/rust-lang-ci/rust/actions/runs/4667818566/jobs/8264150865

error[E0277]: the trait bound `BuildMetrics: Clone` is not satisfied
   --> lib.rs:244:5
    |
193 | #[derive(Clone)]
    |          ----- in this derive macro expansion
...
244 |     metrics: metrics::BuildMetrics,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Clone` is not implemented for `BuildMetrics`
    |
    = note: this error originates in the derive macro `Clone` (in Nightly builds, run with -Z macro-backtrace for more info)

@bors r-

Ezrashaw · 2023-04-12T02:53:06Z

@compiler-errors Could I get a re-roll please? Issue occurred when using a CI-only cargo feature.

albertlarsan68 · 2023-04-12T05:52:24Z

IIIRC, build metrics are enabled in all CI jobs. I'm not happy with the solution you found, but making this work with build metrics can be done as a follow-up.
@bors r=jyn514,albertlarsan68

bors · 2023-04-12T05:52:26Z

📌 Commit a159dcd has been approved by jyn514,albertlarsan68

It is now in the queue for this repository.

Ezrashaw · 2023-04-12T06:34:49Z

@albertlarsan68

IIIRC, build metrics are enabled in all CI jobs. I'm not happy with the solution you found, but making this work with build metrics can be done as a follow-up.

Keep in mind that CI should never need to run x suggest; it is a user-only feature.

Unfortunately, upstream changes are required for BuildMetrics to implement Clone in the sysinfo crate.

…14,albertlarsan68 Create "suggested tests" tool in `rustbuild` Not the claimed person in rust-lang#97339 but: I've done a very rough implementation of this feature in-tree. I'm very new to `rustc` development (outside of docs) so some help would be greatly appreciated. The UI of this new subcommand obviously will change and I need some mentoring with the `--run` flag. r? `@jyn514`

…14,albertlarsan68 Create "suggested tests" tool in `rustbuild` Not the claimed person in rust-lang#97339 but: I've done a very rough implementation of this feature in-tree. I'm very new to `rustc` development (outside of docs) so some help would be greatly appreciated. The UI of this new subcommand obviously will change and I need some mentoring with the `--run` flag. r? ``@jyn514``

Rollup of 7 pull requests Successful merges: - rust-lang#103682 (Stabilize rustdoc `--test-run-directory`) - rust-lang#106249 (Create "suggested tests" tool in `rustbuild`) - rust-lang#110047 (Add link to `collections` docs to `extend` trait) - rust-lang#110269 (Add `tidy-alphabetical` to features in `core`) - rust-lang#110292 (Add `tidy-alphabetical` to features in `alloc` & `std`) - rust-lang#110305 (rustdoc-search: use ES6 `Map` and `Set` where they make sense) - rust-lang#110315 (Add a stable MIR way to get the main function) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…mulacrum Fix the test directories suggested by `./x.py suggest` It seems that these paths were correct when rust-lang#106249 was being written, but since then rust-lang#106458 has been merged (moving `src/test/` to `tests/`), making the tool's suggestions incorrect.

rustbot assigned jyn514 Dec 29, 2022

rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 29, 2022

jyn514 reviewed Dec 29, 2022

View reviewed changes

src/bootstrap/flags.rs Outdated Show resolved Hide resolved

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 29, 2022

jyn514 reviewed Dec 29, 2022

View reviewed changes

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 30, 2022

jyn514 reviewed Dec 30, 2022

View reviewed changes

albertlarsan68 reviewed Dec 30, 2022

View reviewed changes

src/bootstrap/suggest.rs Outdated Show resolved Hide resolved

Ezrashaw force-pushed the suggest-test-tool branch from 59e32fd to 5b78baa Compare December 31, 2022 02:43

Ezrashaw force-pushed the suggest-test-tool branch from 5b78baa to 655d32d Compare December 31, 2022 03:54

Ezrashaw force-pushed the suggest-test-tool branch from 655d32d to e0a702d Compare December 31, 2022 08:13

rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Dec 31, 2022

Ezrashaw mentioned this pull request Dec 31, 2022

Dont use --merge-base during bootstrap formatting subcommand #106310

Merged

Ezrashaw force-pushed the suggest-test-tool branch from e0a702d to cf29f0d Compare December 31, 2022 08:26

JohnTitor mentioned this pull request Apr 11, 2023

Rollup of 7 pull requests #110184

Closed

Dylan-DPC mentioned this pull request Apr 11, 2023

Rollup of 6 pull requests #110191

Closed

This comment was marked as duplicate.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 11, 2023

fix: disable x suggest when using build-metrics

a159dcd

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 12, 2023

Dylan-DPC mentioned this pull request Apr 13, 2023

Rollup of 6 pull requests #110267

Closed

Dylan-DPC mentioned this pull request Apr 14, 2023

Rollup of 8 pull requests #110308

Closed

JohnTitor mentioned this pull request Apr 14, 2023

Rollup of 7 pull requests #110324

Merged

bors merged commit 7879386 into rust-lang:master Apr 14, 2023

rustbot added this to the 1.70.0 milestone Apr 14, 2023

Ezrashaw deleted the suggest-test-tool branch April 15, 2023 00:33

Zalathar mentioned this pull request Apr 30, 2023

Fix the test directories suggested by ./x.py suggest #111002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create "suggested tests" tool in `rustbuild` #106249

Create "suggested tests" tool in `rustbuild` #106249

Ezrashaw commented Dec 29, 2022

jyn514 left a comment

Ezrashaw commented Dec 29, 2022 •

edited

Loading

jyn514 commented Dec 29, 2022 •

edited

Loading

Ezrashaw commented Dec 29, 2022

jyn514 commented Dec 29, 2022

Ezrashaw commented Dec 29, 2022

jyn514 left a comment

Ezrashaw commented Dec 30, 2022

Ezrashaw commented Dec 30, 2022

jyn514 left a comment •

edited

Loading

jyn514 Dec 30, 2022

Ezrashaw Dec 30, 2022

jyn514 Dec 30, 2022

Ezrashaw Dec 31, 2022

jyn514 Jan 15, 2023

jyn514 Apr 5, 2023

Ezrashaw Apr 6, 2023 •

edited

Loading

Ezrashaw commented Dec 30, 2022

jyn514 commented Dec 30, 2022

Ezrashaw commented Dec 30, 2022

Ezrashaw commented Dec 31, 2022 •

edited

Loading

bors commented Dec 31, 2022

bors commented Dec 31, 2022

compiler-errors commented Apr 11, 2023

This comment was marked as duplicate.

Ezrashaw commented Apr 12, 2023 •

edited

Loading

albertlarsan68 commented Apr 12, 2023

bors commented Apr 12, 2023

Ezrashaw commented Apr 12, 2023

Create "suggested tests" tool in rustbuild #106249

Create "suggested tests" tool in rustbuild #106249

Conversation

Ezrashaw commented Dec 29, 2022

jyn514 left a comment

Choose a reason for hiding this comment

Ezrashaw commented Dec 29, 2022 • edited Loading

jyn514 commented Dec 29, 2022 • edited Loading

Ezrashaw commented Dec 29, 2022

jyn514 commented Dec 29, 2022

Ezrashaw commented Dec 29, 2022

jyn514 left a comment

Choose a reason for hiding this comment

Ezrashaw commented Dec 30, 2022

Ezrashaw commented Dec 30, 2022

jyn514 left a comment • edited Loading

Choose a reason for hiding this comment

jyn514 Dec 30, 2022

Choose a reason for hiding this comment

Ezrashaw Dec 30, 2022

Choose a reason for hiding this comment

jyn514 Dec 30, 2022

Choose a reason for hiding this comment

Ezrashaw Dec 31, 2022

Choose a reason for hiding this comment

jyn514 Jan 15, 2023

Choose a reason for hiding this comment

jyn514 Apr 5, 2023

Choose a reason for hiding this comment

Ezrashaw Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Ezrashaw commented Dec 30, 2022

jyn514 commented Dec 30, 2022

Ezrashaw commented Dec 30, 2022

Ezrashaw commented Dec 31, 2022 • edited Loading

bors commented Dec 31, 2022

bors commented Dec 31, 2022

compiler-errors commented Apr 11, 2023

This comment was marked as duplicate.

Ezrashaw commented Apr 12, 2023 • edited Loading

albertlarsan68 commented Apr 12, 2023

bors commented Apr 12, 2023

Ezrashaw commented Apr 12, 2023

Create "suggested tests" tool in `rustbuild` #106249

Create "suggested tests" tool in `rustbuild` #106249

Ezrashaw commented Dec 29, 2022 •

edited

Loading

jyn514 commented Dec 29, 2022 •

edited

Loading

jyn514 left a comment •

edited

Loading

Ezrashaw Apr 6, 2023 •

edited

Loading

Ezrashaw commented Dec 31, 2022 •

edited

Loading

Ezrashaw commented Apr 12, 2023 •

edited

Loading