Tests on Windows must usually be run in Git Bash or a similar environment #1359
Comments
|
Thanks for the detailed report! It appears that |
This runs `cargo nextest ...` and `cargo test --doc` in separate steps in the `test-fast` job, so that the job fails when `cargo nextest ...` fails. Otherwise, with `pwsh` on Windows, test failures (other than doctests) are masked. Background: Since 89a0567 (GitoxideLabs#1556), doctests are run on all three major platforms, and not only on the full test job done on Ubuntu. But the way this was done relied on a script failing as soon as (or, at least, whenever) any command in the script failed. That works on Ubuntu and macOS, where a `bash` shell is used by default, with `-e` passed. But on Windows, GitHub Actions uses `pwsh` as the default shell. `pwsh` is not run in a way that causes it to stop at the first failing command. So, on Windows, when the `cargo nextest` command failed but the `cargo test --doc` command that followed it in the same script step passed, the step passed, thus allowing the job and workflow to pass. This was observed in GitoxideLabs#1429 after a rebase (see comments). Note that this is not related to the increased use of `nextest`. While that was also done in GitoxideLabs#1556, it did not affect the `test-fast` job where the bug was introduced, which was already using `nextest`. This fixes the problem by putting the two commands in separate steps. This is simpler than doing anything in PowerShell to make the script stop, such as using `&&` or attempting to produce `-e`-like behavior. Another option could be to use `bash` as the shell, which is a Git Bash environment suitable for running the tests. The reason I didn't do that is that I think it is valuable to see the results when the tests are run from a PowerShell environment. In particular, continuing to use PowerShell here should help in investigating GitoxideLabs#1359 (and shows that the claim I made is overly strong, since CI on Windows with `pwsh` not itself started from a Unix-style shell is not "Git Bash or a similar environment").
This comment was marked as off-topic.
This comment was marked as off-topic.
|
As noted in #1559, my claim in this issue is apparently overstated, since tests are able to run in PowerShell on Windows GHA runners, and a |
|
The reason this works on CI as noted in #1359 (comment), while not working locally, is that That the current implementation is ever able to use Git Bash on Windows, even when the tests are run from a Git Bash shell, also depends inadvertently on undocumented behavior of How the tests failConsider the following output, which is typical of the test failures: There are three things going on here:
Why is it even running
|
|
Thanks so much for researching this, and the very informative and interesting read! The special-behaviour in By the looks of it, this does affect Fixing this both in However, I wouldn't mind any fix to start with whatever is needed for |
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 fou 8000 nd 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.
I've opened #1712 for this. |
It looks like that has happened: rust-lang/rust#137673 makes As noted in rust-lang/rust#122660 (comment), this change appears in Rust 1.87.0. That also shows how the old behavior can, if necessary, often be restored, by setting the child I think we do not need to do that. The reasons I think we don't need to do that, in descending order of immediate interest, are:
In addition, if and when we do encounter a problem that seems like it can be worked around by setting the child
Instead, there are alternatives that would overcome those problems and that make sense given what we are already doing:
Such changes can probably wait until after greater unification of path lookups and shell execution across |
|
Thanks a lot for the exhaustive summary! Without claiming I have grokked it all, I think I understand enough to be looking forward to the anticipated changes to how |
Uh oh!
There was an error while loading. Please reload this page.
See #1359 (comment) for updated information.
Current behavior 😯
In this project, tests are typically run on Windows in a Git Bash environment. On CI in Windows,
bashis Git Bash. This is also the environment from which I have typically run tests. This is a native environment; it should not be confused with WSL or even something like Cygwin. Furthermore, tools maintained byrustupare not specifically connected to this environment, though they are run from it.All tests are able to pass when run in Git Bash on Windows, and likely would pass if run in similar environments that provide a POSIX-compatible shell for Windows, set environment variables accordingly, and provide access to POSIX versions of common tools.
But running the tests from PowerShell instead produces 627 test failures. This presumably relates to the associated environment rather than the shell itself, since however the tests are run, a test runner subprocess is controlling the run.
The main problem seems to be the use of paths with backslashes in them, and the way that interacts with fixture scripts. Note, however, that this is not specific to the situation where
GIX_TEST_IGNORE_ARCHIVESis set. That is to say that this is a separate issue from #1358 (which also involves far fewer failures) and occurs even whenGIX_TEST_IGNORE_ARCHIVESis unset.Even the test summary showing one failure per line is too long for GitHub to allow me to include it in this issue description. This gist has it. The full output can be seen in this other gist.
As of now, I have not figured out the cause, which I think is a prerequisite for making a decision about whether to try to support such environments or to instead document them as unsupported.
Expected behavior 🤔
Either all tests should pass even when run from PowerShell, or the need to run them in a Git Bash environment (or whatever other more precise requirement is known) should be documented.
I think which is better depends on how cumbersome it would be to enable the tests to pass when run from PowerShell and, more importantly, how cumbersome it would be to maintain this state. I suspect it may be feasible, though.
Git behavior
A direct comparison is difficult in that this is about running the tests rather than the functionality of Git and gitoxide. However, it is worth noting that Git for Windows has its own SDK environment that is used for building it and running its tests, and this environment is separate even from the more minimal Git Bash environment.
Thus, if it is infeasible to support running tests from PowerShell, documenting that would still not be very restrictive, even compared to what is needed to develop and test Git for Windows, since the non-SDK "Git Bash" environment works fine for running gitoxide's tests.
Steps to reproduce 🕹
Using Windows, make sure the tests can pass when run from Git Bash using the first of two
cargotest-runner commands shown below.Optionally clean the build with
cargo cleanor, if preferred, clean everything withgix clean -xde.There are two approaches. The first approach is to display the output in the console: To do that, in PowerShell, run:
However, that is hard to read because of the very large volume of output combined with the staggering of output lines suggesting a problem identifying the terminal width or computing the width of text written.
Therefore, you may instead wish to write both stdout and stderr to a file, and inspect the file during and/or after the run.
This writes all output to
output.txtin the directory above the current directory. This is to avoid creating a new file in the repository while running the tests, in case that were to interfere with something, though it shouldn't since that should only be able to affect a small number of journey test runs, which are not included when runningcargo nextest.The
*>operator in PowerShell is similar to the&>or>&operators in some shells and to the effect of2>&1 >in most shells.Although Windows comes with Windows PowerShell, I suggest against using it for this, since is somewhat less intuitive, and has fewer convenience features, compared to the newer PowerShell (which is sometimes called PowerShell Core). Windows PowerShell is also less likely to be used by developers on Windows, and thus probably less important to support than recent versions of PowerShell. I used PowerShell 7.4.2 on Windows 10.
The text was updated successfully, but these errors were encountered: