scikit-build / cmake-python-distributions

This project provides the infrastructure to build CMake Python wheels.
https://cmake-python-distributions.readthedocs.io
Apache License 2.0
114 stars 34 forks source link

fix: Ensure importlib_metadata runtime dependency installed with Python < 3.10 #474

Closed jcfr closed 8 months ago

jcfr commented 8 months ago

Fixes a regression introduced in 5f068d5 ("chore(build): move to scikitbuild-core (#455)", 2024-03-01)

jcfr commented 8 months ago

cc: @mayeut @henryiii

jcfr commented 8 months ago

Thanks @agriyakhetarpal, @braindevices, @mloubout and @roastduck for the issue reports and pull requests :pray:

jcfr commented 8 months ago

Once the fix has been reviewed, approved and integrated, suggested path forward:

mudit2812 commented 8 months ago

importlib_metadata is only needed for python 3.7. Versions 3.8 or greater can use importlib.metadata, which ships with python afaik

mloubout commented 8 months ago

Still needs to fix from #473 for correct version checking

agriyakhetarpal commented 8 months ago

@mudit2812 and @mloubout, Python 3.7 support was recently dropped with 3.29.0. Do we even need the conditional import for importlib_metadata at all, then? We can just use importlib.metadata and that should work? i.e., I don't think adding importlib_metadata as a required dependency is required at all, now – it would be better to drop the conditional import from src/cmake/__init__.py.

mloubout commented 8 months ago

Didn't see Python 3.8 was the minimum version now, should be able to completely drop importlib_metadata then as you pointed out.

agriyakhetarpal commented 8 months ago

docs/conf.py seems to have been using the correct Python version check, as reported by @mloubout:

https://github.com/scikit-build/cmake-python-distributions/blob/85a457bf3e0de6efa65cc0f47f017810936772f0/docs/conf.py#L20-L25

henryiii commented 8 months ago

Python 3.7 is the minimum version, not 3.8. We just dropped 2.7 and 3.6.

agriyakhetarpal commented 8 months ago

Oops, thanks! Updated my review to mark that change

henryiii commented 8 months ago

You are correct about this working in 3.8+ though, I just checked. There are bugs in importlib.metadata before 3.10.2, but this shouldn't hit those.

henryiii commented 8 months ago

@jcfr I don't have permission to yank cmake 3.29.0, let's go ahead and do that, since it's breaking people and people probably aren't requiring it yet after a few hours, and if they did, it would still work (due to yanks resolving if they can't resolve to a non-yanked version). CI will take ~20-30 mins here, then the release will take ~3 hours.

jcfr commented 8 months ago

re: yanked

image

jcfr commented 8 months ago

Thanks @henryiii for further tweaking the patch :pray: and @mloubout for testing.

:rocket: :white_check_mark: