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

Remove deprecated support for VCS pseudo URLs #9436

Merged
merged 5 commits into from
Jan 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions news/7554.removal.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Remove support for VCS pseudo URLs editable requirements. It was emitting
deprecation warning since version 20.0.
22 changes: 8 additions & 14 deletions src/pip/_internal/req/constructors.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,23 +115,17 @@ def parse_editable(editable_req):
url = f'{version_control}+{url}'
break

if '+' not in url:
link = Link(url)

if not link.is_vcs:
backends = ", ".join(vcs.all_schemes)
raise InstallationError(
'{} is not a valid editable requirement. '
'It should either be a path to a local project or a VCS URL '
'(beginning with svn+, git+, hg+, or bzr+).'.format(editable_req)
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}).'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should not simply show vcs.all_schemes directly.
Otherwise, we might crash on someone using git+ftp (or something else) and tell him to use a VCS URL beginning with git+ which is already the case.
Yes, it seems far fetched ^^

Copy link
Member Author

@sbidoul sbidoul Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two possible issues with that are
1/ the list of all schemes is very long
2/ it still contains git, svn, etc (without +)

Regarding 2/, VCS schemes without + such as git:// are supported in editable mode only (see a few lines above), and I think it's not something we want to encourage. I suspect we could remove the schemes without + from all_schemes without changing behaviour, but I'm not 100% sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xavfernandez I've double checked and I don't see a reason for having the schemes without + in the list of all VCS schemes. So I removed them and did as you suggested to show the full list of supported schemes in the error message.

)

vc_type = url.split('+', 1)[0].lower()

if not vcs.get_backend(vc_type):
backends = ", ".join([bends.name + '+URL' for bends in vcs.backends])
error_message = "For --editable={}, " \
"only {} are currently supported".format(
editable_req, backends)
raise InstallationError(error_message)

package_name = Link(url).egg_fragment
package_name = link.egg_fragment
if not package_name:
raise InstallationError(
"Could not detect requirement name for '{}', please specify one "
Expand Down
31 changes: 6 additions & 25 deletions src/pip/_internal/req/req_install.py
Original file line number Diff line number Diff line change
Expand Up @@ -625,31 +625,12 @@ def update_editable(self):
if self.link.scheme == 'file':
# Static paths don't get updated
return
assert '+' in self.link.url, \
"bad url: {self.link.url!r}".format(**locals())
vc_type, url = self.link.url.split('+', 1)
vcs_backend = vcs.get_backend(vc_type)
if vcs_backend:
if not self.link.is_vcs:
reason = (
"This form of VCS requirement is being deprecated: {}."
).format(
self.link.url
)
replacement = None
if self.link.url.startswith("git+git@"):
replacement = (
"git+https://[email protected]/..., "
"git+ssh://[email protected]/..., "
"or the insecure git+git://[email protected]/..."
)
deprecated(reason, replacement, gone_in="21.0", issue=7554)
hidden_url = hide_url(self.link.url)
vcs_backend.obtain(self.source_dir, url=hidden_url)
else:
assert 0, (
'Unexpected version control type (in {}): {}'.format(
self.link, vc_type))
vcs_backend = vcs.get_backend_for_scheme(self.link.scheme)
# Editable requirements are validated in Requirement constructors.
# So here, if it's neither a path nor a valid VCS URL, it's a bug.
assert vcs_backend, f"Unsupported VCS URL {self.link.url}"
hidden_url = hide_url(self.link.url)
vcs_backend.obtain(self.source_dir, url=hidden_url)

# Top-level Actions
def uninstall(self, auto_confirm=False, verbose=False):
Expand Down
4 changes: 2 additions & 2 deletions src/pip/_internal/vcs/bazaar.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ class Bazaar(VersionControl):
dirname = '.bzr'
repo_name = 'branch'
schemes = (
'bzr', 'bzr+http', 'bzr+https', 'bzr+ssh', 'bzr+sftp', 'bzr+ftp',
'bzr+lp',
'bzr+http', 'bzr+https', 'bzr+ssh', 'bzr+sftp', 'bzr+ftp',
'bzr+lp', 'bzr+file'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'bzr+lp', 'bzr+file'
'bzr+lp', 'bzr+file',

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

)

@staticmethod
Expand Down
2 changes: 1 addition & 1 deletion src/pip/_internal/vcs/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class Git(VersionControl):
dirname = '.git'
repo_name = 'clone'
schemes = (
'git', 'git+http', 'git+https', 'git+ssh', 'git+git', 'git+file',
'git+http', 'git+https', 'git+ssh', 'git+git', 'git+file',
)
# Prevent the user's environment variables from interfering with pip:
# https://github.com/pypa/pip/issues/1130
Expand Down
2 changes: 1 addition & 1 deletion src/pip/_internal/vcs/mercurial.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class Mercurial(VersionControl):
dirname = '.hg'
repo_name = 'clone'
schemes = (
'hg', 'hg+file', 'hg+http', 'hg+https', 'hg+ssh', 'hg+static-http',
'hg+file', 'hg+http', 'hg+https', 'hg+ssh', 'hg+static-http',
)

@staticmethod
Expand Down
4 changes: 3 additions & 1 deletion src/pip/_internal/vcs/subversion.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ class Subversion(VersionControl):
name = 'svn'
dirname = '.svn'
repo_name = 'checkout'
schemes = ('svn', 'svn+ssh', 'svn+http', 'svn+https', 'svn+svn')
schemes = (
'svn+ssh', 'svn+http', 'svn+https', 'svn+svn', 'svn+file'
)

@classmethod
def should_add_vcs_url_prefix(cls, remote_url):
Expand Down
5 changes: 3 additions & 2 deletions tests/functional/test_install_vcs_git.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import pytest

from pip._internal.utils.urls import path_to_url
from tests.lib import pyversion # noqa: F401
from tests.lib import (
_change_test_package_version,
Expand Down Expand Up @@ -454,7 +455,7 @@ def test_check_submodule_addition(script):
)

install_result = script.pip(
'install', '-e', 'git+' + module_path + '#egg=version_pkg'
'install', '-e', 'git+' + path_to_url(module_path) + '#egg=version_pkg'
)
install_result.did_create(
script.venv / 'src/version-pkg/testpkg/static/testfile'
Expand All @@ -467,7 +468,7 @@ def test_check_submodule_addition(script):

# expect error because git may write to stderr
update_result = script.pip(
'install', '-e', 'git+' + module_path + '#egg=version_pkg',
'install', '-e', 'git+' + path_to_url(module_path) + '#egg=version_pkg',
'--upgrade',
)

Expand Down