-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Drop setuptools-scm requirement in wheels #21820
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
Conversation
# Installing from a git checkout. | ||
["setuptools_scm>=4"] if Path(__file__).with_name(".git").exists() | ||
else [] | ||
# Installing from a git checkout that is not producing a wheel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why exactly is having this in setup_requires
insufficient? Or is that only going to work with pyproject.toml
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #18971 (comment) and #18971 (comment): the two options were either
(a) don't have a runtime dep on setuptools_scm, and accept that inplace installs get the wrong version if one does pip install -e .
and then update the git repo (the version comes from _version.py
and is only updated when setup.py is run, either directly (e.g. when recompiling) or via pip); or
(b) make setuptools_scm a dependency so that the version is always correct.
I was actually in favor of (a) (as described in that thread), but didn't feel that strongly about it. I guess that's another opportunity to go back to (a) :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accept that inplace installs get the wrong version if one does pip install -e .
Do you mean that the wrong version gets reported, or that the install doesn't work? I think reporting the current commit in the version is nice, but hardly crucial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wrong version gets reported if one does pip install -e
and then updates the git repo (and even then, there are probably workarounds possible e.g. using git hooks...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted in #21783, my solution has been to import and use setuptools_scm, then fallback to importlib_metadata
. Given the fallback, it seemed fine to treat setuptools_scm
as an optional dependency and only list it as a "build" dependency. It's not perfect, but I've gone to statically listing all package metadata in setup.cfg
.
I agree with @QuLogic that this is the minimal fix for the wheels and adopting something more like @dopplershift 's metpy example for 3.6 |
Here is a test build for wheels with this patch: |
I also checked by hand locally with the right env set and did not have the dependency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine enough for a bugfix release.
…820-on-v3.5.x Backport PR #21820 on branch v3.5.x (Drop setuptools-scm requirement in wheels)
PR Summary
Fixes #21783.
PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).