-
Notifications
You must be signed in to change notification settings - Fork 3k
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
PEP 518: enable source installs for build dependencies #5336
PEP 518: enable source installs for build dependencies #5336
Conversation
TODO:
|
a70ef1e
to
6bcfb60
Compare
You'll want to modify the documentation paragraph about the limitations, changing it to say in pip 10. |
I updated the check-list above. Will amend after rebasing on master once #5286 is merged. |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
6bcfb60
to
6e1ef58
Compare
Rebased on master + updated the documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the bandwidth to do a full review right now.
docs/reference/pip.rst
Outdated
unbounded recursion involved was not considered acceptable, and so | ||
installation of build dependencies from source has been disabled until a safe | ||
resolution of this issue is found. | ||
* ``pip<18.0`` only support installing build requirements from wheels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge it with the next point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me. Inline notes on tests.
@@ -0,0 +1 @@ | |||
Add support for installing PEP 518 build dependencies from source. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove the period?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I said that for consistency across news files -- I usually commit them without periods. Looking at the directory currently, it's inconsistent already so I guess this doesn't matter all that much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd definitely say this doesn't matter. FWIW personally I prefer complete sentences (i.e., periods).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
tests/functional/test_install.py
Outdated
path_to_url(data.packages.join('pep518_forkbomb-235.tar.gz')) | ||
) in result.stdout, result.stdout | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A test for fork-bomb due to a cycle in the build-requirements (instead of self-depending) would be nice.
tests/functional/test_install.py
Outdated
data.src.join("pep518_with_extra_and_markers-1.0"), | ||
use_module=True, | ||
) | ||
|
||
|
||
@pytest.mark.parametrize('command', ('install', 'wheel')) | ||
def test_pep518_forkbomb(script, data, common_wheels, command): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to limit this test so that it doesn't actually fork-bomb the system is we mess up the logic being tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll mark the test with @pytest.mark.timeout(60)
.
@@ -100,10 +100,6 @@ def packages2(self): | |||
def packages3(self): | |||
return self.root.join("packages3") | |||
|
|||
@property | |||
def packages4(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bye bye packages4! 👋
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me. Inline notes on tests.
e3b2efb
to
4dd5b27
Compare
Rebased with additional tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
A gentle ping for other @pypa/pip-committers. Would appreciate more 👀 through this. Happy to merge as is though. :) |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
4dd5b27
to
ec64ca9
Compare
ec64ca9
to
06fb565
Compare
No need for a wheel of `simple` now that build dependencies support source installs.
self.cleanup() | ||
|
||
def _entry_path(self, link): | ||
hashed = hashlib.sha224(link.url_without_fragment.encode()).hexdigest() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting here that I did briefly think that we should split the folder name, to be like pip's cache.
wheels
[...]
├── bf
│ └── c9
│ └── a3
│ └── c538d90ef17cf7823fa51fc701a7a7a910a80f6a405bf15b1a
│ └── future-0.16.0-cp37-none-any.whl
[...]
I understand the cache might be this way to prevent far too many files in a single directory. I doubt that build-trees would get all that large though, considering that we only have a tracker for what's being built right now.
@pypa/pip-committers Does anyone else have bandwidth to review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing outstanding remain imho.
This was reverted because pip didn't support installing build deps from sdists. But this looks fixed since pypa/pip#5336 pip now builds and installs pycairo before building pygobject. This reverts commit ad1bbfa.
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. |
Re-enable support for source installs, and detect fork-bombs.
Constraints:
-t
)prepare requirement -> install build dependencies -> [...] -> try to prepare the original requirement again
, so we can't identify requirements based on aNAME-VERSION
key because those are not always known (or can be dependend upon) before and during preparationSolution:
pip download/install/wheel
(during theinstall build dependencies
phase, the parent process is waiting for the pip sub-process completion)Note: this PR is based on and includes the changes in #5286 (see here for a comparison between the 2).
Fixes #5229.