Skip to content
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

Error installing git+file VCS references #7650

Closed
sbidoul opened this issue Jan 25, 2020 · 19 comments · Fixed by #12300
Closed

Error installing git+file VCS references #7650

sbidoul opened this issue Jan 25, 2020 · 19 comments · Fixed by #12300
Labels
C: direct url Direct URL references (PEP 440, PEP 508, PEP 610) C: error messages Improving error messages C: vcs pip's interaction with version control systems like git, svn and bzr project: vendored dependency Related to a vendored dependency type: bug A confirmed bug or unintended behavior UX User experience related

Comments

@sbidoul
Copy link
Member

sbidoul commented Jan 25, 2020

Environment

  • pip version: 20.0
  • Python version: 3.7
  • OS: linux

Description

The following installation of a PEP 440 local VCS reference fails: (using bash to reproduce)

$ git clone https://github.com/pypa/packaging
...
$ pip install "packaging @ git+file://$PWD/packaging"
ERROR: Invalid requirement: 'packaging @ git+file:///home/.../packaging'
Hint: It looks like a path. File 'packaging @ git+file:///home/.../packaging' does not exist.

Expected behavior

It should work, as does the pip-specific URL form:

$ pip install "git+file://$PWD/packaging#egg=packaging"

and as does the PEP 440 form with a remote VCS URL:

$ pip install "packaging @ git+https://github.com/pypa/packaging"
@triage-new-issues triage-new-issues bot added the S: needs triage Issues/PRs that need to be triaged label Jan 25, 2020
@pradyunsg
Copy link
Member

"packaging @ git+https://github.com/pypa/packaging"

This isn't a valid PEP 508 requirement (see https://www.python.org/dev/peps/pep-0508/#examples) -- it's a weird abomination of pip-specific VCS references and PEP 508.

@pradyunsg
Copy link
Member

I don't think we should mix VCS and PEP 508 as proposed, since we'd be building on top of the standard, which would make pip non-compliant with it, in a very implementation-specific manner, which would defeat the point of the standardization. :)

@pradyunsg pradyunsg added PEP implementation Involves some PEP state: needs discussion This needs some more discussion type: feature request Request for a new feature labels Jan 25, 2020
@triage-new-issues triage-new-issues bot removed the S: needs triage Issues/PRs that need to be triaged label Jan 25, 2020
@sbidoul
Copy link
Member Author

sbidoul commented Jan 25, 2020

This isn't a valid PEP 508 requirement

@pradyunsg how so? IIUC, PEP 508 builds on PEP 440 and the VCS form is documented in PEP 440.
The git+https form is valid and works just fine with pip. It's only the git+file form that fails.

@uranusjr
Copy link
Member

PEP 440 specifically says the URL should point to either an sdist or wheel. It’s possible to make a case for pip to support VCS requirements like this, but that would still be pip-specific, and not covered by PEP 440.

@sbidoul
Copy link
Member Author

sbidoul commented Jan 25, 2020

This section of PEP 440 talks about VCS references. While it does only show git+https examples, I see nothing in it that excludes git+file references.

@sbidoul
Copy link
Member Author

sbidoul commented Jan 26, 2020

So do we agree this is a bug?

It comes from here in packaging:

if parsed_url.scheme == "file":
if urlparse.urlunparse(parsed_url) != req.url:
raise InvalidRequirement("Invalid URL given")
elif not (parsed_url.scheme and parsed_url.netloc) or (
not parsed_url.scheme and not parsed_url.netloc
):
raise InvalidRequirement("Invalid URL: {0}".format(req.url))

The file scheme is special-cased, and git+file fails because it has no netloc.

@pradyunsg
Copy link
Member

So do we agree this is a bug?

No...

Note that OP's packaging @ git+https://github.com/pypa/packaging is not the PEP's syntax. PEP 440 states that these URLs need a @<commit-hash> or the @<tag>#<commit-hash>. There's a note in the PEP in the same section, that explicitly mentions that the PEP's syntax "isn't quite the same as the existing VCS reference notation supported by pip".

To quote examples from PEP 440:

pip @ https://github.com/pypa/pip/archive/1.3.1.zip#sha1=da9234ee9982d4bbb3c72346a6de940a148ea686
pip @ git+https://github.com/pypa/pip.git@7921be1537eac1e97bc40179a57f0349c2aee67d
pip @ git+https://github.com/pypa/pip.git@1.3.1#7921be1537eac1e97bc40179a57f0349c2aee67d

Likewise, the following works:

$ pip install "pip @ git+https://github.com/pypa/pip.git@1.3.1#7921be1537eac1e97bc40179a57f0349c2aee67d"
Collecting pip@ git+https://github.com/pypa/pip.git@1.3.1#7921be1537eac1e97bc40179a57f0349c2aee67d
  Cloning https://github.com/pypa/pip.git (to revision 1.3.1) to /private/var/folders/4d/bt0_xfx56bjfmmt2bv3r5_qh0000gn/T/pip-install-md1mmlg3/pip
  Running command git clone -q https://github.com/pypa/pip.git /private/var/folders/4d/bt0_xfx56bjfmmt2bv3r5_qh0000gn/T/pip-install-md1mmlg3/pip
  Running command git checkout -q 7a414f574bcaf1a5f5bbd4684446ffd88c740025
Building wheels for collected packages: pip
  Building wheel for pip (setup.py) ... done
  Created wheel for pip: filename=pip-1.3.1-py3-none-any.whl size=233312 sha256=854661d9ba65bc0c619e360e5659657667644a0162a8e32723b6e60c083c24d5
  Stored in directory: /private/var/folders/4d/bt0_xfx56bjfmmt2bv3r5_qh0000gn/T/pip-ephem-wheel-cache-46prpvqk/wheels/34/b7/bb/9d986aee7562ebed1d4a8afbd4814605ffe65eee6d746395fd
Successfully built pip
Installing collected packages: pip
  Attempting uninstall: pip
    Found existing installation: pip 20.0.2
    Uninstalling pip-20.0.2:
      Successfully uninstalled pip-20.0.2
Successfully installed pip-1.3.1

(sorry that it says pip too many time, I literally copy-pasted the PEP's example)

@pradyunsg
Copy link
Member

I do think we'd want to figure out a better error message here though. :)

@pradyunsg pradyunsg added this to the Print Better Error Messages milestone Jan 27, 2020
@pradyunsg
Copy link
Member

And, while there's definite back-and-forth going on here about what's actually the correct behavior, thanks for exploring and spotting this edge case. :)

(also, yay 2:30am! I really should fix my body clock.)

@sbidoul
Copy link
Member Author

sbidoul commented Jan 28, 2020

I'm really confused now. To be honest, this git+file scheme is not that important to me. I stumbled upon it while writing tests for #7612. However this discussion here raises a more fundamental point.

I always understood that PEP 440 direct references (name @ URL) were meant at some point to replace the legacy pip URL references (URL#egg=name). Is that correct? That is how I interpreted the PEP language saying "PEP's syntax "isn't quite the same as the existing VCS reference notation supported by pip". For instance that section explicitly notes that the package name goes in front instead of in the egg= fragment. It also says that a commit id must be provided in addition to a tag for better pinning. But I certainly does not read it as excluding other forms.

For instance referring to a branch (packaging @ git+https://github.com/pypa/packaging@master) is not explicitly mentioned in PEP 440. It is however a perfectly valid and useful use case and pip happily accepts it. pip also accepts subdirectory fragments in PEP 440 direct requirements. Should pip start rejecting those too? I certainly hope not!

That is why this issue seemed so trivial to me at first. BTW in the meantime I had proposed a fix in packaging: pypa/packaging#264

@pfmoore
Copy link
Member

pfmoore commented Jan 28, 2020

It sounds to me like we might need a discussion on the Packaging discourse, with a view to clarifying the details of direct URLs, and updating the spec to be more precise on what the intended and supported uses are.

Installing from VCS is, I believe, an entirely implementation-defined feature of pip. Short of standardising it (which would be a pretty big exercise) if we wanted to discuss VCS links in standards, we'd need to be viewing them as "a direct URL link to a PEP 517 source tree", but going much beyond that would need a lot more thought.

FWIW, my view is that packaging @ git+file://$PWD/packaging looks like a perfectly good direct URL to me, and should be acceptable wherever pip accepts a direct URL. But having said that, the PEP says "the exact URLs and targets supported will be tool dependent" - in principle, pip @ mailto:pip@python.org?Subject="Please send me pip" is a valid PEP 440 direct reference 😉

@sbidoul
Copy link
Member Author

sbidoul commented Jan 28, 2020

It sounds to me like we might need a discussion on the Packaging discourse, with a view to clarifying the details of direct URLs, and updating the spec to be more precise on what the intended and supported uses are.

Yep. I had identified that PEP 440 needed some clarifications in that area while working on PEP 610. I'll consider tackling that when PEP 610 is out of Draft state, because I certainly don't have the mental energy to have two PEP discussions in flight at the same time - this is damn exhausting.

@pradyunsg
Copy link
Member

It sounds to me like we might need a discussion on the Packaging discourse, with a view to clarifying the details of direct URLs, and updating the spec to be more precise on what the intended and supported uses are.

Yea, this sounds about right to me and you basically said what I was gonna say. What @sbidoul is saying also makes sense to me, so we'd definitely want to have a broader discussion about the handling of direct URLs (and what's valid vs what's not and why).

Further, rereading my earlier comments today, I realize that I likely came across as dismissive in my comments above, definitely which wasn't the intention. Sorry about that! I know that the web as a whole, isn't a great medium for conveying intent and should've been a bit more careful here. :)

@sbidoul
Copy link
Member Author

sbidoul commented Jan 28, 2020

No worries, @pradyunsg. All that matter at the end of the day is that we reach a better collective understanding of these complex matters, and produce better specs and implementations.

@pfmoore
Copy link
Member

pfmoore commented Jan 28, 2020

I certainly don't have the mental energy to have two PEP discussions in flight at the same time - this is damn exhausting.

Lol, I know what you mean. One of the reasons things can move slowly in packaging standards is that everyone is permanently teetering on the brink of burnout. It's only due to the amazing commitment of the people who participate that we get as much done as we do.

PS From the other various comments around, I suspect that the change to the direct URL spec might be as simple as changing it to say that PEP 440 defines nothing beyond the package @ URL syntax. Interpretation of the URL is not covered in PEP 440 (although leave the existing text as examples of what can be done) and should/will be standardised elsewhere. This would mean that the code you referred to here probably is a bug. (I say "probably" simply because I'm not completely against packaging having some level of sanity checks on allowed URLs).

@sbidoul sbidoul added project: vendored dependency Related to a vendored dependency type: bug A confirmed bug or unintended behavior and removed PEP implementation Involves some PEP state: needs discussion This needs some more discussion type: feature request Request for a new feature labels Feb 1, 2020
@sbidoul sbidoul added C: vcs pip's interaction with version control systems like git, svn and bzr C: direct url Direct URL references (PEP 440, PEP 508, PEP 610) labels May 21, 2020
@nlhkabu
Copy link
Member

nlhkabu commented Jul 28, 2020

I do think we'd want to figure out a better error message here though. :)

@pradyun do you want me to include this in #8516 ? If yes, can you please brief me on this? Thx

@nlhkabu nlhkabu removed this from the Print Better Error Messages milestone Jul 29, 2020
@pradyunsg
Copy link
Member

I think that's "just" a bug in the logic, and this doesn't need to be included in #8516.

@sbidoul sbidoul changed the title Error installing local VCS reference in PEP 440 format Error installing git+file VCS references Jan 7, 2023
@God-damnit-all
Copy link

@pradyunsg If I'm not mistaken, pypa/packaging#684 fixed this issue so this can be closed, yes?

@sbidoul
Copy link
Member Author

sbidoul commented Mar 10, 2024

@ImportTaste I added this to be closed by #12300

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: direct url Direct URL references (PEP 440, PEP 508, PEP 610) C: error messages Improving error messages C: vcs pip's interaction with version control systems like git, svn and bzr project: vendored dependency Related to a vendored dependency type: bug A confirmed bug or unintended behavior UX User experience related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants