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

Wrong return value of a Link.is_artifact for git+user@hostname editable packages #6293

Closed
atugushev opened this issue Feb 22, 2019 · 11 comments
Labels
auto-locked Outdated issues that have been locked by automation C: download About fetching data from PyPI and other sources C: vcs pip's interaction with version control systems like git, svn and bzr state: needs discussion This needs some more discussion type: enhancement Improvements to functionality

Comments

@atugushev
Copy link
Contributor

atugushev commented Feb 22, 2019

A Link.is_artifact returns True for URLs like git+user@hostname/repo.git which is probably a bug, see:

if self.scheme in vcs.all_schemes:
return False

That's because a Link.scheme returns an empty string for git+user@... url, since there is no the explicit scheme (which is ssh://). It may cause potential bugs. For example:

if self._download_should_save:
# Make a .zip of the source_dir we already created.
if req.link.scheme in vcs.all_schemes:
req.archive(self.download_dir)

Refs jazzband/pip-tools#738

@cjerdonek
Copy link
Member

Can you exhibit a problem this causes in pip? The linked issue shows pip code being called from pip-tools, so it would be nice to have a pip-specific issue to get a better understanding of what code paths this affects.

@atugushev
Copy link
Contributor Author

atugushev commented Feb 23, 2019

Sorry, the post turns out a bit messy. Perhaps, there is not an issue at all, but i've been trying to explain, that conditionals like self.scheme in vcs.all_schemes (which occurs several times the code) might not be quiet correct for the URLs kind of git+user@hostname/repo.git (which perfectly works with pip install by the way) and may cause potential bugs. Anyways, I've tried to find any bug related to this kind of URLs and found that pip download git+git@... is not working, see:

$ pip download [email protected]:django/django#egg=django
Invalid requirement: '[email protected]:django/django#egg=django'
It looks like a path. File '[email protected]:django/django#egg=django' does not exist.

On the other hand the URL like git+ssh:// works like a charm:

$ pip download git+ssh://[email protected]/django/django#egg=django
Collecting django from git+ssh://[email protected]/django/django#egg=django
...

Pip install works well with git+git@..., see:

$ pip install -e [email protected]:django/django#egg=django
Obtaining django from [email protected]:django/django#egg=django
...

Regarding to the pip download doc there's no specs for VCS URLs, just mentioned pip download [options] <vcs project url> .... But the spec can be found in the pip install doc, see:

 Here are the supported forms:
 ...
 -e [email protected]:MyProject#egg=MyProject

The question is "the bug or not the bug"?

@atugushev
Copy link
Contributor Author

atugushev commented Feb 24, 2019

@cjerdonek i've prepared a PR #6296 to fix the bug.

@cjerdonek
Copy link
Member

cjerdonek commented Feb 24, 2019

@atugushev I wouldn't call it a bug because, as you pointed out, that form of url isn't listed in the docs as one of the "supported forms" of VCS urls. So it would need to be discussed whether to expand what urls are accepted.

PS - thank you for writing up your findings from the documentation so nicely!

@cjerdonek cjerdonek added type: enhancement Improvements to functionality C: vcs pip's interaction with version control systems like git, svn and bzr C: download About fetching data from PyPI and other sources state: needs discussion This needs some more discussion labels Feb 24, 2019
@xavfernandez
Copy link
Member

xavfernandez commented Feb 24, 2019

This is strongly related to #2994.
And 3 years later, I'm still in favor of sticking with URLs and dropping the other syntax (cf #2994 (comment))

@uranusjr
Copy link
Member

FWIW, we had a similar discussion for Pipenv as well. The decision was to support SSH-like URIs, but I would strongly warn anyone attempting to do the same. It’s very messy. Don’t do it if you have a choice.

@cjerdonek
Copy link
Member

I think we have a choice in this case. :)

@pradyunsg
Copy link
Member

Let's get rid of git+git.

If anyone is opposed to that, please holler.

@xavfernandez
Copy link
Member

I'm still in favor but I'd be in favor of quite long deprecation period (at least 6 months, maybe a year ?)

@sbidoul
Copy link
Member

sbidoul commented Jan 2, 2020

I made the PR to deprecate git+git@: #7543

@sbidoul
Copy link
Member

sbidoul commented Jan 4, 2020

Closing as we propose to remove the git+git@ form in #7554.

@sbidoul sbidoul closed this as completed Jan 4, 2020
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Feb 3, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: download About fetching data from PyPI and other sources C: vcs pip's interaction with version control systems like git, svn and bzr state: needs discussion This needs some more discussion type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants