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

Don't install setup_requires when run as a PEP-517 backend. #2306

Merged
merged 5 commits into from Aug 13, 2020

Conversation

takluyver
Copy link
Member

@takluyver takluyver commented Aug 6, 2020

Summary of changes

Under PEP 517, installing build dependencies is up to the frontend. setup_requires are already reported as build dependencies, but setuptools should not attempt to install them itself when used as a PEP 517 backend.

Closes #2303

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details

@@ -380,7 +380,7 @@ def test_setup_requires(self, setup_literal, requirements, use_wheel,
setup(
name="qux",
version="0.0.0",
py_modules=["hello.py"],
py_modules=["hello"],
Copy link
Member

@pganssle pganssle Aug 10, 2020

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

@takluyver takluyver Aug 10, 2020

Choose a reason for hiding this comment

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

The test was failing locally without it. All the other samples in this file with py_modules don't have the .py extension, and the packaging guide says it doesn't belong there.

Copy link
Member

@pganssle pganssle Aug 10, 2020

Choose a reason for hiding this comment

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

Hm.. It's disturbing that the tests are currently passing if they fail for you locally without that change, no?

Copy link
Member Author

@takluyver takluyver Aug 10, 2020

Choose a reason for hiding this comment

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

I think they may have been passing on master and only failed when I made the change I was interested in. As this seemed like a simple mistake and nothing else was failing, I confess I didn't try to understand what was happening.

@jaraco
Copy link
Member

jaraco commented Aug 12, 2020

I'm inclined to accept this change, especially since it brings the codebase one step closer to fully deprecating setup_requires.

You've flagged the change not as breaking. Are we confident there aren't any use cases that will break with this change? In other words, are there users that rely on setup_requires in a PEP 517 environment?

@takluyver
Copy link
Member Author

takluyver commented Aug 13, 2020

The build dependencies are reported to the frontend which is responsible for installing them in a build environment. You can disable that, e.g. with the --no-build-isolation flag for pip, in which case the caller is responsible for ensuring the build dependencies are installed in the environment where pip is running.

So I don't believe that this breaks any scenario which is meant to work. But as with any change, someone could have relied on something that wasn't meant to work. For instance:

  • If someone uses pip install --no-build-isolation without all build dependencies installed, and is used to setuptools attempting to install build dependencies anyway.
  • If there are packages which can be installed by easy_install but not by pip, they may not work with PEP 517.
  • If setup scripts sniff what command is being run and specify setup_requires for some commands but not others, the dependencies may not be reported to the frontend.

Would you prefer that I called that breaking?

@jaraco
Copy link
Member

jaraco commented Aug 13, 2020

Based on that description, I'm inclined to say it's not a breaking change. Thanks for adding that detail so those that are affected can understand better the context.

@jaraco jaraco merged commit 6f7c262 into pypa:master Aug 13, 2020
5 checks passed
@jaraco
Copy link
Member

jaraco commented Aug 13, 2020

Released as 49.5.

@takluyver
Copy link
Member Author

takluyver commented Aug 14, 2020

Thanks Jason!

@takluyver takluyver deleted the pep517-setup-requires branch Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't install setup_requires when run as a PEP 517 backend
3 participants