Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Python version and OS compatibility, fixing deprecations #1654

Merged
merged 17 commits into from Sep 12, 2023

Conversation

EliahKagan
Copy link
Contributor

@EliahKagan EliahKagan commented Sep 10, 2023

Fixes #1640
Fixes #1651
Fixes #1652
Fixes #1653

Overview

This changes installation instructions, test code, scripts including setup.py, and CI workflows to support Python 3.12, to fix a test that always failed on native Windows systems, and to avoid using deprecated setuptools features or recommending their use. I've attempted to fix problems and replace the use of deprecated features in a way that increases rather than decreasing robustness, clarity, and ease of installation. Besides the practical overlap (see below), the conceptual thread that holds all these changes together is that they are about improving compatibility with current and future Python installations.

The actual GitPython library code itself seems already compatible with 3.12, and this PR does not change anything in git/. It modifies README.md including to change the recommend way to install GitPython from a downloaded source code archive or cloned repository; edits scripts related to installation and building, including setup.py; and modifies a test to fix some OS and Python version compatibilities, to make it test the changed installation procedure in the readme, and to make it slightly more robust; and modifies the two CI workflows that run tests.

It seemed to me that the four issues mentioned above, although they are distinct issues with none being a duplicate of any others, inherently overlap and are best fixed in a way that involves overlapping code changes as well as overlapping considerations for reviewing the changes. So although I am a bit concerned about the scope of this pull request, I've done these changes together in one PR. However, they are separated across a number of narrowly scoped commits, with most of the commit messages detailing the change the commit makes and its purpose.

Documentation changes

In parts of the readme that had to be changed to replace python setup.py with pip install, I applied other updates and clarifications too. I avoided doing this in any other parts of the readme. I reorganized the installation instructions for clarity, subdividing them into separate <h4> sections so that the distinctions between different installation approaches is readily apparent and readers can immediately find the part of the instructions they are looking for.

When making those changes, I included the tag-fetching step that was recently added in one place but not another where it is relevant, explicitly mentioned that the instructions should usually be carried out in a virtual environment, and addressed forks so users less familiar with common GitHub workflows would not be misled into thinking they should clone the upstream repository to propose changes. (I was a bit uncomfortable doing the latter as part of this already broad PR, but it actually relates to the tag-fetching step: if the gh command is used for cloning, tags on the upstream repo are available even if the fork does not have them, allowing local tests to pass.)

I did not also update the documentation in doc/. Although this should be done, that documentation is already considerably out of date in other ways including with respect to installation and dependencies, and this PR is already fairly large in scope. Note that the old approach of running python setup.py install does still work in all the cases where it worked before. So this does not break the old documentation, it just doesn't bring it up to date.

CI changes

This adds 3.12, which is currently at RC2, to be tested on CI, permitting setup-python to install prereleases for 3.12 but not for other versions. That only affects the Ubuntu workflow.

It also updates both CI test workflows to test the new installation procedure and to harmonize them with the updated README.md instructions, and this attempts to make them clearer both in terms of the workflow files' own readability and in terms of the output generated in the GitHub Actions web-based interface.

This does not add any new Windows tests. While it would be valuable to have native (non-Cygwin) Windows tests on CI, and it would be valuable (if it does not cause CI checks to take too long) to have Cygwin and non-Cygwin Windows tests on multiple Python versions (as well as to test on macOS), this PR does not propose any such changes. This would be nontrivial, at least in the case of native Windows tests, and in my opinion significantly beyond the scope of this PR. I also did not want to wait to fix #1640, since Python 3.12 is already at RC2, with stable 3.12.0 coming out in a month.

Because there are still no non-Cygwin Windows tests on CI, the CI tests results shown for this pull request do not demonstrate that it resolves #1651. However, I have shown the failure in that issue, and I have verified that the changes here fix it, causing test_installation to pass. I tested on Windows 10 with Python 3.11.5, and also on the same system with Python 3.12.0rc2 to verify that it works for the combination of 3.12 and Windows.

One of the setup.py changes deserves special scrutiny in review

The main change in this PR that I anticipate might not be wanted is the addition of a test extra. This approach seemed best to me, but only by a very narrow margin, and I am not at all sure that I am right. The reason I did this, as well as why some other approach might be preferred, are detailed in #1652.

My rationale hinges on the assumption that it is a goal for there to be a way to install the package for local development that also takes care of installing test dependencies. Although test dependencies are not installed unless the test extra is called for, it may nonetheless be surprising for an extra to exist that provides dependencies that none of the code in the PyPI package uses. (A possible counterargument is that running the tests is a way of using the code under test, and the code under test is part of the PyPI package.)

If the test extra is not wanted, I would be pleased to remove it and to update the readme and CI workflows accordingly. This could still resolve #1652, because I could appropriately weaken the claim it makes about automatic dependency installation.

Starting in Python 3.12, global and virtual Python environments no
longer automatically ship setuptools (per the "ensurepip" item in
https://docs.python.org/3.12/whatsnew/3.12.html#removed). Projects
that use setuptools as a build backend are still supported,
including with setup.py using techniques such as "pip install .".

In Windows, the "bin" subdir of a virtual environment dir is called
"Scripts" instead. Unlike in a global environment (where no names
are universal, and "python3" and "pip3" are more common for the
Python 3 commands on some popular Unix-like systems), in a virtual
environment the "python" and "pip" commands are always present and
"python3" and "pip3" are not guaranteed to be present.

This commit changes test_installation accordingly. The CI workflows
and documentation still need to be updated.
This changes the installation instructions in README.md to
recommend "pip install ." instead of "python setup.py install". The
former is compatible with Python 3.12 which doesn't have setuptools
installed by default (so setup.py, which imports it, can be
indirectly but not directly used). This also matches the
corresponding change made in the installation unit test.

While doing so, I've also clarified the instructions, and added the
implied "cd" command as well as the "git fetch --tags" command in
the position where a later section was recently updated to mention
it should have been run.

Using "pip install ." creates the opportunity to pass "-e" to make
an editable install, which users who clone the repository to work
on changes should do, because the effect of an editable install is
only partially simulated by pytest, and so that manual testing of
changes actually uses the changes intended for testing.

This increases the length and detail of the instructions, so I've
added h4 subsections to clarify the separations between them and
make it easier for readers to find the part they're looking for. In
doing so, I've reordered these subsections accordingly. Because
greater detail can create the impression that all important steps
are mentioned, I've made the general good advice to use a virtual
environment explicit. For brevity, I have not added venv commands.
This adds a #! line to the top of setup.py, because it is a script
with the executable bit set. Although neither recent nor current
documentation in the project recommends to run "./setup.py", this
should probably have the intuitive effect of attempting to run the
script with a Python interpreter rather than a Unix-style shell.

It also uses the "env trick" in init-tests-after-clone.sh so that
script runs with whatever bash interpreter is found in a normal
PATH search. While "sh" is expected to be found in /bin on all
Unix-like systems, that is not always the case for "bash". This
change slightly improves compatibility by supporting systems that
don't ship with bash but on which it has been installed.
- Remove "gitdb" from test-requirements.txt, because it already a
  dependency of the project (listed in requirements.txt, which is
  used to build the value passed for install_requires in setup.py).

- Remove "black" from requirements-dev.txt, because it is listed in
  test-requirement.txt (which requirements-dev.txt sources).
Because tests_require is deprecated since setuptools 41.5.0 with
the intention that it will be removed in some future version (noted
in https://setuptools.pypa.io/en/latest/references/keywords.html).

It is somewhat unintuitive for GitPython to have a "test" extra, as
it makes it so "GitPython[test]" can be specified for installation
from PyPI to get test dependencies, even though the PyPI package
doesn't include the unit test themselves.

However, this makes the statement in README.md that the installer
takes care of both requirements.txt and test-requirements.txt
dependencies fully true, instead of moving further away from that.

Because it is now possible to make an editable GitPython install
with test as well as minimal dependencies installed, this commit
also updates the readme to document and recommend this.
Instead of directly running setup.py.

This allows Python 3.12 (as well as previous versions) to be used
for building. Although setuptools could be added as a development
dependency to run setup.py, using "build" instead is recommended in
https://setuptools.pypa.io/en/latest/userguide/quickstart.html.

Those docs likewise recommend only listing "wheel" in the
build-system section of pyproject.toml if setup.py actually imports
the wheel module. So this removes that. (Running "make release",
which now uses "build", will continue to build wheels.)

The "build" package is not conceptually a testing dependency, but
test-requirements.txt is currently the de facto list of all stable
development dependencies for regular use.
This is cleanup related to the previous commit. As that file grows,
it is harder to tell immediately if a particular package is in it
when not alphabetized. (The groups were also not intuitive, with
ddt listed separately from other unit test related dependencies.)
This removes the step in test_installation that did the equivalent
of "pip install -r requirements.txt", because installing GitPython
is sufficient to install all its required dependencies, and it is
more important to test that than to test requirements.txt directly.

Removing this causes the test to fail if installing the project
doesn't entail installation of the requirements necessary to import
the git module or to cause gitdb to be found in a sys.path search.
Key changes:

- Update the two CI workflows to install the project and its
  dependencies in accordance with the changed recommendations in
  README.md. (This is to test that those recommendations work,
  which the changed test_installation test case partially but not
  completely tests. The old approach to installation still works
  too, so this change on CI is not required to keep CI working.)

- Add Python 3.12 to the CI test matrix in pythonpackage.yml,
  testing it on Ubuntu. (The Cygwin workflow still tests only 3.9.)

Maintenance changes, made to avoid decreasing readability with the
other changes (and hopefully even increase it somewhat):

- Separate commands into more steps, grouping them by more specific
  purposes.

- Decrease the ways the two workflows differ from each other that
  do not represent actual intended behavioral differences. This is
  to make the important differences easier to stop, and to make it
  easier to determine when the same change has or has not been made
  to both workflows.
The omission of "set -x" was intentional and is currently necessary
on Cygwin (but not on Ubuntu), per aafb92a.
@EliahKagan EliahKagan changed the title Improve Python version and OS compatibility, fix deprecations Improve Python version and OS compatibility fixing deprecations Sep 10, 2023
@EliahKagan EliahKagan changed the title Improve Python version and OS compatibility fixing deprecations Improve Python version and OS compatibility, fixing deprecations Sep 10, 2023
setup.py Show resolved Hide resolved
Setting the "fetch-depth" to 0 does a deep (i.e., ordinary) fetch,
fetching all commits and tags. Setting "submodules" to "recursive"
clones and checks out all submodules. These options allow commands
that were doing those things to be removed from the later steps.
This also adds "--noprofile --norc" to the Cygwin shell command
as a speed optimization (bash doesn't need to source its scripts).
That only changes the Cygwin workflow; in the Ubuntu workflow,
"--noprofile --norc" had already been included by default when no
shell was specified, so having it there is to *keep* the optimized
behavior that was already in use.
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great work, thank again.

I find your ability to explain changes downright super-human, along with keeping track of their internal and external dependencies, combining it all in various orthogonal issues along with a PR to fixing all of them, explaining even why this is chosen over alternatives. This is certainly appreciated, as it forces me to follow your train of thought while catching up with python. There is still a part of me that simply wants to give you write access to main and say: You know what's best, go ahead :).

With that said, I would definitely appreciated if native Windows testing could, one day, be added to CI. The time budget of about 20 minutes seems quite feasible, a bound that is currently set by cygwin, and I would be surprised if native windows manages to be worse.

For me it's most important that I can still make releases, and I think I will try to perform one from this branch and if it works, merge it right away.

.github/workflows/pythonpackage.yml Show resolved Hide resolved
setup.py Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@Byron
Copy link
Member

Byron commented Sep 11, 2023

It looks like the script currently expects python to be present, even though for me it is python3.

Maybe I am holding it wrong. Should I follow the installation instructions to get a local dev environment, and then try again from there? I might add that twine is used to perform the release, is this something that should then be added to the venv? What's the standard for that in the python world?

Thanks for your help.

@EliahKagan
Copy link
Contributor Author

EliahKagan commented Sep 11, 2023

Note: Edited to fix a badly unclear part.

I don't think we need to add twine to any of the requirements files, unless you prefer that it be added, because I don't think the situation where a twine command local to a virtual environment would work better can arise. It uploads packages, but doesn't build them, so it doesn't require the project's other build dependencies even in principle.

Although following the instructions to get a virtual environment and running python -m build (or make publish which runs it) is one option, I think it is reasonable to modify Makefile to make it so that it is not necessary for you to do this. If you haven't been using a virtual environment to build GitPython, I don't think you should have to start now.

In virtual environments (and Docker, CI, etc.) python should be preferred... but for scripts that are useful to run with a system Python, it is subjective what is best. So if running make publish for this project is meant to be compatible with a system Python when possible, then changing Makefile to use python3 instead of python is reasonable. Should I make this change?

You would still need build to be installed globally. I don't think pipx can help, since build is not available as a command (i.e., you have to run python -m build, not just build). For a system Python on Debian or a derivative of it such as Ubuntu, there is the python3-build deb package (i.e. sudo apt install python3-build). If necessary you could of course install it with pip outside any virtual environment (e.g. pip install --user build).

If you're on a system that has a global python3 command and no global python command (rather than python being a Python 2 interpreter), and this is a system like Debian or Ubuntu, then another option may be to install the python-is-python3 package (i.e., sudo apt install python-is-python3), which gives you a python command equivalent to python3. But whether or not that is applicable in your case, and whether or not you decide to do that, it seems to me that it is very reasonable to change python to python3 in Makefile if that is more convenient. I definitely do not think Makefile is one of the places where using python instead of python3 is decisively preferable.

There the system python interpreter is referred to as `python3`,
at least when installed by homebrew.
@Byron
Copy link
Member

Byron commented Sep 11, 2023

Thanks for elaborating.

I have changed the Makefile to python3 for convenience, right after installing dependencies with pip install -e ".[test]". That seems to have installed build for the current user (Collecting build (from GitPython==3.1.36) Downloading build-1.0.3-py3-none-any.whl (18 kB)), but it would still not find it when running python3 -m build.

In order to run twine on my system, I already have to set the PATH accordingly. Now I also set the PYTHON_PATH to /Users/byron/Library/Python/3.9/lib/python/site-packages:/Users/byron/Library/Python/3.9/lib which seemingly is where build was installed into, but to no avail.

Maybe a venv installation is best to solve both problems. Could you add a commit that adjusts the README.md and/or scripts that would make it work assuming only virtualenv? That at least I can execute successfully.

Thank you 😅

If a virtual environment (created by venv or virtualenv) is active,
running "make release" or "make force_release" now automatically
installs/upgrades the "build" and "twine" packages in it. This is
only done if "make" is run in a virtual environment.

This can be a fresh environment: neither the project nor its
dependencies need to be installed in it. Because the "build" module
is not currently used in any tests and running "make" in a virtual
environment takes care of installing "build" (and "twine"), "build"
is now removed from test-requirements.txt.

The publishing instructions in the readme are updated accordingly,
to mention the optional step of creating and activating a virtual
environment, and to briefly clarify why one might want to do that.

Running "make" outside a virtual environment remains supported,
except that, due to recent changes, whatever environment it is run
in needs to have a usable "build" module.
@EliahKagan
Copy link
Contributor Author

EliahKagan commented Sep 11, 2023

Could you add a commit that adjusts the README.md and/or scripts that would make it work assuming only virtualenv?

Good idea. I have now done this in 5343aa0.

There are various approaches that could be taken for this. The approach I picked was:

  • Have make release/make force_release check if a virtual environment is active, and install the build and twine packages in it if so. (The environment does not need to have the project or any dependencies installed in it, though a project virtual environment can be reused for this purpose.)
  • Update the publishing instructions in README.md with the optional step of creating and activating a virtual environment, and to mention that doing so causes the make release step to automatically install build and twine.
  • Remove build from test-requirements.txt, since no tests currently use it and make now automates its installation.

I have tested this (as best as I can) on Ubuntu with Python 3.11, in virtual environments created with virtualenv, as well as with python -m venv as I am more accustomed to using. As expected, both worked, within the limits of my testing. Besides running various commands manually, I tested make force_release (and pressed Ctrl+C when twine prompted me for credentials).

I have kept the change of python to python3 that you made in f86f09e. I considered having it use python if in a virtual environment and python3 otherwise, but I don't think that would increase portability yet. I believe--and testing seems to confirm--that the ref checking in make release is currently nonportable. It passes head to git where HEAD would be required on most other *nix systems due to case sensitivity. It also uses sort -n with tag names of the form x.y.z (on systems I have tested this on, only the x and y parts are ordered numerically). Making Makefile portable is something that could be done later. Even though portability is the theme of this PR, I think it's better to defer that, provided make release is working at least on macOS.

The installation instructions and release instructions link to two different pages on virtual environments. This is intentional, but I'd be pleased to change if it you prefer. Because venv is more common than virtualenv and more widely recommended today, and to avoid overwhelming developers less familiar with virtual environments with choices they may not be interested in, I have retained the link to venv-only documentation that I previously added to the installation instructions. But in the new step of the release instructions, I linked to another (also official) page that talks both about venv and virtualenv.

@Byron Byron merged commit ded846d into gitpython-developers:main Sep 12, 2023
16 checks passed
@EliahKagan EliahKagan deleted the setup branch September 12, 2023 06:01
@Byron
Copy link
Member

Byron commented Sep 12, 2023

Thank you so much! The trick really was to run python3 -m venv env to get a virtual-env, and then installing any package actually works.

I just created a new release which, in hindsight, I was unnecessary as it doesn't contain user-facing changes.

Regarding make release, I haven't been using this make target in a long time as it didn't really work and I could just use make force_release while ideally not forgetting to create a tag. Automating this further isn't needed, I believe, but won't hurt either. In theory, a python project about git should be able to use itself to deal with tags and tag creation, I'd think 😁.

@EliahKagan
Copy link
Contributor Author

EliahKagan commented Sep 13, 2023

I just created a new release which, in hindsight, I was unnecessary as it doesn't contain user-facing changes.

I'm not sure if this should've had a release or not.

Starting in 3.1.36, it is possible to run pip install "GitPython[test]", which downloads and installs a GitPython wheel from PyPI, along with its test dependencies. Users should not actually do this, because the test extra is not useful to use in that particular way, but it is possible. Since the test extra is only useful for development, I think it makes sense that it is documented only for that, and referenced only indirectly in the release description.

When I saw d99b2d4, I thought to ask if it should be a post release instead. But I realized that, due to the new extra, that should probably not be done (since I think it is best for post releases to carry zero installed difference, even in situations that are unusual or not recommended).

On the other hand, it might've been best to avoid it, because it's not necessary to upgrade to it, and because the resulting downstream 3.1.36 in conda-forge really is identical to the corresponding downstream 3.1.35. I don't know what the best practice is in this situation.

Regarding make release, I haven't been using this make target in a long time as it didn't really work and I could just use make force_release while ideally not forgetting to create a tag.

make release uses git tag | sort -nr. Do you happen to know if, on macOS, that sorts according to SemVer, at least for the tag names that actually exist in this repository? On my Ubuntu system, it lists 2.1.2 above 2.1.15 and, more subtly, lists 0.3.2-RC1 above 0.3.2 and lists 0.1.4-pre above 0.1.4. Because that's supposed to be later releases above earlier releases (as -r reverses the order), this behavior is not correct.

On GNU/Linux systems including my Ubuntu system, sort is GNU sort, which supports -V/--version-sort. When used in place of -n, this does the right thing with 2.1.2/2.1.15, but still not with 0.3.2-RC1/0.3.2 or 0.1.4-pre/0.1.4. This is the same problem the otherwise more elegant git tag --sort=-v:refname has.

If git tag | sort -nr does what Makefile intends it to do on macOS, then I'd be interested in ways to preserve something like the currently written release target while making it more portable. If not, I'd be more interested in replacing it with something else.

@EliahKagan
Copy link
Contributor Author

EliahKagan commented Sep 13, 2023

Replying to #1654 (review):

I find your ability to explain changes downright super-human, along with keeping track of their internal and external dependencies, combining it all in various orthogonal issues along with a PR to fixing all of them, explaining even why this is chosen over alternatives. This is certainly appreciated, as it forces me to follow your train of thought while catching up with python.

Thanks! :) :)

There is still a part of me that simply wants to give you write access to main and say: You know what's best, go ahead :).

It's an honor to hear that such a thing has even crossed your mind! However, I recommend against doing any such thing, at least at this time. The reason is that I actually have only minimal knowledge of GitPython. I use GitPython indirectly--because, as a very popular library, it is a dependency of other projects I use directly, especially nbdime. As a result, if I did have write access, it would be the current situation, just with extra steps: I would not be comfortable making any unreviewed changes.

That's the origin of my interest in GitPython. I learned of CVE-2023-40590 (#1635) from security alerts on a couple of my repositories that declare nbdime as a development dependency, so I decided to contribute #1636. Unfortunately, even once amended in #1650, my fix also introduces an os.environ race condition, so maybe it could be done better. Given that the still-flawed fix (like the CVE it fixes) affects native Windows only, I would feel better experimenting with such improvements once we have...

With that said, I would definitely appreciated if native Windows testing could, one day, be added to CI. The time budget of about 20 minutes seems quite feasible, a bound that is currently set by cygwin, and I would be surprised if native windows manages to be worse.

Yes, this is something I am very much interested in doing. (Though I certainly do not want to stop anybody else who wants to work on it!)

I'm interested in figuring out (a) which, of tests that do not pass for me on Windows, should pass because they fail due to a problem specific to my environment or fork, and, (b) of others that are not already skipped on Windows, if the bugs that break them on native Windows python can simply be fixed. #1651 is an example of such a bug. I'd rather find and fix more bugs like that--if there are any--than obscure them to get CI to pass.

I'm also hoping to find some way to make it significantly faster than the Cygwin test job (currently) is, even if not nearly as fast as the Ubuntu test jobs. If the tests can be made to run fast enough on native Windows, then a matrix strategy could be used as in the Ubuntu test workflow to test on all Python versions GitPython supports (possibly even as part of that workflow, by adding an OS dimension to the existing matrix, depending on how much Window-specific preparation ends up having to be done). Because each job gets its own runner and the runners operate in parallel, six 20-minute checks are no problem for one push, but could slow things down a lot with many pushes in a short time, due to limits on the number of GitHub Actions runners per user or organization.

If worse comes to worse, there is always the middle ground of testing a smaller number of versions on native Windows but still more than one. Perhaps 3.8 and 3.12.

@hugovk
Copy link
Contributor

hugovk commented Sep 13, 2023

make release uses git tag | sort -nr. Do you happen to know if, on macOS, that sorts according to SemVer, at least for the tag names that actually exist in this repository?

macOS Ventura doesn't sort according to SemVer:

sort --version
2.3-Apple (154)git tag | sort -nr
3.1.9
3.1.8
3.1.7
3.1.6
3.1.5
3.1.4
3.1.36
3.1.35
3.1.34
3.1.33
3.1.32
3.1.31
3.1.30
3.1.3
3.1.29
3.1.28
3.1.27
3.1.26
3.1.25
3.1.24
3.1.23
3.1.22
3.1.20
3.1.2
3.1.19
3.1.18
3.1.17
3.1.16
3.1.15
3.1.14
3.1.13
3.1.12
3.1.11
3.1.10
3.1.1
3.1.0
3.0.9
3.0.8
3.0.7
3.0.6
3.0.5
3.0.4
3.0.3
3.0.2
3.0.1
3.0.0
2.1.9
2.1.8
2.1.7
2.1.6
2.1.5
2.1.4
2.1.3
2.1.2
2.1.15
2.1.14
2.1.13
2.1.12
2.1.11
2.1.10
2.1.1
2.1.0
2.0.9
2.0.8
2.0.7
2.0.6
2.0.5
2.0.4
2.0.3
2.0.2
2.0.1
2.0.0
1.0.2
1.0.1
1.0.0
0.3.7
0.3.6
0.3.5
0.3.4
0.3.3
0.3.2.1-patched
0.3.2.1
0.3.2-RC1
0.3.2
0.3.1-beta2
0.3.1-beta1
0.3.0-beta2
0.3.0-beta1
0.2.0-beta1
0.1.7
0.1.6
0.1.5
0.1.4-pre
0.1.4
winerr_show

@Byron
Copy link
Member

Byron commented Sep 13, 2023

Thanks for sharing your "origin story" - I start liking CVEs much more now, given they made you aware of this project and caused the first contribution :).
I understand your position regarding becoming a maintainer, and if there is ever a change to that so there is benefit to make you one, just let me know :).

I'd rather find and fix more bugs like that--if there are any--than obscure them to get CI to pass.

If I recall correctly, most test-failures on windows arise from its 'unique' way of handling open files, which is quite incompatible with the way files are handled on Unix systems. That said, by now things might have changed and it's probably worth to at least retry the ignored tests to see if some of them work now.

I also think that having any kind of native windows CI job is orthogonal to making more tests pass - the latter is entirely optional to me and definitely nothing more than 'nice to have' at this point. CI on native python on Windows would be a great step forward already.

[..] but could slow things down a lot with many pushes in a short time, due to limits on the number of GitHub Actions runners per user or organization.

These days there isn't too much CI activity here, but if it was a problem I am willing to cancel some jobs by hand. It's something I do quite regularly in gitoxide where CI takes 30 to 40 minutes.

@EliahKagan
Copy link
Contributor Author

@hugovk Thanks--I find that very helpful, because it tells me sort -n doesn't work for this on any common systems and should be abandoned.

Based on that, I looked for alternatives to sort -n and sort -V that better support SemVer, and found versionsort.suffix, which will let git tag --sort=-v:refname do it well enough:

git -c versionsort.suffix=-alpha -c versionsort.suffix=-beta -c versionsort.suffix=-pre -c versionsort.suffix=-rc -c versionsort.suffix=-RC tag --sort=-v:refname

If git's -c supported a form that allowed it and its operand to be passed as a single command-line argument, then brace expansion (in a shell that has it) could abbreviate that nicely. Unfortunately it doesn't. But:

git $(printf ' -c versionsort.suffix=-%s' alpha beta pre rc RC) tag --sort=-v:refname

Then, to also filter out non-version tags (they're at the top now, so must be dropped before | head -n1):

git $(printf ' -c versionsort.suffix=-%s' alpha beta pre rc RC) tag -l '[0-9]*' --sort=-v:refname

(Where the [0-9]* is a glob, not a regex, and matches the entire tag name, so it's like greping for ^[0-9].)

@EliahKagan
Copy link
Contributor Author

EliahKagan commented Sep 13, 2023

@Byron

I understand your position regarding becoming a maintainer, and if there is ever a change to that so there is benefit to make you one, just let me know :).

Thanks! That might make sense if/when I have learned more about the codebase (though I would still likely request reviews on many changes).

I also think that having any kind of native windows CI job is orthogonal to making more tests pass - the latter is entirely optional to me and definitely nothing more than 'nice to have' at this point. CI on native python on Windows would be a great step forward already.

That makes sense. If it's okay for native Windows CI to be set up even with tests failing that are not currently skipped or documented to fail on Windows, then I may be able to do it sooner. I think there's still some stuff I need to figure out, though. I think some of the tests that fail on my Windows system are failing only because not everything is set up properly. Furthermore, none of the changes I've made that needed to be tested on Windows required me to set up git-daemon, and I have never done so.

Since I believe Windows GitHub Actions runners include full Git for Windows installations, and Git for Windows does ship git-daemon.exe in its /mingw64/libexec/git-core directory (where / is not really the root of a Windows drive, but somewhere deeper, for example that path on my Windows system is really C:\Users\ek\scoop\apps\git\2.42.0.2\mingw64\libexec\git-core), any insights from figuring that out locally should carry over.

@Byron
Copy link
Member

Byron commented Sep 13, 2023

git $(printf ' -c versionsort.suffix=-%s' alpha beta pre rc RC) tag -l '[0-9]*' --sort=-v:refname

This is absolutely amazing! I know git can do a lot, but wasn't aware of this configuration option.

That makes sense. If it's okay for native Windows CI to be set up even with tests failing that are not currently skipped or documented to fail on Windows, then I may be able to do it sooner.

Yes, absolutely. I think it's better to split the task into CI Setup and Tests themselves. That way, Tests can simply be ignored when found failing at first until the is time to see if they could also work. Additionally, I think it's perfectly alright to only test the borders of the supported version range of Python, and for that test different Windows versions. I know that GHA supports quite some parallelism, but I'd prefer to conserve the compute anyway and optimize for actual coverage. Limiting oneself somewhat when setting up additional jobs seems particularly easy here since this GitPython seems to have been fine without any of it for more than a decade now.

renovate bot added a commit to allenporter/flux-local that referenced this pull request Sep 23, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [GitPython](https://togithub.com/gitpython-developers/GitPython) |
`==3.1.36` -> `==3.1.37` |
[![age](https://developer.mend.io/api/mc/badges/age/pypi/GitPython/3.1.37?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/GitPython/3.1.37?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/GitPython/3.1.36/3.1.37?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/GitPython/3.1.36/3.1.37?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>gitpython-developers/GitPython (GitPython)</summary>

###
[`v3.1.37`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.37):
- a proper fix CVE-2023-41040

[Compare
Source](https://togithub.com/gitpython-developers/GitPython/compare/3.1.36...3.1.37)

#### What's Changed

- Improve Python version and OS compatibility, fixing deprecations by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1654
- Better document env_case test/fixture and cwd by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1657
- Remove spurious executable permissions by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1658
- Fix up checks in Makefile and make them portable by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1661
- Fix URLs that were redirecting to another license by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1662
- Assorted small fixes/improvements to root dir docs by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1663
- Use venv instead of virtualenv in test_installation by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1664
- Omit py_modules in setup by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1665
- Don't track code coverage temporary files by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1666
- Configure tox by [@&#8203;EliahKagan](https://togithub.com/EliahKagan)
in
[gitpython-developers/GitPython#1667
- Format tests with black and auto-exclude untracked paths by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1668
- Upgrade and broaden flake8, fixing style problems and bugs by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1673
- Fix rollback bug in SymbolicReference.set_reference by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1675
- Remove `@NoEffect` annotations by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[gitpython-developers/GitPython#1677
- Add more checks for the validity of refnames by
[@&#8203;facutuesca](https://togithub.com/facutuesca) in
[gitpython-developers/GitPython#1672

**Full Changelog**:
gitpython-developers/GitPython@3.1.36...3.1.37

</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:eyJjcmVhdGVkSW5WZXIiOiIzNi45Ny4xIiwidXBkYXRlZEluVmVyIjoiMzYuOTcuMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
otc-zuul bot pushed a commit to opentelekomcloud-infra/grafana-docs-monitoring that referenced this pull request Oct 25, 2023
Bump gitpython from 3.1.35 to 3.1.37

Bumps gitpython from 3.1.35 to 3.1.37.

Release notes
Sourced from gitpython's releases.

3.1.37 - a proper fix CVE-2023-41040
What's Changed

Improve Python version and OS compatibility, fixing deprecations by @​EliahKagan in gitpython-developers/GitPython#1654
Better document env_case test/fixture and cwd by @​EliahKagan in gitpython-developers/GitPython#1657
Remove spurious executable permissions by @​EliahKagan in gitpython-developers/GitPython#1658
Fix up checks in Makefile and make them portable by @​EliahKagan in gitpython-developers/GitPython#1661
Fix URLs that were redirecting to another license by @​EliahKagan in gitpython-developers/GitPython#1662
Assorted small fixes/improvements to root dir docs by @​EliahKagan in gitpython-developers/GitPython#1663
Use venv instead of virtualenv in test_installation by @​EliahKagan in gitpython-developers/GitPython#1664
Omit py_modules in setup by @​EliahKagan in gitpython-developers/GitPython#1665
Don't track code coverage temporary files by @​EliahKagan in gitpython-developers/GitPython#1666
Configure tox by @​EliahKagan in gitpython-developers/GitPython#1667
Format tests with black and auto-exclude untracked paths by @​EliahKagan in gitpython-developers/GitPython#1668
Upgrade and broaden flake8, fixing style problems and bugs by @​EliahKagan in gitpython-developers/GitPython#1673
Fix rollback bug in SymbolicReference.set_reference by @​EliahKagan in gitpython-developers/GitPython#1675
Remove @NoEffect annotations by @​EliahKagan in gitpython-developers/GitPython#1677
Add more checks for the validity of refnames by @​facutuesca in gitpython-developers/GitPython#1672

Full Changelog: gitpython-developers/GitPython@3.1.36...3.1.37



Commits

b27a89f fix makefile to compare commit hashes only
0bd2890 prepare next release
832b6ee remove unnecessary list comprehension to fix CI
e98f57b Merge pull request #1672 from trail-of-forks/robust-refname-checks
1774f1e Merge pull request #1677 from EliahKagan/no-noeffect
a4701a0 Remove @NoEffect annotations
d40320b Merge pull request #1675 from EliahKagan/rollback
d1c1f31 Merge pull request #1673 from EliahKagan/flake8
e480985 Tweak rollback logic in log.to_file
ff84b26 Refactor try-finally cleanup in git/
Additional commits viewable in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually 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.

Reviewed-by: Vladimir Vshivkov
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Nov 14, 2023
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.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Nov 14, 2023
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.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request Nov 15, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants