Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

beyera
Copy link
Contributor

@beyera beyera commented Apr 29, 2021

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 beyera changed the title Don’t lipo binaries that are already a universal file or the same arch fix: Don’t lipo binaries that are already a universal file or the same arch Apr 29, 2021
…e arch electron#17

Some Mach-O files may have already been fat binaries and will throw an error if lipoed again.

Co-authored-by: Mitch Cohen <mitch@1password.com>
Co-authored-by: Nick McGuire <nick.mcguire@1password.com>
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@beyera
Copy link
Contributor Author

beyera commented May 9, 2021

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
Copy link
Member

@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
Copy link
Contributor Author

beyera commented May 9, 2021

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
Copy link

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

@gaodeng
Copy link

gaodeng commented Jan 9, 2022

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

jhohiselkeeper added a commit to Keeper-Security/universal that referenced this pull request Mar 16, 2022
Copy link

@gaodeng gaodeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarshallOfSound Please merge this pr

@MarshallOfSound
Copy link
Member

@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
Copy link

has-n commented Apr 21, 2022

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

@MarshallOfSound
Copy link
Member

Implemented in #47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Universal Binaries in Resources Don't Lipo
5 participants