fix(packaging): Format METADATA correctly if given empty requires_file#2771
fix(packaging): Format METADATA correctly if given empty requires_file#2771aignas merged 12 commits intobazel-contrib:mainfrom
METADATA correctly if given empty requires_file#2771Conversation
aignas
left a comment
aignas
left a comment
There was a problem hiding this comment.
Thanks, I am not sure I like using regex to solve this, but I see how doing that makes the change smaller and less invasive.
Do you know where the Requires-Dist appears from in the first place? Maybe we should error if the requirements are empty/not-specified? I see that the metadata file is read from a file, so I assume this could be templated out somewhere else in our code.
Extra things that this PR will need:
- The packaging rule should have
:::versionchanged:::when we agree on the behaviour change. - The CHANGELOG entry should be there mentioning the fix briefly.
examples/wheel/wheel_test.py
Outdated
| if line.startswith("Requires-Dist:"): | ||
| requires.append(line.strip()) | ||
|
|
||
| print(requires) |
There was a problem hiding this comment.
Yep, I just copied and pasted it from the other test. Should I remove the print statement there as well?
I don't really like it either. If you look at the commit history you will see I attempted to reconstruct the
It comes in via this file as far as I can tell: rules_python/python/private/py_wheel.bzl Line 396 in 2cb920c Personally, I would prefer the empty file case to be handled seamlessly (like in this PR), than fail. I believe this is what Right now, we wrap
Adding now. |
2 participants
An empty
requires_fileused to be okay, but at some point regressed to leaving an empty line (due to themetadata.replace(...)) in theMETADATAfile - rendering the wheel uninstallable.This PR initially attempted to solve that by introducing a new list that processed
METADATAlines go into, rather than relying on repeated string replacement. But it seems like the repeated string replace actually did more than simply process one line at a time, so I reverted to a single substitution at the end.