Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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.

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

Conversation

keith
Copy link
Member

@keith keith commented Apr 21, 2025

The wheel METADATA parsing implemented in 1.4 missed the fact
that whitespace is significant and sometimes License is included
inline in the METADATA file itself.

This change ensures that we stop parsing the METADATA file only
on first completely empty line.

Fixes #2796

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

Thank you! CHANGELOG will need to be updated, but I can also do this.

EDIT: decided to not update the changelog since there have been no stable release yet.

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

@aignas aignas enabled auto-merge April 22, 2025 12:46
@aignas aignas added this pull request to the merge queue Apr 22, 2025
Merged via the queue into bazel-contrib:main with commit 1d69ad6 Apr 22, 2025
3 checks passed
aignas added a commit that referenced this pull request Apr 22, 2025
The wheel `METADATA` parsing implemented in 1.4 missed the fact
that whitespace is significant and sometimes License is included
inline in the `METADATA` file itself.

This change ensures that we stop parsing the `METADATA` file only
on first completely empty line.

Fixes #2796

---------

Co-authored-by: Ignas Anikevicius <240938+aignas@users.noreply.github.com>
(cherry picked from commit 1d69ad6)
@keith keith deleted the ks/fix-parsing-metadata-with-inline-licenses branch April 22, 2025 16:43
@JeroenSchmidt
Copy link
Contributor

Thanks for the quick fix @keith the issue looks resolved on my side!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression (1.4.0-rc0) | Transient python pypi deps not working
3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.