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

feat: don't lipo binaries that are identical in the x64 and arm64 versions and match an allowlist #47

Merged
merged 2 commits into from
Jun 1, 2022

Conversation

obra
Copy link
Contributor

@obra obra commented Jun 1, 2022

This PR builds on #18 and adds an allowlist, currently called x64ArchFiles as requested by @MarshallOfSound.

I based the allowlist implementation on the existing 'checkSingleArch' allowlist.

@MarshallOfSound If this isn't up to snuff, let me know what you'd like and I'd be happy to improve it. (If it's more convenient for you to just take over the PR, that works for me, too.)

Andrew Beyer and others added 2 commits June 1, 2022 11:42
…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 <[email protected]>
Co-authored-by: Nick McGuire <[email protected]>
@MarshallOfSound MarshallOfSound changed the title Don't lipo binaries that are identical in the x64 and arm64 versions and match an allowlist feat: don't lipo binaries that are identical in the x64 and arm64 versions and match an allowlist Jun 1, 2022
@MarshallOfSound MarshallOfSound merged commit 01dfb8a into electron:master Jun 1, 2022
@electron-bot
Copy link

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jtbandes
Copy link

  /**
    * Minimatch pattern of binaries that are expected to be the same x64 binary in both of the ASAR files.
    */
   x64ArchFiles?: string;

I find the naming of this allowlist a bit strange. The files aren't necessarily x64 binaries, they might be universal binaries. In fact, that was the original motivation presented in #17.

@obra
Copy link
Contributor Author

obra commented Jun 13, 2022

  /**
    * Minimatch pattern of binaries that are expected to be the same x64 binary in both of the ASAR files.
    */
   x64ArchFiles?: string;

I find the naming of this allowlist a bit strange. The files aren't necessarily x64 binaries, they might be universal binaries. In fact, that was the original motivation presented in #17.

I was working from the specific request and rationale from @MarshallOfSound here: #18 (comment)

@jtbandes
Copy link

jtbandes commented Jun 13, 2022

That makes sense – I believe that comment is saying that we need an allowlist for x64 binaries, because we should assume by default that it's an error/mistake if there is an x64-only copy of a binary being included in a universal app.

However, in this original suggestion (#18 (review)), the idea was to skip lipo automatically (with no error/warning) for universal binaries. That behavior has been lost.

@jtbandes
Copy link

To clarify, this is how I interpreted the discussion in #18:

Andrew: "Let's skip lipo if binary files are identical."
Samuel: "That's good for universal binaries, but shouldn't it be an error if both files are x64-only?
Andrew: "But some people might want x64-only files."
Samuel: "Good point, let's make x64-only possible via an allowlist, but reject arm64-only."

The implication is that lipo should still be skipped for universal binaries without requiring the universal binary to be in the allowlist. That is, the allowlist was suggested specifically for x64 files.

This PR doesn't have a specific carveout for universal binaries and instead requires them to be put in the x64ArchFiles allowlist. That's why I said the name of the allowlist is confusing 🙂

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

Successfully merging this pull request may close these issues.

4 participants