Run test fixture scripts on Windows with Git Bash #1712

EliahKagan · 2024-12-01T14:29:07Z

Background

When executing a test fixture script directly fails, we use the fallback strategy of running it with bash. Running them directly always fails on Windows (at least in gitoxide's own test suite, where fixture scripts really are shell scripts, rather than binary executables or batch files). That's fine, since we fall back to running them with bash and (at least in gitoxide's own test suite) they are all bash scripts.

The problem, though, is that this does not always run the correct bash. This is analogous to the problem described in gitpython-developers/GitPython#1791, except that, due to the subtleties of how std::process::Command performs path search, there are, for the time being, readily available workarounds.

What this changes

This pull request fixes that problem in gix-testtools by using the Git Bash bash.exe interpreter associated with a Git for Windows installation, if that can be found. See the commit messages for substantial further details, especially fe3f2d1, which covers various design considerations.

A few other minor improvements are also included, when they directly helped with implementing this fix or with making it understandable; see the other commit messages for those.

Testing

This problem is not currently observed on CI because of specific details of the environment there and how it interacts with std::process::Command path search. But I have verified the fix locally on a Windows 10 machine where--as in most Windows 10 systems--attempting to run the test suite from PowerShell without modifying the PATH produces numerous failures, with or without the use of generated archives.

With the changes in this PR, there are no more failures that way than when running it in a Git Bash environment (which had worked around #1359).

Without GIX_TEST_IGNORE_ARCHIVES, there are zero failures now.
With GIX_TEST_IGNORE_ARCHIVES=1, there are only the 15 known failures documented in #1358.
Such a run, with the code as it is in this PR, is shown in full in this new gist.

I also included a test to verify that Bash is an MSYS-like build and that the environment it runs Unix commands in appears to be an MSYS-like environment.

Prior work and acknowledgement

The in-progress PR gitpython-developers/GitPython#1791 in GitPython, which is about finding the bash.exe associated with a Git for Windows installation to run hooks, is highly relevant.

I'd like to acknowledge @emanspeaks, who implemented the original git-executable-relative approach – to which I had proposed finding bash.exe relative to git-core as a refinement in gitpython-developers/GitPython#1791 (comment) – and who also implemented that refinement itself:

if os.name != "nt":
    return "bash"
gitcore = Path(cls()._call_process("--exec-path"))
gitroot = gitcore.parent.parent.parent
gitbash = gitroot / "bin" / "bash.exe"
return str(gitbash) if gitbash.exists() else "bash.exe"

Note the very similar logic that this PR subsequently implements:

gitoxide/tests/tools/src/lib.rs

Lines 648 to 660 in fe3f2d1

    
           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") 
        
           }

Where GIT_CORE_DIR is obtained from a call to git --exec-path.

Scope

This PR only modifies gix-testtools. Such an approach should possibly also be part of the logic for running scripts in gix-command but is initially implemented only for running fixture scripts in the test suite, as discussed in #1359 (comment) and #1359 (comment), as well as in the fe3f2d1 commit message.

Future directions

Two possible further changes and their relationship to this change are noted at the bottom of the fe3f2d1 commit message.

Before bringing this into other crates, I would be interested to examine whether it is compatible with installations of Git for Windows that differ from the usual layout. I believe the most common way such an installation arises is with MinGit.

The function checks the `git` version. It was accidentally renamed to `parse_gix_version` in 00286c9 along with other `git` -> `gix` changes for gitoxide crates that were renamed `git-*` to `gix-*`. This also makes the same correction to a local variable.

In the test tools, this always runs the `git` program as `git.exe` on Windows, while continuing always to run it as `git` on other systems. Prior to this change, on Windows `gix-testtools` used `git` in some operations and `git.exe` in others: - `parse_git_version` used `git.exe`. - Other functions used `git`. - For the git daemon, `git-daemon.exe` was used. For the way `gix-testtools` uses the `git` program, it would be fine to call it `git` on all platforms. For example, it does not form full paths to the executable that have to be found to exist in operations other than running it. (For running it, the `.exe` suffix is allowed to be omitted.) So it would probably be fine to use the even simpler logic of having it be `git` everywhere. But since `git.exe` was sometimes used, `git-daemon.exe` was used, and using `git.exe` is more similar to the behavior in `git-path`, it is changed to use `git.exe` when the platform is Windows. Because `gix-testtools` does not depend on `gix-path`, it doesn't use `gix_path::env::exe_invocation` to decide how to call `git`. That keeps it from finding `git` in some Windows environments (those where `git` is in a standard/predictable location but not in `PATH`). This change has no effect on that limitation.

This enforces requirements more precisely and, overall, more robustly, when validating and converting `git --exec-path` output to a `PathBuf`. This also no longer redirect standard error to the null device when running that command. No stderr output is expected; if there is any such output, it's better seen than thrown away.

@emanspeaks

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, its comments, and the implementation of the approach by @emanspeaks: https://github.com/gitpython-developers/GitPython/blob/f065d1fba422a528a133719350e027f1241273df/git/cmd.py#L398-L403 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.

Byron

Thanks a lot, much appreciated!

It's great to hear that running tests on Windows is now more likely to succeed without needing additional modifications.

EliahKagan added 6 commits November 30, 2024 12:57

Run cargo fmt

da03932

Fix an ambiguous expect message

1f6a866

EliahKagan mentioned this pull request Dec 1, 2024

Tests on Windows must usually be run in Git Bash or a similar environment #1359

Closed

Byron approved these changes Dec 1, 2024

View reviewed changes

Byron merged commit fadf106 into GitoxideLabs:main Dec 1, 2024
20 checks passed

EliahKagan deleted the run-ci/git-bash branch December 1, 2024 18:30

EliahKagan mentioned this pull request Jan 9, 2025

git login shell #1752

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run test fixture scripts on Windows with Git Bash #1712

Run test fixture scripts on Windows with Git Bash #1712

EliahKagan commented Dec 1, 2024 •

edited

Loading

Byron left a comment

	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")
	}

Run test fixture scripts on Windows with Git Bash #1712

Run test fixture scripts on Windows with Git Bash #1712

Conversation

EliahKagan commented Dec 1, 2024 • edited Loading

Background

What this changes

Testing

Prior work and acknowledgement

Scope

Future directions

Byron left a comment

Choose a reason for hiding this comment

EliahKagan commented Dec 1, 2024 •

edited

Loading