Skip to content

fix: parsing metadata with inline licenses #2806

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

Merged
merged 3 commits into from
Apr 22, 2025
Merged

fix: parsing metadata with inline licenses #2806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7351614
fix: parsing metadata with inline licenses
keith Apr 21, 2025
167131b
parse the whole METADATA file for Requires-Dist entries
aignas Apr 22, 2025
889f80c
Revert "parse the whole METADATA file for Requires-Dist entries"
aignas Apr 22, 2025
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
2 changes: 1 addition & 1 deletion python/private/pypi/whl_metadata.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def parse_whl_metadata(contents):
"version": "",
}
for line in contents.strip().split("\n"):
if not line.strip():
if not line:
# Stop parsing on first empty line, which marks the end of the
# headers containing the metadata.
break
Expand Down
31 changes: 31 additions & 0 deletions tests/pypi/whl_metadata/whl_metadata_tests.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,37 @@ Requires-Dist: this will be ignored

_tests.append(_test_parse_metadata_all)

def _test_parse_metadata_multiline_license(env):
got = _parse_whl_metadata(
env,
# NOTE: The trailing whitespace here is meaningful as an empty line
# denotes the end of the header.
contents = """\
Name: foo
Version: 0.0.1
License: some License

some line

another line

Requires-Dist: bar; extra == "all"
Provides-Extra: all

Requires-Dist: this will be ignored
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that it is probably more sensible to not ignore this line when parsing METADATA. I think the number of packages that will have Requires-Dist: in their description will be relatively small or non-existent, so it is better to go with that assumption at first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, let's not do that for now since, the two packages referenced in the issue will be handled by the fix here, e.g.: https://pypi-browser.org/package/mlflow/mlflow-2.22.0rc0-py3-none-any.whl/mlflow-2.22.0rc0.dist-info/METADATA

Copy link
Member Author

Choose a reason for hiding this comment

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

yea i was thinking that too, also as evidence by the previous parsing implementation parsed the whole file

""",
)
got.name().equals("foo")
got.version().equals("0.0.1")
got.requires_dist().contains_exactly([
"bar; extra == \"all\"",
])
got.provides_extra().contains_exactly([
"all",
])

_tests.append(_test_parse_metadata_multiline_license)

def whl_metadata_test_suite(name): # buildifier: disable=function-docstring
test_suite(
name = name,
Expand Down