GeezerVison is showing github.com — Fix various version-related CI breakages by EliahKagan · Pull Request #1987 · gitpython-developers/GitPython
Conversation
EliahKagan
commented
Jan 2, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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 a pip command in the global environment. Ultimately, what not using a venv broke was pytest-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 using pip 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:
- The environment variables that are documented to be set when a venv is activated.
- Subtle distinctions between activating a venv in the usual way and setting those variables (maybe).
- How setting variables for steps via
GITHUB_ENV composes with sudo passthrough.
Of those, the third is the most significant: sudo is being used as the shell 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 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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.)
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.
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 :).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
EliahKagan
commented
Jan 2, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Let's hope it will last till 2026 :).
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 os.path.isabs in Python 3.13 in Windows.
|
Byron reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Reviewers
Byron
Byron approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|