search

electron / universal

Create Universal macOS applications from two x64 and arm64 Electron applications
MIT License
112 stars 43 forks source link

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

Closed obra closed 2 years ago

obra commented 2 years ago

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.)

electron-bot commented 2 years ago

:tada: This PR is included in version 1.3.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

jtbandes commented 2 years ago 
  /**
    * 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 https://github.com/electron/universal/issues/17.

obra commented 2 years ago 
  /**
    * 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: https://github.com/electron/universal/pull/18#issuecomment-835654328

jtbandes commented 2 years ago

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 (https://github.com/electron/universal/pull/18#pullrequestreview-655014650), the idea was to skip lipo automatically (with no error/warning) for universal binaries. That behavior has been lost.

jtbandes commented 2 years ago

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 🙂