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

Upgrade pygit2 to allow version until <2.0 #330

Merged
merged 6 commits into from Mar 24, 2022

Conversation

paulodiovani
Copy link
Contributor

@paulodiovani paulodiovani commented Mar 14, 2022

Current tartufo version requires dependency pygit2 <1.7, which depends on libgit2 v1.1.x (see https://github.com/libgit2/pygit2/blob/master/CHANGELOG.rst).
Most recent operating systems packages include libgit2 v1.2 or newer (newest version is 1.4.2), which would break the installation of tartufo.

This PR updates pygit2 dependency to allow until version <2.0, which should be ok as no breaking changes have been added until v1.9.


To help us get this pull request reviewed and merged quickly, please be sure to include the following items:

  • Tests (if applicable)
  • Documentation (if applicable)
  • Changelog entry
  • A full explanation here in the PR description of the work done

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Tests
  • Other

Backward Compatibility

Is this change backward compatible with the most recently released version? Does it introduce changes which might change the user experience in any way? Does it alter the API in any way?

  • Yes (backward compatible)
  • No (breaking changes)

Breaking change notice

  • pygit2 v1.8.0 introduces a breaking change by renaming RemoteCallbacks.progress() to RemoteCallbacks.sideband_progress(), but I didn't find any ocurrence of this function call in the source code.

Issue Linking

What's new?

  • Compatible to pygit2 from versions 1.6 to 1.9.x

@tarkatronic
Copy link
Contributor

Unfortunately this is not a possible upgrade at the moment. We had to make the decision at the time to pin pygit2 to an older version, as we had to maintain Python 3.6 support, and this was the newest version of pygit2 that allowed that.

It would be great to drop Python 3.6 support, but that will require a major version bump (v4.0) which requires a lot harder consideration.

One thing that has been discussed, but not yet experimented with, is that we might possibly be able to require different versions of pygit2 depending on the Python version. If this works, it could be a very nice solution.

Keep old pygit2 version pinned for python 3.6, but allow newer versions
to use the current version (which supports 3.10).

Add 3.10 to tox and CI version matrices.
@rbailey-godaddy
Copy link
Contributor

I merged a commit that twiddles the dependency along the line @tarkatronic suggested, together with adding python 3.10 to the unit test matrices. My PC doesn't have fully-baked environments for all of these versions available, so we'll need to see what verdict is returned by the CI tests and then potentially do some followup triage.

@paulodiovani
Copy link
Contributor Author

I merged a commit that twiddles the dependency along the line @tarkatronic suggested, together with adding python 3.10 to the unit test matrices. My PC doesn't have fully-baked environments for all of these versions available, so we'll need to see what verdict is returned by the CI tests and then potentially do some followup triage.

I was doing the multiple dependency change and when I pushed you havea lready done it 😅

Good job. And thanks.

@rbailey-godaddy
Copy link
Contributor

Good job. And thanks.

Thank me when it works. I am about to commit a further change to rebase the docker image onto 3-slim (Debian) instead of alpine, as this significantly simplifies the build and eliminates weird libgit2 dependencies (because we can install wheel packages).

This switches from alpine to Debian. The old base had become unworkable
because alpine ships only libgit 1.3.0, and pygit2 needs libgit 1.4.0.

Debian is GLIBC-based, enabling the use of binary wheels, so pygit2
includes the necessary pre-built libraries and we no longer have
dependencies on the OS-provided library.
Copy link
Contributor

@tarkatronic tarkatronic left a comment

Choose a reason for hiding this comment

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

Just one small change and one question here -- thanks for putting in this work, this is going to help a lot of people!

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -1,4 +1,4 @@
FROM python:3-alpine3.14 as base
FROM python:3-slim as base
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the resulting change in image size here?

Copy link
Contributor

Choose a reason for hiding this comment

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

3-alpine3.14 is 45.2 MB; 3-slim is 122 MB -- the final image is 260 MB, which is noticeably larger than the 88.1 MB of the current alpine-based build from main.

On the other hand, it's not quite apples-to-apples because the current code (in this branch) can't be built on any existing version of alpine at all, because compilation requires newer versions of some libraries than anything shipped by alpine. We could potentially fudge this by backleveling versions (to less backleveled than they were) in hopes of finding some happy medium, but it just didn't seem worth it.

That was before I read https://pythonspeed.com/articles/base-image-python-docker-images/ yesterday. :)

Co-authored-by: Joey Wilhelm <tarkatronic@gmail.com>
Copy link
Contributor

@smimani-godaddy smimani-godaddy left a comment

Choose a reason for hiding this comment

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

LGTM 🦈

@tarkatronic tarkatronic merged commit d5e4968 into godaddy:main Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants