Rollup merge of #131181 - dev-ardi:custom-differ, r=jieyouxu

Compiletest: Custom differ

This adds support for a custom differ for compiletests. It’s purely visual and helps produce cleaner output when UI tests fail.

I’m using an environment variable for now since it’s experimental and I don’t want to drill the cli arguments all the way down. Also did a bit of general cleanup while I was at it.

This is how it looks [with debug info silenced](#131182) (#131182)
`COMPILETEST_DIFF_TOOL="/usr/bin/env difft --color always --background light --display side-by-side" ./x test tests/ui/parser`
![image](https://github.com/user-attachments/assets/f740ce50-7564-4469-be0a-86e24bc50eb8)

config.example.toml

@@ -419,6 +419,9 @@
 # passed to cargo invocations.
 #jobs = 0
 
+# What custom diff tool to use for displaying compiletest tests.
+#compiletest-diff-tool = <none>
+
 # =============================================================================
 # General install configuration options
 # =============================================================================

src/bootstrap/src/core/build_steps/test.rs

@@ -1836,6 +1836,9 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
         if builder.config.cmd.only_modified() {
             cmd.arg("--only-modified");
         }
+        if let Some(compiletest_diff_tool) = &builder.config.compiletest_diff_tool {
+            cmd.arg("--compiletest-diff-tool").arg(compiletest_diff_tool);
+        }
 
         let mut flags = if is_rustdoc { Vec::new() } else { vec!["-Crpath".to_string()] };
         flags.push(format!("-Cdebuginfo={}", builder.config.rust_debuginfo_level_tests));

src/bootstrap/src/core/config/config.rs

@@ -368,6 +368,9 @@ pub struct Config {
     /// The paths to work with. For example: with `./x check foo bar` we get
     /// `paths=["foo", "bar"]`.
     pub paths: Vec<PathBuf>,
+
+    /// Command for visual diff display, e.g. `diff-tool --color=always`.
+    pub compiletest_diff_tool: Option<String>,
 }
 
 #[derive(Clone, Debug, Default)]
@@ -892,6 +895,7 @@ define_config! {
         android_ndk: Option<PathBuf> = "android-ndk",
         optimized_compiler_builtins: Option<bool> = "optimized-compiler-builtins",
         jobs: Option<u32> = "jobs",
+        compiletest_diff_tool: Option<String> = "compiletest-diff-tool",
     }
 }
 
@@ -1512,6 +1516,7 @@ impl Config {
             android_ndk,
             optimized_compiler_builtins,
             jobs,
+            compiletest_diff_tool,
         } = toml.build.unwrap_or_default();
 
         config.jobs = Some(threads_from_config(flags.jobs.unwrap_or(jobs.unwrap_or(0))));
@@ -2158,6 +2163,7 @@ impl Config {
         config.rust_debuginfo_level_tests = debuginfo_level_tests.unwrap_or(DebuginfoLevel::None);
         config.optimized_compiler_builtins =
             optimized_compiler_builtins.unwrap_or(config.channel != "dev");
+        config.compiletest_diff_tool = compiletest_diff_tool;
 
         let download_rustc = config.download_rustc_commit.is_some();
         // See https://github.com/rust-lang/compiler-team/issues/326

src/bootstrap/src/utils/change_tracker.rs

@@ -280,4 +280,9 @@ pub const CONFIG_CHANGE_HISTORY: &[ChangeInfo] = &[
         severity: ChangeSeverity::Info,
         summary: "Allow setting `--jobs` in config.toml with `build.jobs`.",
     },
+    ChangeInfo {
+        change_id: 131181,
+        severity: ChangeSeverity::Info,
+        summary: "New option `build.compiletest-diff-tool` that adds support for a custom differ for compiletest",
+    },
 ];

src/tools/compiletest/src/common.rs

@@ -387,6 +387,9 @@ pub struct Config {
     /// True if the profiler runtime is enabled for this target.
     /// Used by the "needs-profiler-runtime" directive in test files.
     pub profiler_runtime: bool,
+
+    /// Command for visual diff display, e.g. `diff-tool --color=always`.
+    pub diff_command: Option<String>,
 }
 
 impl Config {

src/tools/compiletest/src/lib.rs

@@ -175,6 +175,12 @@ pub fn parse_config(args: Vec<String>) -> Config {
             "git-merge-commit-email",
             "email address used for finding merge commits",
             "EMAIL",
+        )
+        .optopt(
+            "",
+            "compiletest-diff-tool",
+            "What custom diff tool to use for displaying compiletest tests.",
+            "COMMAND",
         );
 
     let (argv0, args_) = args.split_first().unwrap();
@@ -364,6 +370,7 @@ pub fn parse_config(args: Vec<String>) -> Config {
         git_merge_commit_email: matches.opt_str("git-merge-commit-email").unwrap(),
 
         profiler_runtime: matches.opt_present("profiler-runtime"),
+        diff_command: matches.opt_str("compiletest-diff-tool"),
     }
 }
 

src/tools/compiletest/src/runtest.rs

@@ -2459,7 +2459,7 @@ impl<'test> TestCx<'test> {
         }
     }
 
-    fn compare_output(&self, kind: &str, actual: &str, expected: &str) -> usize {
+    fn compare_output(&self, stream: &str, actual: &str, expected: &str) -> usize {
         let are_different = match (self.force_color_svg(), expected.find('\n'), actual.find('\n')) {
             // FIXME: We ignore the first line of SVG files
             // because the width parameter is non-deterministic.
@@ -2499,56 +2499,66 @@ impl<'test> TestCx<'test> {
             (expected, actual)
         };
 
+        // Write the actual output to a file in build/
+        let test_name = self.config.compare_mode.as_ref().map_or("", |m| m.to_str());
+        let actual_path = self
+            .output_base_name()
+            .with_extra_extension(self.revision.unwrap_or(""))
+            .with_extra_extension(test_name)
+            .with_extra_extension(stream);
+
+        if let Err(err) = fs::write(&actual_path, &actual) {
+            self.fatal(&format!("failed to write {stream} to `{actual_path:?}`: {err}",));
+        }
+        println!("Saved the actual {stream} to {actual_path:?}");
+
+        let expected_path =
+            expected_output_path(self.testpaths, self.revision, &self.config.compare_mode, stream);
+
         if !self.config.bless {
             if expected.is_empty() {
-                println!("normalized {}:\n{}\n", kind, actual);
+                println!("normalized {}:\n{}\n", stream, actual);
             } else {
-                println!("diff of {}:\n", kind);
-                print!("{}", write_diff(expected, actual, 3));
+                println!("diff of {stream}:\n");
+                if let Some(diff_command) = self.config.diff_command.as_deref() {
+                    let mut args = diff_command.split_whitespace();
+                    let name = args.next().unwrap();
+                    match Command::new(name)
+                        .args(args)
+                        .args([&expected_path, &actual_path])
+                        .output()
+                    {
+                        Err(err) => {
+                            self.fatal(&format!(
+                                "failed to call custom diff command `{diff_command}`: {err}"
+                            ));
+                        }
+                        Ok(output) => {
+                            let output = String::from_utf8_lossy(&output.stdout);
+                            print!("{output}");
+                        }
+                    }
+                } else {
+                    print!("{}", write_diff(expected, actual, 3));
+                }
             }
-        }
-
-        let mode = self.config.compare_mode.as_ref().map_or("", |m| m.to_str());
-        let output_file = self
-            .output_base_name()
-            .with_extra_extension(self.revision.unwrap_or(""))
-            .with_extra_extension(mode)
-            .with_extra_extension(kind);
-
-        let mut files = vec![output_file];
-        if self.config.bless {
+        } else {
             // Delete non-revision .stderr/.stdout file if revisions are used.
             // Without this, we'd just generate the new files and leave the old files around.
             if self.revision.is_some() {
                 let old =
-                    expected_output_path(self.testpaths, None, &self.config.compare_mode, kind);
+                    expected_output_path(self.testpaths, None, &self.config.compare_mode, stream);
                 self.delete_file(&old);
             }
-            files.push(expected_output_path(
-                self.testpaths,
-                self.revision,
-                &self.config.compare_mode,
-                kind,
-            ));
-        }
 
-        for output_file in &files {
-            if actual.is_empty() {
-                self.delete_file(output_file);
-            } else if let Err(err) = fs::write(&output_file, &actual) {
-                self.fatal(&format!(
-                    "failed to write {} to `{}`: {}",
-                    kind,
-                    output_file.display(),
-                    err,
-                ));
+            if let Err(err) = fs::write(&expected_path, &actual) {
+                self.fatal(&format!("failed to write {stream} to `{expected_path:?}`: {err}"));
             }
+            println!("Blessing the {stream} of {test_name} in {expected_path:?}");
         }
 
-        println!("\nThe actual {0} differed from the expected {0}.", kind);
-        for output_file in files {
-            println!("Actual {} saved to {}", kind, output_file.display());
-        }
+        println!("\nThe actual {0} differed from the expected {0}.", stream);
+
         if self.config.bless { 0 } else { 1 }
     }