Validate and update macOS platform tag in wheel name

[Czaki]

This PR introduces scanning all binaries in the wheel and checking if the platform tag in the wheel name is correct in the context of embedded binaries.

By default, it updates the wheel name. If --require-target-macos-version is provided or MACOSX_DEPLOYMENT_TARGET is provided, then delocate fails if the wheel does not meet versions provided this way.

[HexDecimal]

I'm in a good position to review this, and merge it once it's finished.

The following is required:

• New CLI flags mentioned in changelog
• Test passing and failing wheel names for --verify-name
• Test changed and unchanged names for --fix-name

test_scripts.py would be the appropriate place to add these tests.

[Czaki]

I will add tests later, but if you could clarify how it will best look for you the test fail.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eaba43d) 96.45% compared to head (51f03fc) 96.80%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   96.45%   96.80%   +0.35%     
==========================================
  Files          15       15              
  Lines        1184     1285     +101     
==========================================
+ Hits         1142     1244     +102     
+ Misses         42       41       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[HexDecimal]

Minor issue with code coverage, simply re-running the tests solved it. Or were you asking for advice on writing tests?

[Czaki]

Or were you asking for advice on writing tests?

No. I do not want to start writings test before clarifying the interface.

[Czaki]

One additional question. If both checks should be optional or some should be default behavior with option to disable?

[HexDecimal]

One additional question. If both checks should be optional or some should be default behavior with option to disable?

If you made this not optional then that would simplify the code. There would be no need to verify the versions anymore if this always sets the version correctly. No need for the additional CLI flags either. I can't imagine use cases where anyone would want to keep the wrong version intentionally, but feel free to correct me.

I looked at issue #56 but it appears you're the one who made that issue. I don't have enough MacOS experience to add much additional insight. Maybe reduce the checks suggested here to only --require-target-macos-version <max_version> which errors if an upper bound is exceeded. I'll assume this PR closes #56.

To clarify what I'm suggesting:

• Always rename wheels to the current minimum supported version regardless of flags given.
• Verify minimum version compared to --require-target-macos-version <max_version> when provided. If verification fails then output which libraries are the cause and which versions they wanted.
• Remove --verify-name and --fix-name. Fixing is always implied and verification needs a target version to compare to which can't be determined automatically.

If you want an error message format, how about this:

Library dependencies do not satisfy target MacOS version {require_target_macos_version}:
{lib_path} has a minimum target of {lib_macos_version}
...  # repeat for each failed library

[Czaki]

I have applied requested changes. I have added the option to activate require-target-macos-version by setting MACOSX_DEPLOYMENT_TARGET environment variable.

Should I propose changes in README about new error?

EDIT. If wheel name does not fit the proper pattern, should run fail (and I need to fix all tests) or it should skip name validation then?

[HexDecimal]

If wheel name does not fit the proper pattern, should run fail (and I need to fix all tests) or it should skip name validation then?

Wheel names must be valid after renaming, there are PEP's for what is a valid name. Wheel inputs should be valid enough for tools to work. I'll look into the test errors and the wheel spec to figure out why tests are currently failing.

Should I propose changes in README about new error?

The most important place is the CHANGELOG. It should note the new CLI flag and that wheels might be tagged with a different MacOS version than before. It should also note that new ENV variables will be read.

You can also explain these in the README, but the CHANGELOG is more important since there needs to be an easy to read record of when these changes have happened.

[HexDecimal]

It seems that current tests assume wheels can be given arbitrary names when used with Delocate, and your new code assumes wheels have a full valid name to work with to pull tags from the filename. I think your code renaming files has broken the assumption made by Delocate that wheels will always have the same name before and after delocation and that the actual name doesn't matter.

After some consideration I think Delocate is wrong to have this assumption, and I think it's safe to force wheels to have valid input names. This will require rewriting some tests. This might break the tool for users abusing wheel names, but regular users will be unaffected. Stricter inputs will need to be mentioned in the CHANGELOG.

Wheels have tag information included in the wheel metadata, this can be read to get the tags of a wheel with an invalid name. I assume it must be updated when the tags of a wheel are changed as well. The metadata location is based on a valid wheel name itself so it isn't an alternative to a valid name.

More info: https://packaging.python.org/en/latest/specifications/binary-distribution-format/

I think Delocate might be breaking several packaging rules, but I'd have to take a longer look to know for sure how bad it is.

If wheel name does not fit the proper pattern, should run fail (and I need to fix all tests) or it should skip name validation then?

With all this in mind, the current tests are broken and need to be fixed by giving temporary wheels valid names (or by putting the copied wheels into their own temporary directories so they that don't conflict with each other.)

[Czaki]

Ok. I have added tests and changed behavior to requested.

I do not have an idea how properly test test_check_plat_archs (failing currently).

As the parser is defined globaly in delocate.cmd.delocate_wheel it is not easy to run test_delocate_wheel_verify_name_universal2_verify_crash with MACOSX_DEPLOYMENT_TARGET environment variable. It could be fixed by moving the parser definition to a function or reload module using importlib.reload.

[Czaki]

I assume the tags in {distribution}-{version}.dist-info/WHEEL were not updated yet. It is probably incorrect to leave them unchanged.

I miss this. Earlier. I have fixed it now.

I think this currently leaves the original file alone if the name is changed. This will mess up the distribution and deployment of most projects. If this renames a wheel then the original must be deleted (if it's in the same directory).

You were right. It is fixed now.

[HexDecimal]

No major issues as far as I can tell. There's one last thing I'd like to change, after that I think this PR is finished.

[Czaki]

When checking the code again, I realized that without adding this check, the error output may be a little too verbose.

Case.
Someone compiled unniversal2 wheel where all x86_64 deps have targeted 10.9 and all except one arm64 deps have targeted 11.0. The problematic one had target 12.0. Without this fix, all arm64 deps will be listed as problematic. With these lines added (and modified call), only single problematic one.

Should I add tests?

[HexDecimal]

I have trouble understanding this. I do not know or remember enough about the architecture versioning history. I'll trust your knowledge on this is correct.

Adding a test might help with explaining this to me. This is also handling a edge case which should have its own test.

[Czaki]

Short summary.
The first macOS that supports arm64 processors is macOS 11.0. So no arm64 binary will be compatible with macOS from line 10.x

Also, compilers will just simple bump the deployment target to 11.0 if the requested value is lower.

Some time ago, wheel maintainers decided that universal2 wheels could have a platform target below 11.0 if all arm64 binaries are 11.0 compatibles.

I have added a comment with an explanation of the function, could you check?

[HexDecimal]

That sounds right. It was a good idea to add this extra check. Everything so far looks good.