Test native Windows on CI #1745
Merged
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
This expands the CI test matrix in the main testing workflow to test on both Ubuntu and Windows, instead of just Ubuntu. It does not attempt to merge in the Cygwin workflow at this time, which may or may not end up being useful to do in the future. The new Windows test jobs all fail currently: the runs fail as a result of various tests being consistently unable to pass but not yet being marked xfail (or skip). It should be feasible to mark these xfail with informative messages, but this commit doesn't include that.
8 of the tests that fail on native Windows systems fail due to IndexFile.from_tree being broken on Windows, causing gitpython-developers#1630. This commit marks those tests as xfail. This is part, though not all, of the changes to get CI test jobs for native Windows that are passing, to guard against new regressions and to allow the code and tests to be gradually fixed (see discussion in gitpython-developers#1654). When fixing the bug, this commit can be reverted.
This other GitCommandError on Windows is not related to IndexFile.from_tree whose 8 related failing tests were marked xfail in the preceding commit. Also, test_clone_command_injection should not be confused with test_clone_from_command_injection, which passes on all platforms. The problem here appears to be that, on Windows, the path of the directory GitPython is intended to clone to -- when the possible security vulnerability this test checks for is absent -- is not valid. This suggests the bug may only be in the test and that the code under test may be working on Windows. But the test does not establish that, for which it would need to test with a payload clearly capable of creating the file unexpected_path refers to when run on its own. This doesn't currently seem to be reported as a bug, but some general context about the implementation can be examined in gitpython-developers#1518 where it was introduced, and gitpython-developers#1531 where it was modified.
Everything attempted in the test case method is actually working and passes, but when the tearDown method calls shutil.rmtree, it fails with "Access is denied" and raises PermissionError. The reason and exception are accordingly noted in the xfail decoration. While adding a pytest import to apply the pytest xfail mark, I also improved grouping/sorting of other imports in the test_diff module.
The test fails when attempting to evaluate the expression:
sm.move(new_path).name
as part of the assertion:
assert sm.move(new_path).name == new_path
But it is the call to sm.move that fails, not the assertion itself.
The condition is never evaluated, because the subexpression raises
PermissionError. Details are in the xfail reason string.
This test_submodule.TestSubmodule.test_rename method should not be
confused with test_config.TestBase.test_rename, which passes on all
platforms.
On CI this has XPASS status on Python 3.7 and possibly some other
versions. This should be investigated and, if possible, the xfail
markings should be made precise. (When working on this change
before a rebase, I had thought only 3.7 xpassed, and that the XPASS
was only on CI, never locally. However, I am unable to reproduce
that or find CI logs of it, so some of these observations may have
been mistaken and/or or specific to a narrow environment.)
As noted, the second of the config._included_paths() assertions
fails, which is in the "Ensure that config is included if path is
matching git_dir" sub-case.
It is returning 0 on Windows. THe GitConfigParser._has_includes
function returns the expression:
self._merge_includes and len(self._included_paths())
Since _merge_includes is a bool, it appears the first branch of the
"and" is True, but then _included_paths returns an empty list.
In modules soon to be modified, so if subsequent commits are later reverted, these import tweaks are not automatically undone.
The test mostly works on Windows, but it fails because the tmp_file path is expected to appear in remote_url with backslash separators, but (forward) slashes appear instead.
The tests of unsafe options are among those introduced originally in gitpython-developers#1521. They are regression tests for gitpython-developers#1515 (CVE-2022-24439). The unsafe options tests are paired: a test for the usual, default behavior of forbidding the option, and a test for the behavior when the option is explicitly allowed. In each such pair, both tests use a payload that is intended to produce the side effect of a file of a specific name being created in a temporary directory. All the tests work on Unix-like systems. On Windows, the tests of the *allowed* cases are broken, and this commit marks them xfail. However, this has implications for the tests of the default, secure behavior, because until the "allowed" versions work on Windows, it will be unclear if either are using a payload that is effective and that corresponds to the way its effect is examined. What *seems* to happen is this: The "\" characters in the path are treated as shell escape characters rather than literally, with the effect of disappearing in most paths since most letters lack special meaning when escaped. Also, "touch" is not a native Windows command, and the "touch" command provided by Git for Windows is linked against MSYS2 libraries, causing it to map (some?) occurrences of ":" in filenames to a separate code point in the Private Use Area of the Basic Multilingual Plane. The result is a path with no directory separators or drive letter. It denotes a file of an unintended name in the current directory, which is never the intended location. The current directory depends on GitPython implementation details, but at present it's the top-level directory of the rw_repo working tree. A new unstaged file, named like "C\357\200\272UsersekAppDataLocalTemptmpc7x4xik5pwn", can be observed there (this is how "git status" will format the name). Fortunately, this and all related tests are working on other OSes, and the affected code under test does not appear highly dependent on OS. So the fix is *probably* fully working on Windows as well.
Along with the version and platform information that is already shown. This is to make debugging easier.
Only on native Windows systems (since this is for debugging WSL). The Windows CI runners have WSL itself installed but no WSL systems installed. So some of these commands may fail if they call the bash.exe in the System32 directory, but this should still be illuminating. Results after a WSL system is set up will likely be more important, but doing this first allows for comparison. The reason PATH directories are no longer listed is to decrease noise.
This makes three of the four hook-related tests pass instead of failing, and changes the way the fourth fails. GitPython uses bash.exe to run hooks that are (or appear to be) shell scripts. On many Windows systems, this is the bash.exe in the system32 directory, which delegates to bash in a WSL system if at least one such system is installed (for the current user), and gives an error otherwise. It may be a bug that GitPython ends up using this bash.exe when WSL is installed but no WSL systems exist, since that is actually a fairly common situation. One place that happened was on the GitHub Actions runners used for Windows jobs. Those runners have WSL available, and are capable of running WSL 1 systems (not currently WSL 2 systems), but no WSL systems were actually installed. This commit fixes that cause of failure, for all four tests it happened in, by setting up a Debian WSL system on the test runner. (This increases the amount of time it takes Windows jobs to run, but that might be possible to improve on.) Three of those four tests now pass, while the other fails for another reason. The newly passing tests are: - test/test_index.py::TestIndex::test_commit_msg_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_success The test that still fails, but differently, is: - test/test_index.py::TestIndex::test_commit_msg_hook_success I had previously found that test to fail on a local WSL 2 system, and attempted to add a suitable xfail marking in 881456b (gitpython-developers#1679). But the condition I wrote there *appears* to have a bug related to the different orders in which subproces.Popen and shutil.which find executables, causing it not always to detect when the WSL-related bash.exe is the one the Popen call in git.index.fun.run_commit_hook will use.
881456b (gitpython-developers#1679) expanded the use of shutil.which in test_index.py to attempt to mark test_commit_msg_hook_success xfail when bash.exe is a WSL bash wrapper in System32 (because that test currently is not passing when the hook is run via bash in a WSL system, which the WSL bash.exe wraps). But this was not reliable, due to significant differences between shell and non-shell search behavior for executable commands on Windows. As the new docstring notes, lookup due to Popen generally checks System32 before going through directories in PATH, as this is how CreateProcessW behaves. - https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw - python/cpython#91558 (comment) The technique I had used in 881456b also had the shortcoming of assuming that a bash.exe in System32 was the WSL wrapper. But such a file may exist on some systems without WSL, and be a bash interpreter unrelated to WSL that may be able to run hooks. In addition, one common situation, which was the case on CI before a step to install a WSL distribution was added, is that WSL is present but no WSL distributions are installed. In that situation bash.exe is found in System32, but it can't be used to run any hooks, because there's no actual system with a bash in it to wrap. This was not covered before. Unlike some conditions that prevent a WSL system from being used, such as resource exhaustion blocking it from being started, the absence of a WSL system should probably not fail the pytest run, for the same reason as we are trying not to have the complete *absence* of bash.exe fail the pytest run. Both conditions should be xfail. Fortunately, the error message when no distribution exists is distinctive and can be checked for. There is probably no correct and reasonable way to check LBYL-style which executable file bash.exe resolves to by using shutil.which, due to shutil.which and subprocess.Popen's differing seach orders and other subtleties. So this adds code to do it EAFP-style using subprocess.run (which itself uses Popen, so giving the same CreateProcessW behavior). It tries to run a command with bash.exe whose output pretty reliably shows if the system is WSL or not. We may fail to get to the point of running that command at all, if bash.exe is not usable, in which case the failure's details tell us if bash.exe is absent (xfail), present as the WSL wrapper with no WSL systems (xfail), or has some other error (not xfail, so the user can become aware of and proably fix the problem). If we do get to that point, then a file that is almost always present on both WSL 1 and WSL 2 systems and almost always absent on any other system is checked for, to distinguish whether the working bash shell operates in a WSL system, or outside any such system as e.g. Git Bash does. See https://superuser.com/a/1749811 on various techniques for checking for WSL, including the /proc/sys/fs/binfmt_misc/WSLInterop technique used here (which seems overall may be the most reliable). Although the Windows CI runners have Git Bash, and this is even the bash.exe that appears first in PATH (giving rise to the problem with shutil.which detailed above), it would be a bit awkward to test the behavior when Git Bash is actually used to run hooks on CI, because of how Popen selects the System32 bash.exe first, whether or not any WSL distribution is present. However, local testing shows that when Git Bash's bash.exe is selected, all four hook tests in the module pass, both before and after this change, and furthermore that bash.exe is correctly detected as "native", continuing to avoid an erroneous xfail mark in that case.
- Make the design of the enum simpler. - Move some informaton to the check method's docstring. - Call the method once instead of in each decoration. Doing the check only once makes things a little faster, but the more important reason is that the checks can become stale very quickly in some situations. This is unlikely to be an issue on CI, but locally WSL may fail (or not fail) to start up when bash.exe is used in a check, then not fail (or fail) when actaully needed in the test, if the reason it fails is due to resource usage, such as most RAM being used on the system. Although this seems like a reason to do the check multiple times, doing it multiple times in the decorations still does it ahead of when any of the tests is actually run. In contrast, with the change here, we may get outdated results by the time the tests run, but the xfail effects of the check are always consistent with each other and are no longer written in a way that suggests they are more current than they really are.
This reverts "Install WSL system on CI for hook tests", commit 9717b8d. Assuming both cases are found to be working, the removed step will either be brought back or replaced with a very similar step that installs a different distribution.
This undoes "Temporarily don't install WSL system to test xfail" (cabb572). It keeps Debian as the distribution. (Although the Debian WSL system installs pretty fast already, it may still make sense to try switching to Alpine in the future. But that might need to wait until Vampire/setup-wsl#50 is fixed.) This also removes most of the commands in the WSL debugging step, since the related machinery in test_index.py (_WinBashStatus) seems to be in okay shape, condenses the smaller number of commands that are retained there, and makes much less extensive reduction in the general version and platform information commands as well. This is to make workflow output easier to read, understand, and navigate.
This causes full "failure" output to be printed when a test marked xfail unexpectedly passes, and for the test run to be considered failing as a result. The immediate purpose of this change is to facilitate efficient identification of recently introduced wrong or overbroad xfail markings. This behavior may eventually become the pytest default (see gitpython-developers#1728 and references therein), and this could be retained even after the current xpassing tests are investigated, to facilitate timely detection of tests marked xfail of code that is newly working. (Individual tests decorated `@pytest.mark.xfail` can still be allowed to unexpectedly pass without it being treated like a test failure, by passing strict=False explicitly.)
The xfail mark was added in 42a3d74, where the XPASS status on 3.7 was observed but not included in the condition. It turns out this seems to XPASS on a much wider range of versions: all but 3.12. Currently 3.12.0 is the latest stable version and no testing has been done with alpha for 3.13. Most likely whatever causes this test to fail on 3.12 would also apply to 3.13, but because I don't understand the cause, I don't want to guess that, and instead wrote the new condition to expect failure only on 3.12.* versions.
For its immediate goal, this facilitated identifying the XPASS status of TestSubmodule.test_rename and verifying that the revised xfail condition eliminated it. In the test output, XPASS is written as a failure, with strict xfail behavior noted. Contributors less familiar with marking tests xfail may mistake this to mean some behavior of the code under test was broken. So if strict_xfail is to be enabled, it might be best to do that in a pull request devoted to it, and maybe even to add to the docs, about how to recognize and handle newly xpassing tests. This reverts commit 0f8cd4c.
This further improves the condition that was corrected in 82c361e. Testing on Python 3.13.0 alpha 2 shows the same failure as on 3.12 (that I'm at least right now consistently unable to produce on any lower versions). In addition, on both versions of CPython on Windows, the failure seems to consistently resolve if two gc.collect() are placed just above the code that calls sm.move(). A single call is consistently insufficient. I haven't included any such calls in this commit, since the focus here is on fixing xfail markings, and becuse such a change may benefit from being evaluated separately and may merit further accompanying changes. But that this behavior is exhibited on both 3.12 and the 3.13 alpha further supports removing the upper bound on the xfail marking.
This changes _WinBashStatus from an enum to a sum type so that, rather than having a global _WinBashStatus.ERROR_WHILE_CHECKING object whose error_or_process attribute may be absent and, if present, carries an object set on the most recent call to check(), we get a _WinBashStatus.ErrorWhileChecking instance that carries an error_or_process attribute specific to the call. Ordinarily this would not make a difference, other than that the global attribute usage was confusing in the code, because typically _WinBashStatus.check() is called only once. However, when debugging, having the old attribute on the same object be clobbered is undesirable. This adds the sumtypes library as a test dependency, to allow the enum to be rewritten as a sum type without loss of clarity. This is a development-only dependency; the main dependencies are unchanged.
This removes text=True from the subprocess.run call, changing str
literals to bytes where appropriate and (less importantly) using
"%r" instead of "%s" in log messages so it's clear that printing
the repr of a bytes object is, at least for now, intentional.
The reason for this is that the encoding of error messages produced
by running the WSL bash.exe, when it attempts but fails to use a
WSL system, varies based on what error occurred. When no systems
are installed, the output can be decoded as UTF-8. When an error
from "deeper down" is reported, at least for Bash/Service errors,
the output is encoded in UTF-16LE, and attempting to decode it as
UTF-8 interleaves lots of null characters in the best case. With a
bytes object, loss of information is avoided, and it is clear on
inspection that the output requires decoding.
The most common case of such an error is *probably*:
Insufficient system resources exist to complete the requested service.
Error code: Bash/Service/CreateInstance/CreateVm/HCS/0x800705aa
However, that is tricky to produce intentionally on some systems.
To produce a test error, "wsl --shutdown" can be run repeatedly
while a _WinBashStatus.check() call is in progress. This produces:
The virtual machine or container was forcefully exited.
Error code: Bash/Service/0x80370107
Assuming the output always includes the text "Error code:", it may
be feasible to reliably detect which cases it is. This could
especially improve the log message. But for now that is not done.
In adddition to changing from text to binary mode, this commit also
temporarily removes the setup-wsl step from the CI workflow again,
to verify on CI that the text-to-binary change doesn't break the
WslNoDistro case. Manual testing shows the other cases still work.
This affects the test suite only. It improves _WinBashStatus. When bash.exe is the WSL wrapper, and it reports an error that prevents bash in a WSL system from ever actually being run, the message may be encoded in a narrow or multibyte encoding, or may instead be in Windows's native UTF-16LE encoding. This was partly handled before, by assuming the message indicating the absence of any installed WSL distribuions could be interpreted as UTF-8, but matching against bytes and not actually decoding it or other error messages. That presented a few problems, which are rememedied here: - It turns out that this "Windows Subsystem for Linux has no installed distributions" message actually can be in UTF-16LE too. This happens when it is part of a message that also reports a textual error code like: Bash/Service/CreateInstance/GetDefaultDistro/WSL_E_DEFAULT_DISTRO_NOT_FOUND Therefore, narrow-encoding matching was not sufficient. - Logged messages were hard to understand, because they were printing the reprs of bytes objects, which sometimes were UTF-16LE and thus had many "\x00" interspersed in them. - The cases were not broken down elegantly. The ErrorWhileChecking case could hold a CompletedProcess, a CalledProcessError, or an OSError. This is now replaced with a WinError case for the rare scenario where CreateProcessW fails in a completely unexpected way, and a CheckError case for when the exit status or output of bash.exe indicates an error other than one we want to handle. CheckError has two attributes: `process` for the CompletedProcess (CalledProcessError is no longer used, and CompletedProcess has `returncode` and other attributes that provide the same info), and `message` for the decoded output.
Instead of searching for an English sentence from the error message, this searches for the specific aka.ms short URL it contains in all languages, which points to a page about downloading distributions for WSL from the Microsoft Store (Windows Store). The URL is controlled and hosted by Microsoft and intended as a permalink. Thus this approach should be more reliable even for English, as the specific wording of the message might be rephrased over time, so this may have a lower risk of false negatives. Because it only makes sense to give a link for obtaining a distribution in an error message when no distribution is installed already, the risk of false positives should also be fairly low. The URL is included in the output telling the user they have no distributions installed even on systems like Windows Server where the Microsoft Store is not itself available, and even in situations where WSL has successfully downloaded and unpacked a distribution but could not run it the first time because it is using WSL 2 but the necessary virtualization is unavailable. Because these are included among the situations where the hook tests should be marked xfail, it is suitable for the purpose at hand. Furthermore, while this only works if we correctly guess if the encoding is UTF-16LE or not, it should be *fairly* robust against incorrect decoding of the message where a single-byte encoding is treated as UTF-8, or UTF-8 is treated as a single-byte encoding, or one single-byte encoding is treated as another (i.e., wrong ANSI code page). But some related encoding subtleties remain to be resolved. Although reliability is likely improved even for English, to faciliate debugging the WslNoDistro case now carries analogous `process` and `message` arguments to the `CheckError` case. On some Windows systems, wsl.exe exists in System32 but bash.exe does not. This is not inherently a problem for run_commit_hook as it currently stands, which is just trying to use whatever bash.exe is available. (It is not clear that WSL should be preferred.) This is currently so, on some systems, where WSL is not really set up; wsl.exe can be used to do so. This is a situation where no WSL distributions are present, but because the bash.exe WSL wrapper is also absent, it is not a problem. However, I had wrongly claimed in the _WinBashStatus.check docstring that bash.exe is in System32 whenever WSL is present. So this also revises the docstring to say only that this is usually so (plus some further revision for clarity).
Windows does not have a direct analogue of LANG=C or LC_ALL=C. Some programs give them special treatment, but they do not affect the way localized behavior of the Windows API operates. In particular, the bash.exe WSL wrapper, as well as wsl.exe and wslconfig.exe, do not produce their own localized messages (for errors not originating in a running distribution) when they are set. Windows also provides significant localization through localized versions of Windows, so changing language settings in Windows, even system-wide, does not always produce the same effect that many or most Windows users who use a particular language would experience. Various encodings may appear when bash.exe is WSL-related but gives its own error message. Such a message is often in UTF-16LE, which is what Windows uses internally, and preserves any localization. That is the more well behaved scenario and already was detected; this commit moves, but does not change, the code for that. The situation where it is not UTF-16LE was previously handled by treating it as UTF-8. Because the default strict error treatment was used, this would error out in test discovery in some localized setups, preventing all tests in test_index from running, including the majority of them that are not related to hooks. This fixes that by doing better detection that should decode the mesages correctly most of the time, that should in practice decode them well enough to tell (by the aka.ms URL) if the message is complaining about there being no installed distribution all(?) of the time, and that should avoid breaking unrelated tests even if that can't be done. An English non-UTF-16LE message appears on GitHub Actions CI when no distribution is installed. Testing of this situation on other languages was performed in a virtual machine on a development machine. That the message is output in a narrow character set some of the time when bash.exe produces it appears to be a limitation of the bash.exe wrapper. In particular, with a zh-CN version of Windows (and with the language not changed to anything else), a localized message in Simplified Chinese was correctly printed when running wsl.exe, but running bash.exe produced literal "?" characters in place of Chinese characters (it was not a display or font issue, and they were "?" and not Unicode replacement characters). The change here does not overcome that; the literal "?" characters will be included. But "https://aka.ms/wslstore" is still present if it is an error about not having any distributions, so the correct status is still inferred. For more information on code pages in Windows, see: https://serverfault.com/a/836221 The following alternatives to all or part of the approach taken here were considered but, at least for now, not done, because they would not clearly be simpler or more correct: - Abandoning this "run bash.exe and see what it shows us" approach altogether and instead reimplementing the rules CreateProcessW uses, to find if the bash.exe the system finds is the one in System32, and then, if so, checking the metadata in that executable to determine if it's the WSL wrapper. I believe that would be even more complex to do correctly than it seems; the behavior noted in the WinBashStatus docstring and recent commit messages is not the whole story. The documented search order for CreateProcessW seems not to be followed in some circumstances. One is that the Windows Store version of Python seems always to forgo the usual System32 search that precedes seaching directories in PATH. It looks like there may also be some subtleties in which directories 32-bit builds search in. - Using chardet. Although the chardet library is excellent, it is not clear that the code needed to bridge this highly specific use case to it would be simpler than the code currently in use. Some of the work might still need to be done by other means; when I tested it out for this, this did not detect the UTF-16LE messages as such for English. (They are often also valid UTF-8, because interleaving null characters is, while strange, permitted.) - Also calling wsl.exe and/or wslconfig.exe. It's still necessary to call bash.exe, because it may not be the WSL bash, even on a system with WSL fully set up. Furthermore, those tools' output seem to vary in some complex ways, too. Using only one subprocess for the detection seemed the simplest. Even using "wsl --list" would introduce significant additional logic. Sometimes its output is a list of distributions, sometimes it is an error message, and if WSL is not set up it may be a help message. - Using the Windows API to check for WSL systems. https://learn.microsoft.com/en-us/windows/win32/api/wslapi/ does not currently include functions to list registered distributions. - Attempting to get wsl.exe to produce an English message using Windows API techniques like those used in Locale Emulator. This would be complicated, somewhat unintuitive and error prone to do in Python, and I am not sure how well it would work on a system that does not have an English language pack installed. - Checking on disk for WSL distributions in the places they are most often expected to be. This would intertwine WinBashStatus with deep details of how WSL actually operates, and this seems like the sort of thing that is likely to change in the future. However, there may be a more straightforward way to do this (that is at least as correct and that remains transparent to debug). Especially if the WinBashStatus class remains in test_index for long (rather than just being used to aid in debugging existing test falures and possible subsequent design decisions for making commit hooks work more robustly on Windows in GitPython), then this may be worth revisiting. Thus it is *not* with the intention of treating WinBashStatus as a "stable" part of the test suite that it is renamed from _WinBashStatus. This is instead done because: - Like HOOKS_SHEBANG, it makes sense to import it when figuring out how the tests work or debugging them. - When debugging, it is intended that it be imported to call check() and examine the resulting `process` and `message` information, at least in the CheckError case.
So the Windows test jobs can run the three out of four hook tests that are able to pass with WSL bash, and so that we are able to observe the expected failure on the one that still fails. This undoes the removal of the setup-wsl step in d5ed266, now that the test suite's bash.exe status checking logic, used for xfail marks (and also usable in debugging), is internationalized. The CI step was omitted during some of this process, to make sure WinBashStatus would still detect the specific situation on the GitHub Actions runner that occurs when no WSL distro is available. (This commit thus has much of the same relationship to d5ed266 as 2875ffa had to cabb572.)
- Factor out the code to get the Windows ACP to a helper function, and comment to explain why it doesn't use locale.getencoding. - Remove nonessential material in the WinBashStatus.check docstring and reword the rest for clarity. - Drop reStructuredText notation in the WinBashStatus docstrings, because in this case it seems to be making them harder to read in the code (we are not generating Sphinx documentation for tests.) - Revise the comments on specific steps in WinBashStatus._decode for accuracy and clarity.
This removes the parenthesized examples from the per-step comments in the WinBashStatus._decode helper.
Byron
approved these changes
Nov 29, 2023
There was a problem hiding this comment.
Thanks so much for landing this long-awaited PR that finally adds regression tests for native Windows system. I think it's a big day for GitPython, which should help to make it better on Windows in general.
I wrote
WinBashStatuswith the intention that, if necessary, it could be retained for an extended time. However, my hope for it is that it will actually disappear (perhaps with some insights from the process making their way into other code). If, in the future, hooks are run on Windows in a way that works on more systems, and has fewer strange edge cases, then there may be no more need for it or anything like it.
Just as a note in case it's ever relevant: I have been mimicking gits way of launching programs at least to the extend that I understand it with the gix-command crate, and it has some special handling for sh actually for Windows as well. From what I can tell, git doesn't even try to run a shell on Windows, or it's so well hidden that I didn't see it. If that's true, then being able to run hooks more consistently on Windows would definitely be a big deal, and is maybe something that gix-command can also learn from.
With many pushes over a short time--as for example in this pull request, where I pushed the commits separately with ten second delays in between, to make it easy to inspect intermediate CI results--there can be more jobs queued than immediately available runners. (I believe the number of runners is limited per organization.) It is in that case that the number of tests can be most of an issue. Even if you decide to accept this PR without the number of Windows jobs being decreased, it might turn out later that this is annoying. I accept that you may know now that you want fewer Windows test jobs, but also, I understand that if you accept this as it is now, that doesn't mean all the jobs will be kept forever.
I am also positively surprised by the speed at which these Runners work, and agree that at least for now it's probably more beneficial to test all python versions, than only testing a subset. If there are low-hanging fruit to be picked, like intentionally not installing WSL everywhere maybe to also have more platform-diversity, this could be done in follow-up PRs. It also is, however, optional and depends on how much value you see in testing more variations effectively. If building documentation is not depending on the python versions but costs significant time, it might be a candidate for (maybe partial) culling as well.
Overall, GitPython's CI is very fast at least compared to what I am used to, and only Cygwin sours it a bit. Over time, maybe there are ways to improve that too, but I wouldn't even consider that very important unless it's particularly annoying during development.
I am unsure if caching will actually work on it in the CI jobs run on this repository due to the
pull-requesttrigger. My vague recollection is that caching does not happen for open PRs that introduce it. If that is the case, then the amount by which installing WSL slows down each Windows job, as observed here, may be more than I have observed.
I think it would be worth keeping an eye out for the effects of caching in relation to WSL installation and maybe in the context of PRs. It's easy for caches to end up as net-negative if they are too expensive themselves - this easily happens with Rust which produces caches so large they take too much time to update and restore. Probably this project is still far and away from such an effect though.
What is not done here
This section is full of ideas that I very much look forward to, in particular:
- test caching and the effect of using Alpine as DSL instead of Debian. This would also be a first for using a MUSL linux system.
- adding MacOS runners - I am using MacOS and could at least double-check. Also, I haven't run the tests in a long time.
Some more remarks
I really like the way xfail is used to declare where and under which circumstances failures are expected. Seeing the python version of an enum was interesting for sure, and I can see how it can serve as vessel to document various cases, and store information depending on the case as needed. I thought it was interesting to see how this could look if translated to Rust (please note that this was just for fun and only glanced at the result).
Otherwise, it's great work as always and I hope that the users of GitPython will eventually feel the increase in quality (or at the minimum the lack of loss of quality) that all this work will bring.
Thank you!
Merged
Cool! I'll take a look at this soon--and reply further.
I haven't looked into this for hooks. But custom At the time the There is also a current proposal in Git for Windows to make what shell it uses configurable at runtime.
I am also looking forward to this. Of course, this is just for the WSL system used to run shell scripts on Windows (in the cases that WSL is what's used to do that). Although this does not diminish the likely value in replacing Debian with Alpine for WSL, I think the even more interesting and potentially illuminating thing to do would be to run GitPython itself, and its whole test suite, on Alpine Linux. I don't think GitHub Actions has Alpine Linux runners, but this could be done in a Docker container on an Ubuntu runner. |
That's a very interesting proposal, thanks for making me aware! It's worth noting that (I mix both libraries because it seems that GitPython could also benefit from that outcome at some point.)
Yes, |
I think it wouldn't work the same, but that the spirit of this idea is true. Specifically, I think the question is whether it would find bugs not already in practice found by testing on both Ubuntu and macOS. I am thinking it probably would not, but it would be interesting to see. Because Alpine Linux has a different set of executable commands available with different implementations, testing on it could catch unwarranted assumptions. For example, Alpine Linux Maybe a more interesting use of Docker and Alpine Linux could be to provide an alternative to remotes over a non-local network for tests that use them. Because not everyone can run Docker, I wouldn't advocate making currently working tests require it, but it could be useful. Furthermore, at least one test that is not currently working, |
|
I see! The value of running the test-suite within Docker, Alpine or not, probably also shouldn't be under-estimated. I am definitely open to that if you also think it's worth exploring - failing tests could probably be marked with |
This was referenced Dec 12, 2023
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this pull request
Dec 21, 2023
Precise xfail marks were added to commit hook tests in gitpython-developers#1745, but in a few tests I didn't get it right for WinBashStatus.Absent. That is, on a Windows system that has no bash.exe at all: - More of the tests are unable to pass than have xfail marks. - One of the tests, test_commit_msg_hook_success, which does have an xfail mark for that, wrongly combines it with the xfail mark for WinBashStatus.Wsl. That test is the only one where the WSL bash.exe, even when a working WSL distribution is installed, is unable to pass. But using a single mark there is wrong, in part because the "reason" is not correct for both, but even more so because the exceptions they raise are different: AssertionError is raised when the WSL bash.exe is used in that test, but when bash.exe is altogether absent, HookExecutionError is raised. This fixes that by adding xfail marks for WinBashStatus.Absent where missing, and splitting test_commit_msg_hook_success's xfail mark that unsuccessfully tried to cover two conditions into two separate marks, each of which gives a correct reason and exception. This commit also rewords the xfail reason given for WslNoDistro, which was somewhat unclear and potentially ambiguous, to make it clearer.
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this pull request
Dec 21, 2023
This has three benefits: - run_commit_hook is public, being listed in git.index.fun.__all__, so it makes sense for it to have its own test. - When investigating (future, or current xfail-covered) failure of functions that use run_commit_hook, it will be useful to compare the results of other tests that already do exist to that of a direct test of run_commit_hook. - When changing which bash.exe run_commit_hook selects to use on Windows (including to ameliorate the limitation covered by the WinBashStatus.WslNoDistro xfail marks, or to attempt other possible changes suggested in gitpython-developers#1745), or even just to investigate the possibility of doing so, it will make sense to add tests like this test but for more specific conditions or edge cases. Having this typical-case test to compare to should be helpful both for writing such tests and for efficiently verifying that the conditions they test are really the triggers for any failures.
Merged
lettuce-bot bot
added a commit
to lettuce-financial/github-bot-signed-commit
that referenced
this pull request
Jan 10, 2024
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [GitPython](https://togithub.com/gitpython-developers/GitPython) | `==3.1.40` -> `==3.1.41` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>gitpython-developers/GitPython (GitPython)</summary> ### [`v3.1.41`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.41): - fix Windows security issue [Compare Source](https://togithub.com/gitpython-developers/GitPython/compare/3.1.40...3.1.41) The details about the Windows security issue [can be found in this advisory](https://togithub.com/gitpython-developers/GitPython/security/advisories/GHSA-2mqj-m65w-jghx). Special thanks go to [@​EliahKagan](https://togithub.com/EliahKagan) who reported the issue and fixed it in a single stroke, while being responsible for an incredible amount of improvements that he contributed over the last couple of months ❤️. #### What's Changed - Add `__all__` in git.exc by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1719 - Set submodule update cadence to weekly by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1721 - Never modify sys.path by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1720 - Bump git/ext/gitdb from `8ec2390` to `ec58b7e` by [@​dependabot](https://togithub.com/dependabot) in [gitpython-developers/GitPython#1722 - Revise comments, docstrings, some messages, and a bit of code by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1725 - Use zero-argument super() by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1726 - Remove obsolete note in \_iter_packed_refs by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1727 - Reorganize test_util and make xfail marks precise by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1729 - Clarify license and make module top comments more consistent by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1730 - Deprecate compat.is\_<platform>, rewriting all uses by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1732 - Revise and restore some module docstrings by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1735 - Make the rmtree callback Windows-only by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1739 - List all non-passing tests in test summaries by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1740 - Document some minor subtleties in test_util.py by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1749 - Always read metadata files as UTF-8 in setup.py by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1748 - Test native Windows on CI by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1745 - Test macOS on CI by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1752 - Let close_fds be True on all platforms by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1753 - Fix IndexFile.from_tree on Windows by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1751 - Remove unused TASKKILL fallback in AutoInterrupt by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1754 - Don't return with operand when conceptually void by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1755 - Group .gitignore entries by purpose by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1758 - Adding dubious ownership handling by [@​marioaag](https://togithub.com/marioaag) in [gitpython-developers/GitPython#1746 - Avoid brittle assumptions about preexisting temporary files in tests by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1759 - Overhaul noqa directives by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1760 - Clarify some Git.execute kill_after_timeout limitations by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1761 - Bump actions/setup-python from 4 to 5 by [@​dependabot](https://togithub.com/dependabot) in [gitpython-developers/GitPython#1763 - Don't install black on Cygwin by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1766 - Extract all "import gc" to module level by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1765 - Extract remaining local "import gc" to module level by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1768 - Replace xfail with gc.collect in TestSubmodule.test_rename by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1767 - Enable CodeQL by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1769 - Replace some uses of the deprecated mktemp function by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1770 - Bump github/codeql-action from 2 to 3 by [@​dependabot](https://togithub.com/dependabot) in [gitpython-developers/GitPython#1773 - Run some Windows environment variable tests only on Windows by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1774 - Fix TemporaryFileSwap regression where file_path could not be Path by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1776 - Improve hooks tests by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1777 - Fix if items of Index is of type PathLike by [@​stegm](https://togithub.com/stegm) in [gitpython-developers/GitPython#1778 - Better document IterableObj.iter_items and improve some subclasses by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1780 - Revert "Don't install black on Cygwin" by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1783 - Add missing pip in $PATH on Cygwin CI by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1784 - Shorten Iterable docstrings and put IterableObj first by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1785 - Fix incompletely revised Iterable/IterableObj docstrings by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1786 - Pre-deprecate setting Git.USE_SHELL by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1782 - Deprecate Git.USE_SHELL by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1787 - In handle_process_output don't forward finalizer result by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1788 - Fix mypy warning "Missing return statement" by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1789 - Fix two remaining Windows untrusted search path cases by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1792 #### New Contributors - [@​marioaag](https://togithub.com/marioaag) made their first contribution in [gitpython-developers/GitPython#1746 - [@​stegm](https://togithub.com/stegm) made their first contribution in [gitpython-developers/GitPython#1778 **Full Changelog**: gitpython-developers/GitPython@3.1.40...3.1.41 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/lettuce-financial/github-bot-signed-commit). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMjcuMCIsInVwZGF0ZWRJblZlciI6IjM3LjEyNy4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
renovate bot
added a commit
to allenporter/flux-local
that referenced
this pull request
Jan 11, 2024
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [GitPython](https://togithub.com/gitpython-developers/GitPython) | `==3.1.40` -> `==3.1.41` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>gitpython-developers/GitPython (GitPython)</summary> ### [`v3.1.41`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.41): - fix Windows security issue [Compare Source](https://togithub.com/gitpython-developers/GitPython/compare/3.1.40...3.1.41) The details about the Windows security issue [can be found in this advisory](https://togithub.com/gitpython-developers/GitPython/security/advisories/GHSA-2mqj-m65w-jghx). Special thanks go to [@​EliahKagan](https://togithub.com/EliahKagan) who reported the issue and fixed it in a single stroke, while being responsible for an incredible amount of improvements that he contributed over the last couple of months ❤️. #### What's Changed - Add `__all__` in git.exc by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1719 - Set submodule update cadence to weekly by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1721 - Never modify sys.path by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1720 - Bump git/ext/gitdb from `8ec2390` to `ec58b7e` by [@​dependabot](https://togithub.com/dependabot) in [gitpython-developers/GitPython#1722 - Revise comments, docstrings, some messages, and a bit of code by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1725 - Use zero-argument super() by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1726 - Remove obsolete note in \_iter_packed_refs by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1727 - Reorganize test_util and make xfail marks precise by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1729 - Clarify license and make module top comments more consistent by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1730 - Deprecate compat.is\_<platform>, rewriting all uses by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1732 - Revise and restore some module docstrings by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1735 - Make the rmtree callback Windows-only by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1739 - List all non-passing tests in test summaries by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1740 - Document some minor subtleties in test_util.py by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1749 - Always read metadata files as UTF-8 in setup.py by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1748 - Test native Windows on CI by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1745 - Test macOS on CI by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1752 - Let close_fds be True on all platforms by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1753 - Fix IndexFile.from_tree on Windows by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1751 - Remove unused TASKKILL fallback in AutoInterrupt by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1754 - Don't return with operand when conceptually void by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1755 - Group .gitignore entries by purpose by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1758 - Adding dubious ownership handling by [@​marioaag](https://togithub.com/marioaag) in [gitpython-developers/GitPython#1746 - Avoid brittle assumptions about preexisting temporary files in tests by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1759 - Overhaul noqa directives by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1760 - Clarify some Git.execute kill_after_timeout limitations by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1761 - Bump actions/setup-python from 4 to 5 by [@​dependabot](https://togithub.com/dependabot) in [gitpython-developers/GitPython#1763 - Don't install black on Cygwin by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1766 - Extract all "import gc" to module level by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1765 - Extract remaining local "import gc" to module level by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1768 - Replace xfail with gc.collect in TestSubmodule.test_rename by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1767 - Enable CodeQL by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1769 - Replace some uses of the deprecated mktemp function by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1770 - Bump github/codeql-action from 2 to 3 by [@​dependabot](https://togithub.com/dependabot) in [gitpython-developers/GitPython#1773 - Run some Windows environment variable tests only on Windows by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1774 - Fix TemporaryFileSwap regression where file_path could not be Path by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1776 - Improve hooks tests by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1777 - Fix if items of Index is of type PathLike by [@​stegm](https://togithub.com/stegm) in [gitpython-developers/GitPython#1778 - Better document IterableObj.iter_items and improve some subclasses by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1780 - Revert "Don't install black on Cygwin" by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1783 - Add missing pip in $PATH on Cygwin CI by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1784 - Shorten Iterable docstrings and put IterableObj first by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1785 - Fix incompletely revised Iterable/IterableObj docstrings by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1786 - Pre-deprecate setting Git.USE_SHELL by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1782 - Deprecate Git.USE_SHELL by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1787 - In handle_process_output don't forward finalizer result by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1788 - Fix mypy warning "Missing return statement" by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1789 - Fix two remaining Windows untrusted search path cases by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1792 #### New Contributors - [@​marioaag](https://togithub.com/marioaag) made their first contribution in [gitpython-developers/GitPython#1746 - [@​stegm](https://togithub.com/stegm) made their first contribution in [gitpython-developers/GitPython#1778 **Full Changelog**: gitpython-developers/GitPython@3.1.40...3.1.41 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/allenporter/flux-local). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMjcuMCIsInVwZGF0ZWRJblZlciI6IjM3LjEyNy4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
This was referenced Feb 15, 2024
otc-zuul bot
pushed a commit
to opentelekomcloud-infra/grafana-docs-monitoring
that referenced
this pull request
Mar 6, 2024
Bump gitpython from 3.1.37 to 3.1.41 Bumps gitpython from 3.1.37 to 3.1.41. Release notes Sourced from gitpython's releases. 3.1.41 - fix Windows security issue The details about the Windows security issue can be found in this advisory. Special thanks go to @EliahKagan who reported the issue and fixed it in a single stroke, while being responsible for an incredible amount of improvements that he contributed over the last couple of months ❤️. What's Changed Add __all__ in git.exc by @EliahKagan in gitpython-developers/GitPython#1719 Set submodule update cadence to weekly by @EliahKagan in gitpython-developers/GitPython#1721 Never modify sys.path by @EliahKagan in gitpython-developers/GitPython#1720 Bump git/ext/gitdb from 8ec2390 to ec58b7e by @dependabot in gitpython-developers/GitPython#1722 Revise comments, docstrings, some messages, and a bit of code by @EliahKagan in gitpython-developers/GitPython#1725 Use zero-argument super() by @EliahKagan in gitpython-developers/GitPython#1726 Remove obsolete note in _iter_packed_refs by @EliahKagan in gitpython-developers/GitPython#1727 Reorganize test_util and make xfail marks precise by @EliahKagan in gitpython-developers/GitPython#1729 Clarify license and make module top comments more consistent by @EliahKagan in gitpython-developers/GitPython#1730 Deprecate compat.is_, rewriting all uses by @EliahKagan in gitpython-developers/GitPython#1732 Revise and restore some module docstrings by @EliahKagan in gitpython-developers/GitPython#1735 Make the rmtree callback Windows-only by @EliahKagan in gitpython-developers/GitPython#1739 List all non-passing tests in test summaries by @EliahKagan in gitpython-developers/GitPython#1740 Document some minor subtleties in test_util.py by @EliahKagan in gitpython-developers/GitPython#1749 Always read metadata files as UTF-8 in setup.py by @EliahKagan in gitpython-developers/GitPython#1748 Test native Windows on CI by @EliahKagan in gitpython-developers/GitPython#1745 Test macOS on CI by @EliahKagan in gitpython-developers/GitPython#1752 Let close_fds be True on all platforms by @EliahKagan in gitpython-developers/GitPython#1753 Fix IndexFile.from_tree on Windows by @EliahKagan in gitpython-developers/GitPython#1751 Remove unused TASKKILL fallback in AutoInterrupt by @EliahKagan in gitpython-developers/GitPython#1754 Don't return with operand when conceptually void by @EliahKagan in gitpython-developers/GitPython#1755 Group .gitignore entries by purpose by @EliahKagan in gitpython-developers/GitPython#1758 Adding dubious ownership handling by @marioaag in gitpython-developers/GitPython#1746 Avoid brittle assumptions about preexisting temporary files in tests by @EliahKagan in gitpython-developers/GitPython#1759 Overhaul noqa directives by @EliahKagan in gitpython-developers/GitPython#1760 Clarify some Git.execute kill_after_timeout limitations by @EliahKagan in gitpython-developers/GitPython#1761 Bump actions/setup-python from 4 to 5 by @dependabot in gitpython-developers/GitPython#1763 Don't install black on Cygwin by @EliahKagan in gitpython-developers/GitPython#1766 Extract all "import gc" to module level by @EliahKagan in gitpython-developers/GitPython#1765 Extract remaining local "import gc" to module level by @EliahKagan in gitpython-developers/GitPython#1768 Replace xfail with gc.collect in TestSubmodule.test_rename by @EliahKagan in gitpython-developers/GitPython#1767 Enable CodeQL by @EliahKagan in gitpython-developers/GitPython#1769 Replace some uses of the deprecated mktemp function by @EliahKagan in gitpython-developers/GitPython#1770 Bump github/codeql-action from 2 to 3 by @dependabot in gitpython-developers/GitPython#1773 Run some Windows environment variable tests only on Windows by @EliahKagan in gitpython-developers/GitPython#1774 Fix TemporaryFileSwap regression where file_path could not be Path by @EliahKagan in gitpython-developers/GitPython#1776 Improve hooks tests by @EliahKagan in gitpython-developers/GitPython#1777 Fix if items of Index is of type PathLike by @stegm in gitpython-developers/GitPython#1778 Better document IterableObj.iter_items and improve some subclasses by @EliahKagan in gitpython-developers/GitPython#1780 Revert "Don't install black on Cygwin" by @EliahKagan in gitpython-developers/GitPython#1783 Add missing pip in $PATH on Cygwin CI by @EliahKagan in gitpython-developers/GitPython#1784 Shorten Iterable docstrings and put IterableObj first by @EliahKagan in gitpython-developers/GitPython#1785 Fix incompletely revised Iterable/IterableObj docstrings by @EliahKagan in gitpython-developers/GitPython#1786 Pre-deprecate setting Git.USE_SHELL by @EliahKagan in gitpython-developers/GitPython#1782 ... (truncated) Commits f288738 bump patch level ef3192c Merge pull request #1792 from EliahKagan/popen 1f3caa3 Further clarify comment in test_hook_uses_shell_not_from_cwd 3eb7c2a Move safer_popen from git.util to git.cmd c551e91 Extract shared logic for using Popen safely on Windows 15ebb25 Clarify comment in test_hook_uses_shell_not_from_cwd f44524a Avoid spurious "location may have moved" on Windows a42ea0a Cover absent/no-distro bash.exe in hooks "not from cwd" test 7751436 Extract venv management from test_installation 66ff4c1 Omit CWD in search for bash.exe to run hooks on Windows Additional commits viewable in compare view You can trigger a rebase of this PR by commenting @dependabot rebase. Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: @dependabot rebase will rebase this PR @dependabot recreate will recreate this PR, overwriting any edits that have been made to it @dependabot merge will merge this PR after your CI passes on it @dependabot squash and merge will squash and merge this PR after your CI passes on it @dependabot cancel merge will cancel a previously requested merge and block automerging @dependabot reopen will reopen this PR if it is closed @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the Security Alerts page. Note Automatic rebases have been disabled on this pull request as it has been open for over 30 days. Reviewed-by: Vladimir Vshivkov
JoeWang1127
pushed a commit
to googleapis/sdk-platform-java
that referenced
this pull request
Apr 6, 2024
](https://renovatebot.com){kind=link}
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [GitPython](https://togithub.com/gitpython-developers/GitPython) | `==3.1.40` -> `==3.1.41` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. ### GitHub Vulnerability Alerts #### [CVE-2024-22190](https://togithub.com/gitpython-developers/GitPython/security/advisories/GHSA-2mqj-m65w-jghx) ### Summary This issue exists because of an incomplete fix for CVE-2023-40590. On Windows, GitPython uses an untrusted search path if it uses a shell to run `git`, as well as when it runs `bash.exe` to interpret hooks. If either of those features are used on Windows, a malicious `git.exe` or `bash.exe` may be run from an untrusted repository. ### Details Although GitPython often avoids executing programs found in an untrusted search path since 3.1.33, two situations remain where this still occurs. Either can allow arbitrary code execution under some circumstances. #### When a shell is used GitPython can be told to run `git` commands through a shell rather than as direct subprocesses, by passing `shell=True` to any method that accepts it, or by both setting `Git.USE_SHELL = True` and not passing `shell=False`. Then the Windows `cmd.exe` shell process performs the path search, and GitPython does not prevent that shell from finding and running `git` in the current directory. When GitPython runs `git` directly rather than through a shell, the GitPython process performs the path search, and currently omits the current directory by setting `NoDefaultCurrentDirectoryInExePath` in its own environment during the `Popen` call. Although the `cmd.exe` shell will honor this environment variable when present, GitPython does not currently pass it into the shell subprocess's environment. Furthermore, because GitPython sets the subprocess CWD to the root of a repository's working tree, using a shell will run a malicious `git.exe` in an untrusted repository even if GitPython itself is run from a trusted location. This also applies if `Git.execute` is called directly with `shell=True` (or after `Git.USE_SHELL = True`) to run any command. #### When hook scripts are run On Windows, GitPython uses `bash.exe` to run hooks that appear to be scripts. However, unlike when running `git`, no steps are taken to avoid finding and running `bash.exe` in the current directory. This allows the author of an untrusted fork or branch to cause a malicious `bash.exe` to be run in some otherwise safe workflows. An example of such a scenario is if the user installs a trusted hook while on a trusted branch, then switches to an untrusted feature branch (possibly from a fork) to review proposed changes. If the untrusted feature branch contains a malicious `bash.exe` and the user's current working directory is the working tree, and the user performs an action that runs the hook, then although the hook itself is uncorrupted, it runs with the malicious `bash.exe`. Note that, while `bash.exe` is a shell, this is a separate scenario from when `git` is run using the unrelated Windows `cmd.exe` shell. ### PoC On Windows, create a `git.exe` file in a repository. Then create a `Repo` object, and call any method through it (directly or indirectly) that supports the `shell` keyword argument with `shell=True`: ```powershell mkdir testrepo git init testrepo cp ... testrepo git.exe # Replace "..." with any executable of choice. python -c "import git; print(git.Repo('testrepo').git.version(shell=True))" ``` The `git.exe` executable in the repository directory will be run. Or use no `Repo` object, but do it from the location with the `git.exe`: ```powershell cd testrepo python -c "import git; print(git.Git().version(shell=True))" ``` The `git.exe` executable in the current directory will be run. For the scenario with hooks, install a hook in a repository, create a `bash.exe` file in the current directory, and perform an operation that causes GitPython to attempt to run the hook: ```powershell mkdir testrepo cd testrepo git init mv .git/hooks/pre-commit.sample .git/hooks/pre-commit cp ... bash.exe # Replace "..." with any executable of choice. echo "Some text" >file.txt git add file.txt python -c "import git; git.Repo().index.commit('Some message')" ``` The `bash.exe` executable in the current directory will be run. ### Impact The greatest impact is probably in applications that set `Git.USE_SHELL = True` for historical reasons. (Undesired console windows had, in the past, been created in some kinds of applications, when it was not used.) Such an application may be vulnerable to arbitrary code execution from a malicious repository, even with no other exacerbating conditions. This is to say that, if a shell is used to run `git`, the full effect of CVE-2023-40590 is still present. Furthermore, as noted above, running the application itself from a trusted directory is not a sufficient mitigation. An application that does not direct GitPython to use a shell to run `git` subprocesses thus avoids most of the risk. However, there is no such straightforward way to prevent GitPython from running `bash.exe` to interpret hooks. So while the conditions needed for that to be exploited are more involved, it may be harder to mitigate decisively prior to patching. ### Possible solutions A straightforward approach would be to address each bug directly: - When a shell is used, pass `NoDefaultCurrentDirectoryInExePath` into the subprocess environment, because in that scenario the subprocess is the `cmd.exe` shell that itself performs the path search. - Set `NoDefaultCurrentDirectoryInExePath` in the GitPython process environment during the `Popen` call made to run hooks with a `bash.exe` subprocess. These need only be done on Windows. --- ### Release Notes <details> <summary>gitpython-developers/GitPython (GitPython)</summary> ### [`v3.1.41`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.41): - fix Windows security issue [Compare Source](https://togithub.com/gitpython-developers/GitPython/compare/3.1.40...3.1.41) The details about the Windows security issue [can be found in this advisory](https://togithub.com/gitpython-developers/GitPython/security/advisories/GHSA-2mqj-m65w-jghx). Special thanks go to [@​EliahKagan](https://togithub.com/EliahKagan) who reported the issue and fixed it in a single stroke, while being responsible for an incredible amount of improvements that he contributed over the last couple of months ❤️. #### What's Changed - Add `__all__` in git.exc by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1719 - Set submodule update cadence to weekly by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1721 - Never modify sys.path by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1720 - Bump git/ext/gitdb from `8ec2390` to `ec58b7e` by [@​dependabot](https://togithub.com/dependabot) in [gitpython-developers/GitPython#1722 - Revise comments, docstrings, some messages, and a bit of code by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1725 - Use zero-argument super() by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1726 - Remove obsolete note in \_iter_packed_refs by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1727 - Reorganize test_util and make xfail marks precise by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1729 - Clarify license and make module top comments more consistent by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1730 - Deprecate compat.is\_<platform>, rewriting all uses by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1732 - Revise and restore some module docstrings by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1735 - Make the rmtree callback Windows-only by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1739 - List all non-passing tests in test summaries by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1740 - Document some minor subtleties in test_util.py by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1749 - Always read metadata files as UTF-8 in setup.py by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1748 - Test native Windows on CI by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1745 - Test macOS on CI by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1752 - Let close_fds be True on all platforms by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1753 - Fix IndexFile.from_tree on Windows by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1751 - Remove unused TASKKILL fallback in AutoInterrupt by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1754 - Don't return with operand when conceptually void by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1755 - Group .gitignore entries by purpose by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1758 - Adding dubious ownership handling by [@​marioaag](https://togithub.com/marioaag) in [gitpython-developers/GitPython#1746 - Avoid brittle assumptions about preexisting temporary files in tests by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1759 - Overhaul noqa directives by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1760 - Clarify some Git.execute kill_after_timeout limitations by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1761 - Bump actions/setup-python from 4 to 5 by [@​dependabot](https://togithub.com/dependabot) in [gitpython-developers/GitPython#1763 - Don't install black on Cygwin by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1766 - Extract all "import gc" to module level by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1765 - Extract remaining local "import gc" to module level by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1768 - Replace xfail with gc.collect in TestSubmodule.test_rename by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1767 - Enable CodeQL by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1769 - Replace some uses of the deprecated mktemp function by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1770 - Bump github/codeql-action from 2 to 3 by [@​dependabot](https://togithub.com/dependabot) in [gitpython-developers/GitPython#1773 - Run some Windows environment variable tests only on Windows by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1774 - Fix TemporaryFileSwap regression where file_path could not be Path by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1776 - Improve hooks tests by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1777 - Fix if items of Index is of type PathLike by [@​stegm](https://togithub.com/stegm) in [gitpython-developers/GitPython#1778 - Better document IterableObj.iter_items and improve some subclasses by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1780 - Revert "Don't install black on Cygwin" by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1783 - Add missing pip in $PATH on Cygwin CI by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1784 - Shorten Iterable docstrings and put IterableObj first by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1785 - Fix incompletely revised Iterable/IterableObj docstrings by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1786 - Pre-deprecate setting Git.USE_SHELL by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1782 - Deprecate Git.USE_SHELL by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1787 - In handle_process_output don't forward finalizer result by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1788 - Fix mypy warning "Missing return statement" by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1789 - Fix two remaining Windows untrusted search path cases by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1792 #### New Contributors - [@​marioaag](https://togithub.com/marioaag) made their first contribution in [gitpython-developers/GitPython#1746 - [@​stegm](https://togithub.com/stegm) made their first contribution in [gitpython-developers/GitPython#1778 **Full Changelog**: gitpython-developers/GitPython@3.1.40...3.1.41 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/googleapis/sdk-platform-java). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjkuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
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.
This expands the CI test matrix in the main test workflow,
pythonpackage.yml, so that it is parameterized not only by Python version but also by operating system. Now it tests on both Ubuntu and Windows. It also adds conditionalxfailmarkings with precise conditions and brief descriptions of the expected failures.Most of the detailed information about specific changes in this pull request is in the commit messages. I give general information here, as well as covering, in some detail, a few areas where I think it is not readily apparent (and maybe I am wrong) what the best approach is, or what changes should be included.
Approach to adding
xfailmarkingsA number of tests were known always to fail on Windows, with some others known to fail on Windows on particular versions or other conditions. As discussed in #1654 (comment), I have not attempted to actually fix the failures. Instead, I have marked each failing test case with a conditional
xfailmarking with a description of the failure and the exception with which the test fails. I have sought to make the conditions precise. Most of them are simply that a test fails on native Windows, so the condition isos.name == "nt", but some of the conditions are more specific. In some cases there are really multiple independent expected causes of failure, so some have multiplexfailmarkings. (pytestsupports stacking them in this way.)I have tried to document what is currently known about the failures, including known or suspected information about their causes, in the descriptions, and sometimes in more detail in the commit messages that added them. Sometimes this takes the form of a reference to an existing issue, and for some others there may be an existing issue I did not manage to identify, but it is likely that a few bugs are currently only "reported" in commit messages in this PR. In some cases I anticipate I can fix a bug soon; others might benefit from having an issue opened for them.
(An exception is #1630, for which I added
xfailmarks in 6e477e3, which I did not document as thoroughly as I could have. I hope to open a PR to remedy #1630 as soon as Windows has CI test jobs--which if this PR is approved would be at that point--so I figured the commit message didn't need much information.)The
WinBashStatusclass intest_indexSome more detailed information about this appears in commit messages.
The original motivation for more precise detection of when
bash.exeis the WSL wrapper was to fix CI. On the Windows CI runners, runningbash.exein a shell (any kind of shell) runs Git Bash, which is not related to WSL. ButPopenfinds the WSL-wrapperbash.exeinSystem32instead, because the way searching for executables works on Windows differs significantly between the shell and non-shell case. This is an intentional design choice in Windows, not GitPython nor even Python itself. But I think it's accurate to say GitPython must contend with it and, more importantly, some users of GitPython may be relying on it, perhaps even without knowing they are.When a program is run without using a shell and a search must be performed because just the name of the executable is given, a few directories, including
System32, are expected to be (and typically are) searched first, before thePATHenvironment variable is even used. This keeps ashutil.which-based technique from working. There are other important differences in search behavior ofshutil.whichandPopen, but that's the difference that's relevant here.There are three related issues:
bash, while passing on nativebash(like Git Bash).bash.exeis found inSystem32, and it is the WSL wrapper, that does not imply the existence of any installed distributions (i.e., GNU/Linux systems to run in WSL). This is a common situation. Even when some Windows components needed for WSL to work are not yet installed,bash.execan exist there, yet it does not always. (This is also the situation on CI when WSL is not set up.) Because the tests arexfailwhenbash.exeis altogether absent, I think it makes sense also toxfailthem in the common situation thatbash.execan't runbashin a WSL-hosted distribution because no such distribution is present.But the advantage of doing it seems significant to me: not only does this make it more efficient to interpret and document test failures, it also should be helpful in investigating and debugging possible future design changes that seek to build on #1399, in a sufficiently backward-compatible way, to retain its benefits (see #703, #971), while avoiding unexpected behavior. (If it can be done in a way that is non-breaking for current users of hooks on Windows, perhaps in the future Git Bash can be used as the default way to run hooks, with other approaches, including WSL, available as backup strategies or if specified.)
I wrote
WinBashStatuswith the intention that, if necessary, it could be retained for an extended time. However, my hope for it is that it will actually disappear (perhaps with some insights from the process making their way into other code). If, in the future, hooks are run on Windows in a way that works on more systems, and has fewer strange edge cases, then there may be no more need for it or anything like it.I have tried to make it work properly locally as well as on CI. To work locally, it needs to avoid language and locale related limitations other than those those already present. Because some aspects of the approach I used are not obvious, I've included comments about it. They were originally longer, with parenthesized examples of locale-related situations, but that seemed more detailed than necessary, so I removed them in e00fffc. (But I can bring those back if desired.)
Which versions should we test?
This PR currently deviates from the preferences expressed in #1654 (comment) in one significant way, which can be changed if you wish. At the time of that discussion, we both held the view that it would be good to test only the lower and upper bounds of the supported version range for Windows. The process of adding these tests has led me to the view that it may be better, at least for now, to test all six versions on Windows (as is done on Ubuntu).
Initially I had them all enabled for the purpose of developing the changes here and planned to remove some at the end, but the reason I now advocate keeping them all for the time being, are, from most to least significant:
test_index.py. I think there is value in running those tests, but theirxfailmarks do considerbash.exebeing the WSL wrapper, yet no WSL systems being installed, as an expected failure condition. So if necessary, thesetup-wslstep can be dropped to gain that minute. As detailed below, I don't know if caching works for the runs in an open PR, so you may observe further delay here.)TestSubmodule.test_renamenewly fails in 3.12 with a "being used by another process"PermissionError, possibly due to GC changes. Because of the role of such errors in #790 and #1333, it seems to me that easily distinguishing what versions have a failure, before and after code changes that potentially affect the test, might be helpful in the near future. To do this would at least require a 3.11 job.Most of the benefit could be gotten by testing 3.7, 3.8, 3.11, and 3.12 (the last point above is less compelling than the others, I think). However, this is still much more than the original idea of testing just a couple versions. It seems to me that the additional resource usage of testing two more versions is worthwhile for the benefit of comprehensively testing all the versions GitPython officially supports. I also think other approaches for decreasing undesired load might be preferable, such as removing
fail-fast: false(the Ubuntu jobs, if desired, could still be set to continue even if another job has failed), skipping the documentation-building step on some or all Windows jobs, and/or either not installing WSL for the hook tests or switching the distribution to Alpine Linux.With many pushes over a short time--as for example in this pull request, where I pushed the commits separately with ten second delays in between, to make it easy to inspect intermediate CI results--there can be more jobs queued than immediately available runners. (I believe the number of runners is limited per organization.) It is in that case that the number of tests can be most of an issue. Even if you decide to accept this PR without the number of Windows jobs being decreased, it might turn out later that this is annoying. I accept that you may know now that you want fewer Windows test jobs, but also, I understand that if you accept this as it is now, that doesn't mean all the jobs will be kept forever.
setup-wsland cachingOn my fork, the
setup-wslstep seems usually to take between 30 and 90 seconds, and most often near the low end.I did not disable GitHub Actions caching on
setup-wsl. Based on how it works in my fork, I expect it to take up 82 MB. I think this is probably fine, and it can be turned off if desired, but I wanted to mention it just in case. I believe GitHub Actions caching quotas are per-organization. I expect that turning off caching (while still installing WSL) would make things somewhat slower.I am unsure if caching will actually work on it in the CI jobs run on this repository due to the
pull-requesttrigger. My vague recollection is that caching does not happen for open PRs that introduce it. If that is the case, then the amount by which installing WSL slows down each Windows job, as observed here, may be more than I have observed.What is not done here
It seemed to me that the
bash.exestatus classification logic intest_index(described above) was easier to do in this PR than separately, because it facilitates precisexfailmarkings. However, other substantial changes that could in principle be part of this PR but can be omitted are omitted. Specifically:cygwin-test.ymlintopythonpackage.yml. I had hoped to do that while adding native Windows tests, but I now think it is best deferred to a later PR. One reason is for more effective reviewing: if merging them produces conditional logic whose complexity is undesirable, it will be easier to notice the problem after the native Windows tests already exist for comparison. Another reason, though, is that I don't know what it will look like after changes have been made to it to speed it up (see above). If this involves adding caching, including for installingGitPython's dependencies, then that might cause it to diverge sufficiently frompythonpackage.ymlthat they should no longer be merged.setup-wslstep). This requires either a fix for setup-wsl#50 or a workaround.rmtreeandHIDE_WINDOWS_KNOWN_ERRORStests intest_util. With native Windows CI jobs, some of the behavior that is specific to Windows (especially since #1739) can probably be tested only on Windows, and skipped otherwise. I think there should be a way to make this change in a way that simplifies the tests. (As one example, everyos.name == "nt"in a@pytest.mark.parametrizeparameter set would just becomeTrue.) But it's not at all necessary to do that to achieve the aims here, and it would delay this PR.unittest.SkipTestbeing raised ingit.util.rmtreeintoxfailtests, madeHIDE_WINDOWS_KNOWN_ERRORSdefault toFalse, or decreased its use (though I have made sure not to increase its use). It seems to me that such steps would be good progress toward a solution for #790, but I think they will be easier to do, as well as to review, if done after, and separately from, the addition of native Windows CI jobs.