Skip to content

Drop packaging dependency#3205

Merged
milas merged 1 commit intodocker:mainfrom
valohai:drop-packaging-dep
Jan 3, 2024
Merged

Drop packaging dependency#3205
milas merged 1 commit intodocker:mainfrom
valohai:drop-packaging-dep

Conversation

@akx
Copy link
Copy Markdown
Contributor

@akx akx commented Dec 21, 2023 •
edited
Loading

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
@akx akx mentioned this pull request Dec 21, 2023
Merged
milas
milas suggested changes Jan 3, 2024
View reviewed changes
Copy link
Copy Markdown
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:

docker-py/docker/utils/utils.py

Lines 58 to 59 in f467fd9

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
Copy Markdown
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
Copy Markdown
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
@akx
Drop packaging dependency
249654d 
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
Copy Markdown
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
milas
milas approved these changes Jan 3, 2024
View reviewed changes
Copy link
Copy Markdown
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
@akx akx deleted the drop-packaging-dep branch January 3, 2024 19:19
@akx akx mentioned this pull request Jan 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@milas milas milas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

dependencies Pull requests that update a dependency file

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@akx @milas