Support pip-style VCS requirements. #15097
Conversation
[ci skip-rust] [ci skip-build-wheels]
| return cls(pkg_resources.Requirement.parse(line)) | ||
| except Exception as e: | ||
| scheme, netloc, path, query, fragment = urllib.parse.urlsplit(line) | ||
| if scheme and fragment.startswith("egg="): |
There was a problem hiding this comment.
You'll want to be able to deal with more going on in the fragment since Pip can use subdirectory:
- https://github.com/pantsbuild/pex/blob/904eef37fa36d738cc6409045239a7d80800e456/tests/test_requirements.py#L248
- https://github.com/pantsbuild/pex/blob/904eef37fa36d738cc6409045239a7d80800e456/tests/test_requirements.py#L275-L276
And subdirectory is supported by Pex:
https://github.com/pantsbuild/pex/blob/904eef37fa36d738cc6409045239a7d80800e456/tests/integration/cli/commands/test_vcs_lock.py#L157-L168
There was a problem hiding this comment.
Hmm, what do we do with those other fragments in the converted URL? I suppose we can leave them all in and assume they will be used if needed and ignored if not needed.
Experiments show that this works (e.g., leaving the fragment #egg=Django in Django @ git+https://github.com/django/django.git@stable/2.1.x#egg=Django works, and also the subdirectory gets used correctly in sdev_logging_utils @ git+https://github.com/SerialDev/sdev_py_utils.git@bd4d36a0#egg=sdev_logging_utils&subdirectory=sdev_logging_utils)
There was a problem hiding this comment.
BTW, re the comment here re "This is a trick you cannot do with a direct reference VCS URL". Does "this" refer to specifying a subdirectory?
Because that does seem to work with a direct reference VCS URL, in practice anyway. E.g., sdev_logging_utils @ git+https://github.com/SerialDev/sdev_py_utils.git@bd4d36a0#egg=sdev_logging_utils&subdirectory=sdev_logging_utils works (And omitting the subdirectory does not work)
There was a problem hiding this comment.
Aha. Gotcha. It works because Pip is in the pipeline and the PEP-440 is agnostic about URL content. Feed that PEP-440 to Poetry and it will fail (I'd bet).
[ci skip-rust] [ci skip-build-wheels]
| pep_440_req_str = f"{project}@ {full_url}" | ||
| try: | ||
| ret = cls(pkg_resources.Requirement.parse(pep_440_req_str)) | ||
| logger.warning( |
There was a problem hiding this comment.
This is unfortunate. I asked seperately, but why are we getting in the way here still - if even just in a naggy way with a warning? My only guess is we ~need the project name parsed out for dep inference.
There was a problem hiding this comment.
I think that is exactly it. I can remove the warning though.
There was a problem hiding this comment.
Yeah I don't think that this warning is good. I'm excited about being able to remove the section in the docs explaining how you must use pep 440.
There was a problem hiding this comment.
Oh - you definitely can't remove that because of this change. As pointed out in the comments there are plenty of valid Pip requirements this does not handle. Like . (Pex uses this to package itself) and more. We're still getting in the way here and should just be more lax and try to get a project name and if we can't, move on.
There was a problem hiding this comment.
We also need some string to use as the target name, but we can synthesize one
| pep_440_req_str = f"{project}@ {full_url}" | ||
| try: | ||
| ret = cls(pkg_resources.Requirement.parse(pep_440_req_str)) | ||
| logger.warning( |
There was a problem hiding this comment.
Yeah I don't think that this warning is good. I'm excited about being able to remove the section in the docs explaining how you must use pep 440.
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
|
I'd like to merge this without tackling the handful of remaining cases just yet (e.g., an omitted egg= fragment). This is already a huge improvement on the status quo, and finessing those last few cases will require a little more thought. |
[ci skip-rust]
[ci skip-build-wheels]