Deprecate requirements format "base=>1.0[extra]"

[jku]

Note: I'm new to pip code and it was not obvious where this should be done: feel free to point me to another direction. I chose this spot in parse_req_from_line() because there's other validation right next to it.

There's currently no "gone_in" defined for the deprecation: let me know if there's a policy I should follow here.

The bug also has my comments on some other weirdness that may or may not be related bugs.

Fixes #8288

---

This requirements format does not conform to PEP-508. Add deprecation
warning and fix the corresponding test.

Note that the actual added check ensures the specifier is compliant
with PEP-440 (the specifier version is invalid "1.0[extra]").

[pradyunsg]

Thank you for filing this PR @jku!

A couple of minor code-style related comments. The rest looks great to me! ^>^

[jku]

thanks for review @pradyunsg, I've done the requested changes and a minor tweak to the actual message.

[pfmoore]

If I'm reading the PR correctly, we're actually deprecating all non-PEP 440 version numbers, not just the 1.0[extra] style. But it's not the pip user that's in control of the version numbering used by the projects they are installing, so this has the potential to (in effect) make certain packages not installable with pip.

I'm not against this in principle, but have we checked how many packages this will apply to? Does PyPI enforce PEP 440 version format? I'd be fine with only considering PyPI here (private indexes can police themselves in this matter).

[jku]

If I'm reading the PR correctly, we're actually deprecating all non-PEP 440 version numbers, not just the 1.0[extra] style. But it's not the pip user that's in control of the version numbering used by the projects they are installing, so this has the potential to (in effect) make certain packages not installable with pip.

Fair point. I think having a look through pypi is prudent in any case... I'll think about this

[uranusjr]

If I’m understanding the situation correctly, base>=1.0[add] is actually being parsed as:

• Name: base
• Version specifier: >=1.0[add] (which is a legacy specifier)

And somehow this is would later be used to select extra add?

If this is a case, the fix likely should not be to deprecate the legacy specifiers, but to figure out why it’s selecting the extra, and make it not do that.

[jku]

If I’m understanding the situation correctly, base>=1.0[add] is actually being parsed as:

• Name: base
• Version specifier: >=1.0[add] (which is a legacy specifier)

And somehow this is would later be used to select extra add?

Correct. I tried to start a discussion on this in the bug, please see my last three comments in #8288 and comment there if you have ideas.

If this is a case, the fix likely should not be to deprecate the legacy specifiers, but to figure out why it’s selecting the extra, and make it not do that.

Well, it might still be prudent to warn or stop processing as early as possible -- but let's figure out the big picture first in any case: I think the issue discussion might be the right place for that

[jku]

If I'm reading the PR correctly, we're actually deprecating all non-PEP 440 version numbers, not just the 1.0[extra] style. But it's not the pip user that's in control of the version numbering used by the projects they are installing, so this has the potential to (in effect) make certain packages not installable with pip.

I'm not against this in principle, but have we checked how many packages this will apply to? Does PyPI enforce PEP 440 version format? I'd be fine with only considering PyPI here (private indexes can police themselves in this matter).

pypi does enforce pep-440: upload fails with non-conformant version.

I've had a look at wheel filenames in pypi repo: out of 1.1 million wheel files 84 have non-conforming versions (and are not somehow otherwise broken). 9 of them are current versions. Something prevents installing them without the version: e.g. pip install HolyGrail fails with No matching distribution found but pip install HolyGrail==0.2.1.Perceval succeeds.

So there is a small number of packages with non-conforming version numbers that currently are somewhat installable from pypi -- they are not likely to be very popular as the "non-versioned" installation method already fails.

[pfmoore]

Thanks for doing that analysis. It sounds like if we decide to deprecate, the impact will be sufficiently low to make it a reasonable option (we'd still need the deprecation period, to cover non-PyPI projects, of course).

[jku]

Changes since the previous review:

• added test_install_deprecated_extra() to test command line
• changed what is actually deprecated based on feedback: now it's only warning on any version string that ends with ']' (as a proxy to finding misparsed version strings that contains [extras]) instead of warning on every legacy version specifier. No deprecating LegacySpecifiers means there may be other cases like this ... but there may also be side-effects to deprecating them. Feel free to tell me to switch this back if you think LegacySpecifiers should be deprecated instead -- I don't quite feel qualified to decide this

If this is a case, the fix likely should not be to deprecate the legacy specifiers, but to figure out why it’s selecting the extra, and make it not do that.

I don't think this is an option (as long as there is no refactoring of the whole function). We know why this happens: the version string is wrong so Requirement() fails to parse either extras or the version properly but _strip_extras() happens store the extras by accident and the buggy version specifier is close enough to often work. The parsing in Requirement() is correct -- it just does not complain about using LegacySpecifiers. Before calling Requirement() we cannot reliably know that the version is fishy.

I do not think we should add more parsing code to figure it out earlier in parse_req_from_line() either: the correct path would probably be to try to handle the PEP-440 compatible requirement strings first and then add cases for the other requirements string formats (directory, file, url) -- possibly forming a PEP-440 compliant requirement string as result -- as well as any workarounds for deprecations and such. But as discussed in the bug that's likely to be a much bigger job.

[jku]

Interesting, it looks like the command line test I added fails on windows: the extra package does not get created (alternatively, I do something wrong in the test).

There's no stdout/stderr listed for the AssertionError (not sure why, it does happen locally) but it's clear from the "created" list that the simple package does not get installed on windows. I think this is not related to my changes but can't be sure. I guess I'll have to open another PR to add just the test to confirm that (I don't have easy ways to test on windows)

https://dev.azure.com/pypa/pip/_build/results?buildId=25874&view=logs&j=d5516b16-d635-5f23-ed1f-618b454a3b40&t=4b6551ce-09cf-548f-8bd5-83779dcb7360&l=90

[uranusjr]

You’re hit by a quirk in pip’s test utilities. pip install base>=1.0[extra] is being interpreted as pip install base with the result being piped into a file named =1.0[extra]. The only good solution (aside from actually fixing the utility) is to use another operator…

[uranusjr]

Or alterntively you can write the requirement line into a file and use -r, although I’m not sure if that would hit the code path you want to test (pip’s requirement parsing logic is complicated).

[jku]

Thank you, that seems obvious now that you pointed it out.

This was a good find though: I had not extensively tested what actually passes the version check and I really should have... As I mentioned the specifier version becomes garbled: The tests I had picked all worked but it turns out quite a few of the comparisons do not, e.g. these do not work at all currently (examples from the test in question, requires_simple_extra == 0.1):

• requires_simple_extra==0.1[extra] fails to find a version
• requires_simple_extra<=0.1[extra] fails to find a version

I wonder if the right choice is actually to error out without a deprecation -- this seems to be quite thoroughly broken right now.

[jku]

To summarize the slightly meandering thread, current state of the PR is:

• the last added test fails on windows so this should not be merged. I could workaround this but then...
• I realized this requirements format already fails for common version specifiers like "base==0.1[extra]"

I think the right thing to do might be to just error out with the current check (does version end in "]"?) instead of deprecating. I can do one more pass of this if that sounds good (although how useful this is without a refactoring of parse_req_from_line() is debatable).

[jku]

latest pushes change

• test_install_deprecated_extra(): it now uses a requirements file to avoid the shell redirect testing issue
• test_new_resolver_installs_extras_deprecated(): use --use-feature=2020-resolver

[pradyunsg]

Thanks @jku! ^>^