Skip to content

Disable import of setuptools for sdist builds #4285

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

Closed
wants to merge 2 commits into from

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented Feb 14, 2017

Fixes #4264 and pypa/setuptools#951.


This change is Reviewable

@dstufft
Copy link
Member

dstufft commented Feb 21, 2017

@jaraco Where's this PR at? It looks like there are conflicts now and something happened so the tests never actually ran.

@jaraco
Copy link
Member Author

jaraco commented Feb 21, 2017

Well, it got one downvote without any comment. Not sure what that means.

I did commit the change against 9.0.1 because I wasn't sure where you would want this to land in the tree. Perhaps you would want to release it as a hotfix and not with other changes pending in the tree.

The change is fairly straightforward. I'm looking for a review, endorsement, and guidance on where you'd like it to land. I will merge it with master in order to bypass the merge conflicts.

@dstufft
Copy link
Member

dstufft commented Feb 21, 2017

Dunno what that downvote means, I'm not worried about it.

Making the change to master is fine and just put it under 9.1.0. If we do a 9.0.2 I'll backport the fix myself.

The change itself looks fine, my only real comment there is it'd be nice if there was still a test of this behavior, but that might be too hard to do.

@jaraco
Copy link
Member Author

jaraco commented Feb 21, 2017

it'd be nice if there was still a test for this behavior.

I had the same feeling.

It depends too on which behavior it is you want to test. If you want to test what is the behavior when setuptools isn't present but the setup script imports it, yes, that's hard to test, in part because pip itself is invoked in a subprocess, which then invokes setup.py in a subprocess, so the behavior is two subprocesses away.

If the behavior you want to test is that setuptools is not in sys.modules or that distutils remains unpatched after invoking a pip install operation on an sdist, that too might be tricky to capture, as again, pip is invoked in a subprocess, so there's little ability to hook into the behavior and make assertions.

We could even go so far as to bring back the original behavior and test for the existence of setuptools in a subprocess. That alternate approach would allow retaining the existing test, but would avoid the monkeypatching by virtue of not importing setuptools in the main process.

@dstufft
Copy link
Member

dstufft commented Feb 21, 2017

What about just testing to make sure that if setuptools ins't installed it gives the error message? Each test can be run in an isolated virtual environment so you can just uninstall setuptools then try to install something with --no-binary=:all:.

@jaraco
Copy link
Member Author

jaraco commented Feb 21, 2017

There's already a test for that, 'test_without_setuptools', which does assert the error message. What it no longer tests and which is no longer the case is that a nice message is printed when setuptools fails to import. So if it's installed but broken, it's not clear what the output will be.

@dstufft
Copy link
Member

dstufft commented Feb 21, 2017

Ah is there? I wouldn't worry too much about it then. If someone has a broken setuptools they can handle that themselves I guess.

@jaraco
Copy link
Member Author

jaraco commented Feb 21, 2017

I've got another branch which is less invasive.

@jaraco jaraco closed this Feb 21, 2017
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

move_wheel_files fails on setuptools dependencies
2 participants