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

Add setuptools and wheel dependencies to the setup.cfg #889

Merged
merged 4 commits into from Jun 5, 2021

Conversation

jayvdb
Copy link
Member

@jayvdb jayvdb commented Sep 10, 2019

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
  • Requested a review from another contributor.
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@jayvdb jayvdb force-pushed the missing-deps branch 3 times, most recently from a17b845 to 07977cd Compare September 11, 2019 22:15
setup.py Outdated Show resolved Hide resolved
atugushev
atugushev previously approved these changes Sep 17, 2019
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

LGTM.

@atugushev
Copy link
Member

@jayvdb i've also updated "Changelog-friendly one-liner" in the issue to add more details to the changes, feel free to redact it.

@atugushev atugushev closed this Sep 17, 2019
@atugushev atugushev reopened this Sep 17, 2019
@atugushev
Copy link
Member

Reopened to trigger codecov bot.

@jayvdb
Copy link
Member Author

jayvdb commented Sep 18, 2019

All green @atugushev (your changes were good of course)

@atugushev atugushev added this to the 4.2.0 milestone Sep 18, 2019
@atugushev atugushev added the enhancement Improvements to functionality label Sep 26, 2019
@atugushev atugushev changed the title setup.py: Add missing dependencies Add missing dependencies to the setup.py Sep 26, 2019
@vphilippon
Copy link
Member

While it's technically true that pip-tools requires pip and setuptools, declaring them in install_requires can cause some headache for Windows user.

If you were to execute pip install --upgrade pip-tools in a Windows environment with an older version of pip, you'll get some weird error because Windows won't allow pip to overwrite its currently executed pip.exe file. Same goes for setuptools IIRC.

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 --upgrade-strategy changed since, but its a risky situation for Windows users nonetheless.

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 pip 9.0.1. Maybe things changed.)

@atugushev
Copy link
Member

atugushev commented Sep 26, 2019

@vphilippon

If you were to execute pip install --upgrade pip-tools in a Windows environment with an older version of pip, you'll get some weird error because Windows won't allow pip to overwrite its currently executed pip.exe file. Same goes for setuptools IIRC.

FYI, there was an issue pypa/pip#6841 with pip install --upgrade {package_wich_depends_on_pip}, which I fixed in pypa/pip#6864 (will be released in 19.3).

@atugushev atugushev added the needs discussion Need some more discussion label Sep 26, 2019
@atugushev
Copy link
Member

@vphilippon

What do you think about tests_require=["mock", "pytest!=5.1.2"],? Honestly, i'd prefer extras_require={"tests": ["mock", "pytest!=5.1.2"]} instead. Even offical tutorial recommends that way. However, why can't we have both options at the same time?

@jayvdb
Copy link
Member Author

jayvdb commented Sep 27, 2019

If you were to execute pip install --upgrade pip-tools in a Windows environment with an older version of pip, you'll get some weird error because Windows won't allow pip to overwrite its currently executed pip.exe file.

The workaround is those Windows users to use python -m pip upgrade ...

Avoiding declaring dependencies because of bugs elsewhere isnt a good way forward.

But if there is extreme problems, we could not add pip to the dependencies on Windows. It is mostly Unix systems which need proper declaration of dependencies.

Same goes for setuptools IIRC.

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..?

What do you think about tests_require=["mock", "pytest!=5.1.2"],? Honestly, i'd prefer extras_require={"tests": ["mock", "pytest!=5.1.2"]} instead. Even offical tutorial recommends that way. However, why can't we have both options at the same time?

Both is good. The docs don't include tests_require because there is a rather large and long-running debate about whether to deprecate tests_require and a bunch of other stuff which has been in setuptools since very early in its history.

@atugushev atugushev removed this from the 4.2.0 milestone Oct 11, 2019
@atugushev
Copy link
Member

@jayvdb
Copy link
Member Author

jayvdb commented Oct 16, 2019

Commented on both. Thanks for the pointers.

@atugushev
Copy link
Member

atugushev commented Oct 16, 2019

Also, I've tried to unify test requirements (#923), so it also might be related.

Copy link
Member

@codingjoe codingjoe left a 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.

setup.py Outdated Show resolved Hide resolved
click >= 7
six
pip >= 20.1

# indirect dependencies
setuptools # typically needed when pip-tools invokes setup.py
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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"

Copy link
Member

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.

Copy link
Member

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

@jdufresne
Copy link
Member

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.

@jayvdb
Copy link
Member Author

jayvdb commented Nov 15, 2020

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.

@lsaavedr
Copy link

please add toml:

Traceback (most recent call last):
  File "/home/rpalma/envSmi/bin/pip-compile", line 5, in <module>
    from piptools.scripts.compile import cli
  File "/home/rpalma/envSmi/lib/python3.9/site-packages/piptools/scripts/compile.py", line 9, in <module>
    from pep517 import meta
  File "/home/rpalma/envSmi/lib/python3.9/site-packages/pep517/meta.py", line 19, in <module>
    from .envbuild import BuildEnvironment
  File "/home/rpalma/envSmi/lib/python3.9/site-packages/pep517/envbuild.py", line 6, in <module>
    import toml
ModuleNotFoundError: No module named 'toml'

@ssbarnea
Copy link
Member

ssbarnea commented Jun 5, 2021

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 ssbarnea closed this Jun 5, 2021
@jayvdb
Copy link
Member Author

jayvdb commented Jun 5, 2021

@ssbarnea , which direct dependency has setuptools and wheel as direct non-optional dependencies?
Currently the direct dependencies are click >= 7, pep517, pip >= 20.3 .

https://www.wheelodex.org/projects/pep517/ lists toml. ping @lsaavedr , I guess that was fixed in pep517.

https://www.wheelodex.org/projects/pip/ and https://www.wheelodex.org/projects/click/ do not list them.

@jayvdb jayvdb reopened this Jun 5, 2021
@jazzband jazzband deleted a comment from codecov bot Jun 5, 2021
@codecov

This comment has been minimized.

@ssbarnea
Copy link
Member

ssbarnea commented Jun 5, 2021

@jayvdb I had to fix conflicts, please confirm that it looks ok now.

@jayvdb
Copy link
Member Author

jayvdb commented Jun 5, 2021

lgtm. thx

@ssbarnea ssbarnea added this to the 6.1.1 milestone Jun 5, 2021
@ssbarnea ssbarnea merged commit 511a7a1 into jazzband:master Jun 5, 2021
@webknjaz
Copy link
Member

webknjaz commented Jun 5, 2021

I don't like that the deps are marked as indirect because if they are used in our runtime, they are definitely direct.

@jayvdb
Copy link
Member Author

jayvdb commented Jun 5, 2021

The "indirect" was requested at #889 (review) . I also dont like that term, but I'm just happy we've finally listed the dependencies.

@atugushev atugushev added dependency Related to a dependency and removed enhancement Improvements to functionality needs discussion Need some more discussion labels Jun 21, 2021
@atugushev atugushev changed the title Add missing dependencies to the setup.cfg Add setuptools and wheel dependencies to the setup.cfg Jun 21, 2021
@atugushev atugushev modified the milestones: 6.1.1, 6.2.0 Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency Related to a dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants