Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
Fix various version-related CI breakages#1987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This is analogous to the 3.7-related CI change in gitdb that waspart ofgitpython-developers/gitdb#114, asto part ofgitpython-developers/smmap#58.Since some tests are not yet passing on 3.13, this does not add3.13 to CI, nor to the documentation of supported versions in`setup.py`. Note that the list there is not enforced; GitPython canalready be installed on Python 3.13 and probably *mostly* works.(Seegitpython-developers#1955for details on other changes that should be made to fully supportrunning GitPython on Python 3.13.)
Bumps [Vampire/setup-wsl](https://github.com/vampire/setup-wsl) from 3.1.1 to 4.0.0.- [Release notes](https://github.com/vampire/setup-wsl/releases)- [Commits](Vampire/setup-wsl@v3.1.1...v4.0.0)---updated-dependencies:- dependency-name: Vampire/setup-wsl dependency-type: direct:production update-type: version-update:semver-major...Signed-off-by: dependabot[bot] <support@github.com>
To try to find the bug that causes it to fail on Cygwin on CI, butnot on other systems on CI, and not locally on Cygwin.It looks like there's an extra line being read from stderr when thetest fails, so let's examine the lines themselves.
The main change here is to the Cygwin test workflow, which now runsthe tests in a venv (a Python virtual environment), to avoid mixingup our intended dependencies with other files installed by Pythonrelated packages on the system. This should either fix the problemwhere `test_handle_process_output` reports an extra line in stderrfor the `cat_file.py` subprocess on CI or Cygwin, or at least makeit somewhat easier to continue investigating the problem. I thinkthis change is also valuable for its own sake.The connection to the `test_handle_process_output` failure is thatthe extra line in stderr appears at the beginning and is an errormessage about a missing import needed for subprocess code coverage.With the recently added more detailed error reporting, it shows: self.assertEqual(line_counts[1], expected_line_count, repr(lines[1])) > self.assertEqual(line_counts[2], expected_line_count, repr(lines[2])) E AssertionError: 5003 != 5002 : ['pytest-cov: Failed to setup subprocess coverage. Environ: {\'COV_CORE_SOURCE\': \'git\', \'COV_CORE_CONFIG\': \':\', \'COV_CORE_DATAFILE\': \'/cygdrive/d/a/GitPython/GitPython/.coverage\'} Exception: ModuleNotFoundError("No module named \'iniconfig\'")\n', 'From github.com:jantman/gitpython_issue_301\n', ' = [up to date] master -> origin/master\n', ' = [up to date] testcommit1 -> origin/testcommit1\n', ' = [up to date] testcommit10 -> origin/testcommit10\n', ' = [up to date] testcommit100 -> origin/testcommit100\n', ' = [up to date] testcommit1000 -> origin/testcommit1000\n', ' = [up to date] testcommit1001 -> origin/testcommit1001\n', ' = [up to date] testcommit1002 -> origin/testcommit1002\n', ' = [up to date] testcommit1003 -> origin/testcommit1003\n', ' = [up to date] testcommit1004 -> origin/testcommit1004\n', ' = [up to date] testcommit1005 -> origin/testcommit1005\n', ' = [up to date] testcommit test/test_git.py:793: AssertionErrorThis could possibly be worked around by attempting to install apackage to provide `iniconfig`, by configuring `pytest-cov` in away that does not require it, or by temporarily disabling codecoverage reports on Cygwin. Before exploring those or otheroptions, this change seeks to prepare a more isolated environmentin which different package versions are more likely to workproperly together.In addition to that change to the Cygwin workflow, this alsochanges the way the Alpine Linux test workflow is made to use avenv, using the technique that is used here, and undoing somechanges in the Alpine Linux workflow that should not be necessary.The reason for making that change together with this Cygwin changeis that if either does not work as expected, it might shed light onwhat is going wrong with the other.Although the same technique is used on Cygwin and in Alpine Linux,it looks a little different, because the environment variable onCygwin is `BASH_ENV` (since script steps are run in `bash`), whilethe environment variable is `ENV` (since script steps are run in`busybox sh`) and this must also be allowed to pass through `sudo`(since `sh`, which is `busybox sh` on this system, is being invokedthrough `sudo`).
`busybox sh` does not appear to read commands from a file whosepath is given as the value of `$ENV`, in this situation. I think Imay have misunderstood that; the documentation does not say muchabout it and maybe, in Almquist-style shells, it is only read byinteractive shells? I am not sure.This removes everything about `ENV` and activates the venv in eachstep where the venv should be used.The good news is that the technique did work fully in Cygwin: notonly did `BASH_ENV` work (which was not much in doubt), but usinga virtual environment for all steps that run test code on Cygwinfixed the problem and allowed all tests to pass. That seems to havebeen the reason I didn't reproduce the problem locally: on my localsystem I always use a venv in Cygwin since I would otherwise nothave adequate isolation.Thus, this commit changes only the Alpine workflow and not theCygwin workflow.
Sincegitpython-developers#1987, test jobs from `pythonpackage.yml` appear in anunintuitive order, and some show an extra bool matrix variable intheir names while others don't (this corresponds to `experimental`,which was always set to some value, but was set in different ways).This fixes that by:- Listing all tested versions, rather than introducing some in an `include` key. (The `include:`-introduced jobs didn't distinguish between originally-present matrix variables and those that are introduced based on the values of the original ones.)- Replacing `os` with `os-type`, which has only the first part of the value for `runs-on:` (e.g., `ubuntu`), and adding `os-ver` to each matrix job, defaulting it to `latest`, but using `22.04` for Python 3.7 on Ubuntu.This should also naturally extend to adding 3.13, with or withoutsetting `continue-on-error` to temporarily work around the problemsobseved ingitpython-developers#1955, but nothing 3.13-related is done in this commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks so much - I definitely couldn't have produced a fix like this, if at all!
Let's hope it will last till 2026 :).
cc1c643
intogitpython-developers:mainUh oh!
There was an error while loading.Please reload this page.
Sincegitpython-developers#1987, test jobs from `pythonpackage.yml` appear in anunintuitive order, and some show an extra bool matrix variable intheir names while others don't (this corresponds to `experimental`,which was always set to some value, but was set in different ways).This fixes that by:- Listing all tested versions, rather than introducing some in an `include` key. (The `include:`-introduced jobs didn't distinguish between originally-present matrix variables and those that are introduced based on the values of the original ones.)- Replacing `os` with `os-type`, which has only the first part of the value for `runs-on:` (e.g., `ubuntu`), and adding `os-ver` to each matrix job, defaulting it to `latest`, but using `22.04` for Python 3.7 on Ubuntu.This should also naturally extend to adding 3.13, with or withoutsetting `continue-on-error` to temporarily work around the problemsobseved ingitpython-developers#1955, but nothing 3.13-related is done in this commit.
EliahKagan commentedJan 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Well, there's still#1955. I hope to do that long before 2026 😄 (unless someone else gets to it first). Further CI refinements could probably be included there. Actually, that's pretty much all I have there now, though I have also updated the description to state in detail what the situation is with |
Sincegitpython-developers#1987, test jobs from `pythonpackage.yml` appear in anunintuitive order, and some show an extra bool matrix variable intheir names while others don't (this corresponds to `experimental`,which was always set to some value, but was set in different ways).This fixes that by:- Listing all tested versions, rather than introducing some in an `include` key. (The `include:`-introduced jobs didn't distinguish between originally-present matrix variables and those that are introduced based on the values of the original ones.)- Replacing `os` with `os-type`, which has only the first part of the value for `runs-on:` (e.g., `ubuntu`), and adding `os-ver` to each matrix job, defaulting it to `latest`, but using `22.04` for Python 3.7 on Ubuntu.This should also naturally extend to adding 3.13, with or withoutsetting `continue-on-error` to temporarily work around the problemsobseved ingitpython-developers#1955, but nothing 3.13-related is done in this commit.
Uh oh!
There was an error while loading.Please reload this page.
This applies the fixes to GitPython that correspond togitpython-developers/smmap#58, as well as other fixes which were along the lines of the prediction ine51bf80#commitcomment-150862474. Taken together, this fixes CI for GitPython.
The Cygwin and Alpine Linux failures were due to not using a virtual environment.
In Cygwin, I had deliberately not done that, since we're not doing it the other non-container workflows, and I wanted the Cygwin workflow to be similar, with the hope that its job might even one day become a matrix job in
pythonpackage.yml
. But there had already been signs, accumulating over time, that this was not the way to go, such as the extra step to make sure there would be apip
command in the global environment. Ultimately, what not using a venv broke waspytest-cov
, which didn't have a dependency it needed, even after all dependencies appeared to install okay. Using a venv fixed that automatically.In Alpine Linux, I was deliberately using a virtual environment, since
python
in Alpine Linux discourages usingpip
to install packages in the global environment managed by the system, which by default is automatically rejected, and which would not necessarily work well even if forced to happen. However, at some point, the virtual environment stopped actually being activated. This causes that error, which had previously only occurred in this workflow while it was originally being developled, to return.I am not clear on specifically why this happened, but I think it is due to the interaction between:
GITHUB_ENV
composes withsudo
passthrough.Of those, the third is the most significant:
sudo
is being used as theshell
for most steps in this job, in order to run code as a non-root user (since some tests do not work when run as root and are not, if I recall correctly, intended to). But it was not configured to pass through variables related to Python virtual environments. I'm not sure what changed, exactly, but in hindsight I don't think I could ever have proved that it wassupposed to work as I intended, when implemented the way I had. The fix here is slightly ugly in that it explicitly activates the venv in each of the steps that are supposed to use it, but I believe this is more simple and robust than alternatives I considered.As for the other, unrelated Alpine problem in WSL, where the image didn't downloading, upgrading the setup-wsl action fixed it. (To do this, I cherry-picked a commit from the Dependabot PR that had proposed it.)
See commit messages for full details on each of the changes themselves.