Fix various version-related CI breakages #1987
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 is analogous to the 3.7-related CI change in gitdb that was part of gitpython-developers/gitdb#114, as to part of gitpython-developers/smmap#58. Since some tests are not yet passing on 3.13, this does not add 3.13 to CI, nor to the documentation of supported versions in `setup.py`. Note that the list there is not enforced; GitPython can already be installed on Python 3.13 and probably *mostly* works. (See gitpython-developers#1955 for details on other changes that should be made to fully support running 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, but not on other systems on CI, and not locally on Cygwin. It looks like there's an extra line being read from stderr when the test fails, so let's examine the lines themselves.
The main change here is to the Cygwin test workflow, which now runs
the tests in a venv (a Python virtual environment), to avoid mixing
up our intended dependencies with other files installed by Python
related packages on the system. This should either fix the problem
where `test_handle_process_output` reports an extra line in stderr
for the `cat_file.py` subprocess on CI or Cygwin, or at least make
it somewhat easier to continue investigating the problem. I think
this change is also valuable for its own sake.
The connection to the `test_handle_process_output` failure is that
the extra line in stderr appears at the beginning and is an error
message 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: AssertionError
This could possibly be worked around by attempting to install a
package to provide `iniconfig`, by configuring `pytest-cov` in a
way that does not require it, or by temporarily disabling code
coverage reports on Cygwin. Before exploring those or other
options, this change seeks to prepare a more isolated environment
in which different package versions are more likely to work
properly together.
In addition to that change to the Cygwin workflow, this also
changes the way the Alpine Linux test workflow is made to use a
venv, using the technique that is used here, and undoing some
changes in the Alpine Linux workflow that should not be necessary.
The reason for making that change together with this Cygwin change
is that if either does not work as expected, it might shed light on
what 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 on
Cygwin is `BASH_ENV` (since script steps are run in `bash`), while
the 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 invoked
through `sudo`).
`busybox sh` does not appear to read commands from a file whose path is given as the value of `$ENV`, in this situation. I think I may have misunderstood that; the documentation does not say much about it and maybe, in Almquist-style shells, it is only read by interactive shells? I am not sure. This removes everything about `ENV` and activates the venv in each step where the venv should be used. The good news is that the technique did work fully in Cygwin: not only did `BASH_ENV` work (which was not much in doubt), but using a virtual environment for all steps that run test code on Cygwin fixed the problem and allowed all tests to pass. That seems to have been the reason I didn't reproduce the problem locally: on my local system I always use a venv in Cygwin since I would otherwise not have adequate isolation. Thus, this commit changes only the Alpine workflow and not the Cygwin workflow.
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this pull request
Jan 2, 2025
Since gitpython-developers#1987, test jobs from `pythonpackage.yml` appear in an unintuitive order, and some show an extra bool matrix variable in their 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 without setting `continue-on-error` to temporarily work around the problems obseved in gitpython-developers#1955, but nothing 3.13-related is done in this commit.
Byron
approved these changes
Jan 2, 2025
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this pull request
Jan 2, 2025
Since gitpython-developers#1987, test jobs from `pythonpackage.yml` appear in an unintuitive order, and some show an extra bool matrix variable in their 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 without setting `continue-on-error` to temporarily work around the problems obseved in gitpython-developers#1955, but nothing 3.13-related is done in this commit.
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 |
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this pull request
Jan 4, 2025
Since gitpython-developers#1987, test jobs from `pythonpackage.yml` appear in an unintuitive order, and some show an extra bool matrix variable in their 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 without setting `continue-on-error` to temporarily work around the problems obseved in gitpython-developers#1955, but nothing 3.13-related is done in this commit.
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 applies the fixes to GitPython that correspond to gitpython-developers/smmap#58, as well as other fixes which were along the lines of the prediction in e51bf80#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 apipcommand 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
pythonin Alpine Linux discourages usingpipto 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_ENVcomposes withsudopassthrough.Of those, the third is the most significant:
sudois being used as theshellfor 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 was supposed 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.