Skip to content

Allow non lowercase extras #4901

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

Closed
Closed
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
1 change: 1 addition & 0 deletions news/4617.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix bug that prevents pip from installing extras with names containing upper-case letters.
25 changes: 16 additions & 9 deletions src/pip/_internal/req/req_install.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

from pip._vendor import pkg_resources, six
from pip._vendor.packaging import specifiers
from pip._vendor.packaging.markers import Marker
from pip._vendor.packaging.markers import Marker, UndefinedEnvironmentName
from pip._vendor.packaging.requirements import InvalidRequirement, Requirement
from pip._vendor.packaging.utils import canonicalize_name
from pip._vendor.packaging.version import parse as parse_version
Expand Down Expand Up @@ -712,15 +712,22 @@ def _clean_zip_name(self, name, prefix):
name = name.replace(os.path.sep, '/')
return name

def match_markers(self, extras_requested=None):
if not extras_requested:
# Provide an extra to safely evaluate the markers
# without matching any extra
extras_requested = ('',)
def match_markers(self):

if self.markers is not None:
return any(
self.markers.evaluate({'extra': extra})
for extra in extras_requested)

# In certain situations,
# the parsing and evaluation of extra markers is unreliable.
# By the time this code is executed,
# the existence of necessary extras
# has already been reliably determined.
# try/except clause forces extra marker to evaluate as True,
# avoiding incorrect and unnecessary evaluations.

try:
return self.markers.evaluate()
except UndefinedEnvironmentName:
return True
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really make sense to me. It seems to just be hiding an error rather than fixing it. The comment seems to be trying to justify doing so, but "in certain situations" doesn't exactly sound like we know what's going on here :-(

else:
return True

Expand Down
7 changes: 2 additions & 5 deletions src/pip/_internal/req/req_set.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ def __repr__(self):
return ('<%s object; %d requirement(s): %s>'
% (self.__class__.__name__, len(reqs), reqs_str))

def add_requirement(self, install_req, parent_req_name=None,
extras_requested=None):
def add_requirement(self, install_req, parent_req_name=None):
"""Add install_req as a requirement to install.

:param parent_req_name: The name of the requirement that needed this
Expand All @@ -58,14 +57,12 @@ def add_requirement(self, install_req, parent_req_name=None,
links that point outside the Requirements set. parent_req must
already be added. Note that None implies that this is a user
supplied requirement, vs an inferred one.
:param extras_requested: an iterable of extras used to evaluate the
environement markers.
:return: Additional requirements to scan. That is either [] if
the requirement is not applicable, or [install_req] if the
requirement is applicable and has just been added.
"""
name = install_req.name
if not install_req.match_markers(extras_requested):
if not install_req.match_markers():
logger.warning("Ignoring %s: markers '%s' don't match your "
"environment", install_req.name,
install_req.markers)
Expand Down
7 changes: 3 additions & 4 deletions src/pip/_internal/resolve.py
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ def _resolve_one(self, requirement_set, req_to_install):

more_reqs = []

def add_req(subreq, extras_requested):
def add_req(subreq):
sub_install_req = InstallRequirement.from_req(
str(subreq),
req_to_install,
Expand All @@ -271,7 +271,6 @@ def add_req(subreq, extras_requested):
more_reqs.extend(
requirement_set.add_requirement(
sub_install_req, req_to_install.name,
extras_requested=extras_requested
)
)

Expand All @@ -280,7 +279,7 @@ def add_req(subreq, extras_requested):
# can refer to it when adding dependencies.
if not requirement_set.has_requirement(req_to_install.name):
# 'unnamed' requirements will get added here
requirement_set.add_requirement(req_to_install, None)
requirement_set.add_requirement(req_to_install)

if not self.ignore_dependencies:
if req_to_install.extras:
Expand All @@ -301,7 +300,7 @@ def add_req(subreq, extras_requested):
set(dist.extras) & set(req_to_install.extras)
)
for subreq in dist.requires(available_requested):
add_req(subreq, extras_requested=available_requested)
add_req(subreq)

if not req_to_install.editable and not req_to_install.satisfied_by:
# XXX: --no-install leads this to report 'Successfully
Expand Down
20 changes: 20 additions & 0 deletions tests/functional/test_install_extras.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import sys
import textwrap
from os.path import join

Expand Down Expand Up @@ -126,3 +127,22 @@ def test_install_special_extra(script):
assert (
"Could not find a version that satisfies the requirement missing_pkg"
) in result.stderr, str(result)


@pytest.mark.skipif(sys.version_info < (3, 0),
Copy link
Member

Choose a reason for hiding this comment

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

Why is this test dependant on Py3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

django-rest-swagger[reST]==0.3.10 requires python3. There's probably a better way to test this problem, but this works.

Copy link
Member

Choose a reason for hiding this comment

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

I don't really like having a test that:

  1. Depends on an external package that "happens" to have this issue (I describe it like that because we don't appear to have established precisely what causes the issue).
  2. Can only be tested on Python 3, even though the problem can happen on Python 2.
  3. May stop working if the external package changes.

I'd rather we tested this using an artificially-constructed example that we ship with the test suite, so we can control it better.

reason="requires Python3")
@pytest.mark.network
def test_extras_with_mixed_case_name(script):
"""
Test installing a package
"""
extra_name = 'reST'
dependency_with_extra = 'django-rest-swagger[{}]==0.3.10'.format(
extra_name
)
result = script.pip(
'install', dependency_with_extra, expect_stderr=True,
)
template = """markers 'extra == "{}"' don't match your environment"""
error_message = template.format(extra_name)
assert error_message not in result.stderr