fix: Don’t lipo binaries that are already a universal file or the same arch

electron / universal

Create Universal macOS applications from two x64 and arm64 Electron applications

MIT License

112 stars 43 forks source link

fix: Don’t lipo binaries that are already a universal file or the same arch #18

Closed beyera closed 2 years ago

beyera commented 3 years ago

Currently, there is no check in place to prevent from attempting to lipo Mach-O files that may have already been by another stage in the build process.

Additionally, we believe there is a use case for having some libraries that can't be run through lipo due to only having an x86_64 version.

This MR adds a check that will prevent lipo from being run on files that are the same and therefore don't have a chance to lipo successfully.

Resolves: #17

beyera commented 3 years ago

I think we should instead try to determine the arch of the given file and if it is already universal skip. If both builds contain an x64 binary we should probably error out?

I considered that, and I'd be happy to make that change. However, some folks might want to ship part of their codebase in x64. For example, if they have a stray library that they can't yet build for arm64. That's why we went for a log instead of erroring out. In my experience, I've still seen a stray dylib in x64 even after building for Universal in Xcode.

Thoughts?

FWIW: it's a an easy process to verify the arch using:

lipo -verify_arch <arch_type>

MarshallOfSound commented 3 years ago

@beyera Hm, reasonable point, I'd like to err on the side of highlighting these things to the user somehow though. How about this as a proposal:

arm64 only binaries should be rejected (because you can't run arm64 on x64 hardware)
x64 only binaries should initially be rejected but can be allow-listed by path in the options passed to this package

That way folks adding x64 only packages without realizing will still get warnings / errors but if they want to bypass that warning for reasons you've outlined above they still can?

beyera commented 3 years ago

That sounds very reasonable. Thanks for your input. I probably won’t get to it tonight, but I’ll make those changes within the next day or two. Cheers!

kiritnarain commented 2 years ago

Hey @beyera, it would be great to get this in!

gaodeng commented 2 years ago

@MarshallOfSound Could you please help to merge this PR and publish it. Maybe the improvements you mentioned can wait for the next PR

MarshallOfSound commented 2 years ago

@gaodeng Please do not spam maintainers, my comment above stands and spamming will only annoy me, not make me want to help you.

If someone either updates this PR or raises a new one with the changes / modifications that were requested that PR can be considered. Until then there is no action to be taken here.

has-n commented 2 years ago

@beyera are you able to make the requested changes? This PR would unblock a lot of things for us.

MarshallOfSound commented 2 years ago

Implemented in #47