Replace GitPython with pygit2 #2120

mgorny · 2024-11-06T16:16:18Z

Checklist

Added a news entry
Regenerated schema JSON if schema altered (python conda_smithy/schema.py)

Replace the use of GitPython package with pygit2. The latter seems to have better git support, in particular it supports the newer index versions 3 and 4. Since it is backed by the libgit2 library that is also used by Cargo, it seems to have the best chances of being updated for compatibility with new git versions.

Admittedly, the API feels very low-level. In particular, it is necessary to explicitly request writing changes to index back, and explicitly reread it when it's modified externally (e.g. via another pygit2.Repository instance, as in tests). On the plus side, it does not invoke git at all -- everything is done by the library.

Fixes #2116

So far focused on feedstock_io.py and its tests. I need to figure out how to test the changes to other files properly, given that the tests mock the entire git.Repo.clone_from call.

conda_smithy/feedstock_io.py

Replace the use of GitPython package with pygit2. The latter seems to have better git support, in particular it supports the newer index versions 3 and 4. Since it is backed by the libgit2 library that is also used by Cargo, it seems to have the best chances of being updated for compatibility with new git versions. Admittedly, the API feels very low-level. In particular, it is necessary to explicitly request writing changes to index back, and explicitly reread it when it's modified externally (e.g. via another `pygit2.Repository` instance, as in tests). On the plus side, it does not invoke `git` at all -- everything is done by the library. Fixes conda-forge#2116

Remove the `search_parent_directories` kwarg that's never been used, and instead always enable searching parent directories for better cross-version pygit2 compatibility.

beckermr

This is an API change and is not allowed under semantic versioning. Please put back this functionality.

conda_smithy/feedstock_io.py

This reverts commit a21d135.

mgorny · 2024-11-11T15:24:41Z

Restored, and instead added backwards compatibility for pygit2 < 1.14.

mgorny · 2024-11-11T15:40:32Z

@beckermr, another use is in conda_smithy.feedstocks.feedstocks_repos() function. This function is used only in conda_smithy.feedstocks.feedstocks_yaml(), and FWICS the latter isn't exposed anywhere in the CLI. On top of that, feedstocks_repos() is currently broken:

In [10]: list(conda_smithy.feedstocks.feedstocks_repos(None, "/home/mgorny/git/conda"))
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In[10], line 1
----> 1 list(conda_smithy.feedstocks.feedstocks_repos(None, "/home/mgorny/git/conda"))

File ~/git/conda-smithy/conda_smithy/feedstocks.py:197, in feedstocks_repos(organization, feedstocks_directory, pull_up_to_date, randomise, regexp)
    195 for feedstock in feedstocks:
    196     repo = git.Repo(feedstock.directory)
--> 197     upstream = repo.remotes.upstream
    199     if pull_up_to_date:
    200         print("Fetching ", feedstock.package)

File ~/miniforge3/envs/conda-smithy/lib/python3.12/site-packages/git/util.py:1198, in IterableList.__getattr__(self, attr)
   1196         return item
   1197 # END for each item
-> 1198 return list.__getattribute__(self, attr)

AttributeError: 'IterableList' object has no attribute 'upstream'

FWICS git.Repo.remotes was always a list, so I don't think this code ever worked. Should I migrate it and fix it, or remove both this function and feedstocks_yaml()?

beckermr · 2024-11-11T15:43:22Z

We need to migrate and fit.

mgorny · 2024-11-11T15:55:56Z

Ah, sorry — I was wrong, it wasn't broken. I've just realized it expected a remote called upstream, my bad.

environment.yml

mgorny · 2024-11-11T17:18:05Z

Another question: do we need SSH support for feedstock_tokens? FWICS the default is to use the repository over HTTPS. If we were to support SSH as well, there are two snags:

libgit2 on Conda has a bug that results in SSH support being disabled. I've filed Actually enabling SSH support while building libgit2-feedstock#80 to fix that.
Since libgit2 is quite low-level by design, SSH support requires a bit of effort. I'm not sure how much yet, but the example is a bit worrying.

Of course, another option is to call git directly for the push (much like GitPython does).

beckermr · 2024-11-11T17:28:03Z

Anything that is currently supported in the code needs to be supported in this PR. We cannot have API changes like this.

mgorny · 2024-11-11T17:37:43Z

Anything that is currently supported in the code needs to be supported in this PR. We cannot have API changes like this.

Is it okay to call git push as a subprocess then? That would guarantee the best compatibility (and be pretty much what GitPython did anyway). There's also a matter of whether to call git commit, or reimplement getting author/committer name from git config.

beckermr · 2024-11-11T17:55:29Z

Subprocesses are fine, but we should be mindful of how complex this might get. The point of this PR is to support new git index versions. IDK anything about those. Are these in use? How have we not hit bugs for this before new?

mgorny · 2024-11-11T18:05:48Z

Subprocesses will probably be less complex than doing everything via libgit2. Another option is to stick to GitPython for some of the code, at least until it actually breaks for someone.

From what I understand, to hit this issue you need to use new enough git to clone the repository. It is also possible that the repository itself must have some characteristics that actually trigger the use of new index format. Maybe I was just unlucky that the first feedstock that I've cloned triggered this, or it is possible that more people will hit this as they upgrade their git to new versions and clone new feedstocks.

beckermr · 2024-11-11T18:20:05Z

I'd rather have one tool used and defer to subprocesses. Let's use a subprocess.

mgorny · 2024-11-11T19:09:29Z

I've updated the remaining code. However, I still need to update the tests. I'll do that tomorrow. However, feel free to point out if you don't like some of the changes and would prefer a different approach.

jaimergp · 2024-11-12T15:58:17Z

conda_smithy/feedstock_io.py

-        )
+        if search_parent_directories:
+            path = pygit2.discover_repository(path)
+        if path is not None:


What happens if path is none? Should we raise?

The original behavior was to return repo, i.e. None then, and I've preserved that. In other words, if anything fails (pygit2 is not installed, there is no repo, opening repo fails for some reason), the function returns None, as it used to.

conda_smithy/feedstock_tokens.py

jaimergp · 2024-11-12T16:02:52Z

conda_smithy/feedstocks.py

+        clone = pygit2.clone_repository(repo.clone_url, clone_directory)
+        clone.remotes.delete("origin")


So wait, now we can clone? I thought we were using git subprocesses for this.

Yes, libgit2 can clone directly. However, as noted in the PR discussion this (as implemented here) works for public HTTPS repos only. Using libgit2 to clone via SSH would require fixed libgit2 on conda and explicit credential handling which would be a lot of code and not 100% equivalent to external git anyway.

jaimergp · 2024-11-12T16:30:23Z

I think we just need a news item now!

mgorny · 2024-11-12T17:35:11Z

Ok, I'm done updating tests. I think the code is ready now.

jaimergp · 2024-11-12T21:42:41Z

Looks like some tests are still using gitpython @mgorny

mgorny · 2024-11-13T05:55:19Z

My bad. Really should've uninstalled GitPython when testing locally.

mgorny · 2024-11-13T11:16:57Z

I'm really sorry about that. Fixed now. Tests pass for me now with GitPython uninstalled, and I can't grep any more import git or from git.

beckermr

Needs a news item.

mgorny · 2024-11-13T12:34:25Z

Added.

news/2120-replace-gitpython-with-pygit2.rst

Co-authored-by: Matthew R. Becker <beckermr@users.noreply.github.com>

beckermr

So this PR changes some pretty low-level functionality. The test suite uses mocks which is great, but we don't have tests of functioning code for all of the changes. We need to test these changes live for both staged-recipes and feedstock token handling before we can merge them.

beckermr · 2024-11-13T13:47:07Z

I am happy to do the testing of these changes live, but it will be a bit before I can get to it.

mgorny · 2024-11-13T14:06:31Z

Not sure if you're asking me to do anything here. If you're asking whether I've tested them locally, I did test calling every function with some args suitable for local/semi-remote testing.

beckermr · 2024-11-13T14:08:23Z

Yep, nothing for you to do here. Sorry for blocking this one. Testing much of the code in smithy is hard since it runs against external services. :/

mgorny · 2024-11-13T14:11:59Z

Yeah, I know. If I only had more time, I'd have added some more tests.

mgorny requested a review from a team as a code owner November 6, 2024 16:16

mgorny marked this pull request as draft November 6, 2024 16:16

mgorny force-pushed the pygit2 branch 2 times, most recently from f6dadb8 to e90a067 Compare November 6, 2024 16:21

mgorny commented Nov 6, 2024

View reviewed changes

conda_smithy/feedstock_io.py Outdated Show resolved Hide resolved

mgorny force-pushed the pygit2 branch from e90a067 to a5ac0b5 Compare November 6, 2024 16:34

feedstock_io: Always search parent directories

a21d135

Remove the `search_parent_directories` kwarg that's never been used, and instead always enable searching parent directories for better cross-version pygit2 compatibility.

mgorny marked this pull request as ready for review November 11, 2024 14:20

Merge branch 'main' into pygit2

a6eb2e2

beckermr requested changes Nov 11, 2024

View reviewed changes

beckermr reviewed Nov 11, 2024

View reviewed changes

conda_smithy/feedstock_io.py Show resolved Hide resolved

Revert "feedstock_io: Always search parent directories"

6b820d7

This reverts commit a21d135.

Fix compatibility with pygit2 < 1.14

fc4dd1c

mgorny force-pushed the pygit2 branch from 3aa51f9 to fc4dd1c Compare November 11, 2024 15:25

beckermr requested changes Nov 11, 2024

View reviewed changes

environment.yml Outdated Show resolved Hide resolved

Migrate conda_smithy.feedstocks to pygit2

1f77dde

mgorny added 3 commits November 11, 2024 19:45

Migrate conda_smithy.feedstock_tokens to pygit2+git

e199014

Update feedstocks_yaml() for pygit2 as well

be0d63e

Remove GitPython from environment.yml

127a1b9

Reformat

5c0eeb5

jaimergp reviewed Nov 12, 2024

View reviewed changes

conda_smithy/feedstock_tokens.py Show resolved Hide resolved

jaimergp reviewed Nov 12, 2024

View reviewed changes

conda_smithy/feedstock_tokens.py Outdated Show resolved Hide resolved

jaimergp reviewed Nov 12, 2024

View reviewed changes

Use f-string for commit message in feedstock_tokens

a628f1b

mgorny added 2 commits November 12, 2024 18:33

Readd missing clone call to feedstock_tokens

285ad42

Update mocks for feedstock_tokens tests

a23beb3

Migrate conda_smithy.github to pygit2

f31439f

Merge branch 'main' into pygit2

0e208a2

beckermr requested changes Nov 13, 2024

View reviewed changes

Add a news entry

5cad8bd

beckermr reviewed Nov 13, 2024

View reviewed changes

news/2120-replace-gitpython-with-pygit2.rst Outdated Show resolved Hide resolved

Apply suggestions from code review

ad3118e

Co-authored-by: Matthew R. Becker <beckermr@users.noreply.github.com>

beckermr requested changes Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace GitPython with pygit2 #2120

Replace GitPython with pygit2 #2120

mgorny commented Nov 6, 2024 •

edited

Loading

beckermr left a comment

mgorny commented Nov 11, 2024

mgorny commented Nov 11, 2024

beckermr commented Nov 11, 2024

mgorny commented Nov 11, 2024

mgorny commented Nov 11, 2024

beckermr commented Nov 11, 2024

mgorny commented Nov 11, 2024

beckermr commented Nov 11, 2024

mgorny commented Nov 11, 2024

beckermr commented Nov 11, 2024

mgorny commented Nov 11, 2024

jaimergp Nov 12, 2024

mgorny Nov 12, 2024

jaimergp Nov 12, 2024

mgorny Nov 12, 2024

jaimergp commented Nov 12, 2024

mgorny commented Nov 12, 2024

jaimergp commented Nov 12, 2024

mgorny commented Nov 13, 2024

mgorny commented Nov 13, 2024

beckermr left a comment

mgorny commented Nov 13, 2024

beckermr left a comment

beckermr commented Nov 13, 2024

mgorny commented Nov 13, 2024

beckermr commented Nov 13, 2024

mgorny commented Nov 13, 2024

		clone = pygit2.clone_repository(repo.clone_url, clone_directory)
		clone.remotes.delete("origin")

Replace GitPython with pygit2 #2120

Are you sure you want to change the base?

Replace GitPython with pygit2 #2120

Conversation

mgorny commented Nov 6, 2024 • edited Loading

beckermr left a comment

Choose a reason for hiding this comment

mgorny commented Nov 11, 2024

mgorny commented Nov 11, 2024

beckermr commented Nov 11, 2024

mgorny commented Nov 11, 2024

mgorny commented Nov 11, 2024

beckermr commented Nov 11, 2024

mgorny commented Nov 11, 2024

beckermr commented Nov 11, 2024

mgorny commented Nov 11, 2024

beckermr commented Nov 11, 2024

mgorny commented Nov 11, 2024

jaimergp Nov 12, 2024

Choose a reason for hiding this comment

mgorny Nov 12, 2024

Choose a reason for hiding this comment

jaimergp Nov 12, 2024

Choose a reason for hiding this comment

mgorny Nov 12, 2024

Choose a reason for hiding this comment

jaimergp commented Nov 12, 2024

mgorny commented Nov 12, 2024

jaimergp commented Nov 12, 2024

mgorny commented Nov 13, 2024

mgorny commented Nov 13, 2024

beckermr left a comment

Choose a reason for hiding this comment

mgorny commented Nov 13, 2024

beckermr left a comment

Choose a reason for hiding this comment

beckermr commented Nov 13, 2024

mgorny commented Nov 13, 2024

beckermr commented Nov 13, 2024

mgorny commented Nov 13, 2024

mgorny commented Nov 6, 2024 •

edited

Loading