Skip to content

Maintenance (PEP 632): Remove dependency on distutils #513

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 16, 2023
Merged

Maintenance (PEP 632): Remove dependency on distutils #513

merged 2 commits into from
Feb 16, 2023
+512 −10

Conversation

amotl
Copy link
Member

@amotl amotl commented Feb 9, 2023 •
edited
Loading

About

The distutils package will become deprecated, see PEP 632 – Deprecate distutils module.

DeprecationWarning: The distutils package is deprecated and slated for removal in Python 3.12. Use setuptools or check PEP 632 for potential alternatives

Solution

The accepted solution is to vendor packaging.version, like how NumPy, pandas and SciPy are doing it equally.

References

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 9, 2023
View reviewed changes
@amotl amotl force-pushed the amo/remove-distutils branch from 0ba89a7 to 58378fc Compare February 9, 2023 16:53
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 9, 2023
View reviewed changes
@amotl amotl force-pushed the amo/remove-distutils branch from 58378fc to 82138a0 Compare February 9, 2023 17:13
@amotl amotl requested review from mfussenegger, matriv and seut February 9, 2023 19:26
@amotl amotl marked this pull request as ready for review February 9, 2023 19:26
@amotl amotl force-pushed the amo/remove-distutils branch from 6e06373 to 2c1d460 Compare February 9, 2023 20:12
@amotl amotl changed the title Maintenance: Remove dependency on distutils Maintenance (PEP 632): Remove dependency on distutils Feb 10, 2023
seut
seut requested changes Feb 10, 2023
View reviewed changes
src/crate/client/pep440.py Outdated
Comment on lines 1 to 8
# Vendored from `packaging` on 2021-30-04, via pandas.
# - https://github.com/pypa/packaging/blob/ae891fd74/packaging/_structures.py
# - https://github.com/pypa/packaging/blob/ae891fd74/packaging/version.py
# - https://github.com/pandas-dev/pandas/blob/cf2b88b5ea/pandas/util/version/__init__.py

# This file is dual licensed under the terms of the Apache License, Version
# 2.0, and the BSD License. See the LICENSE file in the root of this repository
# for complete details.
Copy link
Member

@seut seut Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we use this license related header, we'd have to include the original license note to our LICENSE file afaik similar how pandas did it pandas-dev/pandas@f40e58c#diff-17d4b6290d4495e094bd6dc27902057dcb25ca7f73d595162899c22d1b95f43eR180.

Or we keep the license header of donald like numpy did https://github.com/numpy/numpy/pull/21000/files#diff-41a2b10dd4230acb81d1f9f7dedd8e8e6f6ef4f44d15f20105e9e82cf5a489ebR7.

Copy link
Member Author

@amotl amotl Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Response

Thank you for your attention.

Thoughts

I think it would be even better if someone would release and maintain a dedicated versions, or pep440 package, that this code would not have to be vendored over and over again.

Discovery

Better late than never: It turns out, that happened already!

Thoughts

Maybe better spend another iteration to re-use one of those packages instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe better spend another iteration to re-use one of those packages instead?

Hm. While versions feels a bit too heavy, I think pep440 is a bit too thin.

Copy link
Member

Choose a reason for hiding this comment

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

Yes sounds good, lets use one of them.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe better spend another iteration to re-use one of those packages instead?

Hm. While versions feels a bit too heavy, I think pep440 is a bit too thin.

Another one: https://pypi.org/project/pep440-version-utils/
But also not sure if this is really worth of a dependency. Maybe we should just fix the license header instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Another one: https://pypi.org/project/pep440-version-utils/.

Thanks, but this one actually pulls in packaging, see pyproject.toml#L16.

But also not sure if this is really worth of a dependency.

Yeah, I think that was the same reasoning of the other maintainers.

Maybe we should just fix the license header instead?

Sure, let's do it like that for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've reworked the patch to use the _pep440.py file from current NumPy 1, as you suggested.

Footnotes

  1. https://github.com/numpy/numpy/blob/v1.25.0.dev0/numpy/compat/_pep440.py ↩

@amotl amotl force-pushed the amo/remove-distutils branch 2 times, most recently from 0df0951 to 82ce1e4 Compare February 10, 2023 11:52
@amotl
PEP 632 / PEP 440: Vendor packaging.version to compare version numbers
de4c4bf 
This effectively removes the direct dependency to `distutils`, which
will become deprecated with Python 3.12.

The patch uses the implementation from NumPy 1:1, originally authored by
Donald Stufft and individual contributors.
@amotl amotl force-pushed the amo/remove-distutils branch from 82ce1e4 to 454e85b Compare February 10, 2023 11:55
@amotl
PEP 632 / PEP 440: Use vendored _pep440.Version, with little patch …
744d4a5 
…to it

A little patch to re-add the `version()` method was needed for version
comparison backwards-compatibility with code using it.

Also, don't do code coverage reporting on the vendored code.
@amotl amotl force-pushed the amo/remove-distutils branch from 454e85b to 744d4a5 Compare February 10, 2023 11:57
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 10, 2023
View reviewed changes
src/crate/client/_pep440.py
return method(self._key, other._key)


class LegacyVersion(_BaseVersion):

Check warning

Code scanning / CodeQL

`__eq__` not overridden when adding attributes

The class 'LegacyVersion' does not override ['__eq__'](1), but adds the new attribute [_version](2). The class 'LegacyVersion' does not override ['__eq__'](1), but adds the new attribute [_key](3).
src/crate/client/_pep440.py
"""


class Version(_BaseVersion):

Check warning

Code scanning / CodeQL

`__eq__` not overridden when adding attributes

The class 'Version' does not override ['__eq__'](1), but adds the new attribute [_version](2). The class 'Version' does not override ['__eq__'](1), but adds the new attribute [_key](3).
@mfussenegger
Copy link
Member

Wouldn't a tuple(map(int, version.split('.'))) be good enough for our purposes? CrateDB versions follow x.y.z and don't have any other components. A tuple with 3 integers has the correct comparison semantics

@amotl
Copy link
Member Author

amotl commented Feb 10, 2023 •
edited
Loading

CrateDB versions follow x.y.z and don't have any other components.

I think the version comparison is also used for comparing versions of Python packages, specifically SQLAlchemy. I recently already struggled very hard, because the older distutils parser did not manage to do all the x.x.xa / x.x.xb / x.x.xrc pre-release variants correctly, while I needed it to work with corresponding pre-release packages of SQLAlchemy.

crate-python/src/crate/client/_pep440.py

Lines 1 to 4 in 744d4a5

"""Utility to compare pep440 compatible version strings.
The LooseVersion and StrictVersion classes that distutils provides don't
work; they don't recognize anything like alpha/beta/rc/dev versions.

@amotl amotl requested a review from seut February 10, 2023 14:28
@amotl amotl merged commit 69f8584 into master Feb 16, 2023
@amotl amotl deleted the amo/remove-distutils branch February 16, 2023 13:38
@amotl amotl mentioned this pull request Feb 16, 2023
Merged
@amotl amotl mentioned this pull request Oct 13, 2023
Closed
@amotl amotl mentioned this pull request Oct 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seut seut seut approved these changes

@mfussenegger mfussenegger Awaiting requested review from mfussenegger

@matriv matriv Awaiting requested review from matriv

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amotl @mfussenegger @seut