req: Add warning of not recording VCS info with legacy 'setup.py install'

[NoahGorny]

Closes #9176

[NoahGorny]

@uranusjr I rebased this to latest master, but I think this can be merged

[uranusjr]

I'm waiting for a second opinion if possible.

[pfmoore]

Let's leave it until after the release this weekend, in that case.

[sbidoul]

I'm not 100% convinced this the right place to warn users about this, as this will show a warning for users that may not care about pip freeze at all.

There is also REQUESTED that is not generated in this case.

Also, the setup.py install code path will be called out systematically when #9422 and #8560 are complete, so we might want to rather make it a more generic warning that some features may not work perfectly when the legacy setup.py install is used.

Regarding freeze, we could detect legacy installed distributions when running pip freeze and warn about those there ?

[uranusjr]

Regarding freeze, we could detect legacy installed distributions when running pip freeze and warn about those there?

Is it possible/easy to detect an setup.py install-ed distribution apart from a wheel-installed one? Does the format guaranteed to be a .egg-info, for example?

[sbidoul]

Or can we assume the presence of .dist-info means the installation is standards-based ?

[pradyunsg]

We should probably add a test to ensure that this warning is printed.

[NoahGorny]

We should probably add a test to ensure that this warning is printed.

Addded 😄

[pradyunsg]

Uhhh, don't do this in a unit test... This has side effects that affect the interpreter it's running in. Specifically, it'll install the package when you run the tests

[NoahGorny]

What is the correct way to do it? This is how it was done in test_req_file.py

[NoahGorny]

lemme know what you think @pradyunsg

[NoahGorny]

ping @pradyunsg

[pradyunsg]

Thanks for the ping! This got buried in my notifications. :(

Write an integration test that exercises this code path. Here's an example, that you'll likely need to change the name of the package for. Feel free to add the source code for a minimal reproducer package in tests/data/packages or use a script.pip("install", "...") for getting something minimal from the internet.

def test_install_vcs_something_something_triggers_a_warning(
    script,
    data,
):
    to_install = data.packages.joinpath("a-thing-that-triggers-this")
    result = script.pip_install_local(
        to_install,
        allow_stderr_warning=True,
    )
    msg = "Direct URL of package 'a-thing-that-triggers-this' will not be recorded when using"
    assert msg in result.stderr

[pradyunsg]

See test_basic_install_editable_from_svn for an example of a local-only VCS test.

[sbidoul]

Installing the wheel package may not be sufficient, as there are other reason for falling back to setup.py install (such as using --no-binary or --{install,global}-options. So we may want to drop this recommendation line.

[sbidoul]

Suggested change

	if self.link.is_vcs:
	if self.original_link:

To better match the condition used to create direct_url.json ?

Addressed earlier.

[pradyunsg]

To communicate with sufficient clarity: I do not intend to block the 21.3 release on this PR, since this doesn't seem to be a show-stopper issue; even though this would be a nice-to-have improvement!

To that end, I've gone ahead and dropped this from the release milestone. :)

[pradyunsg]

Going to go ahead and close this, since it has merge conflicts that haven't been resolved in a while.

Please feel welcome to file a new PR or to reopen and update this one (assuming that's what the existing discussions point toward)!