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

Stop using distutils #6113

Merged
merged 6 commits into from
Dec 13, 2021
Merged

Stop using distutils #6113

merged 6 commits into from
Dec 13, 2021

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Oct 25, 2021

Distutils was officially deprecated in Python 3.10 and will be removed from the standard library completely in 3.12. This PR thus eliminates almost all of the uses of distutils from the code.

Note that I had to leave in some uses of LooseVersion because I wasn't sure whether the stricter packaging.version.Version would accomodate all of its uses. If we're only using LooseVersion for versions of the form N(.N)*, then Version will work out fine, but anything more exotic is not guaranteed to work. If we need support for more exotic versions, we may have to roll our own version class.

@jwodder jwodder added the semver-internal Changes only affect the internal API label Oct 25, 2021
yarikoptic added a commit to yarikoptic/datalad-crawler that referenced this pull request Oct 25, 2021
used only in the crawler, and relies on distutils LooseVersion
and distutils are getting deprecated.  Relevant PR in datalad for
distutils removal: datalad/datalad#6113
@yarikoptic
Copy link
Member

FWIW -- LooseVersion was used in not used in core support/versions.py which I am moving to datalad-crawler.
#6115 datalad/datalad-crawler#111 . So the main "use" of LooseVersion would be datalad/support/external_versions.py for versions comparison etc for external libraries/utilities.

LooseVersion was convenient due to its forgiveness and us rarely need "exact" comparison, and largely to capture and then print those versions (e.g. in datalad wtf). packaging.version.Version I guess would strictly follow Python's versioning scheme, and not even semver and blow up on any odd looking version string. For external python modules we can assume that they follow Python versioning and there this Version should be good. But for external tools, we might want either annotate explicitly either it is following Python versioning or semver or ... and then have adapter classes for those types so we could still store them to be displayed and compare whenever needed?

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

This looks all sane to me. Thanks! I left a few comment which are all relatively minor.

The tests seem to suffer from a str vs bytes problem, seems solvable.

_datalad_build_support/setup.py Outdated Show resolved Hide resolved
datalad/cmdline/tests/test_main.py Outdated Show resolved Hide resolved
@mih
Copy link
Member

mih commented Nov 19, 2021

Test failure seems legitimately caused by these changes.

@jwodder
Copy link
Member Author

jwodder commented Nov 19, 2021

@mih I've pushed a fix.

@codecov
Copy link

codecov bot commented Nov 19, 2021

Codecov Report

Merging #6113 (6234d5b) into maint (ece1cbc) will decrease coverage by 38.88%.
The diff coverage is 37.33%.

❗ Current head 6234d5b differs from pull request most recent head b27bd95. Consider uploading reports for the commit b27bd95 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##            maint    #6113       +/-   ##
===========================================
- Coverage   90.23%   51.34%   -38.89%     
===========================================
  Files         312      312               
  Lines       42219    42242       +23     
===========================================
- Hits        38095    21691    -16404     
- Misses       4124    20551    +16427     
Impacted Files Coverage Δ
datalad/cmdline/tests/test_main.py 0.00% <0.00%> (-84.84%) ⬇️
datalad/core/distributed/tests/test_clone.py 0.00% <0.00%> (-97.44%) ⬇️
datalad/core/local/create.py 87.50% <ø> (-10.30%) ⬇️
datalad/core/local/tests/test_create.py 0.00% <ø> (-100.00%) ⬇️
datalad/customremotes/tests/test_archives.py 0.00% <0.00%> (-89.41%) ⬇️
datalad/customremotes/tests/test_base.py 0.00% <0.00%> (-97.90%) ⬇️
datalad/customremotes/tests/test_datalad.py 0.00% <0.00%> (-97.88%) ⬇️
datalad/distributed/ora_remote.py 21.23% <0.00%> (-10.05%) ⬇️
datalad/distributed/tests/test_ora_http.py 27.53% <0.00%> (-72.47%) ⬇️
datalad/distribution/create_sibling.py 15.31% <0.00%> (-50.56%) ⬇️
... and 282 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ece1cbc...b27bd95. Read the comment docs.

@mih
Copy link
Member

mih commented Dec 13, 2021

OK, this seems ready. I have kicked off the errored test again, but I expect no failure. The only thing holding me back from merging is the draft state of this PR. @jwodder is this intentionally not meant to be merged?

@jwodder
Copy link
Member Author

jwodder commented Dec 13, 2021

@mih It should be merged at some point. The only thing this PR is currently lacking is an alternative to the use of distutils.version.LooseVersion, though it could be merged as-is right now and a separate issue created for that.

@jwodder jwodder marked this pull request as ready for review December 13, 2021 18:43
@mih
Copy link
Member

mih commented Dec 13, 2021

@jwodder I would prefer if we get this done here, and leave the LooseVersion issue for later. Right now it seems there is no strict requirement to keep it in -core, but maybe I am missing some case.

@jwodder
Copy link
Member Author

jwodder commented Dec 13, 2021

@mih

I would prefer if we get this done here, and leave the LooseVersion issue for later.

Then feel free to merge.

Right now it seems there is no strict requirement to keep it in -core, but maybe I am missing some case.

I don't know what you're referring to here.

@mih
Copy link
Member

mih commented Dec 13, 2021

Thanks!

I was referring to keeping LooseVersion or a replacement in this package (vs an extension, for example).

@mih mih merged commit 6a28583 into datalad:maint Dec 13, 2021
@jwodder jwodder deleted the no-distutils branch May 12, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-internal Changes only affect the internal API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants