From 98aa8b5298340dd57b1e02d24943410491a261b9 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 22 Nov 2022 15:00:04 -0500 Subject: [PATCH 01/10] Restrict `#egg=` fragments to valid PEP 508 names This should help reduce user confusion about what can go in a URI's egg fragment. Fixes #11567. Signed-off-by: William Woodruff --- src/pip/_internal/exceptions.py | 8 ++++++++ src/pip/_internal/models/link.py | 16 +++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/exceptions.py b/src/pip/_internal/exceptions.py index 2ab1f591f12..ac4057733e1 100644 --- a/src/pip/_internal/exceptions.py +++ b/src/pip/_internal/exceptions.py @@ -658,3 +658,11 @@ def __str__(self) -> str: assert self.error is not None message_part = f".\n{self.error}\n" return f"Configuration file {self.reason}{message_part}" + + +class InvalidEggFragment(InstallationError): + """A link's `#egg=` fragment doesn't look like a valid PEP 508 project + name.""" + + def __init__(self, fragment: str) -> None: + super().__init__(f"egg fragment is not a bare project name: {fragment}") diff --git a/src/pip/_internal/models/link.py b/src/pip/_internal/models/link.py index c792d128bcf..27001b2bbc6 100644 --- a/src/pip/_internal/models/link.py +++ b/src/pip/_internal/models/link.py @@ -18,6 +18,7 @@ Union, ) +from pip._internal.exceptions import InvalidEggFragment from pip._internal.utils.filetypes import WHEEL_EXTENSION from pip._internal.utils.hashes import Hashes from pip._internal.utils.misc import ( @@ -358,12 +359,25 @@ def url_without_fragment(self) -> str: _egg_fragment_re = re.compile(r"[#&]egg=([^&]*)") + # Per PEP 508. + _project_name_re = re.compile( + r"^([A-Z0-9]|[A-Z0-9][A-Z0-9._-]*[A-Z0-9])$", re.IGNORECASE + ) + @property def egg_fragment(self) -> Optional[str]: match = self._egg_fragment_re.search(self._url) if not match: return None - return match.group(1) + + # The egg fragment must look like a project name, and only + # a project name. In particular, it can't contain version constraints + # or anything else like that. + project_name = match.group(1) + if not self._project_name_re.match(project_name): + raise InvalidEggFragment(project_name) + + return project_name _subdirectory_fragment_re = re.compile(r"[#&]subdirectory=([^&]*)") From 4af0984cc31bb62f207d3b1ce220d58b38010af4 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 22 Nov 2022 15:50:45 -0500 Subject: [PATCH 02/10] models/link: reuse pyparsing + requirements combinators for egg fragment This should now be consistent with existing tests (without establishing that those tests are actually well-specified). Signed-off-by: William Woodruff --- src/pip/_internal/models/link.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/pip/_internal/models/link.py b/src/pip/_internal/models/link.py index 27001b2bbc6..3df2d0b5f4c 100644 --- a/src/pip/_internal/models/link.py +++ b/src/pip/_internal/models/link.py @@ -18,6 +18,10 @@ Union, ) +from pip._vendor.packaging.requirements import EXTRAS, NAME +from pip._vendor.pyparsing import Optional as Maybe +from pip._vendor.pyparsing import ParseException, stringEnd, stringStart + from pip._internal.exceptions import InvalidEggFragment from pip._internal.utils.filetypes import WHEEL_EXTENSION from pip._internal.utils.hashes import Hashes @@ -359,10 +363,7 @@ def url_without_fragment(self) -> str: _egg_fragment_re = re.compile(r"[#&]egg=([^&]*)") - # Per PEP 508. - _project_name_re = re.compile( - r"^([A-Z0-9]|[A-Z0-9][A-Z0-9._-]*[A-Z0-9])$", re.IGNORECASE - ) + _fragment_parser = stringStart + NAME + Maybe(EXTRAS) + stringEnd @property def egg_fragment(self) -> Optional[str]: @@ -370,11 +371,12 @@ def egg_fragment(self) -> Optional[str]: if not match: return None - # The egg fragment must look like a project name, and only - # a project name. In particular, it can't contain version constraints - # or anything else like that. + # An egg fragment looks like a PEP 508 project name, along with + # an optional extras specifier. Anything else is invalid. project_name = match.group(1) - if not self._project_name_re.match(project_name): + try: + self._fragment_parser.parseString(project_name) + except ParseException: raise InvalidEggFragment(project_name) return project_name From e26712616e774f5a8d4a732be1fcd281042d414a Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 22 Nov 2022 17:04:23 -0500 Subject: [PATCH 03/10] topics/vcs-support: clarify the egg fragment's syntax This doesn't actually address the semantics of extras in the egg fragment. Signed-off-by: William Woodruff --- docs/html/topics/vcs-support.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/html/topics/vcs-support.md b/docs/html/topics/vcs-support.md index 70bb5beb9dc..af1ebffcff3 100644 --- a/docs/html/topics/vcs-support.md +++ b/docs/html/topics/vcs-support.md @@ -139,9 +139,16 @@ option. pip looks at 2 fragments for VCS URLs: - `egg`: For specifying the "project name" for use in pip's dependency - resolution logic. eg: `egg=project_name` + resolution logic. e.g.: `egg=project_name` + + The `egg` fragment may additionally contain an extras specifier, e.g.: + `egg=project_name[dev,test]`. + + Both the project name and extras specifier must appear in the form + defined by [PEP 508](https://peps.python.org/pep-0508/). + - `subdirectory`: For specifying the path to the Python package, when it is not - in the root of the VCS directory. eg: `pkg_dir` + in the root of the VCS directory. e.g.: `pkg_dir` ````{admonition} Example If your repository layout is: From 3c337a98cc35127c6c47c014e8bd6b737695cebe Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 22 Nov 2022 17:26:40 -0500 Subject: [PATCH 04/10] models/link: make egg fragment evaluation eager This should prevent us from accepting malformed egg fragments that are shadowed by other parts of the requirement specifier. Signed-off-by: William Woodruff --- src/pip/_internal/models/link.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/models/link.py b/src/pip/_internal/models/link.py index 3df2d0b5f4c..fd421dc4208 100644 --- a/src/pip/_internal/models/link.py +++ b/src/pip/_internal/models/link.py @@ -171,6 +171,7 @@ class Link(KeyBasedCompareMixin): "dist_info_metadata", "link_hash", "cache_link_parsing", + "egg_fragment", ] def __init__( @@ -234,6 +235,7 @@ def __init__( super().__init__(key=url, defining_class=Link) self.cache_link_parsing = cache_link_parsing + self.egg_fragment = self._egg_fragment() @classmethod def from_json( @@ -365,8 +367,7 @@ def url_without_fragment(self) -> str: _fragment_parser = stringStart + NAME + Maybe(EXTRAS) + stringEnd - @property - def egg_fragment(self) -> Optional[str]: + def _egg_fragment(self) -> Optional[str]: match = self._egg_fragment_re.search(self._url) if not match: return None From 227fbee124dd2b099b0cdcfbbca661c1928865c2 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 22 Nov 2022 17:40:55 -0500 Subject: [PATCH 05/10] tests: add more Link tests This exercises our expectation that egg fragments don't include version specifiers and are evaluated eagerly. Signed-off-by: William Woodruff --- tests/unit/test_link.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/unit/test_link.py b/tests/unit/test_link.py index 99ed0aba76e..f5c9e84ef3b 100644 --- a/tests/unit/test_link.py +++ b/tests/unit/test_link.py @@ -2,6 +2,7 @@ import pytest +from pip._internal.exceptions import InvalidEggFragment from pip._internal.models.link import Link, links_equivalent from pip._internal.utils.hashes import Hashes @@ -80,6 +81,35 @@ def test_fragments(self) -> None: assert "eggname" == Link(url).egg_fragment assert "subdir" == Link(url).subdirectory_fragment + # Extras are supported and preserved in the egg fragment, + # even the empty extras specifier (since PEP 508 allows it). + url = "git+https://example.com/package#egg=eggname[extra]" + assert "eggname[extra]" == Link(url).egg_fragment + assert None is Link(url).subdirectory_fragment + url = "git+https://example.com/package#egg=eggname[extra1,extra2]" + assert "eggname[extra1,extra2]" == Link(url).egg_fragment + assert None is Link(url).subdirectory_fragment + url = "git+https://example.com/package#egg=eggname[]" + assert "eggname[]" == Link(url).egg_fragment + assert None is Link(url).subdirectory_fragment + + @pytest.mark.parametrize( + "fragment", + [ + # Package names in egg fragments must be in PEP 508 form. + "~invalid~package~name~", + # Version specifiers are not valid in egg fragments. + "eggname==1.2.3", + "eggname>=1.2.3", + # The extras specifier must be in PEP 508 form. + "eggname[!]", + ], + ) + def test_invalid_egg_fragments(self, fragment: str) -> None: + url = f"git+https://example.com/package#egg={fragment}" + with pytest.raises(InvalidEggFragment): + Link(url) + @pytest.mark.parametrize( "yanked_reason, expected", [ From 3b9abbc9872381ffe2c79f0fc0fe5f5b6b06d61a Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 2 Dec 2022 21:12:47 -0800 Subject: [PATCH 06/10] topics/vcs-support: re-qualify egg fragment behavior Signed-off-by: William Woodruff --- docs/html/topics/vcs-support.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/docs/html/topics/vcs-support.md b/docs/html/topics/vcs-support.md index af1ebffcff3..d108f4d825d 100644 --- a/docs/html/topics/vcs-support.md +++ b/docs/html/topics/vcs-support.md @@ -141,11 +141,9 @@ pip looks at 2 fragments for VCS URLs: - `egg`: For specifying the "project name" for use in pip's dependency resolution logic. e.g.: `egg=project_name` - The `egg` fragment may additionally contain an extras specifier, e.g.: - `egg=project_name[dev,test]`. - - Both the project name and extras specifier must appear in the form - defined by [PEP 508](https://peps.python.org/pep-0508/). + The `egg` fragment **should** be a bare + [PEP 508](https://peps.python.org/pep-0508/) project name. Anything else + is not guaranteed to work. - `subdirectory`: For specifying the path to the Python package, when it is not in the root of the VCS directory. e.g.: `pkg_dir` From 464d16618e2929cd6e274ee98348ebcf395d98f9 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 2 Dec 2022 21:33:22 -0800 Subject: [PATCH 07/10] src, tests: use deprecation instead of hard error for egg fragments This turns invalid egg fragments into a soft error, with a scheduled deprecation period of two releases. Signed-off-by: William Woodruff --- src/pip/_internal/exceptions.py | 8 -------- src/pip/_internal/models/link.py | 22 ++++++++++++---------- tests/unit/test_link.py | 4 ++-- 3 files changed, 14 insertions(+), 20 deletions(-) diff --git a/src/pip/_internal/exceptions.py b/src/pip/_internal/exceptions.py index ac4057733e1..2ab1f591f12 100644 --- a/src/pip/_internal/exceptions.py +++ b/src/pip/_internal/exceptions.py @@ -658,11 +658,3 @@ def __str__(self) -> str: assert self.error is not None message_part = f".\n{self.error}\n" return f"Configuration file {self.reason}{message_part}" - - -class InvalidEggFragment(InstallationError): - """A link's `#egg=` fragment doesn't look like a valid PEP 508 project - name.""" - - def __init__(self, fragment: str) -> None: - super().__init__(f"egg fragment is not a bare project name: {fragment}") diff --git a/src/pip/_internal/models/link.py b/src/pip/_internal/models/link.py index fd421dc4208..c7c4b0e9b25 100644 --- a/src/pip/_internal/models/link.py +++ b/src/pip/_internal/models/link.py @@ -18,11 +18,7 @@ Union, ) -from pip._vendor.packaging.requirements import EXTRAS, NAME -from pip._vendor.pyparsing import Optional as Maybe -from pip._vendor.pyparsing import ParseException, stringEnd, stringStart - -from pip._internal.exceptions import InvalidEggFragment +from pip._internal.utils.deprecation import deprecated from pip._internal.utils.filetypes import WHEEL_EXTENSION from pip._internal.utils.hashes import Hashes from pip._internal.utils.misc import ( @@ -365,7 +361,10 @@ def url_without_fragment(self) -> str: _egg_fragment_re = re.compile(r"[#&]egg=([^&]*)") - _fragment_parser = stringStart + NAME + Maybe(EXTRAS) + stringEnd + # Per PEP 508. + _project_name_re = re.compile( + r"^([A-Z0-9]|[A-Z0-9][A-Z0-9._-]*[A-Z0-9])$", re.IGNORECASE + ) def _egg_fragment(self) -> Optional[str]: match = self._egg_fragment_re.search(self._url) @@ -375,10 +374,13 @@ def _egg_fragment(self) -> Optional[str]: # An egg fragment looks like a PEP 508 project name, along with # an optional extras specifier. Anything else is invalid. project_name = match.group(1) - try: - self._fragment_parser.parseString(project_name) - except ParseException: - raise InvalidEggFragment(project_name) + if not self._project_name_re.match(project_name): + deprecated( + reason=f"{self} contains an egg fragment with a non-PEP 508 name", + replacement="to use the req @ url syntax, and remove the egg fragment", + gone_in="25.0", + issue=11617, + ) return project_name diff --git a/tests/unit/test_link.py b/tests/unit/test_link.py index f5c9e84ef3b..0db48194612 100644 --- a/tests/unit/test_link.py +++ b/tests/unit/test_link.py @@ -2,7 +2,6 @@ import pytest -from pip._internal.exceptions import InvalidEggFragment from pip._internal.models.link import Link, links_equivalent from pip._internal.utils.hashes import Hashes @@ -93,6 +92,7 @@ def test_fragments(self) -> None: assert "eggname[]" == Link(url).egg_fragment assert None is Link(url).subdirectory_fragment + @pytest.mark.xfail(reason="Behavior change scheduled for 25.0", strict=True) @pytest.mark.parametrize( "fragment", [ @@ -107,7 +107,7 @@ def test_fragments(self) -> None: ) def test_invalid_egg_fragments(self, fragment: str) -> None: url = f"git+https://example.com/package#egg={fragment}" - with pytest.raises(InvalidEggFragment): + with pytest.raises(Exception): Link(url) @pytest.mark.parametrize( From d9502ff5013d124f7af7d487cae2332b4a462c91 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 2 Dec 2022 21:38:51 -0800 Subject: [PATCH 08/10] tests: fix comment Signed-off-by: William Woodruff --- tests/unit/test_link.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_link.py b/tests/unit/test_link.py index 0db48194612..df4957d5974 100644 --- a/tests/unit/test_link.py +++ b/tests/unit/test_link.py @@ -81,7 +81,8 @@ def test_fragments(self) -> None: assert "subdir" == Link(url).subdirectory_fragment # Extras are supported and preserved in the egg fragment, - # even the empty extras specifier (since PEP 508 allows it). + # even the empty extras specifier. + # This behavior is deprecated and will change in pip 25. url = "git+https://example.com/package#egg=eggname[extra]" assert "eggname[extra]" == Link(url).egg_fragment assert None is Link(url).subdirectory_fragment From 63097482bb8a64cf69fdbd230082999815088343 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 2 Dec 2022 21:41:12 -0800 Subject: [PATCH 09/10] news: add entry Signed-off-by: William Woodruff --- news/11617.bugfix.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 news/11617.bugfix.rst diff --git a/news/11617.bugfix.rst b/news/11617.bugfix.rst new file mode 100644 index 00000000000..02346e49c42 --- /dev/null +++ b/news/11617.bugfix.rst @@ -0,0 +1,3 @@ +Deprecated a historical ambiguity in how ``egg`` fragments in URL-style +requirements are formatted and handled. ``egg`` fragments that do not look +like PEP 508 names now produce a deprecation warning. From 76cff147236d76c3c9560d59311ac8ebf0ed29f3 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sat, 3 Dec 2022 11:28:11 -0800 Subject: [PATCH 10/10] news: recategorize entry Signed-off-by: William Woodruff --- news/{10265.bugfix.rst => 10265.removal.rst} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename news/{10265.bugfix.rst => 10265.removal.rst} (100%) diff --git a/news/10265.bugfix.rst b/news/10265.removal.rst similarity index 100% rename from news/10265.bugfix.rst rename to news/10265.removal.rst