-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Support Direct URL editable requirements #13495
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
Conversation
4164131 to
47e7040
Compare
47e7040 to
fcae387
Compare
|
Ugh, why is everything filled with so many exceptions and undocumented extensions. There is a backwards compatibility break here. I'll fix it, but I do want to take a closer look at why it's happening. |
This is crucial for the deprecation of non-bare egg URL fragments as they used to be the sole way to request an extra for a VCS editable install. I've also removed all references to egg fragments from the user docs and added further explanation of the Direct URL form in the VCS support topic. Getting the error handling right was a pain. That's why I extracted the two parsing flows into their own functions. Co-authored-by: Tzu-ping Chung <[email protected]>
fcae387 to
8850376
Compare
|
@ichard26 is this ready for review? Hard to tell from your last comment. |
|
@notatallshaw short of the one outstanding documentation change that's noted above, this is indeed ready for review. |
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.
Thanks for this work @ichard26. I made a pass on this and did a few tests. Everything works as I expected.
A few minor comments, some of which for possible followups.
I noticed a remaining #egg mention in a warning message:
pip install -e "pkgb @ file://$PWD/pkga"
WARNING: Generating metadata for package pkgb produced metadata for project name pkga. Fix your #egg=pkgb fragments.
| name = req.name | ||
| return (name, req.url, req.extras) | ||
|
|
||
| raise ValueError |
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.
Note: it could be convenient to support pkgname[extras] @ ./localdir as an alternative to ./localdir[extras]. But that can be discussed separately.
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'd rather not add extensions to standard requirement syntax, but yes, this would be a follow up.
|
|
||
| package_name = link.egg_fragment | ||
| if not package_name: | ||
| # The project name can be inferred from local file URIs easily. |
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.
What do you mean with this comment?
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.
With the extraction of _parse_pip_syntax_editable(), now every editable requirement is checked that there is a known project name. This is problematic for local editable reqs like -e . or -e /local/path. Previously these cases would return early in parse_editable() skipping this check, but now they fail it. However, pip can and will infer the project name for local file URIs so we skip the check if that's the case.
|
@notatallshaw I do want to get this into the 25.3 release. I'll aim to get this updated by EOD tomorrow. |
|
@ichard26 👍, I'll try and do a quick pass over this today, but as it's so far outside my expertise it's unlikely I'll have any additional comments. |
Co-authored-by: Stéphane Bidoul <[email protected]>
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.
Don't forget to postpone the #egg deprecation:
pip/src/pip/_internal/models/link.py
Lines 476 to 482 in 4dd68aa
| if not self._project_name_re.match(project_name): | |
| deprecated( | |
| reason=f"{self} contains an egg fragment with a non-PEP 508 name.", | |
| replacement="to use the req @ url syntax, and remove the egg fragment", | |
| gone_in="25.3", | |
| issue=13157, | |
| ) |
At some point this warning will also need addressing to be complete:
pip/src/pip/_internal/req/req_install.py
Lines 424 to 430 in 4dd68aa
| logger.warning( | |
| "Generating metadata for package %s " | |
| "produced metadata for project name %s. Fix your " | |
| "#egg=%s fragments.", | |
| self.name, | |
| metadata_name, | |
| self.name, |
Resolved conflict in tests/functional/test_install_reqs.py by keeping the test_nonpep517_setuptools_import_failure test and TestEditableDirectURL class from the PR branch.
The --no-use-pep517 option has been removed from pip as part of the move to PEP 517-only builds. This test is no longer valid and was causing failures. Also removed the no-longer-needed make_wheel import.
|
Thanks @ichard26 ! I'll follow up in a moment with a PR that bumps any remaining |
This is crucial for the deprecation of non-bare egg URL fragments as they used to be the sole way to request an extra for a VCS editable install.
I've also removed all references to egg fragments from the user docs and added further explanation of the Direct URL form in the VCS support topic.
Getting the error handling right was a pain. That's why I extracted the two parsing flows into their own functions.
Towards #13157.
Towards #1289.
Supersedes #9471.