Skip to content

Only allow the finder to use wheels as build reqs #4987

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

Merged
merged 2 commits into from
Jan 26, 2018

Conversation

pradyunsg
Copy link
Member

Fixes #4983
See also #4802 (comment)

The message isn't ideal but I think that is for a separate PR.

screen shot 2018-01-23 at 1 37 24 am

@pradyunsg pradyunsg added type: enhancement Improvements to functionality skip news Does not need a NEWS file entry (eg: trivial changes) labels Jan 22, 2018
@benoit-pierre
Copy link
Member

Shouldn't the message read "This version of pip does not implement PEP 518", not 516?

@pradyunsg
Copy link
Member Author

The message isn't ideal but I think that is for a separate PR.

@benoit-pierre Yes.

@pfmoore
Copy link
Member

pfmoore commented Jan 22, 2018

Um, PEP 517. 516 is the rejected version of build system abstraction, 517 is the accepted build system abstraction, and 518 is pyproject.toml plus basic install requirements. On its own, PEP 518 still needs setuptools to build wheels.

@pfmoore
Copy link
Member

pfmoore commented Jan 22, 2018

Having said that, the message is not from this PR (which looks fine to me) but from somewhere else. As far as I can see, though, the message is confused - we're not trying to build a wheel without setuptools, we're simply trying to install a build-time dependency of pip-forkbomb-test. The problem is that we have a circular dependency, not that we don't have alternative build tool support.

Regardless, though, that's a cosmetic issue. I'm happy with this PR as a temporary fix for PEP 518 support to require that build dependencies have to be wheels for now. (Although travis isn't - looks like there are tests that need fixing...)

@pradyunsg
Copy link
Member Author

I'll make a separate PR for improving the message.

I'll look into the test failure in ~12 hours. It's probably because something in the test harness is using an sdist build dependency.

@pradyunsg
Copy link
Member Author

Additional investigation needed: can arbitrary URLs be a build-dependency and pip still installs it?

@pradyunsg
Copy link
Member Author

Another idea: Disabling all of the current PEP 518 stuff with a flag, just in case there's some security hole in there?

This has the nice advantage of telling us the exact branches that need to be changed post 10.0.

@pradyunsg
Copy link
Member Author

we're not trying to build a wheel without setuptools

Actually, we are. pip-forkbomb-test doesn't list setuptools in build-system.requires.

@pradyunsg
Copy link
Member Author

Um, PEP 517.

Yes.

(apologies for the terse replies, I'm in the middle of something else)

@pradyunsg
Copy link
Member Author

(Although travis isn't - looks like there are tests that need fixing...)

Fixed by switching to simplewheel from simple in the test.

@ghost
Copy link

ghost commented Jan 23, 2018

Excellent work, @pradyunsg!

@pradyunsg
Copy link
Member Author

When merging this PR, please use plain merge instead of squash/rebase merges since I have another branch sitting on top of this one now.

@ghost ghost mentioned this pull request Jan 24, 2018
@pradyunsg
Copy link
Member Author

@pypa/pip-committers Is there anything else we want to do here before we can merge?

@pfmoore
Copy link
Member

pfmoore commented Jan 26, 2018

Not that I know of

@pradyunsg pradyunsg merged commit e81b602 into pypa:master Jan 26, 2018
@pradyunsg pradyunsg deleted the fix/fork-bombing branch January 26, 2018 09:01
@pradyunsg pradyunsg restored the fix/fork-bombing branch May 27, 2018 09:38
@pradyunsg pradyunsg deleted the fix/fork-bombing branch May 27, 2018 10:08
@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 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 skip news Does not need a NEWS file entry (eg: trivial changes) type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip does not detect circular build dependency
3 participants