Make the bash_program() helper in gix-testtools more robust
#1864
+47
−12
Conversation
This file contains 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
Starting in GitoxideLabs#1712, `gix-testtools` looks for `bash.exe` on Windows in one of its common locations as provided by Git for Windows, by taking the path given by `git --exec-path`, going up by three components, and going down to `bin/bash.exe` under that. But the `bin` and `bash.exe` components were appended in a way that used `\` directory separators. Ordinarily, that would be ideal, since `\` is the primary directory separator on Windows. However, when running `bash` on Windows, the path will either not be directly relevant (because it will turn into something like `/usr/bin/bash` when accessed through the shell itself such as in `$0`), or it will be used in such a way that it may either need to be quoted or appear ambiguous when examined in logs. Furthermore, the path previously mixed `/` and `\` directory separators except in the unusual case that the `GIT_EXEC_PATH` environment variable was set explicitly and its value used `\` separators, since otherwise `git --exec-path` outputs a path with `/` separators. A path with all `/` separators, provided it is a correct path, should be at least as usable as one that mixes `/` and `\`, and may make any error messages where it appears (such as in test failures) more readable. This also refactors for clarity, and adds new tests related to the change.
This is to help support running them with a shell from the Git for
Windows SDK, which, relative to the directory where it's installed
in, provides a `usr/bin` directory with a `bash.exe` executable,
like the same-named directory in a non-SDK full installation of Git
for Windows via the installer or PortableGit or or via package
managers such as `scoop` that repackage PortableGit, but unlike
those other installations does not provide a `bin` directory.
Inside the MSYS2 "Git Bash" provided as part of the Git for
Windows SDK, there are both `/bin` and `/usr/bin` directories, but
this not related to the `bin` subdirectory of the root of the
installation. On both regular Git for Windows "Git Bash" and Git
for Windows SDK "Git Bash" environments, `/bin` in the environment
is mounted to refer to the same directory as `/usr/bin`. In both,
this is separate from `(git root)/bin` as accessed outside that
environment.
For example, in an SDK "Git Bash":
$ mount
C:/git-sdk-64 on / type ntfs (binary,noacl,auto)
C:/git-sdk-64/usr/bin on /bin type ntfs (binary,noacl,auto)
C:/Users/ek/AppData/Local/Temp on /tmp type ntfs (binary,noacl,posix=0,usertemp)
C: on /c type ntfs (binary,noacl,posix=0,user,noumount,auto)
$ cygpath -w /bin /usr/bin
C:\git-sdk-64\usr\bin
C:\git-sdk-64\usr\bin
And in a non-SDK "Git Bash":
$ mount
C:/Users/ek/AppData/Local/Temp on /tmp type ntfs (binary,noacl,posix=0,usertemp)
C:/Users/ek/scoop/apps/git/2.48.1 on / type ntfs (binary,noacl,auto)
C:/Users/ek/scoop/apps/git/2.48.1/usr/bin on /bin type ntfs (binary,noacl,auto)
C: on /c type ntfs (binary,noacl,posix=0,user,noumount,auto)
$ cygpath -w /bin /usr/bin
C:\Users\ek\scoop\apps\git\2.48.1\usr\bin
C:\Users\ek\scoop\apps\git\2.48.1\usr\bin
The former has another `bin` directory alongside the `usr` directory
on disk, containing shims for `git.exe`, `sh.exe`, and `bash.exe`,
while the latter does not, but in neither case does `/bin` inside
Git Bash refer to it. This other `bin` directory was formerly used
to find `bash.exe` to run test fixture scripts.
A possible reason to continue using `(git root)/bin` would be if
the shims modify the environment in a way that makes the fixtures
operate better.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
On Windows, since #1712,
gix-testtoolslooks for abashshell associated with Git for Windows, to run fixture scripts. This logic is broadly similar togix_path::env::shell()(#1758), which it influenced (#1752 (comment)). But there are some differences, even beyondshell()ingix-pathbeing forshandbash_program()ingix-testtoolsbeing forbash. Some differences are stylistic, while others are behavioral.It seems to me that they should be more similar than they are, and that there are also some improvements that should be applied to both in the same way. The changes proposed for
shell()in #1862 include some that make it more similar tobash_program(), such as checking if the shell exists, as well as others such as preferring/, which neithershell()norbash_program()yet do but which makes sense in both.This PR is the
gix-testtoolscounterpart to thegix-pathchanges in #1862. The topical similarity could justify including both in the same PR, but having separate feature branches is more flexible, and I think the effects are also independent. In addition, neither is ready, it is possible that one will be ready before the other, and I do not know which. Currently the changes in this PR are:gix_path::env::shell()./over\in building the path tobash.exe.bash.exein(git root)/usr/bininstead of the shim in(git root)/bin.All but the last of those changes are done together, while the last is in its own commit. See commit messages for more details and rationale. Some code is analogous to code proposed for
gix-pathin #1862 but less commented. More comments could be added, especially if this PR ends up being merged without #1862 being merged or looking like it would be merged soon after.All but the last of these changes also appear to be working fine on all tested systems. In contrast, using the non-shim
bash.exebreaks at least one fixture and many tests when run locally on my Windows 10 development machine from PowerShell. The failing tests are not the ones introduced here, but preexisting tests that passed before, including when run in that environment. The failures appear to be due to errors in test fixture scripts. This situation is similar to the local failures currently blocking #1862 and I suspect the cause is the same. This is what I was referring to in #1758 (comment), and I'll try to give details of the failures (as an edit or comment here) soon.I suspect that, for the
sh.exeandbash.exeprovided by Git for Windows, the shim behavior of addingbindirectories associated with Git for Windows to the front ofPATHis necessary to prevent scripts run in those shells from invoking the wrong implementations of commands, where multiple implementations are available. But I have not determined for sure that this is the case, so this PR is not ready yet.If that is the case, then a possible solution could be to prefer the shims (both here and in #1862), though if the additional subprocess invocations inherent in doing so cause a performance problem, then the existing customization of the environment in both
gix-commandandgix-testtoolscould potentially be extended. If that performance degradation is not excessive, then the ideal solution may be to prefer the shims but fall back to the non-shim versions, which would preserve compatibility with the Git for Windows SDK in scenarios where it currently works (the SDK does not have the shims, noted in #1761 (comment), as observed in #1758 (comment) and rechecked in #1758 (comment)).