Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/integration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -76,5 +76,5 @@ jobs:
env:
INTEGRATION: ${{ matrix.integration }}
TARGET: x86_64-unknown-linux-gnu
run: ./ci/integration.sh
run: cargo run --bin ci-integration
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

  1. Migrate shell scripts to rust
  2. Improve code

Copy link
Member Author

Choose a reason for hiding this comment

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

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

continue-on-error: ${{ matrix.allow-failure == true }}
5 changes: 5 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ path = "src/format-diff/main.rs"
name = "git-rustfmt"
path = "src/git-rustfmt/main.rs"

[[bin]]
name = "ci-integration"
path = "ci/integration.rs"

[features]
default = ["cargo-fmt", "rustfmt-format-diff"]
cargo-fmt = []
Expand Down Expand Up @@ -70,3 +74,4 @@ tempfile = "3.23.0"
[package.metadata.rust-analyzer]
# This package uses #[feature(rustc_private)]
rustc_private = true

248 changes: 248 additions & 0 deletions ci/integration.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,248 @@
use std::collections::HashMap;
use std::env::var;
use std::ffi::OsStr;
use std::path::Path;
use std::process::Command;

fn write_file(file_path: impl AsRef<Path>, content: &str) -> Result<(), String> {
std::fs::write(&file_path, content).map_err(|error| {
format!(
"Failed to create empty `{}` file: {error:?}",
file_path.as_ref().display(),
)
})
}

fn run_command_with_env<I, S>(
bin: &str,
args: I,
current_dir: &str,
env: &HashMap<&str, &str>,
Copy link
Member

Choose a reason for hiding this comment

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

Remark: thought about BTreeMap for stable iteration order but I think that might not matter

) -> Result<(), String>
where
I: IntoIterator<Item = S>,
S: AsRef<OsStr>,
{
let exit_status = Command::new(bin)
.args(args)
.envs(env)
.current_dir(current_dir)
.spawn()
.map_err(|error| format!("Failed to spawn command `{bin}`: {error:?}"))?
.wait()
.map_err(|error| format!("Failed to wait command `{bin}`: {error:?}"))?;
if exit_status.success() {
Ok(())
} else {
Err(format!("Command `{bin}` failed"))
}
}

fn run_command<I, S>(bin: &str, args: I, current_dir: &str) -> Result<(), String>
where
I: IntoIterator<Item = S>,
S: AsRef<OsStr>,
{
run_command_with_env(bin, args, current_dir, &HashMap::new())
}

struct CommandOutput {
output: String,
exited_successfully: bool,
}

fn run_command_with_output_and_env<I, S>(
bin: &str,
args: I,
current_dir: &str,
env: &HashMap<&str, &str>,
) -> Result<CommandOutput, String>
where
I: IntoIterator<Item = S>,
S: AsRef<OsStr>,
{
let cmd_output = Command::new(bin)
.args(args)
.envs(env)
.current_dir(current_dir)
.output()
.map_err(|error| format!("Failed to spawn command `{bin}`: {error:?}"))?;
let mut output = String::from_utf8_lossy(&cmd_output.stdout).into_owned();
output.push_str(&String::from_utf8_lossy(&cmd_output.stderr));
Ok(CommandOutput {
output,
exited_successfully: cmd_output.status.success(),
})
}

fn run_command_with_output<I, S>(
bin: &str,
args: I,
current_dir: &str,
) -> Result<CommandOutput, String>
where
I: IntoIterator<Item = S>,
S: AsRef<OsStr>,
{
run_command_with_output_and_env(bin, args, current_dir, &HashMap::new())
}

// Checks that:
//
// * `cargo fmt --all` succeeds without any warnings or errors
// * `cargo fmt --all -- --check` after formatting returns success
// * `cargo test --all` still passes (formatting did not break the build)
fn check_fmt_with_all_tests(env: HashMap<&str, &str>, current_dir: &str) -> Result<(), String> {
check_fmt_base("--all", env, current_dir)
}

// Checks that:
//
// * `cargo fmt --all` succeeds without any warnings or errors
// * `cargo fmt --all -- --check` after formatting returns success
// * `cargo test --lib` still passes (formatting did not break the build)
fn check_fmt_with_lib_tests(env: HashMap<&str, &str>, current_dir: &str) -> Result<(), String> {
check_fmt_base("--lib", env, current_dir)
}

fn check_fmt_base(
test_args: &str,
env: HashMap<&str, &str>,
current_dir: &str,
) -> Result<(), String> {
fn check_output_does_not_contain(output: &str, needle: &str) -> Result<(), String> {
if output.contains(needle) {
Err(format!("`cargo fmt --all -v` contains `{needle}`"))
} else {
Ok(())
}
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@jieyouxu jieyouxu Mar 10, 2026

Choose a reason for hiding this comment

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

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

This comment was marked as resolved.

.iter()
.any(|needle| output.contains(needle))
{
println!("`cargo test {test_args}` failed: {output}");
return Ok(());
}

let rustfmt_toml = Path::new(current_dir).join("rustfmt.toml");
if !rustfmt_toml.is_file() {
write_file(rustfmt_toml, "")?;
}

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

Choose a reason for hiding this comment

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

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


if !output.exited_successfully {
return Err("`cargo fmt --all -v` failed".to_string());
}

let output = &output.output;
check_output_does_not_contain(output, "internal error")?;
check_output_does_not_contain(output, "internal compiler error")?;
check_output_does_not_contain(output, "warning")?;
check_output_does_not_contain(output, "Warning")?;

let output = run_command_with_output_and_env(
"cargo",
&["fmt", "--all", "--", "--check"],
current_dir,
&env,
)?;

if !output.exited_successfully {
return Err("`cargo fmt --all -- -v --check` failed".to_string());
}
let output = &output.output;
if let Err(error) = write_file(Path::new(current_dir).join("rustfmt_check_output"), output) {
println!("{output}");
return Err(error);
}

// This command allows to ensure that no source file was modified while running the tests.
run_command_with_env("cargo", &["test", test_args], current_dir, &env)

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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

}

fn show_head(integration: &str) -> Result<(), String> {
let head = run_command_with_output("git", &["rev-parse", "HEAD"], integration)?.output;
println!("Head commit of {integration}: {head}");
Ok(())
}

fn run_test<F: FnOnce(HashMap<&str, &str>, &str) -> Result<(), String>>(
integration: &str,
git_repository: String,
env: HashMap<&str, &str>,
test_fn: F,
) -> Result<(), String> {
run_command_with_output("git", &["clone", "--depth=1", git_repository.as_str()], ".")?;
show_head(integration)?;
test_fn(env, integration)
}

fn runner() -> Result<(), String> {
let integration = match var("INTEGRATION") {
Ok(value) if !value.is_empty() => value,
_ => {
return Err("The INTEGRATION environment variable must be set.".into());
}
};

run_command_with_env(
"cargo",
&["install", "--path", ".", "--force", "--locked"],
".",
&HashMap::from([
("CFG_RELEASE", "nightly"),
("CFG_RELEASE_CHANNEL", "nightly"),
]),
)?;

println!("Integration tests for {integration}");

run_command("cargo", &["fmt", "--", "--version"], ".")?;

match integration.as_str() {
"cargo" => run_test(
&integration,
format!("https://github.com/rust-lang/{integration}.git"),
HashMap::from([("CFG_DISABLE_CROSS_TESTS", "1")]),
check_fmt_with_all_tests,
),
"crater" => run_test(
&integration,
format!("https://github.com/rust-lang/{integration}.git"),
HashMap::new(),
check_fmt_with_lib_tests,
),
"bitflags" => run_test(
&integration,
format!("https://github.com/bitflags/{integration}.git"),
HashMap::new(),
check_fmt_with_all_tests,
),
"tempdir" => run_test(
&integration,
format!("https://github.com/rust-lang-deprecated/{integration}.git"),
HashMap::new(),
check_fmt_with_all_tests,
),
_ => run_test(
&integration,
format!("https://github.com/rust-lang/{integration}.git"),
HashMap::new(),
check_fmt_with_all_tests,
),
}
}

fn main() {
if let Err(error) = runner() {
eprintln!("{error}");
std::process::exit(1);
}
}
121 changes: 0 additions & 121 deletions ci/integration.sh

This file was deleted.

Loading