Retag wheels automatically when fusing

[dunkmann00]

This is my attempt at a PR for #174.

This adds the ability to "retag" a universal2 fused wheel. When running delocate-fuse with this flag, it will update the filename and the dist-info file of the fused wheel to reflect that it is a universal2 wheel.

I didn't update the readme or add any tests yet. I wanted to first get feedback on whether or not this is the "retag" you had in mind.

Let me know what you think/if there is anything you would like changed!

[HexDecimal]

I feel that anything updating the wheel tags should reuse the _check_and_update_wheel_name and _update_wheelfile functions.

[dunkmann00]

_update_wheelfile looks good, I'll update it to use that (although its implementation is weird in that it doesn't use read_pkg_info and write_pkg_info)

_check_and_update_wheel_name I'm not sure about. Looking at it, I think I'll still have to manually update the filename to end in universal2.whl, since that function only deals with updating the minimum platform tag, not the arch. And I don't need to do anything fancy to determine the minimum tag like _check_and_update_wheel_name does (like actually checking the binaries with macholib). So I'm not sure using that is helpful, unless I'm missing something?

Are you okay with raising the errors like I did when the two wheels are not correct/have too many tags?

[HexDecimal]

_update_wheelfile looks good, I'll update it to use that (although its implementation is weird in that it doesn't use read_pkg_info and write_pkg_info)

I completely missed those functions. It would be good to update the code to use them.

_check_and_update_wheel_name I'm not sure about. Looking at it, I think I'll still have to manually update the filename to end in universal2.whl, since that function only deals with updating the minimum platform tag, not the arch. And I don't need to do anything fancy to determine the minimum tag like _check_and_update_wheel_name does (like actually checking the binaries with macholib). So I'm not sure using that is helpful, unless I'm missing something?

Since you're updating the tag already then it would be a good idea to verify that the minimum platform tag is correct. I mostly want tag renaming to behave similarly to the wheel-delocate script.

I doubt that _check_and_update_wheel_name currently knows how to handle a universal wheel. I'll need to think about this. There seems to be a lot of edge cases.

I'd like retagging to be the default behavior rather than an optional flag. Not verifying the tags has left a lot of mis-tagged wheels around.

Are you okay with raising the errors like I did when the two wheels are not correct/have too many tags?

I'm not sure. I like the verification and the error massage but not that it's hardcoded to universal2. I also find it awkward to deal with a new exception class but that's not a deal breaker.

I'd prefer a generic extendable solution to expected tags:

_FUSEABLE_TAGS = {
    frozenset(("x86_64", "arm64")): "universal2",
}

[dunkmann00]

I completely missed those functions. It would be good to update the code to use them.

👍

I'd like retagging to be the default behavior rather than an optional flag. Not verifying the tags has left a lot of mis-tagged wheels around.

I agree with this. Wasn't sure that was what the maintainers wanted, but if it is, then sounds good to me.

I'm not sure. I like the verification and the error massage but not that it's hardcoded to universal2. I also find it awkward to deal with a new exception class but that's not a deal breaker.

I'd prefer a generic extendable solution to expected tags:

That approach seems interesting. When I started doing this I was unsure what scope to make this "retagging" functionality. On the one had like you point out, it does seem like it could (should?) be generalized to support various situations. But on the other hand, I was unsure how many different variety of wheels (not binaries, because I think lipo can fuse more than are actually relevant for Python wheels) actually can be fused together. And in particular, what wheels can and are being fused together today. If you look at the project's README:

Making dual architecture binaries

...
One solution to this problem is to do an entire arm64 wheel build, and then an entire x86_64 wheel build, and fuse the two wheels into a universal wheel.

When I looked at that it gave me the impression this is essentially why delocate-fuse exists. Not to handle fusing arbitrary wheels together, but to fuse x86_64 and arm64 wheels together. It might be nice to design the functionality in a way that can be extended if needed, but there seems to be enough edge cases as you mention, that this is a non trivial task. So I decided to add this in a way to only handle a very specific and known situation. This made it much less complex, so easier to get right, and also gives the ability to provide clear feedback to the user as to what "retag" supports.

To be clear, I'm not opposed to your sentiment of wanting a general and clean solution. But I'm not sure that the extra flexibility is worth the extra complexity. If there is a very clear set of what conditions to support with retagging, then that might be workable. But if we are trying to support unknown fusing conditions, which either don't currently exist or at the very least are not reasons people use delocate-fuse today, then that seems like a potentially unnecessarily difficult task.

I'll update the parts we have discussed and see where that takes me.

[dunkmann00]

Just pushed some new commits and I think I may have been wrong about it being overly difficult! With a slight tweak to _get_archs_and_version_from_wheel_name I was able to use the two functions you mentioned to get the universal2 wheel retagged. This new approach is also much simpler and I believe would handle any case. I no longer needed the universal2 checks or any error handling, so I was able to remove all of that.

I left it as a flag as I wanted to double check about making it the default behavior. Since that would be a breaking change, I just wanted to confirm that thats not a problem.

[HexDecimal]

I left it as a flag as I wanted to double check about making it the default behavior. Since that would be a breaking change, I just wanted to confirm that thats not a problem.

I'd like the default behavior changed for the reasons I mentioned above. Added or changed features must be clearly marked in the changelog and anyone relying on old behavior can pin their version of Delocate. It would be nice to have @matthew-brett's opinion as well but I'm not sure if they'll be available.

[HexDecimal]

Make sure to note the changed behavior in the changelog.

[matthew-brett]

Am I right in thinkng this is going to break anyone's automated workflow that is using delocate-fuse currently? Because the output wheel name is going to change?

I suspect use of delocate-fuse is somewhat common in CI - is that a reasonable guess?

If so - is there anything we can do not to have a torrent of 'you broke my CI' complaints? How about (sorry if I missed it), a --retag command line argument, with a --no-retag option giving the prior default, keeping the current default, and warning that the default will change in a future version?

[HexDecimal]

Keep in mind that this situation is a result of delocate-fuse being deployed in such a broken state in the first place.

Am I right in thinkng this is going to break anyone's automated workflow that is using delocate-fuse currently? Because the output wheel name is going to change?

If they follow Semantic Versioning then they're fine. Anyone using the typical dependency specifiers to keep things stable, such as delocate==0.11.0 or delocate~=0.11.0 won't have their scripts broken. Many don't pin their dependencies though.

It's possible for those still using the workaround from the readme's "Making dual-architecture binaries" section to rename the wrong wheel after an update if they decided to not use the --wheel-dir flag in their script. This is the worst case scenario since the issue will be subtle, but at least the workaround suggests and demonstrates using --wheel-dir.

I suspect use of delocate-fuse is somewhat common in CI - is that a reasonable guess?

Not nearly as common as delocate-wheel, but it's enough to see devs making feature requests for delocate-fuse on the issues tab, specifically #174 which was asking for this breaking change in the first place.

In my experience my wheels already have universal platform tags and universal dependencies before I even run Delocate on them so anything involving delocate-fuse would be more complex than plain delocate-wheel. I suspect this is the same for most devs since it's the most intuitive way to use Delocate.

If so - is there anything we can do not to have a torrent of 'you broke my CI' complaints?

These complaints have simple answers. Either they pin version 0.11.0 or they modify their scripts to handle the updated output. If they read the changelog then they'll know everything they need without having to raise an issue.

I think that sorting out complaints would take less time and effort then dealing with the technical debt of keeping the old behavior in the current code.

There were CI complaints for 0.11.0 as well and those weren't hard to deal with.

How about (sorry if I missed it), a --retag command line argument, with a --no-retag option giving the prior default, keeping the current default, and warning that the default will change in a future version?

They can pin delocate==0.11.0 to invoke the old behavior is my proposal.

It's mainly you who wants to keep the old behavior as an option. The main issue solved is that the old behavior is unintuitive especially due to how delocate-fuse never even asks for an output name and simply outputs into one of the inputs. Having to manually rename the wheel afterwards is also prone to issues and typically results in a mis-tagged wheel.

[codecov]

Codecov Report

Attention: Patch coverage is 98.55072% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 96.95%. Comparing base (f6af445) to head (a79cb51).

Files	Patch %	Lines
delocate/cmd/delocate_merge.py	94.44%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #215   +/-   ##
=======================================
  Coverage   96.95%   96.95%           
=======================================
  Files          15       16    +1     
  Lines        1282     1315   +33     
=======================================
+ Hits         1243     1275   +32     
- Misses         39       40    +1     

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

[HexDecimal]

Another option would be to rename the command to delocate-fuse-universal and have delocate-fuse raise an error pointing to delocate -fuse-universal. At least that way no-one will be confused when they get the error.

I've considered replacing the syntax of delocate-fuse to force users to notice the update. It isn't a solution I like, but it's the only option effective at preventing the most subtle issues from ever happening.

Something that would work for most cases would be to make --wheel-dir a required flag. It can be made optional again in a later release.

[matthew-brett]

Anything that raises an informative error would be fine I think.

[dunkmann00]

What if we add a flag --overwrite/--no-overwrite. The default when neither is given, for now, being --overwrite. But in this case, we raise the informative error @matthew-brett would like to see to notify users they must use the --no-overwrite flag in versions 0.12 or over. In a future version we can change the default to --no-overwrite or just remove the flag entirely.

So as an example:

delocate-fuse wheel1.whl wheel2.whl  # Raises error where we inform the need for `--no-overwrite`

delocate-fuse --no-overwrite wheel1.whl wheel2.whl  # Runs without error

In this case, we probably would not need to raise any error if --wheel-dir was used. So maybe thinking about the error message, it could be something that indicates for version 0.12 and above, overwriting the first wheel is an error, you must use either --no-overwrite or --wheel-dir, and in a future version --no-overwrite will become the default behavior.

[dunkmann00]

I just fixed the last test I added. Total user error, forgot to add the right hand side of that assert 😅 sorry about that.

[HexDecimal]

What if we add a flag --overwrite/--no-overwrite. The default when neither is given, for now, being --overwrite. But in this case, we raise the informative error matthew-brett would like to see to notify users they must use the --no-overwrite flag in versions 0.12 or over. In a future version we can change the default to --no-overwrite or just remove the flag entirely.

This would sound perfectly reasonable, but the issue with this is that no matter how loudly you try to announce the change, devs won't notice it until the change is finalized and their scripts break.

The reason I suggest requiring --wheel-dir is that this would break almost any script relying on the wheel reusing the name of the existing wheels. Those scripts would fail to find the wheel with the old name in the given directory and crash. Scripts not using --wheel-dir will never notice that there's a new wheel and will always use the incorrect wheel for the rest of the script which is why they have to be broken deliberately. With --wheel-dir the wheel outputted to the wheel directory is always the fused wheel even when a broad wildcard is used to select it. The only bad case is when --wheel-dir is the same directory as where the inputs are, but that is not how the flag is typically used.

For announcing the changes to the user, I think the most reasonable thing you can do is simply print where wheel was outputted to stdout. By adding the following to the end of the delocate-fuse main function:

print(f"Fused wheel written to {out_wheel}")

If the user doesn't notice this then they're not going to notice anything else less than an error.

[dunkmann00]

I'm suggesting we don't just print something out, but raise an error and print something out. I thought based on @matthew-brett's comments:

Another option would be to rename the command to delocate-fuse-universal and have delocate-fuse raise an error pointing to delocate -fuse-universal
...
Anything that raises an informative error would be fine I think.

And what I believe your position is, which is we should retag by default, not because it is a good idea, but because doing anything else is incorrect and leads to bad wheels out in the wild, when they could have easily been made correct.

That this satisfies both of those interests.

So what I was suggesting would only work if the user specifically adds --no-overwrite, in which case it would retag the wheel and write it to the new output file. Or if the user has the --wheel-dir flag, because for the reasons you just explained, we can write the retagged wheel correctly, and the way a script would find it is most likely unaffected, so no need to let the user know of anything in that case.

But if/when this upgrade is made, and those who have disregarded semantic versioning run their CI the same as before, we can now raise an error explicitly stating they now need to use the --no-overwrite flag or --wheel-dir argument.

Perhaps I was unclear because I included the --overwrite option, in my last message. I just put that because I thought that is how argparse does flags, maybe I am getting it mixed up with click. But the point would be --overwrite wouldn't run successfully. If no flag is passed in, it would get treated as if --overwrite was given on the command line. But this would raise the error where we point them to this new change and what they can do to migrate to the new version. Breaking their script, getting their attention, but because of the useful output from the error message, avoiding a bunch of support issues.

So I think this is a reasonable middle ground where we force the user to change, let them know how they can do that, but don't really add any substantial extra code thats going to lay around in the library and be hard to remove in the future.

[matthew-brett]

Just to clarify - that was my intention with delocate-fuse-universal - that you just strip delocate-fuse back to an informative error, and don't keep the previous behavior anywhere as a maintenance burden.

[HexDecimal]

And what I believe your position is, which is we should retag by default, not because it is a good idea, but because doing anything else is incorrect and leads to bad wheels out in the wild, when they could have easily been made correct.

That's correct. This is why I insist on the new behavior being default and removing even the option (other when pinning an old version of delocate) for the previous behavior. I couldn't come up with anything better than making --wheel-dir a requirement since anything else would make the 'correct' behavior of fixing the platform tags not the default.

--overwrite is also too ambiguous a name for this flag, the word 'overwrite' can mean too many things outside of what this actually does. It also preserves the old behavior which I obviously dislike.

delocate-fuse-universal is better. At the risk of bike-shedding, we could also use a different word with the same meaning such as delocate-splice. Just keep in mind that the new name is permanent. The changelog needs to be clear that the name has changed. Then delocate-fuse prints out the informative error and returns a non-zero exit code no matter what arguments are given. The readme would be updated to use the new name.

I insist on pinning delocate==0.11.0 to be the suggested option for preserving the old behavior. This should be mentioned in the changelog and the informative error.

Although I feel strongly about this, I'll go with whatever solution Matthew asserts.

[matthew-brett]

I'm happy with whatever name for the new command - and it's perfectly reasonable to not want to keep the old pathway around. As long as the error message is clear, and there is no possibility of unexpected behavior without the error, anything is fine with me.

[dunkmann00]

@HexDecimal I don't understand why you feel a new command is better than requiring a new flag on delocate-fuse. I don't have any preference on the name but something like:

# delocate_fuse.py

...

parser.add_argument(
    "--no-overwrite",
    action="store_true"
)

def main() -> None:  # noqa: D103
    args = parser.parse_args()
    if not (args.no_overwrite or args.wheel_dir):
        raise RuntimeError("Useful error message telling user to use '--no-overwrite' or '--wheel-dir'") # This returns 1 as an exit code
    
    ...

This causes the same kind of exposure to the new change as setting up a new command like delocate-fuse-wheel, but has two benefits I can think of:

1. We don't have to bother people already using --wheel-dir
2. It can be temporary. In the future --no-overwrite becomes default:

parser.add_argument(
    "--no-overwrite",
    action="store_true",
    default=True
)

This way its no longer necessary to use and we can go back to telling people to do delocate-fuse wheel1.whl wheel2.whl the way you'd prefer to, but for those who at that point in time have it in their script, it won't cause any unrecognized argument errors so it would be a painless transition.

Another more subjective benefit is we don't have to permanently change the name and lose delocate-fuse which to me is short, sweet, and to the point.

Again if you feel --no-overwrite is a poor name, thats fine, but what I would be more happy with is if you understood the idea I'm trying to get across. If you still feel going with a new command is better, than thats fine as well. But I'm not sure you have fully understood what I'm trying to suggest.

[HexDecimal]

I don't understand why you feel a new command is better than requiring a new flag on delocate-fuse. I don't have any preference on the name but something like:

It's a good attempt, but I see downsides to this approach.

It can be temporary. In the future --no-overwrite becomes default:

Once --no-overwrite becomes the default then that flag becomes a no-op. You then have a flag that does nothing and has to be left in for backwards compatibility since removing it later means a 2nd break in the command API. Renaming the command doesn't have this problem.

Rationally I should be okay with leaving --no-overwrite in forever, but I'm too obsessive compulsive to quietly accept that.

For names, the original name of --retag was good enough, or something like --fix-tags/--update-tags, but again I don't want to leave these in once they're the default.

We don't have to bother people already using --wheel-dir

Those using --wheel-dir are not completely in the clear. Their scripts can still break without an explanation of why. The change in command name will guarantee that all users know exactly what has changed. Changing the command name does the best job of handling Matthew's worry that users will be confused by the change in behavior.

Another more subjective benefit is we don't have to permanently change the name and lose delocate-fuse which to me is short, sweet, and to the point.

Yeah, I got used to the name too, but its API has made it difficult to fix in a graceful way. I'll let you pick the new name if you want.

delocate-fuse-universal is too long. No other commands are three words long and sometimes it feels like technically the command might have utility outside of universal wheels, assuming combined architectures are still called universal in the future. Could've easily been named delocate-fuse-fat with this methodology.

delocate-splice is pretty clear wordplay, but it might be too edgy or fancy.

delocate-merge is clear and is well recognized for merging items together. Also delocate-combine as another generic name.

[dunkmann00]

Sorry I wasn't able to respond to this sooner. I wanted to say I very much appreciate that response @HexDecimal and completely agree with all of your critiques. With that, I've put together the changes necessary to move all the functionality of fusing into delocate-merge (I liked that one the best 👍).

Let me know what you think.

[HexDecimal]

Lots of progress made so far, I think anything left is polish and cleanup. I like the warning added to the readme.

No need to rush. I'd prefer this to be done correctly rather than quickly.

[dunkmann00]

Just a heads up, I'm not going to have much time to work on this for the next week. So I'll do what I can, but probably will get to the remaining things towards the end of next week.

[dunkmann00]

Any thoughts on what's left for this to be ready to merge?

[HexDecimal]

As far as I can tell. Everything is resolved.

This PR can be squashed and merged.

[dunkmann00]

Great! Thanks for working with me through this.

[glyph]

Awesome to have this fixed! I will have to look at fixing Encrust to make sure I'm not expecting the old name :)

[glyph]

And, done: https://pypi.org/project/encrust/2024.9.3/