diff --git a/news/11649.bugfix.rst b/news/11649.bugfix.rst new file mode 100644 index 00000000000..65511711f59 --- /dev/null +++ b/news/11649.bugfix.rst @@ -0,0 +1,5 @@ +Normalize extras according to :pep:`685` from package metadata in the resolver +for comparison. This ensures extras are correctly compared and merged as long +as the package providing the extra(s) is built with values normalized according +to the standard. Note, however, that this *does not* solve cases where the +package itself contains unnormalized extra values in the metadata. diff --git a/src/pip/_internal/metadata/__init__.py b/src/pip/_internal/metadata/__init__.py index 9f73ca7105f..aa232b6cabd 100644 --- a/src/pip/_internal/metadata/__init__.py +++ b/src/pip/_internal/metadata/__init__.py @@ -9,7 +9,7 @@ from .base import BaseDistribution, BaseEnvironment, FilesystemWheel, MemoryWheel, Wheel if TYPE_CHECKING: - from typing import Protocol + from typing import Literal, Protocol else: Protocol = object @@ -50,6 +50,7 @@ def _should_use_importlib_metadata() -> bool: class Backend(Protocol): + NAME: 'Literal["importlib", "pkg_resources"]' Distribution: Type[BaseDistribution] Environment: Type[BaseEnvironment] diff --git a/src/pip/_internal/metadata/base.py b/src/pip/_internal/metadata/base.py index cafb79fb3dc..92491244108 100644 --- a/src/pip/_internal/metadata/base.py +++ b/src/pip/_internal/metadata/base.py @@ -24,7 +24,7 @@ from pip._vendor.packaging.requirements import Requirement from pip._vendor.packaging.specifiers import InvalidSpecifier, SpecifierSet -from pip._vendor.packaging.utils import NormalizedName +from pip._vendor.packaging.utils import NormalizedName, canonicalize_name from pip._vendor.packaging.version import LegacyVersion, Version from pip._internal.exceptions import NoneMetadataError @@ -37,7 +37,6 @@ from pip._internal.utils.compat import stdlib_pkgs # TODO: Move definition here. from pip._internal.utils.egg_link import egg_link_path_from_sys_path from pip._internal.utils.misc import is_local, normalize_path -from pip._internal.utils.packaging import safe_extra from pip._internal.utils.urls import url_to_path from ._json import msg_to_json @@ -460,6 +459,19 @@ def iter_provided_extras(self) -> Iterable[str]: For modern .dist-info distributions, this is the collection of "Provides-Extra:" entries in distribution metadata. + + The return value of this function is not particularly useful other than + display purposes due to backward compatibility issues and the extra + names being poorly normalized prior to PEP 685. If you want to perform + logic operations on extras, use :func:`is_extra_provided` instead. + """ + raise NotImplementedError() + + def is_extra_provided(self, extra: str) -> bool: + """Check whether an extra is provided by this distribution. + + This is needed mostly for compatibility issues with pkg_resources not + following the extra normalization rules defined in PEP 685. """ raise NotImplementedError() @@ -537,10 +549,11 @@ def _iter_egg_info_extras(self) -> Iterable[str]: """Get extras from the egg-info directory.""" known_extras = {""} for entry in self._iter_requires_txt_entries(): - if entry.extra in known_extras: + extra = canonicalize_name(entry.extra) + if extra in known_extras: continue - known_extras.add(entry.extra) - yield entry.extra + known_extras.add(extra) + yield extra def _iter_egg_info_dependencies(self) -> Iterable[str]: """Get distribution dependencies from the egg-info directory. @@ -556,10 +569,11 @@ def _iter_egg_info_dependencies(self) -> Iterable[str]: all currently available PEP 517 backends, although not standardized. """ for entry in self._iter_requires_txt_entries(): - if entry.extra and entry.marker: - marker = f'({entry.marker}) and extra == "{safe_extra(entry.extra)}"' - elif entry.extra: - marker = f'extra == "{safe_extra(entry.extra)}"' + extra = canonicalize_name(entry.extra) + if extra and entry.marker: + marker = f'({entry.marker}) and extra == "{extra}"' + elif extra: + marker = f'extra == "{extra}"' elif entry.marker: marker = entry.marker else: diff --git a/src/pip/_internal/metadata/importlib/__init__.py b/src/pip/_internal/metadata/importlib/__init__.py index 5e7af9fe521..a779138db10 100644 --- a/src/pip/_internal/metadata/importlib/__init__.py +++ b/src/pip/_internal/metadata/importlib/__init__.py @@ -1,4 +1,6 @@ from ._dists import Distribution from ._envs import Environment -__all__ = ["Distribution", "Environment"] +__all__ = ["NAME", "Distribution", "Environment"] + +NAME = "importlib" diff --git a/src/pip/_internal/metadata/importlib/_dists.py b/src/pip/_internal/metadata/importlib/_dists.py index 65c043c87ef..26370facf28 100644 --- a/src/pip/_internal/metadata/importlib/_dists.py +++ b/src/pip/_internal/metadata/importlib/_dists.py @@ -27,7 +27,6 @@ Wheel, ) from pip._internal.utils.misc import normalize_path -from pip._internal.utils.packaging import safe_extra from pip._internal.utils.temp_dir import TempDirectory from pip._internal.utils.wheel import parse_wheel, read_wheel_metadata_file @@ -208,12 +207,16 @@ def _metadata_impl(self) -> email.message.Message: return cast(email.message.Message, self._dist.metadata) def iter_provided_extras(self) -> Iterable[str]: - return ( - safe_extra(extra) for extra in self.metadata.get_all("Provides-Extra", []) + return self.metadata.get_all("Provides-Extra", []) + + def is_extra_provided(self, extra: str) -> bool: + return any( + canonicalize_name(provided_extra) == canonicalize_name(extra) + for provided_extra in self.metadata.get_all("Provides-Extra", []) ) def iter_dependencies(self, extras: Collection[str] = ()) -> Iterable[Requirement]: - contexts: Sequence[Dict[str, str]] = [{"extra": safe_extra(e)} for e in extras] + contexts: Sequence[Dict[str, str]] = [{"extra": e} for e in extras] for req_string in self.metadata.get_all("Requires-Dist", []): req = Requirement(req_string) if not req.marker: diff --git a/src/pip/_internal/metadata/pkg_resources.py b/src/pip/_internal/metadata/pkg_resources.py index f330ef12a2c..bb11e5bd8a5 100644 --- a/src/pip/_internal/metadata/pkg_resources.py +++ b/src/pip/_internal/metadata/pkg_resources.py @@ -24,8 +24,12 @@ Wheel, ) +__all__ = ["NAME", "Distribution", "Environment"] + logger = logging.getLogger(__name__) +NAME = "pkg_resources" + class EntryPoint(NamedTuple): name: str @@ -212,12 +216,16 @@ def _metadata_impl(self) -> email.message.Message: def iter_dependencies(self, extras: Collection[str] = ()) -> Iterable[Requirement]: if extras: # pkg_resources raises on invalid extras, so we sanitize. - extras = frozenset(extras).intersection(self._dist.extras) + extras = frozenset(pkg_resources.safe_extra(e) for e in extras) + extras = extras.intersection(self._dist.extras) return self._dist.requires(extras) def iter_provided_extras(self) -> Iterable[str]: return self._dist.extras + def is_extra_provided(self, extra: str) -> bool: + return pkg_resources.safe_extra(extra) in self._dist.extras + class Environment(BaseEnvironment): def __init__(self, ws: pkg_resources.WorkingSet) -> None: diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index 8110114ca14..f8957e5d994 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -128,7 +128,7 @@ def __init__( if extras: self.extras = extras elif req: - self.extras = {safe_extra(extra) for extra in req.extras} + self.extras = req.extras else: self.extras = set() if markers is None and req: @@ -272,7 +272,12 @@ def match_markers(self, extras_requested: Optional[Iterable[str]] = None) -> boo extras_requested = ("",) if self.markers is not None: return any( - self.markers.evaluate({"extra": extra}) for extra in extras_requested + self.markers.evaluate({"extra": extra}) + # TODO: Remove these two variants when packaging is upgraded to + # support the marker comparison logic specified in PEP 685. + or self.markers.evaluate({"extra": safe_extra(extra)}) + or self.markers.evaluate({"extra": canonicalize_name(extra)}) + for extra in extras_requested ) else: return True diff --git a/src/pip/_internal/resolution/resolvelib/base.py b/src/pip/_internal/resolution/resolvelib/base.py index b206692a0a9..9c0ef5ca7b9 100644 --- a/src/pip/_internal/resolution/resolvelib/base.py +++ b/src/pip/_internal/resolution/resolvelib/base.py @@ -1,7 +1,7 @@ from typing import FrozenSet, Iterable, Optional, Tuple, Union from pip._vendor.packaging.specifiers import SpecifierSet -from pip._vendor.packaging.utils import NormalizedName, canonicalize_name +from pip._vendor.packaging.utils import NormalizedName from pip._vendor.packaging.version import LegacyVersion, Version from pip._internal.models.link import Link, links_equivalent @@ -12,11 +12,11 @@ CandidateVersion = Union[LegacyVersion, Version] -def format_name(project: str, extras: FrozenSet[str]) -> str: +def format_name(project: NormalizedName, extras: FrozenSet[NormalizedName]) -> str: if not extras: return project - canonical_extras = sorted(canonicalize_name(e) for e in extras) - return "{}[{}]".format(project, ",".join(canonical_extras)) + extras_expr = ",".join(sorted(extras)) + return f"{project}[{extras_expr}]" class Constraint: diff --git a/src/pip/_internal/resolution/resolvelib/candidates.py b/src/pip/_internal/resolution/resolvelib/candidates.py index de04e1d73f2..67737a5092f 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -429,7 +429,15 @@ def __init__( extras: FrozenSet[str], ) -> None: self.base = base - self.extras = extras + self.extras = frozenset(canonicalize_name(e) for e in extras) + # If any extras are requested in their non-normalized forms, keep track + # of their raw values. This is needed when we look up dependencies + # since PEP 685 has not been implemented for marker-matching, and using + # the non-normalized extra for lookup ensures the user can select a + # non-normalized extra in a package with its non-normalized form. + # TODO: Remove this attribute when packaging is upgraded to support the + # marker comparison logic specified in PEP 685. + self._unnormalized_extras = extras.difference(self.extras) def __str__(self) -> str: name, rest = str(self.base).split(" ", 1) @@ -480,6 +488,50 @@ def is_editable(self) -> bool: def source_link(self) -> Optional[Link]: return self.base.source_link + def _warn_invalid_extras( + self, + requested: FrozenSet[str], + valid: FrozenSet[str], + ) -> None: + """Emit warnings for invalid extras being requested. + + This emits a warning for each requested extra that is not in the + candidate's ``Provides-Extra`` list. + """ + invalid_extras_to_warn = frozenset( + extra + for extra in requested + if extra not in valid + # If an extra is requested in an unnormalized form, skip warning + # about the normalized form being missing. + and extra in self.extras + ) + if not invalid_extras_to_warn: + return + for extra in sorted(invalid_extras_to_warn): + logger.warning( + "%s %s does not provide the extra '%s'", + self.base.name, + self.version, + extra, + ) + + def _calculate_valid_requested_extras(self) -> FrozenSet[str]: + """Get a list of valid extras requested by this candidate. + + The user (or upstream dependant) may have specified extras that the + candidate doesn't support. Any unsupported extras are dropped, and each + cause a warning to be logged here. + """ + requested_extras = self.extras.union(self._unnormalized_extras) + valid_extras = frozenset( + extra + for extra in requested_extras + if self.base.dist.is_extra_provided(extra) + ) + self._warn_invalid_extras(requested_extras, valid_extras) + return valid_extras + def iter_dependencies(self, with_requires: bool) -> Iterable[Optional[Requirement]]: factory = self.base._factory @@ -489,18 +541,7 @@ def iter_dependencies(self, with_requires: bool) -> Iterable[Optional[Requiremen if not with_requires: return - # The user may have specified extras that the candidate doesn't - # support. We ignore any unsupported extras here. - valid_extras = self.extras.intersection(self.base.dist.iter_provided_extras()) - invalid_extras = self.extras.difference(self.base.dist.iter_provided_extras()) - for extra in sorted(invalid_extras): - logger.warning( - "%s %s does not provide the extra '%s'", - self.base.name, - self.version, - extra, - ) - + valid_extras = self._calculate_valid_requested_extras() for r in self.base.dist.iter_dependencies(valid_extras): requirement = factory.make_requirement_from_spec( str(r), self.base._ireq, valid_extras diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 2eb80d4d552..5ae80a6fc43 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -112,7 +112,7 @@ def __init__( self._editable_candidate_cache: Cache[EditableCandidate] = {} self._installed_candidate_cache: Dict[str, AlreadyInstalledCandidate] = {} self._extras_candidate_cache: Dict[ - Tuple[int, FrozenSet[str]], ExtrasCandidate + Tuple[int, FrozenSet[NormalizedName]], ExtrasCandidate ] = {} if not ignore_installed: @@ -138,9 +138,11 @@ def _fail_if_link_is_unsupported_wheel(self, link: Link) -> None: raise UnsupportedWheel(msg) def _make_extras_candidate( - self, base: BaseCandidate, extras: FrozenSet[str] + self, + base: BaseCandidate, + extras: FrozenSet[str], ) -> ExtrasCandidate: - cache_key = (id(base), extras) + cache_key = (id(base), frozenset(canonicalize_name(e) for e in extras)) try: candidate = self._extras_candidate_cache[cache_key] except KeyError: diff --git a/src/pip/_internal/resolution/resolvelib/requirements.py b/src/pip/_internal/resolution/resolvelib/requirements.py index 06addc0ddce..7d244c6937a 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -43,7 +43,7 @@ class SpecifierRequirement(Requirement): def __init__(self, ireq: InstallRequirement) -> None: assert ireq.link is None, "This is a link, not a specifier" self._ireq = ireq - self._extras = frozenset(ireq.extras) + self._extras = frozenset(canonicalize_name(e) for e in ireq.extras) def __str__(self) -> str: return str(self._ireq.req) diff --git a/tests/functional/test_install_extras.py b/tests/functional/test_install_extras.py index db4a811e0fd..8ccbcf1998d 100644 --- a/tests/functional/test_install_extras.py +++ b/tests/functional/test_install_extras.py @@ -4,7 +4,12 @@ import pytest -from tests.lib import PipTestEnvironment, ResolverVariant, TestData +from tests.lib import ( + PipTestEnvironment, + ResolverVariant, + TestData, + create_basic_wheel_for_package, +) @pytest.mark.network @@ -150,29 +155,42 @@ def test_install_fails_if_extra_at_end( assert "Extras after version" in result.stderr -@pytest.mark.skipif( - "sys.version_info >= (3, 11)", - reason="Setuptools incompatibility with importlib.metadata; see GH-12267", +@pytest.mark.parametrize( + "specified_extra, requested_extra", + [ + ("Hop_hOp-hoP", "Hop_hOp-hoP"), + pytest.param( + "Hop_hOp-hoP", + "hop-hop-hop", + marks=pytest.mark.xfail( + reason=( + "matching a normalized extra request against an" + "unnormalized extra in metadata requires PEP 685 support " + "in packaging (see pypa/pip#11445)." + ), + ), + ), + ("hop-hop-hop", "Hop_hOp-hoP"), + ], ) -def test_install_special_extra(script: PipTestEnvironment) -> None: - # Check that uppercase letters and '-' are dealt with - # make a dummy project - pkga_path = script.scratch_path / "pkga" - pkga_path.mkdir() - pkga_path.joinpath("setup.py").write_text( - textwrap.dedent( - """ - from setuptools import setup - setup(name='pkga', - version='0.1', - extras_require={'Hop_hOp-hoP': ['missing_pkg']}, - ) - """ - ) +def test_install_special_extra( + script: PipTestEnvironment, + specified_extra: str, + requested_extra: str, +) -> None: + """Check extra normalization is implemented according to specification.""" + pkga_path = create_basic_wheel_for_package( + script, + name="pkga", + version="0.1", + extras={specified_extra: ["missing_pkg"]}, ) result = script.pip( - "install", "--no-index", f"{pkga_path}[Hop_hOp-hoP]", expect_error=True + "install", + "--no-index", + f"pkga[{requested_extra}] @ {pkga_path.as_uri()}", + expect_error=True, ) assert ( "Could not find a version that satisfies the requirement missing_pkg" @@ -227,3 +245,20 @@ def test_install_extra_merging( if not fails_on_legacy or resolver_variant == "2020-resolver": expected = f"Successfully installed pkga-0.1 simple-{simple_version}" assert expected in result.stdout + + +def test_install_extras(script: PipTestEnvironment) -> None: + create_basic_wheel_for_package(script, "a", "1", depends=["b", "dep[x-y]"]) + create_basic_wheel_for_package(script, "b", "1", depends=["dep[x_y]"]) + create_basic_wheel_for_package(script, "dep", "1", extras={"x-y": ["meh"]}) + create_basic_wheel_for_package(script, "meh", "1") + + script.pip( + "install", + "--no-cache-dir", + "--no-index", + "--find-links", + script.scratch_path, + "a", + ) + script.assert_installed(a="1", b="1", dep="1", meh="1")