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

Fix is_url from splitting the scheme incorrectly when using PEP 440's direct references #6203

Merged
merged 2 commits into from
Sep 16, 2019
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
18 changes: 16 additions & 2 deletions docs/html/reference/pip_install.rst
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,7 @@ pip supports installing from a package index using a :term:`requirement
specifier <pypug:Requirement Specifier>`. Generally speaking, a requirement
specifier is composed of a project name followed by optional :term:`version
specifiers <pypug:Version Specifier>`. :pep:`508` contains a full specification
of the format of a requirement (pip does not support the ``url_req`` form
of specifier at this time).
of the format of a requirement.

Some examples:

Expand All @@ -265,6 +264,13 @@ Since version 6.0, pip also supports specifiers containing `environment markers
SomeProject ==5.4 ; python_version < '2.7'
SomeProject; sys_platform == 'win32'

Since version 19.1, pip also supports `direct references
<https://www.python.org/dev/peps/pep-0440/#direct-references>`__ like so:

::

SomeProject @ file:///somewhere/...

retpolanne marked this conversation as resolved.
Show resolved Hide resolved
Environment markers are supported in the command line and in requirements files.

.. note::
Expand Down Expand Up @@ -880,6 +886,14 @@ Examples
$ pip install http://my.package.repo/SomePackage-1.0.4.zip


#. Install a particular source archive file following :pep:`440` direct references.

::

$ pip install SomeProject==1.0.4@http://my.package.repo//SomeProject-1.2.3-py33-none-any.whl
pradyunsg marked this conversation as resolved.
Show resolved Hide resolved
$ pip install "SomeProject==1.0.4 @ http://my.package.repo//SomeProject-1.2.3-py33-none-any.whl"


#. Install from alternative package repositories.

Install from a different index, and not `PyPI`_ ::
Expand Down
2 changes: 2 additions & 0 deletions news/6202.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix requirement line parser to correctly handle PEP 440 requirements with a URL
pointing to an archive file.
77 changes: 57 additions & 20 deletions src/pip/_internal/req/constructors.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,60 @@ def install_req_from_editable(
)


def _looks_like_path(name):
retpolanne marked this conversation as resolved.
Show resolved Hide resolved
# type: (str) -> bool
"""Checks whether the string "looks like" a path on the filesystem.

This does not check whether the target actually exists, only judge from the
appearance.

Returns true if any of the following conditions is true:
* a path separator is found (either os.path.sep or os.path.altsep);
* a dot is found (which represents the current directory).
"""
if os.path.sep in name:
return True
if os.path.altsep is not None and os.path.altsep in name:
return True
if name.startswith("."):
return True
Copy link
Member

@uranusjr uranusjr Jul 21, 2019

Choose a reason for hiding this comment

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

I just realised this if does not work as intended, and probably should be removed. A ./whatever string would’ve been caught in previous checks. This only matters for strings like .whatever, which I guess still does look like a path…? (but then the docstring is not accurate)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@uranusjr that's exactly it. I don't really know why some package would start with ., but I'll add a test case for it and add it to the docstring.

Copy link
Member

Choose a reason for hiding this comment

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

Packages can’t start with a dot, so this check doesn’t really matter either way :p But it’s better to remove it since its mere existence can be confusing to future readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. I have removed it then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@uranusjr oops, actually you can use . to install a package. You can use it to install the current directory as a package if it has a setup.py file.

Copy link
Contributor Author

@retpolanne retpolanne Jul 21, 2019

Choose a reason for hiding this comment

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

I still have a problem, though: the Windows tests might fail with the == since the path separator is different. I'll go with name.startswith then (I could make separate test cases for Windows, but that doesn't sound so good).

Copy link
Member

Choose a reason for hiding this comment

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

I believe the sep and altsep parts cover the different separators (if my memory of implementation from other projects serves).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I understood from the docs, it appears to be only available on Windows (the altsep on Windows would be the forward-slash)

https://docs.python.org/3/library/os.html#os.altsep

Copy link
Member

@uranusjr uranusjr Jul 22, 2019

Choose a reason for hiding this comment

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

That is correct, hence the first test would detect \ on Windows and / on POSIX; the second test detects / on Windows (always false on POSIX).

You could add a simple Windows-only test like this, if you’re inclined to:

@pytest.mark.parametrize('path', [
     '.\\path\\to\\installable',
     'relative\\path',
     'C:\\absolute\\path',
 ])
@pytest.skipif(os.path.sep != '\\')
 def test_looks_like_path_win(path):
     assert _looks_like_path(path) == True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@uranusjr I could also skip this test if not sys.platform.startswith("win")

return False


def _get_url_from_path(path, name):
# type: (str, str) -> str
"""
First, it checks whether a provided path is an installable directory
(e.g. it has a setup.py). If it is, returns the path.

If false, check if the path is an archive file (such as a .whl).
The function checks if the path is a file. If false, if the path has
an @, it will treat it as a PEP 440 URL requirement and return the path.
"""
if _looks_like_path(name) and os.path.isdir(path):
if is_installable_dir(path):
return path_to_url(path)
raise InstallationError(
"Directory %r is not installable. Neither 'setup.py' "
"nor 'pyproject.toml' found." % name
)
if not is_archive_file(path):
return None
if os.path.isfile(path):
return path_to_url(path)
urlreq_parts = name.split('@', 1)
if len(urlreq_parts) >= 2 and not _looks_like_path(urlreq_parts[0]):
# If the path contains '@' and the part before it does not look
# like a path, try to treat it as a PEP 440 URL req instead.
return None
logger.warning(
'Requirement %r looks like a filename, but the '
'file does not exist',
name
)
return path_to_url(path)


def install_req_from_line(
name, # type: str
comes_from=None, # type: Optional[Union[str, InstallRequirement]]
Expand Down Expand Up @@ -241,26 +295,9 @@ def install_req_from_line(
link = Link(name)
else:
p, extras_as_string = _strip_extras(path)
retpolanne marked this conversation as resolved.
Show resolved Hide resolved
looks_like_dir = os.path.isdir(p) and (
os.path.sep in name or
(os.path.altsep is not None and os.path.altsep in name) or
name.startswith('.')
)
if looks_like_dir:
if not is_installable_dir(p):
raise InstallationError(
"Directory %r is not installable. Neither 'setup.py' "
"nor 'pyproject.toml' found." % name
)
link = Link(path_to_url(p))
elif is_archive_file(p):
if not os.path.isfile(p):
logger.warning(
'Requirement %r looks like a filename, but the '
'file does not exist',
name
)
link = Link(path_to_url(p))
url = _get_url_from_path(p, name)
if url is not None:
link = Link(url)

# it's a local file, dir, or url
if link:
Expand Down
121 changes: 121 additions & 0 deletions tests/unit/test_req.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
from pip._internal.operations.prepare import RequirementPreparer
from pip._internal.req import InstallRequirement, RequirementSet
from pip._internal.req.constructors import (
_get_url_from_path,
_looks_like_path,
install_req_from_editable,
install_req_from_line,
parse_editable,
Expand Down Expand Up @@ -335,6 +337,33 @@ def test_url_with_query(self):
req = install_req_from_line(url + fragment)
assert req.link.url == url + fragment, req.link

def test_pep440_wheel_link_requirement(self):
url = 'https://whatever.com/test-0.4-py2.py3-bogus-any.whl'
line = 'test @ https://whatever.com/test-0.4-py2.py3-bogus-any.whl'
req = install_req_from_line(line)
parts = str(req.req).split('@', 1)
assert len(parts) == 2
assert parts[0].strip() == 'test'
assert parts[1].strip() == url

def test_pep440_url_link_requirement(self):
url = 'git+http://foo.com@ref#egg=foo'
line = 'foo @ git+http://foo.com@ref#egg=foo'
req = install_req_from_line(line)
parts = str(req.req).split('@', 1)
assert len(parts) == 2
assert parts[0].strip() == 'foo'
assert parts[1].strip() == url

def test_url_with_authentication_link_requirement(self):
url = 'https://[email protected]/test-0.4-py2.py3-bogus-any.whl'
line = 'https://[email protected]/test-0.4-py2.py3-bogus-any.whl'
req = install_req_from_line(line)
assert req.link is not None
assert req.link.is_wheel
assert req.link.scheme == "https"
assert req.link.url == url

def test_unsupported_wheel_link_requirement_raises(self):
reqset = RequirementSet()
req = install_req_from_line(
Expand Down Expand Up @@ -626,3 +655,95 @@ def test_mismatched_versions(caplog, tmpdir):
'Requested simplewheel==2.0, '
'but installing version 1.0'
)


@pytest.mark.parametrize('args, expected', [
# Test UNIX-like paths
(('/path/to/installable'), True),
# Test relative paths
(('./path/to/installable'), True),
# Test current path
(('.'), True),
# Test url paths
(('https://whatever.com/test-0.4-py2.py3-bogus-any.whl'), True),
# Test pep440 paths
(('test @ https://whatever.com/test-0.4-py2.py3-bogus-any.whl'), True),
# Test wheel
(('simple-0.1-py2.py3-none-any.whl'), False),
])
def test_looks_like_path(args, expected):
assert _looks_like_path(args) == expected


@pytest.mark.skipif(
not sys.platform.startswith("win"),
reason='Test only available on Windows'
)
@pytest.mark.parametrize('args, expected', [
# Test relative paths
(('.\\path\\to\\installable'), True),
(('relative\\path'), True),
# Test absolute paths
(('C:\\absolute\\path'), True),
])
def test_looks_like_path_win(args, expected):
assert _looks_like_path(args) == expected


@pytest.mark.parametrize('args, mock_returns, expected', [
# Test pep440 urls
(('/path/to/foo @ git+http://foo.com@ref#egg=foo',
'foo @ git+http://foo.com@ref#egg=foo'), (False, False), None),
# Test pep440 urls without spaces
(('/path/to/foo@git+http://foo.com@ref#egg=foo',
'foo @ git+http://foo.com@ref#egg=foo'), (False, False), None),
# Test pep440 wheel
(('/path/to/test @ https://whatever.com/test-0.4-py2.py3-bogus-any.whl',
'test @ https://whatever.com/test-0.4-py2.py3-bogus-any.whl'),
(False, False), None),
# Test name is not a file
(('/path/to/simple==0.1',
'simple==0.1'),
(False, False), None),
])
@patch('pip._internal.req.req_install.os.path.isdir')
@patch('pip._internal.req.req_install.os.path.isfile')
def test_get_url_from_path(
isdir_mock, isfile_mock, args, mock_returns, expected
):
isdir_mock.return_value = mock_returns[0]
isfile_mock.return_value = mock_returns[1]
assert _get_url_from_path(*args) is expected


@patch('pip._internal.req.req_install.os.path.isdir')
@patch('pip._internal.req.req_install.os.path.isfile')
def test_get_url_from_path__archive_file(isdir_mock, isfile_mock):
isdir_mock.return_value = False
isfile_mock.return_value = True
name = 'simple-0.1-py2.py3-none-any.whl'
path = os.path.join('/path/to/' + name)
url = path_to_url(path)
assert _get_url_from_path(path, name) == url


@patch('pip._internal.req.req_install.os.path.isdir')
@patch('pip._internal.req.req_install.os.path.isfile')
def test_get_url_from_path__installable_dir(isdir_mock, isfile_mock):
isdir_mock.return_value = True
isfile_mock.return_value = True
name = 'some/setuptools/project'
path = os.path.join('/path/to/' + name)
url = path_to_url(path)
assert _get_url_from_path(path, name) == url


@patch('pip._internal.req.req_install.os.path.isdir')
def test_get_url_from_path__installable_error(isdir_mock):
isdir_mock.return_value = True
name = 'some/setuptools/project'
path = os.path.join('/path/to/' + name)
with pytest.raises(InstallationError) as e:
_get_url_from_path(path, name)
err_msg = e.value.args[0]
assert "Neither 'setup.py' nor 'pyproject.toml' found" in err_msg
7 changes: 7 additions & 0 deletions tests/unit/test_req_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,13 @@ def test_yield_line_requirement(self):
req = install_req_from_line(line, comes_from=comes_from)
assert repr(list(process_line(line, filename, 1))[0]) == repr(req)

def test_yield_pep440_line_requirement(self):
line = 'SomeProject @ https://url/SomeProject-py2-py3-none-any.whl'
filename = 'filename'
comes_from = '-r %s (line %s)' % (filename, 1)
req = install_req_from_line(line, comes_from=comes_from)
assert repr(list(process_line(line, filename, 1))[0]) == repr(req)

def test_yield_line_constraint(self):
line = 'SomeProject'
filename = 'filename'
Expand Down