-
-
Notifications
You must be signed in to change notification settings - Fork 615
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
Add setuptools
and wheel
dependencies to the setup.cfg
#889
Conversation
a17b845
to
07977cd
Compare
07977cd
to
86cf59a
Compare
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.
LGTM.
@jayvdb i've also updated "Changelog-friendly one-liner" in the issue to add more details to the changes, feel free to redact it. |
Reopened to trigger codecov bot. |
All green @atugushev (your changes were good of course) |
While it's technically true that If you were to execute That's why I removed those as soon as it became clean-ish to do, as I had to deal with reports of the issue mentioned above a bit toot often. I don't remember if the default I get the intent of properly declaring all the dependencies, but I think this is a corner case where we should make an exception. (Bear in mind this is taken from my memory under |
FYI, there was an issue pypa/pip#6841 with |
What do you think about |
The workaround is those Windows users to use Avoiding declaring dependencies because of bugs elsewhere isnt a good way forward. But if there is extreme problems, we could not add
I don't recall the problem existing in setuptools. The problem is specific to setuptools generated entry-point scripts. The problem probably exists when trying to use easy_install.exe to upgrade easy_install.exe , but ... who does that..?
Both is good. The docs don't include |
There are also relevant and interesting discussions: |
Commented on both. Thanks for the pointers. |
Also, I've tried to unify test requirements (#923), so it also might be related. |
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.
LGTM, my only concern is regarding setuptools
, please see my comment.
I am in favor of using tests_require
, I would also add the pytest-runner
and change the test command. This way you can execute the test alias via setuptools like so: python setup.py test
.
click >= 7 | ||
six | ||
pip >= 20.1 | ||
|
||
# indirect dependencies | ||
setuptools # typically needed when pip-tools invokes setup.py |
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.
I'm not sure if it's allowed to put inline comments here because stdlib configparser treats them as part of the values... Even if some setuptools versions have workarounds for this, there's no guarantee that the end-user has them pre-installed.
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.
That is correct. These comments will become part of the Requirement
, and any tool which reads these must handle comments. Just like requirements.txt lines can contain comments.
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.
If we want to be compliant with some historical PEP, or minimum setuptools version, I'd appreciate knowing what to target. I know that setuptools has supported this for a long time, but I don't recall how far back it goes. Likewise packaging
and more modern requirements toolkits.
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.
FWIW I did some checks on Ubuntu and here's setuptools shipped via apt (pulled in as a dependency of python3-pip
):
- focal -> 45.2.0
- bionic -> 39.0.1
- xenial -> 20.7.0
- trusty -> 3.3
I don't know how old setuptools version should be supported but FYI Travis still allows running all Ubuntu versions all the way back to Precise and defaults to Xenial: https://docs.travis-ci.com/user/reference/overview/#which-one-do-i-use.
Debian:
- buster -> 40.8.0
- stretch -> 33.1.1
- jessie -> 5.5.1
Now, what about something RPM-based? Let's look at the CentOS the same way:
- 8 -> 39.2.0
- 7 -> 39.2.0 # manylinux2014 wheels are to be built here
- 6 -> 0.6.10-4.el6_9 (Python 2) # this version is used for manylinux2010 wheels
- 5 -> none (it ships with Python 2.4.3) # interestingly, this is the version one is supposed to use when building manylinux1 wheels
RHEL:
- 8 -> 39.2.0-5.el8
- 7 -> 0.9.8-7.el7 (Python 2)
- 6 (setuptools unavailable w/o a subscription but should be about the same as CentOS 6) — it's still around in some places, going EOL in the end of November
The above distros are popular in various server/CI envs (and are all available via docker registries). Though I'm not sure which versions are actually widely used nowadays.
If we look at Fedora, they usually ship modern developer-oriented ecosystem but it's mostly used on desktops (AFAICS):
- 32 -> 41.6.0-2.fc32
- 31 -> 50.1.0-1.fc34 (now sure why it pulled repos from rawhide distros)
- 30 -> 40.8.0
- 29 -> 40.4.3
- 28 -> 39.2.0
- 27 -> 37.0.0
- 26 -> 37.0.0
- 25 -> 25.1.1
- 24 -> 20.1.1
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.
Wow. Very helpful.
I am pretty sure that using setup.cfg
already breaks most of those versions of older setuptools.
No doubt you know setuptools adds features bit by bit, rarely (historically at least; im a bit out of the loop) fully implement a standard with adequate tests to have anything resembling compliance. And complimenting that, the setup.cfg syntax niceties has evolved over time. So medium age setuptools might work, but may degrade a bit unless we stick with very basic setup.cfg syntax.
WRT RHEL, anyone seriously using it will be using rpms, built from sdists and .spec and .patch files as needed, not needing pip/pip-tools or setuptools. And devs will be using latest RHEL with Rawhide or other additional channels.
So ... "bionic -> 39.0.1" feels like a good target to start with, as that will include manylinux2014. The reason I would want manylinux2014 support is because pip-compile-multi
depends on pip-tools, and iirc there was another similar tool.
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.
can pip-tools force isolated-builds instead?
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.
A quick repo search didn't reveal any proposals to add pyproject.toml
to this repo but I think its support in modern pip is stable enough nowadays that it's reasonable to add it. OTOH this comment chain is about the legacy systems that wouldn't know anything about PEP518/PEP517.
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.
You can still build a legacy setup.py project in isolated-build "mode"
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.
Of course, you can. As long as you actually have a modern PEP517 build front-end (like pip 10+). But I've listed a lot of platforms above for which it is not true.
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.
pip-tools already depends on pip >= 20.1 so a pep517 frontend is always available
Would it be possible to include tests that demonstrate why these are hard dependencies? It may not be easy, so just asking if it is even possible at this point. Beyond the general usefulness of testing, it would allow dropping the comments. |
No, it is very difficult to do that, because these dependencies are always present in venvs by default, so building tests to prove they are needed requires setting up special venvs without them, and even then they can only be xfail tests, which often cant prove the underlying problem because there are too many other mocks needed to get to the code in question. We are now over a year delaying a PR which adds necessary packaging metadata. This is very odd for a project which is about a packaging tool. |
please add
|
I think this PR is no longer needed because setuptools is an indirect dependency. Probably it was required in 2019 but not in 2021. Let me know if I am wrong, happy to reopen. |
@ssbarnea , which direct dependency has https://www.wheelodex.org/projects/pep517/ lists https://www.wheelodex.org/projects/pip/ and https://www.wheelodex.org/projects/click/ do not list them. |
This comment has been minimized.
This comment has been minimized.
@jayvdb I had to fix conflicts, please confirm that it looks ok now. |
lgtm. thx |
I don't like that the deps are marked as indirect because if they are used in our runtime, they are definitely direct. |
The "indirect" was requested at #889 (review) . I also dont like that term, but I'm just happy we've finally listed the dependencies. |
setuptools
and wheel
dependencies to the setup.cfg
pip, setuptools and wheel are runtime dependencies that should
be explicitly included. If not, they may not be provided on
systems which install packages pre-built, thus these packages
should not be assumed to be installed.
Also pytest and mock are test dependencies.
Changelog-friendly one-liner: Add missing install and test dependencies to
setup.py
.Contributor checklist
Provided the tests for the changes