Restore legacy PyPy wheel interpreter tags

[ncoghlan]

pip used to calculate the interpreter tag for PyPy (and other
non-CPython implementations) incorrectly. This was fixed as part
of the migration to using packaging.tags to generate the list of
potentially compatible wheel tags for a target platform.

However, there are published PyPy wheels that use the old tag
format, so this adds in a compatibility pass that accepts
additional wheel tags in cases where pip versions prior to 20.0
previously did so.

Fixes #7629

[ncoghlan]

@pradyunsg I've added low level unit tests that rely on knowing how the backwards compatibility aliases are generated. These are nice and fast, and can be run on CPython, not just on alternate implementations.

However, I'm wondering if it might be worth also adding an explicit higher level integration test that only runs on PyPy, to prove that I've actually solved the reported bug (the unit test would then be demonstrating that the fix isn't PyPy specific).

[pradyunsg]

LGTM. A few minor non-blocking comments.

[pradyunsg]

However, I'm wondering if it might be worth also adding an explicit higher level integration test that only runs on PyPy, to prove that I've actually solved the reported bug (the unit test would then be demonstrating that the fix isn't PyPy specific).

It would be worthwhile and we should add one.

I was gonna politely ask in a follow up comment, if you'd be willing to do so in this PR, and it looks like you're already thinking about it. :)

[ncoghlan]

I'm happy to add the high level test, but I'm not familiar enough with the layout of pip's test suite to have a good idea on where to add it (by contrast, adding the unit tests was easy, as I just searched for references to "pep425").

The test_install_wheel functionality test seemed like a plausible candidate, but that seemed to be mostly be testing that the wheel was being installed correctly, rather than testing the correct detection of compatible wheel files.

[ncoghlan]

Ah, looking further, I think I found the right place: test/functional/test_download.py.

That already has several tests related to checking that platform-specific wheels are found and downloaded as expected, so we just need to add some extra cases there for non-CPython interpreters.

[ncoghlan]

Alas, looking at the way packaging.tags.interpreter_version is defined as part of figuring out how to write a test case that only runs on platforms with this problem, I now think any high level test case is going to fail on PyPy. The issue is that packaging.tags.interpreter_version returns the PEP 425 interpreter version (which is really a language version), whereas the old PyPy-specific tags really were the result of PyPy-specific code, as seen in 4b5614c#diff-542f0dc2284dcb0cb6a0382dfeeb8ed2

That means this new patch should also be PyPy-specific, specifically looking for pp in the impl arg, just like the old pip 19 code.

[ncoghlan]

Marked as WIP, as I need to significantly rework this based on the extra details I found in the commit that removed the old PyPy-specific code.

I should be able to wrap that up tomorrow my time (it's currently 10 pm in UTC+10).

[ncoghlan]

Also noting another potential location for the higher level test: tests/unit/test_models_wheel.py. That has several tests along these lines already.

However, I'd like to do a true end to end test, as one of the things we're checking here is what gets passed to pep425tags.get_supported() by default in the main installation/download flows.

[chrahunt]

I think we should guard this behavior behind a flag that, if used, should cause a deprecation warning to be traced. The reason for this approach is:

1. to prevent undue burden on other implementations, who would otherwise be forced to copy/paste pep425tags again
2. tracing a warning if a wheel is merely present in the available packages would be annoying during the whole transition period, where we would expect indexes to have wheels with both forms
3. doing anything smarter, like tracing a deprecation warning only if a wheel is "considered" or "used", would get much harder when implementing the new resolver

[ncoghlan]

@chrahunt I've described a new implementation strategy in #7629 (comment), as I no longer think my original idea is viable.

That approach should naturally contain the problem, as the wheels built for currently available versions of PyPy using versions of the wheel prior to 0.34.0 will be grandfathered in, but anything built for PyPy3 8.0.0 or later will need to use the standard tags, or it won't work.

[ncoghlan]

I believe the rework is completed now, so I've removed the WIP marker and asked @pradyunsg to take another look.

[pradyunsg]

I'll take a closer look in the morning. :)

[pradyunsg]

Noting that CI isn't happy with these changes -- looks like tests/functional/test_pep517.py::test_pep517_and_build_options is the only failing test.

[ncoghlan]

The new code was relying on the PyPy tag threshold dict being insertion ordered, but wasn't ensuring that was actually true on Python 2.7 and 3.5. I updated it to use collections.OrderedDict on all versions.

[ncoghlan]

The Windows errors look completely unrelated (something about an EnvironmentError attempt to install files): https://dev.azure.com/pypa/pip/_build/results?buildId=18040&view=logs&j=404e6841-f5ba-57d9-f2c8-8c5322057572&t=0219f6bf-240d-5b08-c877-377b12af5079&l=309

[ncoghlan]

I filed #7667 for the PAX sdist installation issue (I believe the format change in the wheel tarball revealed a latent defect in pip)

There have been significant changes since this review.

[pradyunsg]

This approach does sound reasonable to me -- but I really feel like I should defer to @chrahunt here. :P

[chrahunt]

Overall this approach seems fine to me. I think any other installer that would
want to emulate this behavior (which they shouldn't feel obliged to) could do so
pretty easily.

The current method of generating the legacy PyPy tags looks OK and aligns with
the discussion in #7269.

I have a few comments on the deprecation warning. Given this implementation,
from my perspective, the desired behavior is:

1. Only trace this deprecation notice when the package is actually used - currently
   it would be traced when any wheel with a legacy tag in the repository is considered
2. Trace this deprecation notice for each such package (or all at once) - currently
   it would be traced only once for the first package we consider (since warnings.warn
   with "default" config will only trace the first instance for a given module + line number)

In a more ideal world we would only trace the deprecation notice when the result from the
legacy PyPy tag contributes to the resolution process, but I don't think it will be common
for these wheels to have more than one Python tag so I don't have a problem ignoring that
consideration.

One way to adapt the current implementation could be:

1. Add a member to Wheel like is_using_pypy_legacy_tags to capture whether
   _infer_missing_pypy_version_tags returned anything before adding the result to pyversions.
2. Make a helper function that takes in a list of Wheel and traces a deprecation warning
   for the ones using is_using_pypy_legacy_tags.
3. Call the helper function after resolving in commands/download.py, commands/install.py,
   and commands/wheel.py. We can create the list of wheel files by using the local_path
   member of InstallRequirements for which is_wheel is true.

This won't have the best error message, notably it won't say where the wheel files came
from, but it should satisfy the desired behavior mentioned above and not require a lot
of additional work to preserve the behavior with the upcoming resolver changes.

[pradyunsg]

Gentle nudge on this @ncoghlan. Would it be possible for you to update this PR, or would you prefer/be OK with someone else taking this forward?

[ncoghlan]

I'd definitely be fine with someone else moving it forward (I confess I forgot there were review comments still to be addressed - I'm a few weeks behind on open source email notifications at the moment)

[pradyunsg]

Closing since no one has stepped up to take this forward, and if someone does, it'll be a new PR anyway.