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

Backend should not say that setuptools is needed to build #1594

Merged
merged 2 commits into from Dec 3, 2018

Conversation

Projects
None yet
5 participants
@pfmoore
Copy link
Member

pfmoore commented Nov 14, 2018

No description provided.

@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Nov 14, 2018

There are some tests that need to be updated.

Should we be updating this to setuptools>=39 or whenever this functionality was introduced, instead?

@pfmoore

This comment has been minimized.

Copy link
Member

pfmoore commented Nov 14, 2018

There are some tests that need to be updated.

Sorry, didn't spot them. Fixed now.

Should we be updating this to setuptools>=39 or whenever this functionality was introduced, instead?

Well, if a frontend is calling the setuptools backend, it's by definition got setuptools installed, and a version that has the backend, so it makes no sense to ask the frontend to install setuptools...

It's up to the project being built to specify setuptools in pyproject.toml when it requests the setuptools backend.

@pfmoore

This comment has been minimized.

Copy link
Member

pfmoore commented Nov 28, 2018

Is there anything I can do to move this forward?

@pganssle pganssle requested a review from gaborbernat Nov 29, 2018

@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Nov 29, 2018

This seems fine to me and the logic seems fine, but:

  1. I don't understand well enough the possible consequences of this to personally say, "Yes, this is the right thing to do"
  2. It don't actually understand if this is solving a real problem or if we're just trying to be logically correct about the requirements.

It seems to me that you are saying that setuptools is an implicit dependency here because it is automatically met by virtue of the fact that this is a function in setuptools. If that's the case, then from what I can tell, it doesn't matter one way or the other if setuptools includes itself in the dependency list.

On the other hand, are we entirely sure that no one would want to do a two-stage build, where the requirements are gathered in one environment and then used to build a second environment that may not have setuptools installed? If that's the case, then it makes sense to keep the dependency on setuptools explicit.

So I guess my question is, what is the downside of keeping setuptools in this list?

@pganssle pganssle requested a review from benoit-pierre Nov 29, 2018

@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Nov 29, 2018

Reading pypa/pip#5743 (comment) (and I did not have time to read the entire thread), it seems to me that this is solving a problem in the implementation of PEP 517 where the get_requires_for_build_wheel may cause conflicts where a required specific version of setuptools may be overridden by this requirement.

I am not sure if that was resolved in another way as well, but I would think that you would still have the same problem for the wheel dependency, so another solution to the problem is required anyway, preferably one that takes into account both the requirements from get_requires_for_build_wheel and from pyproject.toml.

I'll also note that the example in PEP 517 specifically includes setuptools on the list of requirements. I agree that it should at least be pinned to the minimum version that supports PEP 517, though.

@pfmoore

This comment has been minimized.

Copy link
Member

pfmoore commented Nov 29, 2018

it seems to me that this is solving a problem in the implementation of PEP 517

Well, it was specifically in our implementation of build isolation, but yes, how we implemented build isolation did mean that we were subject to this issue. We (specifically @benoit-pierre) modified the isolation implementation to work around the problem.

Fundamentally, pip can't incrementally install a set of requirements into an arbitrary directory. It can install one set of requirements (that's the --target option) but it can't then add extra requirements safely (in particular if the new requirements conflict with the original set). That's a pip limitation, but it's somewhat rooted in the fact that "site packages" directories are special, and it's not an easy limitation to remove.

On the other hand, though, the section on build environments in PEP 517 states that hooks are always called in an environment that contains the requirements from pyproject.toml, so yes, we are sure that your two-stage build example isn't possible. We'll never be in a position where the setuptools build_wheel hook can be called without setuptools being present, regardless of what the get_requires_for_build_wheel hook returns.

I would think that you would still have the same problem for the wheel dependency

True, but (a) it's probably less likely to happen, and (b) there's not much we can do about it, as it certainly is correct for the setuptools get_requires_for_build_wheel to say that wheel is needed (as setuptools can't build wheels on its own).

I'll also note that the example in PEP 517 specifically includes setuptools on the list of requirements

I think that example is wrong, to be honest.

I'm not going to insist that this goes in. We've worked around it in pip, and I'm happy to concede that it's a matter of how you interpret PEP 517, so there's no obviously right answer. I do think that omitting "setuptools" will make it easier for other clients to set up a build environment without getting bogged down in complex corner cases, but that's mostly theoretical at the moment (maybe if someone were motivated to improve the environment isolation code in https://github.com/pypa/pep517 they'd have a view, but that's probably not going to be me ;-))

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Nov 29, 2018

I think @pfmoore changes here are logical choices (tox uses its own pep-517 implementation, but all this holds there too).

@pfmoore

This comment has been minimized.

Copy link
Member

pfmoore commented Nov 29, 2018

Thanks all for the reviews.

@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Dec 3, 2018

OK, I defer to the majority here, merging.

@pganssle pganssle merged commit 83ab987 into pypa:master Dec 3, 2018

5 checks passed

codecov/patch 100% of diff hit (target 82.17%)
Details
codecov/project 82.17% (+0%) compared to b62705a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

jaraco added a commit that referenced this pull request Dec 11, 2018

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Dec 11, 2018

Released as 40.6.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment