René's URL Explorer Experiment

Title: Test native Windows on CI by EliahKagan · Pull Request #1745 · gitpython-developers/GitPython · GitHub

Open Graph Title: Test native Windows on CI by EliahKagan · Pull Request #1745 · gitpython-developers/GitPython

X Title: Test native Windows on CI by EliahKagan · Pull Request #1745 · gitpython-developers/GitPython

Description: 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 conditional xfail markings 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 xfail markings A 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 xfail marking 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 is os.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 multiple xfail markings. (pytest supports 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 xfail marks 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 WinBashStatus class in test_index Some more detailed information about this appears in commit messages. The original motivation for more precise detection of when bash.exe is the WSL wrapper was to fix CI. On the Windows CI runners, running bash.exe in a shell (any kind of shell) runs Git Bash, which is not related to WSL. But Popen finds the WSL-wrapper bash.exe in System32 instead, 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 the PATH environment variable is even used. This keeps a shutil.which-based technique from working. There are other important differences in search behavior of shutil.which and Popen, but that's the difference that's relevant here. There are three related issues: One of the tests currently fails on WSL bash, while passing on native bash (like Git Bash). When bash.exe is found in System32, 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.exe can exist there, yet it does not always. (This is also the situation on CI when WSL is not set up.) Because the tests are xfail when bash.exe is altogether absent, I think it makes sense also to xfail them in the common situation that bash.exe can't run bash in a WSL-hosted distribution because no such distribution is present. Distinguishing expected and unexpected WSL failures, and failures not related to WSL, is complicated by how, while determining the status of WSL on a machine is usually fairly easy interactively, it seems there is no trivial way to do so programmatically in way that covers all cases, or even all common cases. 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 WinBashStatus with 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: The native Windows tests are much faster than I had feared, running much closer to the speed of the Ubuntu jobs than of the Cygwin job. (This is especially remarkable considering that it holds up even with them installing Debian via WSL in the Windows test runner to run the hook tests in test_index.py. I think there is value in running those tests, but their xfail marks do consider bash.exe being the WSL wrapper, yet no WSL systems being installed, as an expected failure condition. So if necessary, the setup-wsl step 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.) The conditions under which tests fail on Windows are weirder than I had expected, in at least one way that specifically benefits from testing more versions: TestSubmodule.test_rename newly 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. The lower bound GitPython currently supports is 3.7. There are a number of important changes in 3.8, which so far seem not to have been an issue; most special-casing of 3.7 seems to have been in test suite only. To ensure that changes (including security fixes) are effective on 3.7, it should be tested on Windows. To decide what to do about it when that breaks, 3.8 should be tested as well. The latest Python build the Cygwin project currently packages is 3.9; for this reason, it is probably the most used on Cygwin, and it makes sense that it is the version tested in the Cygwin job. I had not until recently considered the benefits of being able to compare 3.9 results between native Windows and Cygwin. I think this would avoid situations where one might worry the problem is with 3.9, though that may only be a small boon. However, the stark difference in performance between native Windows and Cygwin makes me think the Cygwin job could be sped up. (For example, it may be possible to use the GitHub Actions caching feature to do I/O in a faster way for installing Cygwin and/or GitPython's dependencies in the Cygwin system.) This is another area where the ability to compare could be helpful. 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-wsl and caching On my fork, the setup-wsl step 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-request trigger. 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.exe status classification logic in test_index (described above) was easier to do in this PR than separately, because it facilitates precise xfail markings. However, other substantial changes that could in principle be part of this PR but can be omitted are omitted. Specifically: I have not merged cygwin-test.yml into pythonpackage.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 installing GitPython's dependencies, then that might cause it to diverge sufficiently from pythonpackage.yml that they should no longer be merged. I have not changed the distribution used for WSL in the tests from Debian to Alpine Linux, even though I suspect this would speed things up (perhaps especially if caching is going to be turned off in the setup-wsl step). This requires either a fix for setup-wsl#50 or a workaround. I have not added macOS jobs. I think this may be worth doing. I don't currently have the ability to test on Apple hardware. I can virtualize systems I don't regularly use in development, such as OpenIndiana or the Simplified Chinese localized build of Windows Server, on Ubuntu or Windows hosts. But virtualizing macOS is not similarly straightforward. Furthermore, some subtleties of Unix can be revealed by testing regularly on systems that are not GNU/Linux, of which macOS has GitHub Actions runners. (For #1738, the way I tested on macOS was to run a GitHub Actions job on a macOS runner with a tmate debugging step and SSH into it.) However, omitting that here limits scope and allows it to be evaluated separately. Limited testing indicates that, as on Windows, some different expected timings may have to be given in at least one of the performance tests in order to make macOS jobs pass. I have not reduced the complexity of rmtree and HIDE_WINDOWS_KNOWN_ERRORS tests in test_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, every os.name == "nt" in a @pytest.mark.parametrize parameter set would just become True.) But it's not at all necessary to do that to achieve the aims here, and it would delay this PR. I have not converted tests that skip due to unittest.SkipTest being raised in git.util.rmtree into xfail tests, made HIDE_WINDOWS_KNOWN_ERRORS default to False, 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.

Open Graph Description: 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 Wi...

X Description: 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 Wi...

Opengraph URL: https://github.com/gitpython-developers/GitPython/pull/1745

X: @github

direct link

Domain: github.com

Links:

Viewport: width=device-width