Rustify `ci/integration.sh` script by GuillaumeGomez · Pull Request #6817 · rust-lang/rustfmt

GuillaumeGomez · 2026-03-04T18:03:02Z

As discussed on zulip.

I went the easy road: didn't create a workspace for this script, so it uses the same dependencies as the main project (although it only needs std). It can be run with cargo run --bin ci-integration.

Anyway, this is very straightforward conversion, please tell me if you'd prefer things to be different in any regard.

r? @jieyouxu

matthewhughes934

All my style comments are subjective, so feel free to ignore at your discretion

View changes since this review

matthewhughes934 · 2026-03-05T21:32:27Z

Sign in to view

+    }
+
+    let output = run_command_with_output_and_env("cargo", &["test", test_args], current_dir, &env)?;
+    if ["build failed", "test result: FAILED."]


Will we get here? If tests or builds fail I assume cargo test will exit non-zero and in that case will run_command_with_output_and_env above return an Err?

I kept the same behaviour as before: https://github.com/rust-lang/rustfmt/blob/main/ci/integration.sh#L46-L48

If the output contains one of the two strings, we return the function with 0, meaning success. Sounds super weird but that's the current behaviour.

Btw, run_command_with_output_and_env doesn't check whether or not the command exited successfully.

My guess is that the intention there is to skip over crates which already fails test (if the baseline fails it could be other reasons too).

matthewhughes934 · 2026-03-05T21:37:48Z

+    )?;
+    write_file("rustfmt_check_output", &output)?;
+
+    run_command_with_env("cargo", &["test", test_args], current_dir, &env)


Don't we already run cargo test at line 118?

Yep. But kept the exact same behaviour:

https://github.com/rust-lang/rustfmt/blob/main/ci/integration.sh#L45

https://github.com/rust-lang/rustfmt/blob/main/ci/integration.sh#L73

This might catch really broken --check format where it should not have modified the source in some off chance, seems okay to keep it. Can we add a comment here to say sth to that effect? I imagine we won't be the only ones with that question reading this :)

GuillaumeGomez · 2026-03-06T10:31:03Z

Applied suggestions and fixed file creation.

matthewhughes934

I think I didn't read the original script carefully enough on my first review (I think most my confusion comes from things that script was already doing, sorry about that). So this review is more focused on changes in behaviour from the old script

(also Github isn't letting me resolve some of the conversations your answered? I don't see the button for it for some reason)

View changes since this review

GuillaumeGomez · 2026-03-09T14:20:40Z

Updated!

GuillaumeGomez · 2026-03-10T10:57:34Z

Applied suggestion.

jieyouxu

Sorry for the delay. This broadly looks good to me, some nits.
@rustbot author

View changes since this review

jieyouxu · 2026-03-05T01:19:49Z

        INTEGRATION: ${{ matrix.integration }}
        TARGET: x86_64-unknown-linux-gnu
-      run: ./ci/integration.sh
+      run: cargo run --bin ci-integration


Remark (not for this PR): I wonder if we should coalsce this and Diff Check into one crate, sth akin to citool on r-l/r. The benefit is that the tools could then share common logic easily.

That's what I plan to do. But to reduce the diff, I decided to do it in multiple parts:

Migrate shell scripts to rust

Improve code

Although I think I'll make one crate once I start porting the second one to not duplicate code.

jieyouxu · 2026-03-10T11:58:50Z

+    }
+
+    let output = run_command_with_output_and_env("cargo", &["test", test_args], current_dir, &env)?;
+    if ["build failed", "test result: FAILED."]


My guess is that the intention there is to skip over crates which already fails test (if the baseline fails it could be other reasons too).

jieyouxu · 2026-03-10T12:00:39Z

+
+    let output =
+        run_command_with_output_and_env("cargo", &["fmt", "--all", "-v"], current_dir, &env)?;
+    println!("{}", output.output);


Thought (not for this PR): we might be able to help group the logs with GHA log groups

jieyouxu · 2026-03-10T12:02:20Z

+    )?;
+    write_file("rustfmt_check_output", &output)?;
+
+    run_command_with_env("cargo", &["test", test_args], current_dir, &env)


This might catch really broken --check format where it should not have modified the source in some off chance, seems okay to keep it. Can we add a comment here to say sth to that effect? I imagine we won't be the only ones with that question reading this :)

GuillaumeGomez · 2026-03-10T13:08:09Z

Applied suggestions, I also added comments to explain the final command as well.

jieyouxu

Looks good to me with one tiny nit, thanks

View changes since this review

jieyouxu

This looks good to me now, thanks.
cc @ytmimi maybe a vibe-check when you have time?

View changes since this review

GuillaumeGomez · 2026-03-30T09:23:43Z

It's been 3 weeks. Any news here? I'd like to continue this work with migrating the other shell script as well.

ytmimi

Gave it a once over and I think the new rust version looks good

View changes since this review

rustbot assigned jieyouxu Mar 4, 2026

rustbot added A-CI Area: CI S-waiting-on-review Status: awaiting review from the assignee but also interested parties. labels Mar 4, 2026

matthewhughes934 reviewed Mar 5, 2026

View reviewed changes

GuillaumeGomez force-pushed the rustify-shell branch from 980b07b to 0469d7c Compare March 6, 2026 10:30

matthewhughes934 reviewed Mar 6, 2026

View reviewed changes

Comment thread ci/integration.rs Outdated

Comment thread ci/integration.rs

Comment thread ci/integration.rs

matthewhughes934 approved these changes Mar 10, 2026

View reviewed changes

Comment thread ci/integration.rs Outdated

GuillaumeGomez force-pushed the rustify-shell branch from 4d05d5e to eeb5fe5 Compare March 10, 2026 10:57

jieyouxu reviewed Mar 10, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: 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 Mar 10, 2026

jieyouxu approved these changes Mar 11, 2026

View reviewed changes

Comment thread Cargo.toml Outdated

GuillaumeGomez added 5 commits March 11, 2026 11:51

Rustify ci/integration.sh script

e1f0c8e

Replace map! macro with plain HashMap calls

04840f6

Only create rustfmt.toml if it does not already exist

5706ca5

Stick closer to original shell script behaviour

f07689e

Improve ci/integration.rs code

1789617

GuillaumeGomez force-pushed the rustify-shell branch from 32e6ec5 to 1789617 Compare March 11, 2026 10:52

jieyouxu approved these changes Mar 12, 2026

View reviewed changes

ytmimi approved these changes Apr 1, 2026

View reviewed changes

ytmimi merged commit f9ff702 into rust-lang:main Apr 1, 2026
26 checks passed

rustbot added the release-notes Needs an associated changelog entry label Apr 1, 2026

rustbot removed the S-waiting-on-review Status: awaiting review from the assignee but also interested parties. label Apr 1, 2026

ytmimi removed the pr-ready-to-merge Status: PR is largely ready for merge, waiting for secondary review / last nits label Apr 1, 2026

GuillaumeGomez mentioned this pull request Apr 3, 2026

Rustify ci/build_and_test.* scripts #6855

Merged

Conversation

GuillaumeGomez commented Mar 4, 2026

Uh oh!

matthewhughes934 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GuillaumeGomez commented Mar 6, 2026

Uh oh!

matthewhughes934 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuillaumeGomez commented Mar 9, 2026

Uh oh!

Uh oh!

GuillaumeGomez commented Mar 10, 2026

Uh oh!

jieyouxu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jieyouxu Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GuillaumeGomez commented Mar 10, 2026

Uh oh!

jieyouxu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jieyouxu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Mar 30, 2026

Uh oh!

ytmimi left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

matthewhughes934 left a comment •

edited by rustbot

Loading

jieyouxu Mar 10, 2026 •

edited

Loading

matthewhughes934 left a comment •

edited by rustbot

Loading

jieyouxu left a comment •

edited by rustbot

Loading

jieyouxu Mar 10, 2026 •

edited

Loading

jieyouxu left a comment •

edited by rustbot

Loading

jieyouxu left a comment •

edited by rustbot

Loading

ytmimi left a comment •

edited by rustbot

Loading