-
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
Allow PEP 508 URL spec as editable #9471
Conversation
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.
Looks good, thanks!.
d6c5bb3
to
a320787
Compare
Updated for review comments. |
bc6fecd
to
457f8a6
Compare
Do we have a natural place in the documentation for that ? |
There’s a section in |
👍 to drop #egg= references from the doc now. That's a good way to check that we have an alternative for all documented use cases. |
name = f"{req.name} ; {req.marker}" | ||
else: | ||
name = req.name | ||
return (name, req.url, req.extras) |
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.
Hm, on second look... this will let non VCS URLs go through ? So we need to merge #9436 first and see how we can do the link.is_vcs
here too.
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.
Oh, good point, Requirement()
does not check if the URL is actually valid.
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.
One thing we ca do here is to add an extra check, and only treat the string as PEP 508 if the URL part contains ://
. This means pseudo URL strings will automatically fall back to the old parsing logic, and can be handled independently to this new logic.
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.
@sbidoul I added an additional check to prevent pseudo-URLs from going through here. Do you think it’d help?
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.
@uranusjr I have not tested (I whish I had more time for pip...) but this version would accept any URLs ? Editables URLs must be file://
or vcs+...://
urls only, correct ? And even file URLs must point to a local directory, not a file.
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.
Correct. There are already checks in a later stage to error out on invalid URL values, so I think it’s fine to pass them through here.
pip/src/pip/_internal/req/constructors.py
Lines 116 to 119 in baaf66f
raise InstallationError( | |
f'{editable_req} is not a valid editable requirement. ' | |
f'It should either be a path to a local project or a VCS URL ' | |
f'(beginning with {backends}).' |
I did a mass find-and-replace on the documentation (and rewrote a couple of paragraphs) to ditch |
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 |
This prevents "pseudo URL" VCS requirements from going through the parser and cause issues later.
f894310
to
9defcc0
Compare
@uranusjr this does not seem to work, unfortunately: $ pip install -e "pip-test-package @ git+https://github.com/pypa/pip-test-package"
ERROR: Could not detect requirement name for 'git+https://github.com/pypa/pip-test-package', please specify one with #egg=your_package_name Somehow |
Thanks for catching this. A code path is building editables incorrectly with a bare URL instead of the whole requirement string. This “works” right now because the URL is the requirement string, but not anymore. |
Hmm, turns out Update: Need to first resolve pypa/packaging#264. |
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 |
FWIW, I suggest filing a new PR for this, since our CI pipeline changed significantly; and the existing test results would likely contribute to "noise" in the checks view. :) |
The needed upstream fix in |
Why ? Implementing |
My wording was bad, it’s more like I don’t think it’s a good investment of my time right now 😓 |
See #1289 (comment)