Catch + report `BadZipFile` errors in `get_wheel_distribution` #10535

lukasjuhrich · 2021-10-02T20:57:36Z

Change

Catch BadZipFile errors and re-raise with some context information.
No functional change, hence no news entry.

Motivation

I'm facing a few nasty CI bugs where the pip install command fails with an uncaught BadZipFile exception. A stacktrace looks like this:

  […N similar lines…]
   Building wheel for wrapt (setup.py): started
  Building wheel for wrapt (setup.py): finished with status 'done'
  Created wheel for wrapt: filename=wrapt-1.12.1-cp39-cp39-linux_x86_64.whl size=78277 sha256=337cd2663d837fd8364f46d472964afd4b05eb1fdf68361b26245a866e714227
  Stored in directory: /opt/pycroft/.cache/pip/wheels/98/23/68/efe259aaca055e93b08e74fbe512819c69a2155c11ba3c0f10
ERROR: Exception:
Traceback (most recent call last):
  File "/opt/pycroft/venv/lib/python3.9/site-packages/pip/_internal/cli/base_command.py", line 173, in _main
    status = self.run(options, args)
  File "/opt/pycroft/venv/lib/python3.9/site-packages/pip/_internal/cli/req_command.py", line 203, in wrapper
    return func(self, options, args)
  File "/opt/pycroft/venv/lib/python3.9/site-packages/pip/_internal/commands/install.py", line 342, in run
    _, build_failures = build(
  File "/opt/pycroft/venv/lib/python3.9/site-packages/pip/_internal/wheel_builder.py", line 336, in build
    wheel_file = _build_one(
  File "/opt/pycroft/venv/lib/python3.9/site-packages/pip/_internal/wheel_builder.py", line 228, in _build_one
    _verify_one(req, wheel_path)
  File "/opt/pycroft/venv/lib/python3.9/site-packages/pip/_internal/wheel_builder.py", line 177, in _verify_one
    dist = get_wheel_distribution(wheel_path, canonical_name)
  File "/opt/pycroft/venv/lib/python3.9/site-packages/pip/_internal/metadata/__init__.py", line 48, in get_wheel_distribution
    return Distribution.from_wheel(wheel_path, canonical_name)
  File "/opt/pycroft/venv/lib/python3.9/site-packages/pip/_internal/metadata/pkg_resources.py", line 43, in from_wheel
    with zipfile.ZipFile(path, allowZip64=True) as zf:
  File "/usr/local/lib/python3.9/zipfile.py", line 1257, in __init__
    self._RealGetContents()
  File "/usr/local/lib/python3.9/zipfile.py", line 1324, in _RealGetContents
    raise BadZipFile("File is not a zip file")
zipfile.BadZipFile: File is not a zip file

Since the information which file caused the error is missing, I don't have any means to further narrow down this issue (e.g. check which files are affected, compare checksums, etc). This change should help me and anyone else facing such issues.

NB

Please note that I have no clue about PIP internals; feel free to make any changes if e.g. the error message is factually incorrect.

DiddiLeija · 2021-10-02T21:09:33Z

I don't know if this is necessary. The BadZipFile message looks reasonable to me.

lukasjuhrich · 2021-10-02T21:37:15Z

@DiddiLeija Can you elaborate on your remark? As explained, the message is completely unhelpful to me because I don't even know what file or wheel it is about. It might be useful to the expert who knows the precise internals of what pip install does under the hood, but I'd expect it to come across as rather cryptic to anybody else.

(note also that the stacktrace is an excerpt; I have no reason to assume that the wrapt installation has anything to do with it)

DiddiLeija · 2021-10-02T22:04:16Z

You're right, my misreading:

Since the information which file caused the error is missing, I don't have any means to further narrow down this issue (e.g. check which files are affected, compare checksums, etc). This change should help me and anyone else facing such issues.

Thinking a bit about this, this could be a good aproach, but we may have to sketch how the message must look like.

BTW, before going ahead, I recommend you to write a news entry, because this change is not trivial (I think). See https://pip.pypa.io/en/stable/development/contributing/#news-entries for more information. Also, you may have to write a test for this behavior.

uranusjr · 2021-10-02T22:45:13Z

Instead of raising RuntimeError, this should raise either UnsupportedWheel or a new exception subclassing InstallationError (e.g. InvalidWheel). InstallationError is a misnomer (legacy reasons), it basically fails the process with a nice error message instead of a bunch of traceback (the traceback is only shown in verbose mode).

uranusjr · 2021-10-02T22:49:01Z

I think it's also likely better to handle this in Distribution.from_wheel() instead and add a docstring describing the method should not raise BadZipError and should raise the new exception instead on an invalid wheel. A test that creates and tries to pip install an invalid zip file is needed as well.

pradyunsg · 2021-10-03T11:51:49Z

#9964 is one example of what situations might cause this corruption.

lukasjuhrich · 2021-10-17T16:13:58Z

Small addition: The error occurred with pip==21.2.4, and in the meanwhile, 135faab has introduced the abstraction layer of MemoryWheel / FileSystemWheel.

With that in mind, note how my stracktrace is slightly different as it would be now. It goes

get_wheel_distribution
Distribution.from_wheel
ZipFile.__init__

but now it would instead go

get_wheel_distribution
Distribution.from_wheel
FileSystemWheel.as_zipfile
ZipFile.__init__.

lukasjuhrich · 2021-10-17T16:19:31Z

@DiddiLeija any pointers for which kind of test I should write? A unit test in unit.test_metadata which passes a mocked wheel which manually raises BadZipFile, or a full-on functional test where I try to load a corrupt wheel file, or something inbetween?

I'm not too familiar with pip's test concept.

DiddiLeija · 2021-10-17T17:02:27Z

I think a functional test would be better in this case.

lukasjuhrich · 2021-10-17T17:03:46Z

@uranusjr I followed your instructions. Also, I added a news file as suggested. A test is still missing.

I'm not sure whether InvalidWheel or CorruptWheel is preferable for an exception name.

Note that in the case of HTTP transfer, i.e. a call of dist_from_wheel_url, the wheel.location attribute will amount to the name of the temporary file created by LazyZipOverHTTP. The new message Wheel foo located at /tmp/blah won't be an improvement in this case, but it's probably not worse than an unhandled BadZipFile (and corruptions aren't likely to occur over HTTP, that's what the transport layer is for). But that's not the use case we want to improve here, anyway.

@DiddiLeija gotcha.

uranusjr · 2021-10-17T17:22:49Z

Let's go with InvalidWheel, we can always change it if needed; pip doesn't have a public Python API so renaming things is not a big deal. Adding a new class means it'll be easier to do something to the lazy zip case if we want to, so let's get this in and think about how that can be improved afterwards.

lukasjuhrich · 2021-10-17T17:59:33Z

Following functional.test_install_from_broken_wheel to run pip install $corrupt_file.whl, it's not quite clear to me how I should test for InvalidWheel to be thrown.

I see two options:

somehow grepping through stderr
parametrizing test_install_from_broken_wheel to feed it a wheel broken on the zip level as well. In both cases (BadZipFile vs. in whatever way brokenwheel-1.0-py2.py3-none-any.whl is corrupt) the observable result should be “stacktrace && installation unsuccessful”.

Writing a unit test for this behavior proved a lot simpler, so it sounds reasonable to me to go for option 2 and leave the unit test in, so we can test both the general behavior (“some stacktrace”) and the specific handling (InvalidWheel).

uranusjr · 2021-10-17T18:11:50Z

I think option 2 makes more sense as well.

lukasjuhrich · 2021-10-17T18:18:21Z

Done, and in addition

Incorporated black reformatting
verified that my unit test failed when undoing the change.

lukasjuhrich · 2021-10-17T18:25:27Z

Sorry for all the force-pushes, I know it requires reapproval. I forgot to squash some stuff, I'm finished now.

news/10535.feature.rst

Note that the functional test does not actually detect the behavioral change of throwing unhandled `BadZipFile` → throwing unhandled `InvalidWheel`, whereas the unit test does.

uranusjr · 2021-10-23T16:15:36Z

pre-commit does not like trailing spaces.

lukasjuhrich · 2021-10-23T16:25:14Z

pre-commit does not like trailing spaces.

Ah, I forgot to pre-commit install so I didn't notice that in this session. Does not complain locally, should be green now.

lukasjuhrich · 2021-10-23T17:02:31Z

So sorry for the broken commit message in 6d0612d, fixed by add2ab0. Happened due to a reset so I could run pre-commit again.

lukasjuhrich · 2021-11-09T18:41:30Z

Just checking in, is any further work required from my side?

uranusjr · 2021-11-09T18:43:43Z

Porbably not (at least not from me). I'll leave this open in case someone wants to take a look, but also add this to 22.0 so it can be merged if no-one has any objections.

DiddiLeija

From my side, this PR is ready. Thanks @lukasjuhrich!

news/10535.feature.rst

Co-authored-by: Pradyun Gedam <[email protected]>

pradyunsg · 2021-11-12T09:57:21Z

Thanks a lot for this @lukasjuhrich! ^>^

lukasjuhrich force-pushed the main branch from 0332250 to 60e7b45 Compare October 17, 2021 16:51

lukasjuhrich force-pushed the main branch from 60e7b45 to 22c3c6b Compare October 17, 2021 18:16

lukasjuhrich force-pushed the main branch from 799e54b to ba8ca2a Compare October 17, 2021 18:18

uranusjr reviewed Oct 18, 2021

View reviewed changes

news/10535.feature.rst Outdated Show resolved Hide resolved

lukasjuhrich added 4 commits October 23, 2021 17:42

Improve BadZipFile reporting

03c2ea0

adapt network test to InvalidWheel

cd3aefe

Add tests for BadZipFile handling

58996b5

Note that the functional test does not actually detect the behavioral change of throwing unhandled `BadZipFile` → throwing unhandled `InvalidWheel`, whereas the unit test does.

make linters happy

2e67986

lukasjuhrich force-pushed the main branch from cb53f92 to 2515ac8 Compare October 23, 2021 15:42

uranusjr approved these changes Oct 23, 2021

View reviewed changes

lukasjuhrich force-pushed the main branch from 2515ac8 to 6d0612d Compare October 23, 2021 16:24

Re-word news entry to hide implementation details

add2ab0

lukasjuhrich force-pushed the main branch from 6d0612d to add2ab0 Compare October 23, 2021 16:59

uranusjr added this to the 22.0 milestone Nov 9, 2021

DiddiLeija approved these changes Nov 9, 2021

View reviewed changes

pradyunsg reviewed Nov 10, 2021

View reviewed changes

news/10535.feature.rst Outdated Show resolved Hide resolved

Improve wording of feature summary 10535

a51a644

Co-authored-by: Pradyun Gedam <[email protected]>

pradyunsg merged commit e7c80c7 into pypa:main Nov 12, 2021

messense mentioned this pull request Nov 27, 2021

Occasional BadZipFile error when running cargo test PyO3/maturin#706

Closed

github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch + report `BadZipFile` errors in `get_wheel_distribution` #10535

Catch + report `BadZipFile` errors in `get_wheel_distribution` #10535

lukasjuhrich commented Oct 2, 2021 •

edited

Loading

DiddiLeija commented Oct 2, 2021

lukasjuhrich commented Oct 2, 2021 •

edited

Loading

DiddiLeija commented Oct 2, 2021

uranusjr commented Oct 2, 2021

uranusjr commented Oct 2, 2021

pradyunsg commented Oct 3, 2021

lukasjuhrich commented Oct 17, 2021

lukasjuhrich commented Oct 17, 2021

DiddiLeija commented Oct 17, 2021

lukasjuhrich commented Oct 17, 2021

uranusjr commented Oct 17, 2021

lukasjuhrich commented Oct 17, 2021

uranusjr commented Oct 17, 2021

lukasjuhrich commented Oct 17, 2021

lukasjuhrich commented Oct 17, 2021 •

edited

Loading

uranusjr commented Oct 23, 2021

lukasjuhrich commented Oct 23, 2021

lukasjuhrich commented Oct 23, 2021

lukasjuhrich commented Nov 9, 2021

uranusjr commented Nov 9, 2021

DiddiLeija left a comment

pradyunsg commented Nov 12, 2021

Catch + report BadZipFile errors in get_wheel_distribution #10535

Catch + report BadZipFile errors in get_wheel_distribution #10535

Conversation

lukasjuhrich commented Oct 2, 2021 • edited Loading

Change

Motivation

NB

DiddiLeija commented Oct 2, 2021

lukasjuhrich commented Oct 2, 2021 • edited Loading

DiddiLeija commented Oct 2, 2021

uranusjr commented Oct 2, 2021

uranusjr commented Oct 2, 2021

pradyunsg commented Oct 3, 2021

lukasjuhrich commented Oct 17, 2021

lukasjuhrich commented Oct 17, 2021

DiddiLeija commented Oct 17, 2021

lukasjuhrich commented Oct 17, 2021

uranusjr commented Oct 17, 2021

lukasjuhrich commented Oct 17, 2021

uranusjr commented Oct 17, 2021

lukasjuhrich commented Oct 17, 2021

lukasjuhrich commented Oct 17, 2021 • edited Loading

uranusjr commented Oct 23, 2021

lukasjuhrich commented Oct 23, 2021

lukasjuhrich commented Oct 23, 2021

lukasjuhrich commented Nov 9, 2021

uranusjr commented Nov 9, 2021

DiddiLeija left a comment

Choose a reason for hiding this comment

pradyunsg commented Nov 12, 2021

Catch + report `BadZipFile` errors in `get_wheel_distribution` #10535

Catch + report `BadZipFile` errors in `get_wheel_distribution` #10535

lukasjuhrich commented Oct 2, 2021 •

edited

Loading

lukasjuhrich commented Oct 2, 2021 •

edited

Loading

lukasjuhrich commented Oct 17, 2021 •

edited

Loading