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

Drop packaging dependency #3205

Merged
merged 1 commit into from
Jan 3, 2024
Merged

Drop packaging dependency #3205

merged 1 commit into from
Jan 3, 2024

Conversation

akx
Copy link
Contributor

@akx akx commented Dec 21, 2023

It doesn't seem to be required.

Implement the same version comparison algorithm Moby uses instead.

@akx akx force-pushed the drop-packaging-dep branch from 534c665 to e54691f Compare December 21, 2023 08:21
@akx akx marked this pull request as ready for review December 21, 2023 08:24
Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

packaging.Version is used in docker/utils/utils.py:

s1 = Version(v1)
s2 = Version(v2)

That was changed from distutils.Version:

I'm happy to get rid of the dependency, especially given the small surface area. Do you have a suggested alternative?

@milas milas added the dependencies Pull requests that update a dependency file label Jan 3, 2024
@akx
Copy link
Contributor Author

akx commented Jan 3, 2024

packaging.Version is used in docker/utils/utils.py:

Ah, sorry, yeah, good catch.

Do you have a suggested alternative?

How are Docker versions specified? packaging.Version is, after all, meant to parse Python version specifiers, not versions in general...

If they're specified to be just A.B[.C] where each part is an integer, then it's trivial to split, cast to integers in a tuple and compare them :)

@milas
Copy link
Contributor

milas commented Jan 3, 2024

If they're specified to be just A.B[.C] where each part is an integer, then it's trivial to split, cast to integers in a tuple and compare them :)

Yep, that's effectively what Moby does: https://github.com/moby/moby/blob/e7001c14558af124d7959c1d1cfe3a16b507c521/api/types/versions/compare.go#L8-L40

(Technically there can be an arbitrary number of version sections; in practice, I hope we never make an API version 1.2.3.4.5.6.7.8.9.0 😛)

@akx akx force-pushed the drop-packaging-dep branch from e54691f to 9bf9a02 Compare January 3, 2024 19:12
Compare versions like Moby (api/types/versions/compare.go)

Signed-off-by: Aarni Koskela <akx@iki.fi>
@akx akx force-pushed the drop-packaging-dep branch from 9bf9a02 to 249654d Compare January 3, 2024 19:12
@akx
Copy link
Contributor Author

akx commented Jan 3, 2024

@milas Great :) Rebased, implemented the same algorithm here, added tests.

@akx akx requested a review from milas January 3, 2024 19:13
Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

LGTM and 😲 with that turn-around time 🎉

@milas milas enabled auto-merge January 3, 2024 19:16
@milas milas merged commit bd164f9 into docker:main Jan 3, 2024
10 checks passed
@akx akx deleted the drop-packaging-dep branch January 3, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants