GeezerVison

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.

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 😅

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.

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 😁.

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.

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.

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

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.

@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].)

@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.

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.