From 73238301f452a801fc66b63eaa8fb463bfbac0d1 Mon Sep 17 00:00:00 2001 From: Orion Gonzalez Date: Thu, 3 Oct 2024 03:15:32 +0200 Subject: [PATCH 1/5] add an option for a custom differ --- src/tools/compiletest/src/common.rs | 2 ++ src/tools/compiletest/src/lib.rs | 1 + 2 files changed, 3 insertions(+) diff --git a/src/tools/compiletest/src/common.rs b/src/tools/compiletest/src/common.rs index ff059940f7ccd..5e50e227dede0 100644 --- a/src/tools/compiletest/src/common.rs +++ b/src/tools/compiletest/src/common.rs @@ -387,6 +387,8 @@ 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, + + pub diff_command: Option, } impl Config { diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index 7d6ede9bcdac5..5046eea63e253 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -364,6 +364,7 @@ pub fn parse_config(args: Vec) -> Config { git_merge_commit_email: matches.opt_str("git-merge-commit-email").unwrap(), profiler_runtime: matches.opt_present("profiler-runtime"), + diff_command: env::var("COMPILETEST_DIFF_TOOL").ok(), } } From ef4325eb85766f15916520ee8f6150e93cb45064 Mon Sep 17 00:00:00 2001 From: Orion Gonzalez Date: Thu, 3 Oct 2024 03:21:45 +0200 Subject: [PATCH 2/5] implemented custom differ --- config.example.toml | 3 + src/bootstrap/src/core/build_steps/test.rs | 3 + src/bootstrap/src/core/config/config.rs | 6 ++ src/tools/compiletest/src/lib.rs | 6 +- src/tools/compiletest/src/runtest.rs | 84 ++++++++++++---------- 5 files changed, 63 insertions(+), 39 deletions(-) diff --git a/config.example.toml b/config.example.toml index 168ac353cff75..b155e4731b062 100644 --- a/config.example.toml +++ b/config.example.toml @@ -419,6 +419,9 @@ # passed to cargo invocations. #jobs = 0 +# What custom diff tool to use for displaying compiletest tests. +#display-diff-tool = "difft --color=always --background=light --display=side-by-side" + # ============================================================================= # General install configuration options # ============================================================================= diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index e25b571acbac5..c6c40b0f7f7d3 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -1834,6 +1834,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(display_diff_tool) = &builder.config.display_diff_tool { + cmd.arg("--display-diff-tool").arg(display_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)); diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index aeb81b146382e..ba2ec8181f998 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/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, + + /// What custom diff tool to use for displaying compiletest tests. + pub display_diff_tool: Option, } #[derive(Clone, Debug, Default)] @@ -892,6 +895,7 @@ define_config! { android_ndk: Option = "android-ndk", optimized_compiler_builtins: Option = "optimized-compiler-builtins", jobs: Option = "jobs", + display_diff_tool: Option = "display-diff-tool", } } @@ -1512,6 +1516,7 @@ impl Config { android_ndk, optimized_compiler_builtins, jobs, + display_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.display_diff_tool = display_diff_tool; let download_rustc = config.download_rustc_commit.is_some(); // See https://github.com/rust-lang/compiler-team/issues/326 diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index 5046eea63e253..debc59109e746 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -175,7 +175,9 @@ pub fn parse_config(args: Vec) -> Config { "git-merge-commit-email", "email address used for finding merge commits", "EMAIL", - ); + ) + .optopt("", "display-diff-tool", "What custom diff tool to use for displaying compiletest tests.", "COMMAND") + ; let (argv0, args_) = args.split_first().unwrap(); if args.len() == 1 || args[1] == "-h" || args[1] == "--help" { @@ -364,7 +366,7 @@ pub fn parse_config(args: Vec) -> Config { git_merge_commit_email: matches.opt_str("git-merge-commit-email").unwrap(), profiler_runtime: matches.opt_present("profiler-runtime"), - diff_command: env::var("COMPILETEST_DIFF_TOOL").ok(), + diff_command: matches.opt_str("display-diff-tool"), } } diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 36c5106ddad03..4aca3b34313db 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/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_owned(output.stdout).unwrap(); + 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 } } From 37ffb94093828676d88178063c8930ea83a29386 Mon Sep 17 00:00:00 2001 From: Orion Gonzalez Date: Thu, 3 Oct 2024 17:11:01 +0200 Subject: [PATCH 3/5] update CONFIG_CHANGE_HISTORY --- src/bootstrap/src/utils/change_tracker.rs | 5 +++++ src/tools/compiletest/src/lib.rs | 8 ++++++-- src/tools/compiletest/src/runtest.rs | 2 +- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/bootstrap/src/utils/change_tracker.rs b/src/bootstrap/src/utils/change_tracker.rs index 9169bc90a45dc..fac24572a878c 100644 --- a/src/bootstrap/src/utils/change_tracker.rs +++ b/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", + }, ]; diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index debc59109e746..a008410e34bd9 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -176,8 +176,12 @@ pub fn parse_config(args: Vec) -> Config { "email address used for finding merge commits", "EMAIL", ) - .optopt("", "display-diff-tool", "What custom diff tool to use for displaying compiletest tests.", "COMMAND") - ; + .optopt( + "", + "display-diff-tool", + "What custom diff tool to use for displaying compiletest tests.", + "COMMAND", + ); let (argv0, args_) = args.split_first().unwrap(); if args.len() == 1 || args[1] == "-h" || args[1] == "--help" { diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 4aca3b34313db..7db37af60d2c5 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -2534,7 +2534,7 @@ impl<'test> TestCx<'test> { )); } Ok(output) => { - let output = String::from_utf8_lossy_owned(output.stdout).unwrap(); + let output = String::from_utf8_lossy(&output.stdout); print!("{output}"); } } From c8de61b50de7ca755b704c0314ff3a119be01ae6 Mon Sep 17 00:00:00 2001 From: Orion Gonzalez Date: Fri, 4 Oct 2024 10:54:34 +0200 Subject: [PATCH 4/5] s/display-diff-tool/compiletest-diff-tool/ --- config.example.toml | 2 +- src/bootstrap/src/core/build_steps/test.rs | 4 ++-- src/bootstrap/src/core/config/config.rs | 10 +++++----- src/tools/compiletest/src/common.rs | 1 + src/tools/compiletest/src/lib.rs | 4 ++-- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/config.example.toml b/config.example.toml index b155e4731b062..77efdb6d392e5 100644 --- a/config.example.toml +++ b/config.example.toml @@ -420,7 +420,7 @@ #jobs = 0 # What custom diff tool to use for displaying compiletest tests. -#display-diff-tool = "difft --color=always --background=light --display=side-by-side" +#compiletest-diff-tool = "difft --color=always --background=light --display=side-by-side" # ============================================================================= # General install configuration options diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index c6c40b0f7f7d3..f671b0dcfe6f8 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -1834,8 +1834,8 @@ 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(display_diff_tool) = &builder.config.display_diff_tool { - cmd.arg("--display-diff-tool").arg(display_diff_tool); + 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()] }; diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index ba2ec8181f998..ec3a0a6de8471 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -369,8 +369,8 @@ pub struct Config { /// `paths=["foo", "bar"]`. pub paths: Vec, - /// What custom diff tool to use for displaying compiletest tests. - pub display_diff_tool: Option, + /// Command for visual diff display, e.g. `diff-tool --color=always`. + pub compiletest_diff_tool: Option, } #[derive(Clone, Debug, Default)] @@ -895,7 +895,7 @@ define_config! { android_ndk: Option = "android-ndk", optimized_compiler_builtins: Option = "optimized-compiler-builtins", jobs: Option = "jobs", - display_diff_tool: Option = "display-diff-tool", + compiletest_diff_tool: Option = "compiletest-diff-tool", } } @@ -1516,7 +1516,7 @@ impl Config { android_ndk, optimized_compiler_builtins, jobs, - display_diff_tool, + compiletest_diff_tool, } = toml.build.unwrap_or_default(); config.jobs = Some(threads_from_config(flags.jobs.unwrap_or(jobs.unwrap_or(0)))); @@ -2163,7 +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.display_diff_tool = display_diff_tool; + 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 diff --git a/src/tools/compiletest/src/common.rs b/src/tools/compiletest/src/common.rs index 5e50e227dede0..5070f016d3c62 100644 --- a/src/tools/compiletest/src/common.rs +++ b/src/tools/compiletest/src/common.rs @@ -388,6 +388,7 @@ pub struct Config { /// 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, } diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index a008410e34bd9..0f514db42bf9f 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -178,7 +178,7 @@ pub fn parse_config(args: Vec) -> Config { ) .optopt( "", - "display-diff-tool", + "compiletest-diff-tool", "What custom diff tool to use for displaying compiletest tests.", "COMMAND", ); @@ -370,7 +370,7 @@ pub fn parse_config(args: Vec) -> 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("display-diff-tool"), + diff_command: matches.opt_str("compiletest-diff-tool"), } } From 28095bc54142f9fbfd4a2cac0a7fadf0ae6e9b9e Mon Sep 17 00:00:00 2001 From: Orion Gonzalez Date: Fri, 4 Oct 2024 10:56:08 +0200 Subject: [PATCH 5/5] real default value --- config.example.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.example.toml b/config.example.toml index 77efdb6d392e5..a52968e9a414d 100644 --- a/config.example.toml +++ b/config.example.toml @@ -420,7 +420,7 @@ #jobs = 0 # What custom diff tool to use for displaying compiletest tests. -#compiletest-diff-tool = "difft --color=always --background=light --display=side-by-side" +#compiletest-diff-tool = # ============================================================================= # General install configuration options