Skip to content

Tighten and simplify extraction of metadata from wheels #1201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Tighten and simplify extraction of metadata from wheels #1201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04f497c
Tighten regular expression used to validate wheel filenames
dnicolodi Dec 12, 2024
b34f03a
Tighten and simplify lookup for METADATA member in wheel archives
dnicolodi Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 38 additions & 40 deletions tests/test_wheel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import pathlib
import re
import zipfile

import pretend
import pytest

from twine import exceptions
Expand All @@ -33,26 +31,24 @@ def test_version_parsing(example_wheel):
assert example_wheel.py_version == "py2.py3"


def test_version_parsing_missing_pyver(monkeypatch, example_wheel):
wheel.wheel_file_re = pretend.stub(match=lambda a: None)
assert example_wheel.py_version == "any"
@pytest.mark.parametrize(
"file_name,valid",
[
("name-1.2.3-py313-none-any.whl", True),
("name-1.2.3-42-py313-none-any.whl", True),
("long_name-1.2.3-py3-none-any.whl", True),
("missing_components-1.2.3.whl", False),
],
)
def test_parse_wheel_file_name(file_name, valid):
assert bool(wheel.wheel_file_re.match(file_name)) == valid


def test_find_metadata_files():
names = [
"package/lib/__init__.py",
"package/lib/version.py",
"package/METADATA.txt",
"package/METADATA.json",
"package/METADATA",
]
expected = [
["package", "METADATA"],
["package", "METADATA.json"],
["package", "METADATA.txt"],
]
candidates = wheel.Wheel.find_candidate_metadata_files(names)
assert expected == candidates
def test_invalid_file_name(monkeypatch):
parent = pathlib.Path(__file__).parent
file_name = str(parent / "fixtures" / "twine-1.5.0-py2.whl")
with pytest.raises(exceptions.InvalidDistribution, match="Invalid wheel filename"):
wheel.Wheel(file_name)


def test_read_valid(example_wheel):
Expand All @@ -64,33 +60,35 @@ def test_read_valid(example_wheel):

def test_read_non_existent_wheel_file_name():
"""Raise an exception when wheel file doesn't exist."""
file_name = str(pathlib.Path("/foo/bar/baz.whl").resolve())
with pytest.raises(
exceptions.InvalidDistribution, match=re.escape(f"No such file: {file_name}")
):
file_name = str(pathlib.Path("/foo/bar/baz-1.2.3-py3-none-any.whl").resolve())
with pytest.raises(exceptions.InvalidDistribution, match="No such file"):
wheel.Wheel(file_name).read()


def test_read_invalid_wheel_extension():
"""Raise an exception when file is missing .whl extension."""
file_name = str(pathlib.Path(__file__).parent / "fixtures" / "twine-1.5.0.tar.gz")
with pytest.raises(
exceptions.InvalidDistribution,
match=re.escape(f"Not a known archive format for file: {file_name}"),
):
with pytest.raises(exceptions.InvalidDistribution, match="Invalid wheel filename"):
wheel.Wheel(file_name).read()


def test_read_wheel_missing_metadata(example_wheel, monkeypatch):
"""Raise an exception when a wheel file is missing METADATA."""

def patch(self, name):
raise KeyError

monkeypatch.setattr(zipfile.ZipFile, "read", patch)
parent = pathlib.Path(__file__).parent
file_name = str(parent / "fixtures" / "twine-1.5.0-py2.py3-none-any.whl")
with pytest.raises(exceptions.InvalidDistribution, match="No METADATA in archive"):
wheel.Wheel(file_name).read()


def test_read_wheel_empty_metadata(tmpdir):
def test_read_wheel_empty_metadata(example_wheel, monkeypatch):
"""Raise an exception when a wheel file is missing METADATA."""
whl_file = tmpdir.mkdir("wheel").join("not-a-wheel.whl")
with zipfile.ZipFile(whl_file, "w") as zip_file:
zip_file.writestr("METADATA", "")

with pytest.raises(
exceptions.InvalidDistribution,
match=re.escape(
f"No METADATA in archive or METADATA missing 'Metadata-Version': {whl_file}"
),
):
wheel.Wheel(whl_file).read()
monkeypatch.setattr(zipfile.ZipFile, "read", lambda self, name: b"")
parent = pathlib.Path(__file__).parent
file_name = str(parent / "fixtures" / "twine-1.5.0-py2.py3-none-any.whl")
with pytest.raises(exceptions.InvalidDistribution, match="No METADATA in archive"):
wheel.Wheel(file_name).read()
64 changes: 22 additions & 42 deletions twine/wheel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,68 +14,48 @@
import os
import re
import zipfile
from typing import List
from contextlib import suppress

from twine import distribution
from twine import exceptions

wheel_file_re = re.compile(
r"""^(?P<namever>(?P<name>.+?)(-(?P<ver>\d.+?))?)
((-(?P<build>\d.*?))?-(?P<pyver>.+?)-(?P<abi>.+?)-(?P<plat>.+?)
\.whl|\.dist-info)$""",
r"""^(?P<name>[^-]+)-
(?P<version>[^-]+)
(:?-(?P<build>\d[^-]*))?-
(?P<pyver>[^-]+)-
(?P<abi>[^-]+)-
(?P<plat>[^-]+)
\.whl$""",
re.VERBOSE,
)


class Wheel(distribution.Distribution):
def __init__(self, filename: str) -> None:
m = wheel_file_re.match(os.path.basename(filename))
if not m:
raise exceptions.InvalidDistribution(f"Invalid wheel filename: {filename}")
self.name = m.group("name")
self.version = m.group("version")
self.pyver = m.group("pyver")

if not os.path.exists(filename):
raise exceptions.InvalidDistribution(f"No such file: {filename}")
self.filename = filename

@property
def py_version(self) -> str:
wheel_info = wheel_file_re.match(os.path.basename(self.filename))
if wheel_info is None:
return "any"
else:
return wheel_info.group("pyver")

@staticmethod
def find_candidate_metadata_files(names: List[str]) -> List[List[str]]:
"""Filter files that may be METADATA files."""
tuples = [x.split("/") for x in names if "METADATA" in x]
return [x[1] for x in sorted((len(x), x) for x in tuples)]
return self.pyver

def read(self) -> bytes:
fqn = os.path.abspath(os.path.normpath(self.filename))
Copy link
Member

@sigmavirus24 sigmavirus24 Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we lose this if we just accept filename as passed into the Wheel object, no?

Copy link
Contributor Author

@dnicolodi dnicolodi Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intended. First, os.path.abspath(path) is equivalent to os.path.normpath(os.getcwd(), path) thus normalizing with normpath() before calling abspath() is redundant. Then, normpath() operates only on the representation of the path and does funny things with symbolic links. Most likely corner cases, but making corner cases more confusing is not making them easier to diagnose. Third, I like the error messages to refer to the path as provided by the user and not to a "normalized" path.

TL;DR: I didn't understand why this was a good idea.

if not os.path.exists(fqn):
raise exceptions.InvalidDistribution("No such file: %s" % fqn)
Copy link
Member

@sigmavirus24 sigmavirus24 Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're also losing the clarity from this exception if the give file/path does not exist?

Copy link
Contributor Author

@dnicolodi dnicolodi Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same check existed in the class constructor and in the read() method. I just removed one copy of it. Technically the check in the constructor checks the path as provided by the user, and here the check is on the normalized path, but if the two disagree something went wrong with the normalization (see argument about symlinks above) and it is a bug and not a feature. The SDist class does the check in the constructor, so I kept the check in the constructor here too.

Copy link
Contributor Author

@dnicolodi dnicolodi Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the Wheel class can be used to parse the wheel file name into components (name, version, pyver, and the others could be added, if needed), and that does not depend on accessing the file, thus it could be argued that keeping the file existence check in the read() method would be better. I don't have a preference.


if fqn.endswith(".whl"):
archive = zipfile.ZipFile(fqn)
names = archive.namelist()

def read_file(name: str) -> bytes:
return archive.read(name)

else:
raise exceptions.InvalidDistribution(
"Not a known archive format for file: %s" % fqn
)

searched_files: List[str] = []
try:
for path in self.find_candidate_metadata_files(names):
candidate = "/".join(path)
data = read_file(candidate)
with zipfile.ZipFile(self.filename) as wheel:
with suppress(KeyError):
# The wheel needs to contain the METADATA file at this location.
data = wheel.read(f"{self.name}-{self.version}.dist-info/METADATA")
if b"Metadata-Version" in data:
return data
searched_files.append(candidate)
finally:
archive.close()

raise exceptions.InvalidDistribution(
"No METADATA in archive or METADATA missing 'Metadata-Version': "
"%s (searched %s)" % (fqn, ",".join(searched_files))
"No METADATA in archive or "
f"METADATA missing 'Metadata-Version': {self.filename}"
)
Loading