Skip to content
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.

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

Restore support for pkginfo 1.11 #1123

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions docs/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@
# TODO: Try to add these to intersphinx_mapping
nitpick_ignore_regex = [
(r"py:.*", r"pkginfo.*"),
("py:class", r"warnings\.WarningMessage"),
]

# -- Options for apidoc output ------------------------------------------------
Expand Down
4 changes: 1 addition & 3 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ dependencies = [
"keyring >= 15.1",
"rfc3986 >= 1.4.0",
"rich >= 12.0.0",

# workaround for #1116
"pkginfo < 1.11",
"packaging",
]
dynamic = ["version"]

Expand Down
34 changes: 25 additions & 9 deletions tests/test_package.py
Original file line number Diff line number Diff line change
Expand Up @@ -383,11 +383,6 @@ def test_fips_metadata_excludes_md5_and_blake2(monkeypatch):
@pytest.mark.parametrize(
"read_data, missing_fields",
[
pytest.param(
b"Metadata-Version: 102.3\nName: test-package\nVersion: 1.0.0\n",
"Name, Version",
id="unsupported Metadata-Version",
),
pytest.param(
b"Metadata-Version: 2.3\nName: UNKNOWN\nVersion: UNKNOWN\n",
"Name, Version",
Expand Down Expand Up @@ -421,10 +416,7 @@ def test_fips_metadata_excludes_md5_and_blake2(monkeypatch):
],
)
def test_pkginfo_returns_no_metadata(read_data, missing_fields, monkeypatch):
"""Raise an exception when pkginfo can't interpret the metadata.

This could be caused by a version number or format it doesn't support yet.
"""
"""Raise an exception when pkginfo can't interpret the metadata."""
monkeypatch.setattr(package_file.wheel.Wheel, "read", lambda _: read_data)
filename = "tests/fixtures/twine-1.5.0-py2.py3-none-any.whl"

Expand All @@ -434,9 +426,33 @@ def test_pkginfo_returns_no_metadata(read_data, missing_fields, monkeypatch):
assert (
f"Metadata is missing required fields: {missing_fields}." in err.value.args[0]
)


def test_pkginfo_unrecognized_version(monkeypatch):
"""Raise an exception when pkginfo doesn't recognize the version."""
data = b"Metadata-Version: 102.3\nName: test-package\nVersion: 1.0.0\n"
monkeypatch.setattr(package_file.wheel.Wheel, "read", lambda _: data)
filename = "tests/fixtures/twine-1.5.0-py2.py3-none-any.whl"

with pytest.raises(exceptions.InvalidDistribution) as err:
package_file.PackageFile.from_filename(filename, comment=None)

assert "1.0, 1.1, 1.2, 2.0, 2.1, 2.2" in err.value.args[0]


def test_pkginfo_returns_no_metadata_py_below_1_11(monkeypatch):
"""Raise special msg when pkginfo can't interpret metadata on pkginfo < 1.11."""
data = b"Metadata-Version: 2.2\nName: UNKNOWN\nVersion: 1.0.0\n"
monkeypatch.setattr(package_file.wheel.Wheel, "read", lambda _: data)
monkeypatch.setattr(package_file.importlib_metadata, "version", lambda pkg: "1.10")
filename = "tests/fixtures/twine-1.5.0-py2.py3-none-any.whl"

with pytest.raises(exceptions.InvalidDistribution) as err:
package_file.PackageFile.from_filename(filename, comment=None)

assert "Make sure the distribution includes" in err.value.args[0]


def test_malformed_from_file(monkeypatch):
"""Raise an exception when malformed package file triggers EOFError."""
filename = "tests/fixtures/malformed.tar.gz"
Expand Down
68 changes: 55 additions & 13 deletions twine/package.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,26 @@
import re
import subprocess
import sys
from typing import Any, Dict, List, NamedTuple, Optional, Sequence, Tuple, Union, cast
import warnings
from typing import (
Any,
Dict,
Iterable,
List,
NamedTuple,
Optional,
Sequence,
Tuple,
Union,
cast,
)

if sys.version_info >= (3, 10):
import importlib.metadata as importlib_metadata
else:
import importlib_metadata

import packaging.version
import pkginfo
from rich import print

Expand Down Expand Up @@ -65,12 +78,19 @@ def _safe_name(name: str) -> str:
return re.sub("[^A-Za-z0-9.]+", "-", name)


class CheckedDistribution(pkginfo.Distribution):
"""A Distribution whose name and version are confirmed to be defined."""

name: str
version: str


class PackageFile:
def __init__(
self,
filename: str,
comment: Optional[str],
metadata: pkginfo.Distribution,
metadata: CheckedDistribution,
python_version: Optional[str],
filetype: Optional[str],
) -> None:
Expand Down Expand Up @@ -100,7 +120,8 @@ def from_filename(cls, filename: str, comment: Optional[str]) -> "PackageFile":
for ext, dtype in DIST_EXTENSIONS.items():
if filename.endswith(ext):
try:
meta = DIST_TYPES[dtype](filename)
with warnings.catch_warnings(record=True) as captured:
meta = DIST_TYPES[dtype](filename)
except EOFError:
raise exceptions.InvalidDistribution(
"Invalid distribution file: '%s'" % os.path.basename(filename)
Expand All @@ -112,22 +133,29 @@ def from_filename(cls, filename: str, comment: Optional[str]) -> "PackageFile":
"Unknown distribution format: '%s'" % os.path.basename(filename)
)

# If pkginfo encounters a metadata version it doesn't support, it may give us
supported_metadata = list(pkginfo.distribution.HEADER_ATTRS)
if cls._is_unknown_metadata_version(captured):
raise exceptions.InvalidDistribution(
"Make sure the distribution is using a supported Metadata-Version: "
f"{', '.join(supported_metadata)}."
)
# If pkginfo <1.11 encounters a metadata version it doesn't support, it may give
# back empty metadata. At the very least, we should have a name and version,
# which could also be empty if, for example, a MANIFEST.in doesn't include
# setup.cfg.
missing_fields = [
f.capitalize() for f in ["name", "version"] if not getattr(meta, f)
]
if missing_fields:
supported_metadata = list(pkginfo.distribution.HEADER_ATTRS)
raise exceptions.InvalidDistribution(
"Metadata is missing required fields: "
f"{', '.join(missing_fields)}.\n"
"Make sure the distribution includes the files where those fields "
"are specified, and is using a supported Metadata-Version: "
f"{', '.join(supported_metadata)}."
)
msg = f"Metadata is missing required fields: {', '.join(missing_fields)}."
if cls._pkginfo_before_1_11():
msg += (
"\n"
"Make sure the distribution includes the files where those fields "
"are specified, and is using a supported Metadata-Version: "
f"{', '.join(supported_metadata)}."
)
raise exceptions.InvalidDistribution(msg)

py_version: Optional[str]
if dtype == "bdist_egg":
Expand All @@ -140,7 +168,21 @@ def from_filename(cls, filename: str, comment: Optional[str]) -> "PackageFile":
else:
py_version = None

return cls(filename, comment, meta, py_version, dtype)
return cls(
filename, comment, cast(CheckedDistribution, meta), py_version, dtype
)

@staticmethod
def _is_unknown_metadata_version(
captured: Iterable[warnings.WarningMessage],
) -> bool:
NMV = getattr(pkginfo.distribution, "NewMetadataVersion", None)
return any(warning.category is NMV for warning in captured)

@staticmethod
def _pkginfo_before_1_11() -> bool:
ver = packaging.version.Version(importlib_metadata.version("pkginfo"))
Copy link
Member

Choose a reason for hiding this comment

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

I can't believe we really need packaging just for these two lines. This is just excessive

Choose a reason for hiding this comment

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

Would you prefer to use python-semver? or just some sort of approach to avoid another dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't believe it either. It also requires importlib_metadata.

Unfortunately, packaging is the "standard" way to parse version numbers in Python. Also, this dependency goes away after dropping support for older pkginfo. It felt like a small compromise to require it for that short-term consideration.

I'm very much not interested in writing a version parser/comparator when one already exists. The whole point of libraries is to publish reusable, tested, robust implementations of behavior.

This comment comes off as a criticism without any advice on how to proceed. It's unclear what "excessive" even means. Would it also be excessive to vendor packaging? What about to copy just the version parsing logic? Any change that doesn't use packaging is almost certainly going to require more lines of code than this change, so in some sense is more involved than this excessive approach. I'm uninterested in fulfilling "bring another rock" just to have it declined. Please make a recommendation on what changes would be acceptable or adapt the change to be acceptable.

Choose a reason for hiding this comment

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

Are there any concerns over this PR that still need to be further addressed?

(I've been building and using twine from this branch and it continues to work correctly!)

return ver < packaging.version.Version("1.11")

def metadata_dictionary(self) -> Dict[str, MetadataValue]:
"""Merge multiple sources of metadata into a single dictionary.
Expand Down
5 changes: 4 additions & 1 deletion twine/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,10 @@ def _upload(self, package: package_file.PackageFile) -> requests.Response:

with open(package.filename, "rb") as fp:
data_to_send.append(
("content", (package.basefilename, fp, "application/octet-stream"))
(
"content",
(package.basefilename, fp, "application/octet-stream"),
)
)
encoder = requests_toolbelt.MultipartEncoder(data_to_send)

Expand Down
Loading