Remove support for non-bare egg fragments

[ichard26]

Resolves #13157.

They were deprecated from all the way back to January 2023 🙃.

[ichard26]

Just some notes for reviewers.

[ichard26]

Normally, editable requirements are rejected during the resolution step. This error check is too late for editable URL constraints that also happen to have a non-bare egg fragment, since the fragment check is performed during command startup. As the main issue is that you can't even have an editable constraint, we now check and raise early in requirement construction.

[sbidoul]

@ichard26 I don't quite understand this. Can you show what happens if you don't raise here?

[ichard26]

$ cat constraints.txt
-e pip @ file:///home/ichard26/dev/oss/pip
$ pip install -c constraints.txt pip
ERROR: pip is not a valid editable requirement. It should either be a path to a local project or a VCS URL (beginning with bzr+http, bzr+https, bzr+ssh, bzr+sftp, bzr+ftp, bzr+lp, bzr+file, git+http, git+https, git+ssh, git+git, git+file, hg+file, hg+http, hg+https, hg+ssh, hg+static-http, svn+ssh, svn+http, svn+https, svn+svn, svn+file).

This is causing test_install_with_extras_editable_joined to fail otherwise. Although TBH, I don't actually understand why this PR is causing it to fail :/

script = <tests.lib.PipTestEnvironment object at 0x7a5f7716f890>, data = <tests.lib.TestData object at 0x7a5f7716f9d0>, resolver_variant = 'resolvelib'

    def test_install_with_extras_editable_joined(
        script: PipTestEnvironment, data: TestData, resolver_variant: ResolverVariant
    ) -> None:
        to_install = data.packages.joinpath("LocalExtras")
        file = script.temporary_file(
            "constraints.txt", f"-e LocalExtras[bar] @ {to_install.as_uri()}"
        )
        result = script.pip_install_local(
            "-c",
            file,
            "LocalExtras[baz]",
            allow_stderr_warning=True,
            expect_error=(resolver_variant == "resolvelib"),
        )
        if resolver_variant == "resolvelib":
>           assert "Editable requirements are not allowed as constraints" in result.stderr
E           AssertionError: assert 'Editable requirements are not allowed as constraints' in 'ERROR: LocalExtras[bar] is not a valid editable requirement. It should either be a path to a local project or a VCS URL (beginning with bzr+http, bzr+https, bzr+ssh, bzr+sftp, bzr+ftp, bzr+lp, bzr+file, git+http, git+https, git+ssh, git+git, git+file, hg+file, hg+http, hg+https, hg+ssh, hg+static-http, svn+ssh, svn+http, svn+https, svn+svn, svn+file).\n'
E            +  where 'ERROR: LocalExtras[bar] is not a valid editable requirement. It should either be a path to a local project or a VCS URL (beginning with bzr+http, bzr+https, bzr+ssh, bzr+sftp, bzr+ftp, bzr+lp, bzr+file, git+http, git+https, git+ssh, git+git, git+file, hg+file, hg+http, hg+https, hg+ssh, hg+static-http, svn+ssh, svn+http, svn+https, svn+svn, svn+file).\n' = <tests.lib.TestPipResult object at 0x7a5f77426780>.stderr

tests/functional/test_install_reqs.py:742: AssertionError

[sbidoul]

Strange. On holiday then FOSDEM so no time to dig now. Raising that kind of error at that place raises a little alarm in my head. I might be wrong, so don't block the PR for this.

[ichard26]

Oh wait, I'm being silly. The problem is that editable requirements don't support just a project name (with or without an extra) because there is no editable source. They need to be a VCS URL or local path. So when I rewrote test_install_with_extras_editable_joined to use the Direct URL syntax (as the egg fragment is now prohibited to contain an extra), it's running into this error. The editable requirement check occurs before the editable constraint error check, so that's why this test is failing. It's expecting the latter error.

-    script.scratch_path.joinpath("constraints.txt").write_text(
-        f"-e {to_install.as_uri()}#egg=LocalExtras[bar]"
+    file = script.temporary_file(
+        "constraints.txt", f"-e LocalExtras[bar] @ {to_install.as_uri()}"
    )

TBH, raising either error is reasonable, although it'd be preferable to show the "editable constraints aren't allowed" error since that is the main issue. If it's too sketchy raising an error in a requirement constructor, I can remove this and update the test. FWIW, if the editable requirement (constraint) is valid, then the constraint error will be raised as expected.

[notatallshaw]

The URL reconstruction in the hint message doesn't always look right:

    def test_invalid_egg_fragment_with_extras_and_version_hint(self) -> None:
        """Test that fragments with both extras and version specifiers get proper hint."""

        url = "git+https://example.com/package#egg=eggname[extra]==1.0"
        with pytest.raises(InvalidEggFragment) as exc_info:
            Link(url)
        
        # The hint should suggest Direct URL syntax, not just "remove version specifiers"
        # because removing only the version specifier would still leave invalid extras
        hint = exc_info.value.hint_stmt
        assert "eggname[extra] @ git+https://example.com/package" in hint

    def test_invalid_egg_fragment_with_subdirectory_hint(self) -> None:
        """Test that Direct URL hint correctly handles subdirectory fragments."""

        url = "git+https://example.com/package#egg=eggname[extra]&subdirectory=sub"
        with pytest.raises(InvalidEggFragment) as exc_info:
            Link(url)
        
        # The suggested URL should have proper fragment syntax with # not &
        hint = exc_info.value.hint_stmt
        assert "git+https://example.com/package#subdirectory=sub" in hint

I think you either need to a more complete deconstruct and reconstruction the url and requirements, or just don't attempt to give a corrected URL?

[ichard26]

@notatallshaw I've decided to dumb down the hint logic to stop even attempting to give a fixed requirement. Instead, I just give a generic name[extra] @ URL example. URLs are finicky to manipulate.

[notatallshaw]

@ichard26 there's a small conflict in the import section, you're welcome to fix and merge, if you want to hold off please let me know as I will bump the gone_in in the deprecation warning.

[notatallshaw]

Code looks good to me, we've gone through a couple of reviews, I am merging.