Repository: pypa/pip
Pull request #10535 (2021-10-02 20:57:36)

Catch + report `BadZipFile` errors in `get_wheel_distribution`
## 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: I don't know if this is necessary. The `BadZipFile` message looks reasonable to me.
lukasjuhrich: @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.
DiddiLeija: 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: 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: 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: https://github.com/pypa/pip/issues/9964 is one example of what situations might cause this corruption.
lukasjuhrich: @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.
lukasjuhrich: @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: 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.
uranusjr: I think option 2 makes more sense as well.
lukasjuhrich: Done, and in addition

- [x] Incorporated `black` reformatting
- [x] verified that my unit test failed when undoing the change.
lukasjuhrich: Sorry for all the force-pushes, I know it requires reapproval. I forgot to squash some stuff.

Patch:
diff --git a/news/10535.feature.rst b/news/10535.feature.rst
new file mode 100644
index 000000000..f84301259
--- /dev/null
+++ b/news/10535.feature.rst
@@ -0,0 +1,1 @@
+Present a better error message when an invalid wheel file is encountered, providing more context where the invalid wheel file is.
diff --git a/src/pip/_internal/exceptions.py b/src/pip/_internal/exceptions.py
index ef5bc7511..f9b07eefb 100644
--- a/src/pip/_internal/exceptions.py
+++ b/src/pip/_internal/exceptions.py
@@ -132,6 +132,17 @@ class UnsupportedWheel(InstallationError):
     """Unsupported wheel."""
 
 
+class InvalidWheel(InstallationError):
+    """Invalid (e.g. corrupt) wheel."""
+
+    def __init__(self, location: str, name: str):
+        self.location = location
+        self.name = name
+
+    def __str__(self) -> str:
+        return f"Wheel '{self.name}' located at {self.location} is invalid."
+
+
 class MetadataInconsistent(InstallationError):
     """Built metadata contains inconsistent information.
 
diff --git a/src/pip/_internal/metadata/pkg_resources.py b/src/pip/_internal/metadata/pkg_resources.py
index e8a8a3807..0c054f8c9 100644
--- a/src/pip/_internal/metadata/pkg_resources.py
+++ b/src/pip/_internal/metadata/pkg_resources.py
@@ -1,12 +1,14 @@
 import email.message
 import logging
 from typing import Collection, Iterable, Iterator, List, NamedTuple, Optional
+from zipfile import BadZipFile
 
 from pip._vendor import pkg_resources
 from pip._vendor.packaging.requirements import Requirement
 from pip._vendor.packaging.utils import NormalizedName, canonicalize_name
 from pip._vendor.packaging.version import parse as parse_version
 
+from pip._internal.exceptions import InvalidWheel
 from pip._internal.utils import misc  # TODO: Move definition here.
 from pip._internal.utils.packaging import get_installer, get_metadata
 from pip._internal.utils.wheel import pkg_resources_distribution_for_wheel
@@ -34,8 +36,16 @@ class Distribution(BaseDistribution):
 
     @classmethod
     def from_wheel(cls, wheel: Wheel, name: str) -> "Distribution":
-        with wheel.as_zipfile() as zf:
-            dist = pkg_resources_distribution_for_wheel(zf, name, wheel.location)
+        """Load the distribution from a given wheel.
+
+        :raises InvalidWheel: Whenever loading of the wheel causes a
+            :py:exc:`zipfile.BadZipFile` exception to be thrown.
+        """
+        try:
+            with wheel.as_zipfile() as zf:
+                dist = pkg_resources_distribution_for_wheel(zf, name, wheel.location)
+        except BadZipFile as e:
+            raise InvalidWheel(wheel.location, name) from e
         return cls(dist)
 
     @property


Test patch:
diff --git a/tests/data/packages/corruptwheel-1.0-py2.py3-none-any.whl b/tests/data/packages/corruptwheel-1.0-py2.py3-none-any.whl
new file mode 100644
index 000000000..bf285f13f
--- /dev/null
+++ b/tests/data/packages/corruptwheel-1.0-py2.py3-none-any.whl
@@ -0,0 +1,1 @@
+This is a corrupt wheel which _clearly_ is not a zip file.
diff --git a/tests/functional/test_install_wheel.py b/tests/functional/test_install_wheel.py
index a87fe2933..c7045a9dc 100644
--- a/tests/functional/test_install_wheel.py
+++ b/tests/functional/test_install_wheel.py
@@ -45,13 +45,20 @@ def test_install_from_future_wheel_version(script, tmpdir):
     result.assert_installed("futurewheel", without_egg_link=True, editable=False)
 
 
-def test_install_from_broken_wheel(script, data):
+@pytest.mark.parametrize(
+    "wheel_name",
+    [
+        "brokenwheel-1.0-py2.py3-none-any.whl",
+        "corruptwheel-1.0-py2.py3-none-any.whl",
+    ],
+)
+def test_install_from_broken_wheel(script, data, wheel_name):
     """
     Test that installing a broken wheel fails properly
     """
     from tests.lib import TestFailure
 
-    package = data.packages.joinpath("brokenwheel-1.0-py2.py3-none-any.whl")
+    package = data.packages.joinpath(wheel_name)
     result = script.pip("install", package, "--no-index", expect_error=True)
     with pytest.raises(TestFailure):
         result.assert_installed("futurewheel", without_egg_link=True, editable=False)
diff --git a/tests/unit/test_network_lazy_wheel.py b/tests/unit/test_network_lazy_wheel.py
index 1d959d6b1..79e863217 100644
--- a/tests/unit/test_network_lazy_wheel.py
+++ b/tests/unit/test_network_lazy_wheel.py
@@ -1,9 +1,9 @@
 from typing import Iterator
-from zipfile import BadZipfile
 
 from pip._vendor.packaging.version import Version
 from pytest import fixture, mark, raises
 
+from pip._internal.exceptions import InvalidWheel
 from pip._internal.network.lazy_wheel import (
     HTTPRangeRequestUnsupported,
     dist_from_wheel_url,
@@ -62,5 +62,5 @@ def test_dist_from_wheel_url_no_range(
 @mark.network
 def test_dist_from_wheel_url_not_zip(session: PipSession) -> None:
     """Test handling with the given URL does not point to a ZIP."""
-    with raises(BadZipfile):
+    with raises(InvalidWheel):
         dist_from_wheel_url("python", "https://www.python.org/", session)
diff --git a/tests/unit/test_wheel.py b/tests/unit/test_wheel.py
index 37b5974eb..a698656b2 100644
--- a/tests/unit/test_wheel.py
+++ b/tests/unit/test_wheel.py
@@ -252,6 +252,15 @@ def test_wheel_root_is_purelib(text: str, expected: bool) -> None:
     assert wheel.wheel_root_is_purelib(message_from_string(text)) == expected
 
 
+def test_dist_from_broken_wheel_fails(data: TestData) -> None:
+    from pip._internal.exceptions import InvalidWheel
+    from pip._internal.metadata import FilesystemWheel, get_wheel_distribution
+
+    package = data.packages.joinpath("corruptwheel-1.0-py2.py3-none-any.whl")
+    with pytest.raises(InvalidWheel):
+        get_wheel_distribution(FilesystemWheel(package), "brokenwheel")
+
+
 class TestWheelFile:
     def test_unpack_wheel_no_flatten(self, tmpdir: Path) -> None:
         filepath = os.path.join(DATA_DIR, "packages", "meta-1.0-py2.py3-none-any.whl")