installer: do not fail on invalid wheels, print only a warning

[radoering]

Pull Request Check List

Resolves: #7686

• Added tests for changed code.
• Updated documentation for changed code.

We've decided to print a warning for invalid wheels instead of failing hard because it's technically an unintended breaking change between poetry 1.4.0 and 1.4.1. I was not aware that validate_record() in installer 0.7.0 does more than the asserts in installer 0.6.0 and, thus, leads to additional install failures.

Further, copied from a post of @pradyunsg (PyPA member and maintainer of installer) on the Poetry Discord:

The ecosystem isn't ready for blanket validation of wheel RECORD files TBH, and I suggest that Poetry not blanket fail on that.

I do think performing this validation is valuable though -- and if you can print warnings somehow, that'd be awesome!

[dimbleby]

I see the wind is not blowing in the direction that I would like it to, but I'm going to have one more try at encouraging poetry to Do The Right Thing.

First, I agree that it was an accident that poetry 1.4.1 does more validation than poetry 1.4.0 did. However I think that can be repaired simply by passing validate_contents=False to validate_record(): IIUC that would restore the level of validation that was happening at 1.4.0.

Regardless of what happens at 1.4.x, I still would like to see the master branch of poetry - and therefore the next non-patch release - performing full validation, at least by default. Per a suggestion in one of the related issues I could see a case for a config option (or perhaps an environment variable) to allow turning that validation off: but defaulting to something that people must pay attention to will, I am sure, in the end be the only way to get folk to do anything about invalidly built wheels.

A slight tangent, but I'd like to note that so far the issues and discussions around this have been pleasingly civilized. You'd half expect the shouty people to turn up for this sort of thing, but so far it's all been rather reasonable - thanks all!

In that spirit: I'm not looking to kick up a fuss here, y'all do what you think right, I'm just offering a perspective on what I think is right.

[radoering]

However I think that can be repaired simply by passing validate_contents=False to validate_record(): IIUC that would restore the level of validation that was happening at 1.4.0.

On a quick glance, I don't think so: pypa/installer@49a3540
It seems there was only an assert for "not mentioned in RECORD". validate_record seems to do more even with validate_contents=False.

What I still do not quite understand: Are there any known issues we experience with invalid wheels? If not why should we be the only one failing hard? Do we have to be the one forcing people to fix their things?

In my opinion, a warning (which, by the way, is quite long if there are some invalid entries) should be annoying enough for some people to trigger fixes. With the current state of the ecosystem, no one seems to be aware of invalid wheels because no other tool issues even a warning...

[dimbleby]

I'm not sure what you're seeing in that MR that I'm not? though that code has been refactored again since then so the 0.7.0 implementation would be more relevant

you are right that poetry is under no more obligation than anyone else to follow this part of the spec.

[radoering]

If I don't miss anything 0.6.0 did only one check: https://github.com/pypa/installer/blob/0.6.0/src/installer/sources.py#L157-L161

That's the following check in 0.7.0: https://github.com/pypa/installer/blob/0.7.0/src/installer/sources.py#L260-L264

Additionally in 0.7.0 even with validate_contents=False:

• https://github.com/pypa/installer/blob/0.7.0/src/installer/sources.py#L253-L258
• https://github.com/pypa/installer/blob/0.7.0/src/installer/sources.py#L268-L275
• https://github.com/pypa/installer/blob/0.7.0/src/installer/sources.py#L276-L280

[rbebb]

Honestly, I think it's great that poetry now performs full validation by default. Even though it could cause some short-term headaches, it's certainly a long-term gain. However, I do agree that it should have occurred in a major version increment if the Poetry team considers it a breaking change.

If this is changed to a warning and is not enabled by default in the next patch, then it should be enabled by default and cause an error in the next major version.

[pradyunsg]

IMO, the right thing to do if you want the ~same behaviours as 0.6.0 is to straight up not call validate_record.

The next closest thing is what this PR does, based on the title, which is change things into a warning rather than propagating the error.

[dimbleby]

A progress report on some of the projects I have seen falling foul of this and reported here, in approximate order of good to bad news

• torch noticed the problem in their wheels independent of poetry and already made a fix
• sphinx_theme_builder has fixed its code for generating hashes, and released
  • therefore all projects built with that backend should pick up that fix in due course, eg there is now a furo that is fixed
• xgboost has a fix in its main branch for their windows wheels
• catboost has an open pull request
• debugpy has an open issue, but no-one has submitted a pull request and so far as I can see the only sign of maintainer interaction is that it has been given labels "bug" and "P1"
• no-one has raised an issue against aws_psycopg2

(Of course even though a project like torch has made a fix, the old faulty wheels are still out there)

To my mind this is pretty good progress and would justify turning (leaving?) the validation on by default in 1.5.x.

As for 1.4.x, I'm not so fussed. Mostly folk seem to find the relevant issues and either pin to 1.4.0 or disable the modern installer without getting too upset. Of course it's a shame that those people are missing out on modern-installer and latest-fixes: IMO that suggests that providing an option (environment variable?) to make the validation non-failing would be sufficient at 1.4.x also. But this MR looks more or less ready to go and it's not a hill I'd want to die on.

[poetry-bot]

The backport to 1.4 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.4 1.4
# Navigate to the new working tree
cd .worktrees/backport-1.4
# Create a new branch
git switch --create backport-7694-to-1.4
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c2a7a8d6ea412d021a65068551fa814f0925dc40
# Push it to GitHub
git push --set-upstream origin backport-7694-to-1.4
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.4

Then, create a pull request where the base branch is 1.4 and the compare/head branch is backport-7694-to-1.4.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.