fix(packaging): Format METADATA correctly if given empty requires_file

[FrankPortman]

An empty requires_file used to be okay, but at some point regressed to leaving an empty line (due to the metadata.replace(...)) in the METADATA file - rendering the wheel uninstallable.

This PR initially attempted to solve that by introducing a new list that processed METADATA lines 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.

[aignas]

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.

[aignas]

Should this be removed?

[FrankPortman]

Yep, I just copied and pasted it from the other test. Should I remove the print statement there as well?

[FrankPortman]

I am not sure I like using regex to solve this

I don't really like it either. If you look at the commit history you will see I attempted to reconstruct the METADATA file line by line, but that broke a bunch of tests. I am guessing the metadata.replace(....) calls were doing more than just "fixing" one line at a time, which felt confusing to me.

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.

It comes in via this file as far as I can tell:

rules_python/python/private/py_wheel.bzl

Line 396 in 2cb920c

if ctx.attr.python_requires:

Personally, I would prefer the empty file case to be handled seamlessly (like in this PR), than fail. I believe this is what rules_python used to do, because our code had been working until we upgraded. But I didn't do a thorough bisect search so I could be wrong (maybe we changed something else at the same time).

Right now, we wrap py_wheel in a macro, and it would be nice to not have to push the "is empty requires" to the analysis phase. But, we can always patch this internally if you prefer that the default behavior in rules_python is to error. Let me know how you want to proceed.

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.

Adding now.

[aignas]

Thank you!