Fix ARM64 Windows null device and Git alternative locations #2115
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In addition to the existing tests on the x86-64 `windows-latest` runners (which are currently Windows Server 2022), this now also tests on the new `windows-11-arm` runner, which runs Windows 11 for ARM64/AArch64. The main difference that motivates adding these new CI test jobs is between x86-64 Windows and ARM64 Windows, rather than between Windows Server 2022 and Windows 11, though the latter difference might potentially cause the tests to reveal something. The `windows-11-arm` runners are in preview, and it is possible that they will not be stable enough to justify keeping these new jobs as part of matrix definitions with other jobs: one or both of the new jobs might need to be moved out of its matrix so it can be made non-blocking for PRs, or even removed altogether. For details on this preview runner, see: https://github.blog/changelog/2025-04-14-windows-arm64-hosted-runners-now-available-in-public-preview/
This works around the combination of these two conditions, which individually wouldn't be a problem but are a problem together: - The newly available `windows-11-arm` runner does not currently have `rustup` installed on it. - Although the `dtolnay/rust-toolchain` action will attempt to install `rustup` when it is absent on Unix-like systems, it does not attempt to do so on Windows. (If the latter condition is fixed, however, then depending on *how* it is fixed, it might still be necessary to install `rustup` ourselves. Specifically, it is easy to accidentally download a `rustup-init.exe` for x86-64 even when on ARM64/AArch64 Windows.) The new step here, added for both of the `windows-11-arm` jobs, is intended to be similar in its effect to these commands that `dtolnay/rustup-init` runs on Unix-like systems: https://github.com/actions-rust-lang/setup-rust-toolchain/blob/9399c7bb15d4c7d47b27263d024f0a4978346ba4/action.yml#L136-L139 Note that this is not quite equivalent. It is intentionally less versatile in two ways: - It hard-codes the AArch64 `rustup-init.exe` URL, to ensure that it is selected, rather than not finding anything or wrongly getting a URL for a Unix-like system or for x86-64 Windows. - No attempt is made to respect a preexisting `CARGO_HOME` environment variable (since we do not set one in these jobs). (This uses the style `$Env:varname` rather than `$env:varname` in PowerShell because the former seems to be much more common in the Microsoft documentation, and changes one preexisting occurrence of `$env` to `$Env` for stylistic consistency.)
Instead of `GITHUB_ENV`. `GITHUB_ENV` works, but `GITHUB_PATH` is preferable, and simpler.
`NULL_DEVICE` is `/dev/null` except on Windows, where there are several possible choices. Previously we were using `NUL` because the more modern full path `\\.\NUL` is not supported by `git`. However, `git` also rejects `NUL`, when capitalized, on some Windows systems. This can be observed on Windows 11 ARM64 builds. In contrast, the lower-case `nul`, which Windows itself treats the same as `NUL`, is always accepted by Git for Windows. Although it's not readily apparent why Git for Windows accepts `NUL` on some Windows operating systems and/or platforms and not others, the preferential treatment of `nul` (not extending to `NUL`) can be seen in a few places in the Git for Windows source code, including in `mingw_access` (`strcmp` is case-sensitive): https://github.com/git-for-windows/git/blob/a36f720572491aafc59d63ff926b04d9fa787ec3/compat/mingw.c#L1093 Therefore, this changes `"NUL"` to `"nul"` for the Windows null device path. This allows some functionality where `git` is invoked that had been broken on ARM64 Windows systems to start working. See 7280a2d (GitoxideLabs#1567) and 3cf9fc1 (GitoxideLabs#1570) for further context. Git for Windows also special-cases the literal path `/dev/null` in access checks and when opening it for read or write (thought not if an attempt is made to execute or change directory to it). We nonetheless change `NUL` to `nul` on Windows, rather than changing it to `/dev/null` and using that value on all platforms. The reason is that treating `/dev/null` as the null device on Windows is specific to Git for Windows. `/dev/null` on Windows would not be the null device, nor otherwise special, if we open it ourselves, nor if opened indirectly by other programs that don't special-case it. The Windows path `/dev/null` is equivalent to the Windows path `\dev\null`. These are relative paths, resolved as `X:\dev\null` where `X:` is replaced by the current drive. That is never what we want when we attempt to use the null device.
This is to investigate whether fixtures for other tests are failing due to interaction with them, and it is hoped to be a temporary measure. Temporary or not, this change affects only Windows ARM jobs. Other ARM platforms, and non-ARM Windows, are unchanged. This might remain in place over a longer term: - If the problem is that the current Windows 11 ARM runner is not fast enough to allow the tests to pass. Though adjusting expected timings conditionally for Windows ARM64 might be a workaround. - If including the performance tests, passing are not, makes the runs take too long to do routinely. It looks like this may be the case. It's probably better to test regularly on Windows ARM without the performance tests, than to rarely test Windows ARM.
On CI, `test-fixtures-windows` does `GIX_TEST_IGNORE_ARCHIVES=1` test runs on Windows, which have a number of known and expected failures, as described in GitoxideLabs#1358. Since GitoxideLabs#1663, these are listed in `etc/test-fixtures-windows-expected-failures-see-issue-1358.txt` and `test-fixtures-windows` compares those expected failures to the actual failures of a run. If there are any differences, it shows a unified diff with full context and fails the run. The default Git foreground color scheme has been used for the diff, where any tests that are no longer failing are shown as `-` lines in red, and any tests that are newly failing are shown as `+` lines in green. These are the usual default `git diff` colors; we did not pick those colors specifically to be meaningful to this scenario. While the use of `-` and `+` lines and the specific unified diff format chosen are intuitive and informative, this is less so for the color scheme. That is for two reasons: 1. By default and unless another theme is chosen explicitly or selected based on device settings, the GitHub web interface uses a light theme. In this theme, as currently coded/styled, on the pale background in GitHub Actions, foreground normal-styled green text can be hard to distinguish visually from foreground normal-style black text. So it is often not immediately clear which test failures are new. (New test failures are the typical way a `test-fixtures-windows` job fails, when it does.) 2. A test that is no longer failing would usually mean that a test or implementation bug has been fixed, or that compatibility has otherwise been improved. `test-fixtures-windows` fails on these so that we attend to them, usually by dropping newly passing tests from the list of expected failures. Likewise, a test that is failing, especially a newly failing tests, usually if not always means that something is not right, even if it is merely the test itself that needs to be improved. But showing things that have gotten better in red, and things that have gotten worse in green, is unintuitive. This is partly because these colors have the opposite meaning in numerous contexts, including in some parts of the GitHub Actions interface. It is more specifically because they have the opposite meaning in the context of previous step output, where `nextest` shows stderr in red (and specifically shows stderr on tests that have failed), shows `FAIL` in red, and shows `PASS` in green. It may be possible to make red and green show up better. These colors are clear in the `nextest` output. However, the options for `diff` colors, at least when configuring them with `color.diff.*`, are limited. Furthermore, that would only improve (1), not (2). This bolds both `-` and `+` lines, shows `-` lines in magenta, and shows `+` lines in blue. These colors seem to be visible on both light and dark backgrounds, and they seem to be at least as distinguishable from each other as are red and green, in general. This change only applies to the specific diff that shows newly passing and newly failing tests in `test-fixtures-windows`.
The `windows-11-arm` runner still has Git for Windows 2.48.1. It thus remains affected by GitoxideLabs#1849 (git-for-windows/git#5436) for now. This is responsible for failures seen so far on `windows-11-arm`, other than the failures that appear to be related to performance. This commit restores the pre-GitoxideLabs#1920 logic for upgrading the Git for Windows installation on the runner, but does so *only* in the `windows-11-arm` job, and *not* in the `windows-latest` job. The code is modified to download the ARM64 installer rather than the x86-64 installer. Besides being only done for `windows-11-arm` and installing an ARM64 build, this is the same as the code in GitoxideLabs#1892 (building on GitoxideLabs#1870). It thus attempts to install the latest stable release, but not pre-releases. See 3501e82 (GitoxideLabs#1920) for further background. Especially in view of the preview status of the `windows-11-arm` image, it may be a good idea to inspect and report the status of the Git for Windows installation before and after the upgrade. But this commit does not add such functionality.
The version of Git for Windows after the upgrade (or without the upgrade, if it is not done) is already shown in `actions/checkout` output, since that action defaults to using the installed Git. However, it's valuable to see information about it before and after upgrade in the same run. It's also useful to see more details than the version, such as to confirm the assumption that it is really an ARM64 build. All the information of interest could be obtained by running `git version --build-options`. But the change here continues to avoid running the old Git for any reason. This instead downlods and installs `sigcheck` to inspect a (non-shim) `git.exe`. This is with the idea that running any executables provided with Git for Windows may increase the likelihood of being unable to replace them, if somehow they or their subprocesses do not fully terminate. This is probably the wrong tradeoff, and it will probably be changed to running `git version --build-options` before and after the upgrade. But if that forthcoming change does cause a problem, then it can be reverted and the approach done here used again.
This replaces the more complicated `sigcheck` logic with runs of `git version --build-options` before and after upgrading Git for Windows in the ARM `test-fixtures-windows` job. The reason this wasn't done before was to avoid running the old version at all, to maximize the chance that Inno Setup would be able to remove it. On Windows, it is often difficult or infeasible to remove or replace a running executable or other open file, without terminating the executable or closing the file. However, this was not the best tradeoff. Running `git version` is unlikely to leave a danling process running. What we really want is to avoid using the old Git substantially, such as in `actions/checkout`, and this still avoids that. The other reason to upgrade Git before running `actions/checkout` is to verify that the new version of Git for Windows is working by having `actions/checkout` use it. This change also preserves that. Passing `--build-options` is important here because we do want to verify that both the version of Git for Windows being replaced, and (especially) the upgrade, are ARM64 builds. (The `windows-11-arm` image currently has x86-64 builds of some preinstalled software. But it's not expected to have a non-native build of Git for Windows. If it does, then there may be more steps that should be taken besides attempting to upgrade it.)
The new pattern `speed` matches the `find_speed` test, and may match some future performance tests as well. This is not needed to make tests pass, as `find_speed` was already passing. However, the `test-fixtures-windows` ARM job takes a very long time, and this seems that it may be contributing. (It was also an oversight not to skip it when skipping other performance tests.) However, the pattern `fuzzed_timeout` is intentionally kept, rather than being broadened into the pattern `timeout`, since the latter would match several tests that seem always to complete very fast, and that test important functionality. (Even `fuzzed_timeout` itself *usually* completes in less than a tenth of a second.)
In the hope of improving clarity in the CI workflow.
The "Install rustup" step was duplicated across two jobs: `test-fast` and `test-fixtures-windows`, both conditioned on `windows-11-arm`. There is furthermore the possibility that it may be added in the future to other jobs (such as if it ends up being useful to run the MSRV check on `windows-11-arm`). The duplication was such that it would be easy to wrongly change one but not the other occurrence. If that were to happen, and tests passed/failed or otherwise behaved differently due to differences in their CI environments, then it would create the wrong impression of being related to whether `GIX_TEST_IGNORE_ARCHIVES` is set. To decrease unnecessary duplication, and more specifically to avoid that specific confusion and the wild goose chase that might ensue from it, this extracts the logic of the "Install rustup" steps, other than the `if` key checking that we are running on `windows-11-arm`, to a newly created composite action `setup-windows-arm-rustup`. The reason for keeping the operating system check in the calling jobs is so that it is always immediately clear in the job output whether the step is meant to have an effect. Although composite actions often make sense to publish separately and use across repositories, that is probably not the case here. This logic does only what we need right now, and does not include functionality such as respecting a preexisitng `CARGO_HOME` environment variable or making sure to download `rustup-init.exe` to a location that would be reasonable for all callers. (This does *not* also extract the "Upgrade Git for Windows" step to a composite action. It's not necessary to do so, because the step only currently appears in one job: the `test-fixtures-windows` job, conditioned on `windows-11-arm`. It would, in principle, be useful to extract it nonetheless, because it is cumbersome and also something we've removed and reintroduced before; so making it a composite action could make it more reasonable to keep around for next time it is needed. The problem is that there is a benefit to upgrading Git for Windows before, rather than after, performing any nontrivial Git operations, to decrease the likelihood of dangling subprocesses or otherwise open files preventing the Inno Setup installer from performing the upgrade. There is also a separate specific benefit to upgrading it before `actions/checkout` runs, so that step uses it, thereby effectively validating that it really works. But extracting the "Upgrade Git for Windows" step to a composite action that runs before the `actions/checkout` step is complicated, because the otherwise ideal way to refer to the action locally with a `./` path couldn't be used, because its code would not yet have been checked out. There are ways around this, but their complexity suggests they may not be worthwhile until a composite action is needed in order to actually decrease logic duplication. See the experiments in #68 for details.)
The `windows-11-arm` image has been updated with more of the usual software present on GitHub-hosted GitHub Actions runner images for Windows, at current versions. - Git for Windows is updated to a version not suffering from GitoxideLabs#1849. So this removes the steps that had upgraded Git for Windows and checked the version before and after the upgrade. - `rustup` has been added. So this removes the helper action that installed it, and removes the `uses` step for it from the two jobs that had called it. (If either of these is needed in the future, on this or any other job, then they can be brought back from the commit history.)
In the `test-fast` Windows jobs, this removes descendant directories under the directory where Git for Windows is installed on CI Windows runners, from the `PATH` environment variable. This is done after the full test suite run. The idea is to support running some specific tests, where `gix-path` will have to find `git` by checking common locations. But this does rerun any tests yet. The other change this makes is to remove the CI runner PATH search instrumentation steps in `test-fast` and `test-fixtures-windows` that were just previously added. (Those steps were added temporarily to examine how this change should be made, and they are no longer needed.)
But only in the `test-fast` Windows jobs. This is to test that `gix-path` and `gix-testtools` can find `git` when it is in on of the common Git for Windows install locations.
`gix-testtools` currently uses a hard-coded `GIT_PROGRAM` value of `git.exe` on Windows. `gix-testtools` uses `GIT_PROGRAM` directly in `GIT_CORE_DIR`, `parse_git_version`, `run_git`, and `populate_meta_dir`, as well as in its own test case `configure_command_clears_external_config`. When the path `git.exe` cannot be used to execute the `git` program, these `gix-testtools` tests fail in `test-fast` on `ubuntu-latest`: - tests::bash_program_absolute_or_unrooted - tests::bash_program_ok_for_platform - tests::bash_program_unix_path - tests::configure_command_clears_external_config Various other functionality of `gix-testtools` is also likely to fail. `gix-testtools` will have to be fixed to work with this, probably by causing it to use facilities provided by `gix-path`, as discussed in GitoxideLabs#1992 comments, in GitoxideLabs#1886, and elsewhere. (Those discussions are with a focus on finding shells through `git`, rather than finding `git` itself. But using `gix-path` facilities in `gix-testtools` should be able to solve both, for when the `git` executable can be found on Windows but not in a `PATH` search for `git.exe`.) But let's see if we can already successfully rerun the `gix-path` tests with `git.exe` not in `PATH`. This removes the step to retest `gix-testtools`, for now. This is with the idea that if rerunning `gix-path` tests works in the `test-fast` job on `windows-latest` and the tests pass there but some fail on `windows-11-arm`, then they will have surfaced bugs affecting our use of ARM64 builds of Git for Windows that can be fixed even *before* `gix-testtools` is modified to use more `gix-path` facilities to find `git` and associated executables.
This fixes typos and minor wrong wording, clarifies formatting, and occasionally rewords substantially or expands something for clarity, in some comments and docstrings in `gix_path::env::git`. (Affected docstrings are on nonpublic items and tests, so this change is not user-facing.)
EliahKagan
force-pushed
the
run-ci/arm-windows
branch
from
bfc6919 to
4fe0fc4
Compare
This updates the integration-style tests for Windows-specific `git` executable alternative locations to include `clangarm64/bin` directories when looking under the 64-bit program files directory. (These tests are "integration-style" in the sense that they check the actual value of `ALTERNATIVE_LOCATIONS` against expectations computed from details of the build target and runtime environment.) The tests will now fail until `ALTERNATIVE_LOCATIONS` is fixed for ARM64 Windows. (Because the currently intended approach is to check for both ARM64 and x86_64 installations of Git for Windows on all 64-bit systems where the 64-bit "Program Files" directory location is available -- regardless of whether the system is an x86_64 or ARM64 installation of Windows -- these integration-style tests should currently fail on both x86_64 and ARM64 Windows systems.)
Similar to the preceding commit but for unit-style tests rather than integration-style tests, this updates the unit tests for Windows-specific `git` executable alternative locations to include `clangarm64/bin` directories when looking under the 64-bit program files directory. (The tests that are updated here are those that are true unit tests, because they examine the `locations_under_program_files()` helper function that takes a dependency-injected `var_os()`-like function, rather than examining `ALTERNATIVE_LOCATIONS` itself.) As in the integration-style tests updated in the preceding commit, these tests will now fail until the logic for enumerating alternative locations to check for the `git` executable is fixed to include a location for ARM64, under `clangarm64` (as well as still checking for a x86_64 build, under `mingw64`).
`gix-path` looks for and runs an installed `git` executable, when present, to discover information about how Git is set up. This is used in several functions in `gix_path::env`, especially on Windows, where if `git.exe` is not found in a `PATH` search, then common installation locations for Git for Windows are checked. These locations on Windows are resolved based on information from the current environment, since different systems have different program files directories. This was implemented in GitoxideLabs#1456 (which built on GitoxideLabs#1419). Although this was sufficient to find the most common Git for Windows installation locations, it did not find ARM64 builds of Git for Windows. Such builds place the non-shim `git.exe` program in `(git root)\clangarm64\bin`, rather than in `(git root)\mingw64\bin` or `(git root)\mingw32\bin`. At the time of GitoxideLabs#1419 and GitoxideLabs#1456, no stable ARM64 builds of Git for Windows were available. Since then, Git for Windows began releasing such builds. This modifies the alternative locations examined if `git.exe` is not found in a `PATH` search on Windows so that, where `(git root)` is in a 64-bit program files directory, we check for a `(git root)\clangarm64\bin` directory, in addition to checking for a `(git root)\mingw64\bin` directory as was already done. Although 64-bit and 32-bit program files directories are separate, on ARM64 systems the 64-bit program files directory is used both for ARM64 programs, which the system can run directly, and for x86_64 programs, which the system must run through emulation. This checks both `clangarm64` and `mingw64`, where `mingw64` was checked before. It does so in that order, because if both are available, then we are probably on an ARM64 system, and the ARM64 build of Git for Windows should be preferred, both because it will tend to perform better and because the user is likely to expect that to be used. (An x86_64 build, especially if present directly alongside an ARM64 build, may be left over from a previous version of Git for Windows that didn't have ARM64 builds and that was only incompletely uninstalled.) This checks both, in that order, on all systems where we had checked `mingw64` before, even on x86_64 systems. This is because: - To limit production dependencies and code complexity, we have been examining only environment variables (and information available at build time) to ascertain which program files directories exist and whether they are 64-bit or 32-bit program files directories. At least for now, this preserves that general approach, continuing not to explicitly call Windows API functions or access the Windows registry, other than in tests. - But determining from environment variables whether the system is ARM64 or x86_64 is less straightforward than determining the program files directory locations, in one major case as well as various edge cases. The reason it's less straightforward is that, if our parent process (or other ancestor) passes down a sanitized environment while still attempting to let the program files directories be found, then: - That process should make available all of the `ProgramFiles`, `ProgramW6432`, `ProgramFiles(x86)`, and `ProgramFiles(ARM)` variables that exist in its own environment. (This is because, on 64-bit Windows, the child `ProgramFiles` is populated from the parent `ProgramW6432`, `ProgramFiles(x86)`, or `ProgramFiles(ARM)`, depending on the architectures of the parent and child, if the parent passes down the relevant variable.) - Even if the parent/ancestor is not designed with the WoW64 rules in mind, it will likely pass down at least `ProgramFiles`. This will then be used as the child `ProgramFiles`, if whichever of `ProgramFilesW6432`, `ProgramFiles(x86)`, or `ProgramFiles(ARM)` is relevant is not passed down by the parent. - In contrast, the parent/ancestor may omit the variables `PROCESSOR_ARCHITECTURE` and `PROCESSOR_ARCHITEW6432`, which are not obviously needed for locating programs. In and of itself, this is not necessarily a problem, because on ARM64 Windows, at least one of the two will still be set automatically. (Even if the parent process runs us with an explicitly empty environment, we will get one of these, on ARM64 Windows.) However, in this case, as in others, it will generally be set according to our process architecture, rather than the system architecture. - Thus, even if `PROCESSOR_ARCHITE*` variables are preserved or set correctly, there are common cases where they are not sufficient. The main such case is when we are an x86_64 build, but the system is ARM64, and Git for Windows is ARM64. `gix-path` is a library crate, and it will sometimes to be used in an x86_64 program on such a system. Also, if `gix-path` is used in an Arm64EC program, then even though its code may be ARM64 with the `arm64ec-pc-windows-msvc` Rust target, the program would still be treated as an x86_64 program in its interactions with the system and other programs, including in how its environment variables' values are populated and how their values are affected by WoW64. (`gix-path` may also be x86_64 code where the program is Arm64EC. One way this happens is if `gix-path` is used in an x86_64 DLL, and an Arm64EC program loads the DLL. Using x86_64 DLLs from ARM64 code is one of the reasons a program may target Arm64EC. In this case, the Rust target will be for x86_64, not ARM64 or Arm64EC. So checking our own Rust build target can't fully check if the program is Arm64EC.) - Although the `PROCESSOR_IDENTIFIER` variable is more reliable if present--see actions/partner-runner-images#117 for an example of where this is more reliable than `PROCESSOR_ARCHITECTURE`--it is slightly more complex to parse. Much more importantly, unlike `PROCESSOR_ARCHITE*`, the `PROCESSOR_IDENTIFIER` variable is not set automatically in a child process whose parent/ancestor has removed it. Whether or not a parent/ancestor passes down `PROCESSOR_ARCHITE*` variables, it may still not choose to let `PROCESSOR_IDENTIFIER` through, if it sanitizes the environment. - It would sometimes work to look for `ProgramFiles(ARM)`. This environment variable, if present, gives the 32-bit ARM program files directory location on an ARM64 Windows system. If set, then the Windows system could be treated as ARM64. However, a parent/ancestor process may omit this even when passing down program files related environment variables, including `ProgramFiles(x86)`. It may do so because the `ProgramFiles(ARM)` environment variable is less well known. Or it may omit it intentionally, if the parent process is only passing down variables needed to find Git for Windows, for which one shouldn't need to know the 32-bit ARM program files directory location, since Git for Windows has never had 32-bit ARM builds. Relatedly, augmenting the search by checking the filesystem for a sibling (or hard-coded) directory named `Program Files (ARM)` should not be done, because this directory has no special meaning outside of ARM64 Windows. It may therefore be left over from a previous installation or migration, or even created by a local user as in the CVE-2024-40644 scenario patched in GitoxideLabs#1456. (These complexities all relate to deciding whether and in what order to search `bin` subdirectories of `clangarm64`, `mingw64`, and `mingw32`. They would go away if we looked for the shim rather than the non-shim executable. This is because the path from `(git root)` to the shim does not contain a directory component named after a build target. That simplification would carry its own tradeoffs, and it is unclear if it ought to be done; it is not done here.)
This renames `pf` to `program_files_dir` and refactors how it is set so that its meaning and type are immediately clear.
This consolidates the check that the value is present with the check that it is an absolute path, and simplifies the comment.
The `PlatformArchitecture` test helper struct holds information about whether we are 64-bit or 32-bit and whether the system we are running on is 64-bit or 32-bit. It doesn't hold information about architecture beyond that. The name `PlatformArchitecture` made sense in GitoxideLabs#1456 when this fully captured the relevant architecture distinctions. But now, although `gix-path` doesn't work differently between x86_64 and ARM64 systems, the distinction betwee x86_64 and ARM64 is important in the design. This renames `PlatformArchitecture` to `PlatformBitness`, while names that refer to architecture in a way not completely reducible to a 32-bit vs. 64-bit distinction are not renamed "bitness."
EliahKagan
force-pushed
the
run-ci/arm-windows
branch
from
4b56171 to
5bf265b
Compare
The fixes some wording in `RelativeGitBinPaths::assert_from()` in the `gix-path` test suite, for how many paths we build when we are able to find a 64-bit program files directory.
This refactors `gix_path::env::system_prefix()` by extracting the two strategies used to obtain it on Windows to newly created helper functions: `system_prefix_from_exepath_var()` for the `EXEPATH` optimization, and `system_prefix_from_core_dir()` for the fallback. This is to facilitate testing. The new helpers take the functions they call to obtain systemwide information (`EXEPATH` from the environment, or the once-computed `GIT_CORE_DIR`) by dependency injection, so tests can be added that cover the important cases. This is similar to the approach for `ALTERNATIVE_LOCATIONS` of using a `locations_under_program_files()` helper (since GitoxideLabs#1456). The `system_prefix_from_exepath_var()` helper treats its `var_os_func` parameter analogously to how `locations_under_program_files()` treats its `var_os_func` parameter.
This tests the `EXEPATH` strategy that `system_prefix()` attempts on Windows (before falling back to the `git --exec-path` strategy). Specifically, this tests the `system_prefix_from_exepath_var()` helper that was recently introduced to facilitate testing. Some of these new tests don't pass yet, because they test two scenarios that are not yet covered: - More than one of `mingw32`, `mingw64`, and `clangarm64` exist under the directory named by the value of `EXEPATH`. In this situation, the `EXEPATH` optimization should not be used. - `clangarm64` exists under the directory named by the value of `EXEPATH` (and neither `mingw32` nor `mingw64` exist under it). In this situation, the `clangarm64` subdirectory should be used. Although the `EXEPATH` optimization is only ever used on Windows, nothing in its test or implementation precludes testing elsewhere, so the tests build and run on Unix-like systems too.
On Windows, the `gix_path::env::system_prefix()` function has two strategies. The first is an optimization that doesn't always work, where the `EXEPATH` environment variable is consulted. In Git Bash, this variable is usually available, pointing somewhere into the Git for Windows installation. Most commonly, but in practice not universally, it points to the top-level directory of the Git for Windows installation. `system_prefix()` on Windows looks for a subdirectory of this directory that is named for one of the MSYS2 environment https://www.msys2.org/docs/environments prefixes, of those Git for Windows has builds for. (Otherwise, it falls back to traversing to such a location upward from what `git --exec-path` reveals, which is often slower as `git` must sometimes be run.) However, this `EXEPATH` optimization had only checked for the `mingw64` and `mingw32` environment prefixes. This was ideal before ARM64 builds of Git for Windows were released. But since then, it is useful to allow the `clangarm64` environment prefix as well, to allow the `EXEPATH` optimization to work on ARM64 builds of Git for Windows. This makes that change.
The `EXEPATH` optimization for `gix_path::env::system_prefix()` on Windows previously tried https://www.msys2.org/docs/environments prefixes used by Git for Windows, returning a successful result on the first subdirectory found. However, if `EXEPATH` points to a directory that contains more than one such subdirectory, then the subdirectory returned would not necessarily be the correct one. Getting more than one subdirectory is unlikely. It is expected that at most one of `clangarm64`, `mingw64`, and `mingw32` will be present. However, there are at least three ways it could happen: - `EXEPATH` has the intended meaning as the root of a Git for Windows installation, but the directory it refers to contains multiple directories left over from a previous installation. A corresponding scenario applies to `ALTERNATIVE_LOCATIONS`, but it is resolved by checking all the directories in a reasonable order. Here, we are not checking the contents of the directories, so no matter what order we look for them in, picking one when there are others risks picking the wrong one. - `EXEPATH` has the intended meaning as the root of a Git for Windows installation, but it is a custom installation produced from local build or custom distribution rather than an official build, and it contains multiple such directories because it carries binaries built against more than one of the targets (even if only one of them has `git` itself). A corresponding scenario is fairly unlikely for `ALTERNATIVE_LOCATIONS`, because a custom MSYS2 tree, even if created using the Git for Windows SDK, would probably not be installed in one of the common Git for Windows installation locations, without considering the effects of doing so. In contrast, `EXEPATH` will often be set in a Git Bash environment even in a highly customized Git for Windows tree. - `EXEPATH` has a different meaning from what is intended. For example, the user might set it to the root of an ordinary MSYS2 installation. (Note that it is also likely to have various other meanings even more different from these, but those won't likely cause the `EXEPATH` optimization to be used when it shouldn't, because most possible meanings of `EXEPATH` won't involve a subdirectory of any of the names we look for. Instead of using the first existing subdirectory we fine, this checks for all of them and requires that exactly one exist. If more than one exist, then that is now treated the same as if none exist, falling back to the `git --exec-path`-based strategy, which is sometimes slower but more robust.
The `EXEPATH` optimization in `system_prefix()` on Windows relies on `EXEPATH` either being unset or set to a value where it is okay to attempt to find and use `clangarm64`, `mingw64`, and `mingw32` subdirectories. Like most environment variables, `EXEPATH` is unlikely to be directly controllable by an attacker. But it may exist with a different meaning from any we (and Git for Windows) intend. It also may inadvertently be set to an empty string, or intnetionally set to a relative path; in either case, we cannot safely use it for the `EXEPATH` optimization in `system_prefix()`. In particular, if it is an empty string, then an attempt would be made to use a `clangarm64`, `mingw64`, or `mingw32` subdirectory of the current working directory. This test will fail until the `EXEPATH` optimization is refined to bail out--and fall back to the more robust strategy--if `EXEPATH` is an empty string, or under broader conditions such as it not being an absolute path.
This introduces an `if_exepath()` helper for use in streamlining closures to pass to `system_prefix_from_exepath_var()` in tests.
This tests that a nonempty value of `EXEPATH` that is a realtive path does not trigger the `EXEPATH` optimization of `system_prefix()` on Windows. This is similar to the previously introduced test for an empty value, in that both are treated as relative paths. Like that test, this one currently fails, but both will pass once the `EXEPATH` optimization is refined to refuse to look under a non-absolute path.
The first of two strategies used by the `system_prefix()` function on Windows is an optimization that looks for `clangarm64`, `mingw64`, or `mingw32` subdirectories of a directory whose path is given by the value of the `EXEPATH` environment variable. If `EXEPATH` is absent, can't be interpreted as a path, or can be interpreted as a path but does not point to a directory that can be verified to contain such a subdirectory, then this optimization was already being skipped and the more reliable but sometimes slower `git --exec-path`-based method used. However, when `EXEPATH` contains a relative path, including in the case where it is an empty string (which could be set by accident or if it is being used with some meaning other than what we and Git for Windows recognize), then it was still used, even though that would not usually be correct and would sometimes not be safe. Although an attacker is not expected to be able to control the value of `EXEPATH` (nor most environment variables), a value such as an empty string would cause `clangarm64`, `mingw64`, and `mingw32` subdirectories of the current directory to be used. A user might intentionally set `EXEPATH` to a relative path to leverage the `EXEPATH` optimization of `system_prefix()` with that path. But `EXEPATH` is a short variable name with many plausible meanings that is not named with a distinctive prefix such `GIT_` or `GIX_`. So it would not be a good tradeoff to continue recognizing it for `system_prefix()` anytime it is non-absoliute. Thus, as a stability fix and possible security enhancement, this adds a check that the value of `EXEPATH` is an absolute path before proceeding with the `EXEPATH` optimization.
EliahKagan
force-pushed
the
run-ci/arm-windows
branch
from
a619a52 to
571ca6e
Compare
When retesting `gix-path` without `git` being in the `PATH`, this adds a step to verify that the `EXEPATH` environment variable is not set. If `EXEPATH` is set, the the `EXEPATH` optimization could succeed even if the fallback strategy that is intended to be more reliable would fail due to a regression. This applies only to the `test-fast` jobs on Windows, where we rerun `gix-path` tests (to make sure they work even when `git is not in the path, when Git for Windows is installed in one of the common locations). In contrast, fine if `EXEPATH` is set (or not) in the `test-fixtures-windows` jobs. Although it's possible to run the `gix-path` tests a third time with `EXEPATH` set, it wouldn't really make sense to do so when `git` is not found in `PATH`, since the scenarios where `EXEPATH` is expected to have a usable value are in a Git Bash environment where `git` will be in the `PATH` even if it is not otherwise. (It might make sense to test a Git Bash environment explicitly, but that would still be with `git` in `PATH`, and this does not add such tests. It can be done later if needed to guard against a regression, and it will also likely be done eventually if more Git for Windows installations are tested: Git for Windows SDK, MinGit, MinGit with Busybox, MinGit 32-bit, and/or Git for Windows running inside a native Windows container. The absence of tests using a Git Bash environment should not be confused with tests of interaction with the Git for Windows provided `bash` shell on Windows. That we do test: both directly, and indirectly but heavily in fixtures via `gix-testtools`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Aug 20, 2025
This updates the integration-style tests for Windows-specific `git` executable alternative locations to include locations under the per-user program files directory. The user program files directory, when present, is a subdirectory of the user's local application data directory. (The sense in which these are "integration-style" tests is noted in e9770a7 in GitoxideLabs#2115, where they were last updated.) These tests will now fail until `ALTERNATIVE_LOCATIONS` is enhanced to check in the user program files directory (as well as the global program files directories that are currently checked).
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Aug 21, 2025
This updates the integration-style tests for Windows-specific `git` executable alternative locations to include locations under the per-user program files directory. The user program files directory, when present, is a subdirectory of the user's local application data directory. (The sense in which these are "integration-style" tests is noted in e9770a7 in GitoxideLabs#2115, where they were last updated.) These tests will now fail until `ALTERNATIVE_LOCATIONS` is enhanced to check in the user program files directory (as well as the global program files directories that are currently checked).
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Aug 21, 2025
This updates the integration-style tests for Windows-specific `git` executable alternative locations to include locations under the per-user program files directory. The user program files directory, when present, is a subdirectory of the user's local application data directory. (The sense in which these are "integration-style" tests is noted in e9770a7 in GitoxideLabs#2115, where they were last updated.) These tests will now fail until `ALTERNATIVE_LOCATIONS` is enhanced to check in the user program files directory (as well as the global program files directories that are currently checked).
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Aug 21, 2025
This updates the integration-style tests for Windows-specific `git` executable alternative locations to include locations under the per-user program files directory. The user program files directory, when present, is a subdirectory of the user's local application data directory. (The sense in which these are "integration-style" tests is noted in e9770a7 in GitoxideLabs#2115, where they were last updated.) These tests will now fail until `ALTERNATIVE_LOCATIONS` is enhanced to check in the user program files directory (as well as the global program files directories that are currently checked).
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Aug 25, 2025
This updates the integration-style tests for Windows-specific `git` executable alternative locations to include locations under the per-user program files directory. The user program files directory, when present, is a subdirectory of the user's local application data directory. (The sense in which these are "integration-style" tests is noted in e9770a7 in GitoxideLabs#2115, where they were last updated.) These tests will now fail until `ALTERNATIVE_LOCATIONS` is enhanced to check in the user program files directory (as well as the global program files directories that are currently checked).
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Aug 25, 2025
This updates the integration-style tests for Windows-specific `git` executable alternative locations to include locations under the per-user program files directory. The user program files directory, when present, is a subdirectory of the user's local application data directory. (The sense in which these are "integration-style" tests is noted in e9770a7 in GitoxideLabs#2115, where they were last updated.) These tests will now fail until `ALTERNATIVE_LOCATIONS` is enhanced to check in the user program files directory (as well as the global program files directories that are currently checked).
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Aug 25, 2025
This updates the integration-style tests for Windows-specific `git` executable alternative locations to include locations under the per-user program files directory. The user program files directory, when present, is a subdirectory of the user's local application data directory. (The sense in which these are "integration-style" tests is noted in e9770a7 in GitoxideLabs#2115, where they were last updated.) These tests will now fail until `ALTERNATIVE_LOCATIONS` is enhanced to check in the user program files directory (as well as the global program files directories that are currently checked).
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Aug 25, 2025
This updates the integration-style tests for Windows-specific `git` executable alternative locations to include locations under the per-user program files directory. The user program files directory, when present, is a subdirectory of the user's local application data directory. (The sense in which these are "integration-style" tests is noted in e9770a7 in GitoxideLabs#2115, where they were last updated.) These tests will now fail until `ALTERNATIVE_LOCATIONS` is enhanced to check in the user program files directory (as well as the global program files directories that are currently checked).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
There are two bugs in
gix-paththat occur only on ARM64 Windows and not on x86_64 Windows, and one analogous (to one of them) bug ingix-testtools. ARM64 Windows systems are also, I believe, becoming more common overall, as well as there being awindows-11-armrunner, which is an ARM64 Windows system. That runner initially didn't have some things we need; it was feasible to include steps to add them, which I did; but now it has them, further simplifying things.In view of all that, this PR is to add
test-fastandtest-windows-fixturesjobs forwindows-11-arm, and fix the ARM64 Windows specific bugs.Edit: There are also a couple of small bugs in the
EXEPATHoptimization insystem_prefix(), which I fix here rather than in a separate PR, since I'm adding ARM64 support to that optimization, which might exacerbate the bugs if done without fixing them.Progress
Add/upgrade software on CI(no longer needed)"nul"instead of"NUL"asNULL_DEVICEgix-path(and explain the reason)gix-testtools(and refer to the explanation ingix-path)git.exeinclangarm64\binas well asmingw64\binclangarm64andmingw64where we emittedmingw64beforeEXEPATHoptimization insystem_prefix()to includeclangarm64clangarm64,mingw64,mingw32existgit.exebinsEXEPATHif not absoluteEXEPATHroot candidate loop to a helper and add tests for itEXEPATHis unset on CI whengix-pathis rerunRebase out old CI steps?(may need techniques in the future, keep history)Move some no-(not currently feasible)git-in-path CI steps into the test suite?This is a draft mainly because I haven't implemented the alternative locations fix yet, as attested by the failing CI steps that were added to test that. I don't expect any major delays there, but if there are any, then this could be split into two PRs so the
NULL_DEVICEfix could be merged earlier.Details on the fixes follow.
Use lower-case
"nul"instead of"NUL"asNULL_DEVICESee 9297b06 for details.
I think the subtle asymmetric treatment of the paths
nulandNULis a bug in Git for Windows, and I plan to look into that further. However, whether or not that is so, I think we should fix (or work around) it here, because older versions of Git for Windows might be used for an extended time even if it is fixed there.Look in
clangarm64\binas well asmingw64\binon ARM64Background
The need to adjust how alternative locations are found to accommodate ARM64 builds of Git for Windows was anticipated in #1456 and is reflected in some comments in
gix-path:gitoxide/gix-path/src/env/git/mod.rs
Lines 39 to 40 in d8fbe1e
gitoxide/gix-path/src/env/git/tests.rs
Lines 336 to 342 in d8fbe1e
At that time, there were no official stable ARM64 builds of Git for Windows, but they've existed for a while now.
Plan
The way I currently plan to look in both places on ARM64 is actually to look in both places on all 64-bit Windows systems. This can then be refined, which I am thinking it would be better to do in a subsequent PR.
Currently, while the tests use the Windows API, the code under test in
gix-pathrelies on environment variables to figure out where to look for Git for Windows. This works very well on 32-bit and 64-bit x86 systems, including for all combinations of 32-bit or 64-bit gitoxide and 32-bit or 64-bit Git for Windows. It also works well with ARM64 gitoxide when Git for Windows is 32-bit or 64-bit x86. It works reasonably well even in environments that are overly sanitized or where an unreasonable combination of environment variables have been passed down. That has all been in place since #1456.However, it is actually much harder to correctly use environment variables to distinguish whether a 64-bit Windows system is ARM64 or x86_64. (gitoxide--or whatever program
gix-pathis being used inside--may be an x86_64 build, even on an ARM64 system, in which case ARM64 Git for Windows should be used and even preferred to x86_64 Git for Windows.) ThePROCESSOR_ARCHITECTUREandPROCESSOR_ARCHITEW6432environment variables often facilitate this, but not always. There are also various edge cases, some of which are fairly unintuitive. In experiments, there are plausible scenarios where no way of examining just environment variables will robustly reveal that we are on an ARM64 Windows system.It should not be a problem to search multiple directories instead of one. The current design can even be kept, but replacing one or more path fragments with arrays of path fragments and iterating through them. This also prepares to support finding Git for Windows in the user's
LOCALAPPDATAdirectory--where it is the same location for all builds and thus one should look in all three ofclangarm64,mingw64, andmingw32--though I do not plan to include that enhancement in this PR.