fix: Run test fixture scripts on Windows with Git Bash

EliahKagan · EliahKagan · commit 14fd0c40b439 · 2024-12-01T07:01:44.000-05:00
Rather than hard-coding `bash` on all systems as the fallback interpreter when a fixture script cannot be run directly, this falls back in an operating system specific manner: - Except on Windows, always fall back to `bash`, as before. - On Windows, run `git --exec-path` to find the `git-core` directory. Then check if a `bash.exe` exists at the expected location relative to that. In Git for Windows installations, this will usually work. If so, use that path (with `..` components resolved away). - On Windows, if a specific `bash.exe` is not found in that way, then fall back to using the relative path `bash.exe`. This is to preserve the ability to run `bash` on Windows systems where it may have worked before even without `bash.exe` in an expected location provided by a Git for Windows installation. (The distinction between `bash` and `bash.exe` is only slightly significant: we check for the existence of the interpreter without initially running it, and that check requires the full filename. It is called `bash.exe` elsewhere for consistency both with the checked-for executable and for consistencey with how we run most other programs on Windows, e.g., the `git` vs. `git.exe`.) This fixes GitoxideLabs#1359. That bug is not currently observed on CI, but this change is verified to fix it on a local test system where it previously always occurred when running the test suite from PowerShell in an unmodified environment. The fix applies both with `GIX_TEST_IGNORE_ARCHIVES` unset, in which case there are now no failures, and with `GIX_TEST_IGNORE_ARCHIVES=1`, in which case the failures are now limited to the 15 cases tracked in GitoxideLabs#1358. Previously, fixture scripts had been run on Windows with whatever `bash` was found in a `PATH` search, which had two problems: - On most Windows systems, even if no WSL distribution is installed and even if WSL itself is not set up, the `System32` directory contains a `bash.exe` program associated with WSL. This program attempts to use WSL to run `bash` in an installed distribution. The `wsl.exe` program also provides this functionality and is favored for this purpose, but the `bash.exe` program is still present and is likely to remain for many years for compatibility. Even when this `bash` is usable, it is not suited for running most shell scripts meant to operate on the native Windows system. In particular, it is not suitable for running our fixture scripts, which need to use the native `git` to prepare fixtures to be used natively, among other requirements that would not be satisfied with WSL (except when the tests are actually running in WSL). Since some fixtures are `.gitignore`d because creating them on the test system (rather than another system) is part of the test, this has caused breakage in most Windows environments unless `PATH` is modified -- either explicitly or by testing in an MSYS2 environment, such as the Git Bash environment -- whether or not `GIX_TEST_IGNORE_ARCHIVES` is set. This was the cause of GitoxideLabs#1359. - Although using a Git Bash environment or otherwise adjusting the path *currently* works, the reasons it works are subtle and rely on non-guaranteed behavior of `std::process::Command` path search that may change without warning. On Windows, processes are created by calling the `CreateProcessW` API function. `CreateProcessW` is capable of performing a `PATH` search, but this `PATH` search is not secure in most uses, since it includes the current directory (and searches it before `PATH` directories) unless `NoDefaultCurrentDirectoryInExePath` is set in the caller's environment. While it is the most relevant to security, the CWD is not the only location `CreateProcessW` searches before searching `PATH` directories (and regardless of where, if anywhere, they may also appear in `PATH`). Another such location is the `System32` directory. This is to say that, even when another directory with `bash.exe` precedes `System32` in `PATH`, an executable search will still find the WSL-associated `bash.exe` in `System32` unless it deviates from the algorithm `CreateProcessW` uses. To avoid including the CWD in the search, `std::process::Command` performs its own path search, then passes the resolved path to `CreateProcessW`. The path search it performs is currently almost the same the algorithm `CreateProcessW` uses, other than not automatically including the CWD. But there are some other subtle differences. One such difference is that, when the `Command` instance is configured to create a modified child environment (for example, by `env` calls), the `PATH` for the child is searched early on. This precedes a search of the `System32` directory. It is done even if none of the customizations of the child environment modify its `PATH`. This behavior is not guaranteed, and it may change at any time. It is also the behavior we rely on inadvertently every time we run `bash` on Windows with a `std::process::Command` instance constructed by passing `bash` or `bash.exe` as the `program` argument: it so happens that we are also customizing the child environment, and due to implementation details in the Rust standard library, this manages to find a non-WSL `bash` when the tests are run in Git Bash, in GitHub Actions jobs, and in some other cases. If in the future this is not done, or narrowed to be done only when `PATH` is one of the environment variables customized for the child process, then putting the directory with the desired `bash.exe` earlier than the `System32` directory in `PATH` will no longer prevent `std::proces::Command` from finding the `bash.exe` in `System32` as `CreateProcessW` would and using it. Then it would be nontrivial to run the test suite on Windows. For references and other details, see GitoxideLabs#1359 and comments including: GitoxideLabs#1359 (comment) On the approach of finding the Git for Windows `bash.exe` relative to the `git-core` directory, see the GitPython pull request gitpython-developers/GitPython#1791. Two possible future enhancements are *not* included in this commit: 1. This only modifies how test fixture scripts are run. It only affects the behavior of `gix-testtools`, and not of any other gitoxide crates such as `gix-command`. This is because: - While gitoxide uses information from `git` to find out where it is installed, mainly so we know where to find installation level configuration, we cannot in assume that `git` is present at all. Unlike GitPython, gitoxide is usable without `git`. - We know our test fixture scripts are all (at least currently) `bash` scripts, and this seems likely for other software that currently uses this functionality of `gix-testtools`. But scripts that are run as hooks, or as custom commands, or filters, etc., are often written in other languages, such as Perl. (The fallback here does not examine leading `#!` lines.) - Although a `bash.exe` located at the usual place relative to (but outside of) the `git-core` directory is usually suitable, there may be scenarios where running an executable found this way is not safe. Limiting it to `gix-testtools` pending further research may help mitigate this risk. 2. As in other runs of `git` by `gix-testools`, this calls `git.exe`, letting `std::process::Command` do an executable search, but not trying any additional locations where Git is known sometimes to be installed. This does not find `git.exe` in as many situations as `gix_path::env::exe_invocation` does. The reasons for not (or not quite yet) including that change are: - It would add `gix-path` as a dependency of `gix-testtools`. - Finding `git` in a `std::process::Command` path search is an established (though not promised) approach in `gix-testtools`, including to run `git --exec-path` (to find `git-daemon`). - It is not immediately obvious that `exe_invocation` behavior is semantically correct for `gix-testtools`, though it most likely is reasonable. The main issue is that, in many cases where `git` itself runs scripts, it prepends the path to the `git-core` directory to the `PATH` environment variable for the script. This directory has a `git` (or `git.exe`) executable in it, so scripts run an equivalent `git` associated with the same installation. In contrast, when we run test fixture scripts with a `bash.exe` associated with a Git for Windows installation, we do not customize its path. Since top-level scripts written to use `git` but not to be used *by* `git` are usually written without the expectation of such an environment, prepending this will not necessarily be an improvement.
diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs
@@ -93,6 +93,23 @@ const GIT_PROGRAM: &str = "git.exe";
 #[cfg(not(windows))]
 const GIT_PROGRAM: &str = "git";
 
+static GIT_CORE_DIR: Lazy<PathBuf> = Lazy::new(|| {
+    let output = std::process::Command::new(GIT_PROGRAM)
+        .arg("--exec-path")
+        .output()
+        .expect("can execute `git --exec-path`");
+
+    assert!(output.status.success(), "`git --exec-path` failed");
+
+    output
+        .stdout
+        .strip_suffix(b"\n")
+        .expect("malformed output from `git --exec-path`")
+        .to_os_str()
+        .expect("no invalid UTF-8 in `--exec-path` except as OS allows")
+        .into()
+});
+
 /// The major, minor and patch level of the git version on the system.
 pub static GIT_VERSION: Lazy<(u8, u8, u8)> = Lazy::new(|| parse_git_version().expect("git version to be parsable"));
 
@@ -186,22 +203,6 @@ pub fn run_git(working_dir: &Path, args: &[&str]) -> std::io::Result<std::proces
 
 /// Spawn a git daemon process to host all repository at or below `working_dir`.
 pub fn spawn_git_daemon(working_dir: impl AsRef<Path>) -> std::io::Result<GitDaemon> {
-    static EXEC_PATH: Lazy<PathBuf> = Lazy::new(|| {
-        let output = std::process::Command::new(GIT_PROGRAM)
-            .arg("--exec-path")
-            .output()
-            .expect("can execute `git --exec-path`");
-
-        assert!(output.status.success(), "`git --exec-path` failed");
-
-        output
-            .stdout
-            .strip_suffix(b"\n")
-            .expect("malformed output from `git --exec-path`")
-            .to_os_str()
-            .expect("no invalid UTF-8 in `--exec-path` except as OS allows")
-            .into()
-    });
     let mut ports: Vec<_> = (9419u16..9419 + 100).collect();
     fastrand::shuffle(&mut ports);
     let addr_at = |port| std::net::SocketAddr::from(([127, 0, 0, 1], port));
@@ -210,11 +211,12 @@ pub fn spawn_git_daemon(working_dir: impl AsRef<Path>) -> std::io::Result<GitDae
         listener.local_addr().expect("listener address is available").port()
     };
 
-    let child = std::process::Command::new(EXEC_PATH.join(if cfg!(windows) { "git-daemon.exe" } else { "git-daemon" }))
-        .current_dir(working_dir)
-        .args(["--verbose", "--base-path=.", "--export-all", "--user-path"])
-        .arg(format!("--port={free_port}"))
-        .spawn()?;
+    let child =
+        std::process::Command::new(GIT_CORE_DIR.join(if cfg!(windows) { "git-daemon.exe" } else { "git-daemon" }))
+            .current_dir(working_dir)
+            .args(["--verbose", "--base-path=.", "--export-all", "--user-path"])
+            .arg(format!("--port={free_port}"))
+            .spawn()?;
 
     let server_addr = addr_at(free_port);
     for time in gix_lock::backoff::Exponential::default_with_random() {
@@ -566,7 +568,7 @@ fn scripted_fixture_read_only_with_args_inner(
                     Err(err)
                         if err.kind() == std::io::ErrorKind::PermissionDenied || err.raw_os_error() == Some(193) /* windows */ =>
                     {
-                        cmd = std::process::Command::new("bash");
+                        cmd = std::process::Command::new(bash_program());
                         configure_command(cmd.arg(script_absolute_path), &args, &script_result_directory).output()?
                     }
                     Err(err) => return Err(err.into()),
@@ -642,6 +644,22 @@ fn configure_command<'a, I: IntoIterator<Item = S>, S: AsRef<OsStr>>(
         .env("GIT_CONFIG_VALUE_3", "always")
 }
 
+fn bash_program() -> &'static Path {
+    if cfg!(windows) {
+        static GIT_BASH: Lazy<Option<PathBuf>> = Lazy::new(|| {
+            GIT_CORE_DIR
+                .parent()?
+                .parent()?
+                .parent()
+                .map(|installation_dir| installation_dir.join("bin").join("bash.exe"))
+                .filter(|bash| bash.is_file())
+        });
+        GIT_BASH.as_deref().unwrap_or(Path::new("bash.exe"))
+    } else {
+        Path::new("bash")
+    }
+}
+
 fn write_failure_marker(failure_marker: &Path) {
     std::fs::write(failure_marker, []).ok();
 }
@@ -981,4 +999,43 @@ 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]
+    #[cfg(windows)]
+    fn bash_program_ok_for_platform() {
+        let path = bash_program();
+        assert!(path.is_absolute());
+
+        let for_version = std::process::Command::new(path)
+            .arg("--version")
+            .output()
+            .expect("can pass it `--version`");
+        assert!(for_version.status.success(), "passing `--version` succeeds");
+        let version_line = for_version
+            .stdout
+            .lines()
+            .nth(0)
+            .expect("`--version` output has first line");
+        assert!(
+            version_line.ends_with(b"-pc-msys)"), // On Windows, "-pc-linux-gnu)" would be WSL.
+            "it is an MSYS bash (such as Git Bash)"
+        );
+
+        let for_uname_os = std::process::Command::new(path)
+            .args(["-c", "uname -o"])
+            .output()
+            .expect("can tell it to run `uname -o`");
+        assert!(for_uname_os.status.success(), "telling it to run `uname -o` succeeds");
+        assert_eq!(
+            for_uname_os.stdout.trim_end(),
+            b"Msys",
+            "it runs commands in an MSYS environment"
+        );
+    }
+
+    #[test]
+    #[cfg(not(windows))]
+    fn bash_program_ok_for_platform() {
+        assert_eq!(bash_program(), Path::new("bash"));
+    }
 }
