From fbd4908d8506254e3901bdce66e474ca08ff230b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 1 Sep 2024 04:49:03 -0400 Subject: [PATCH 1/3] Start testing how the MSYS env var is customized --- tests/tools/src/lib.rs | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index b3c0102eab7..7b1ade1521d 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -872,6 +872,7 @@ impl<'a> Drop for Env<'a> { #[cfg(test)] mod tests { use super::*; + use std::ffi::OsStr; #[test] fn parse_version() { @@ -932,4 +933,44 @@ mod tests { assert_eq!(lines, Vec::<&str>::new(), "should be no config variables from files"); assert_eq!(status, 0, "reading the config should succeed"); } + + #[test] + fn configure_command_msys() { + let temp = tempfile::TempDir::new().expect("can create temp dir"); + let mut cmd = std::process::Command::new("not-actually-run"); + + let msys_option = configure_command(&mut cmd, &[], temp.path()) + .get_envs() + .find(|(k, _)| *k == OsStr::new("MSYS")) + .expect("should customize MSYS variable") + .1 + .expect("should set, not unset, MSYS variable") + .to_str() + .expect("valid UTF-8") + .trim_matches(' '); + + assert_eq!(msys_option, "winsymlinks:nativestrict"); + } + + #[test] + fn configure_command_msys_extends() { + let old_msys = r"error_start:C:\gdb.exe"; + let temp = tempfile::TempDir::new().expect("can create temp dir"); + let mut cmd = std::process::Command::new("not-actually-run"); + cmd.env("MSYS", old_msys); + + let msys_options = configure_command(&mut cmd, &[], temp.path()) + .get_envs() + .find(|(k, _)| *k == OsStr::new("MSYS")) + .expect("should customize MSYS variable") + .1 + .expect("should set, not unset, MSYS variable") + .to_str() + .expect("valid UTF-8") + .split(' ') // Just spaces, not arbitrary whitespace. + .filter(|s| !s.is_empty()) + .collect::>(); + + assert_eq!(msys_options, vec![old_msys, "winsymlinks:nativestrict"]); + } } From 8dc5d7aa736059aa45a17dfdc76d9d4c9993f996 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 5 Sep 2024 15:25:34 -0400 Subject: [PATCH 2/3] fix: Append to preexisting `MSYS` env var even if ill-formed The value of an environment variable as obtained by the facilities in `std::env` is not always well-formed Unicode. Specifically, on Windows the values of environment variables, like paths, are natively UTF-16LE strings except that unpaired surrogate code points can also occur. An `&OsStr` on Windows may accordingly not quite be UTF-8. When the `MSYS` variable is absent, we treat this the same as when it is present but empty. However, as described in #1574, an `MSYS` variable that is present but whose value contains an unpaired surrogate would also be replaced entirely, rather than appending to its old value. This changes that, to instead append, retaining whatever was there even if it was ill-formed Unicode. An alternative change could be to panic when the old value is ill-formed Unicode. This commit allows and appends to the old value, rather than panicking or keeping and documenting the previous behavior of discarding the old value, because the appended sequence ` winsymlinks:nativestrict` is effective at causing fixture scripts to attempt to create actual symlinks even if the preceding code point is an unpaired Unicode high surrogate. --- tests/tools/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 7b1ade1521d..13413be72d6 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -593,8 +593,8 @@ fn configure_command<'a>( args: &[String], script_result_directory: &Path, ) -> &'a mut std::process::Command { - let mut msys_for_git_bash_on_windows = std::env::var("MSYS").unwrap_or_default(); - msys_for_git_bash_on_windows.push_str(" winsymlinks:nativestrict"); + let mut msys_for_git_bash_on_windows = std::env::var_os("MSYS").unwrap_or_default(); + msys_for_git_bash_on_windows.push(" winsymlinks:nativestrict"); cmd.args(args) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) From c38d9b9f666cfd0858e46c0f600f4bc17259bb85 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 5 Sep 2024 20:11:19 -0400 Subject: [PATCH 3/3] Remove `configure_command_msys*` tests at least for now Because they are not really testing the right thing, due to the distinction between inherited and `.env()`-specified environment. See #1578. While for `configure_command_clears_external_config` this slightly diminishes but largely leaves intact the value of the test, the `configure_command_msys` and `configure_command_msys_extends` tests are really verifying almost nothing, and the latter (for this reason) cannot pass. A variant of it for testing the preceding fix to append to arbitrary environment variable values would likewise not be able to pass. So that is not added at this time. Because the `configure_command_clears_external_config` test remains useful, it is not removed. --- tests/tools/src/lib.rs | 41 ----------------------------------------- 1 file changed, 41 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 13413be72d6..4af4ed46d9a 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -872,7 +872,6 @@ impl<'a> Drop for Env<'a> { #[cfg(test)] mod tests { use super::*; - use std::ffi::OsStr; #[test] fn parse_version() { @@ -933,44 +932,4 @@ mod tests { assert_eq!(lines, Vec::<&str>::new(), "should be no config variables from files"); assert_eq!(status, 0, "reading the config should succeed"); } - - #[test] - fn configure_command_msys() { - let temp = tempfile::TempDir::new().expect("can create temp dir"); - let mut cmd = std::process::Command::new("not-actually-run"); - - let msys_option = configure_command(&mut cmd, &[], temp.path()) - .get_envs() - .find(|(k, _)| *k == OsStr::new("MSYS")) - .expect("should customize MSYS variable") - .1 - .expect("should set, not unset, MSYS variable") - .to_str() - .expect("valid UTF-8") - .trim_matches(' '); - - assert_eq!(msys_option, "winsymlinks:nativestrict"); - } - - #[test] - fn configure_command_msys_extends() { - let old_msys = r"error_start:C:\gdb.exe"; - let temp = tempfile::TempDir::new().expect("can create temp dir"); - let mut cmd = std::process::Command::new("not-actually-run"); - cmd.env("MSYS", old_msys); - - let msys_options = configure_command(&mut cmd, &[], temp.path()) - .get_envs() - .find(|(k, _)| *k == OsStr::new("MSYS")) - .expect("should customize MSYS variable") - .1 - .expect("should set, not unset, MSYS variable") - .to_str() - .expect("valid UTF-8") - .split(' ') // Just spaces, not arbitrary whitespace. - .filter(|s| !s.is_empty()) - .collect::>(); - - assert_eq!(msys_options, vec![old_msys, "winsymlinks:nativestrict"]); - } }