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: add afterPack call after macOS universal package is created #6887

Merged
merged 3 commits into from
May 28, 2022

Conversation

schetle
Copy link
Contributor

@schetle schetle commented May 25, 2022

The universal target for macOS fires the afterPack hook after both x64 and arm64, but not after the final package has been created. This is a problem, as there may need to be some final opportunity to adjust the final package before signing in doSignAfterPack.

Context:
In my scenario, this comes up when I'm embedding a macOS appex extension (not a kernel extension) in my PlugIns folder. I can't pre-sign my extension, as electron-builder and by extension, @electron/universal seems to require that my extension is separated out into 2 distinct arch extensions (x64, and arm64) for combining them via makeUniversalApp.

I cannot sign these in the current afterPack hooks, because each arch-dependent package gets signed, and then upon the attempted package combination, fails as the packages are different (they'll contain the non-binary signatures, which will be different).

electron-builder will not sign extensions inside of Content/PlugIns when supplied to the "binaries" option, either as it is purposely excluded from signing (even if the assumption that anything here is a kernel extension is erroneous) (see: https://github.com/electron-userland/electron-builder/blob/master/packages/app-builder-lib/src/macPackager.ts#L271). "binaries" cannot contain their own entitlements anyway, making the removal of this exclusion a bad solution.

doSignAfterPack will fail validation unless the embedded appex is properly signed beforehand. It appears the only place to perform this operation will be this PR's new afterPack call after both x64 and arm64 have both been combined into the final package.

@changeset-bot
Copy link

changeset-bot bot commented May 25, 2022

🦋 Changeset detected

Latest commit: ea0400b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
app-builder-lib Minor
dmg-builder Minor
electron-builder-squirrel-windows Minor
electron-builder Minor
electron-forge-maker-appimage Minor
electron-forge-maker-nsis-web Minor
electron-forge-maker-nsis Minor
electron-forge-maker-snap Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented May 25, 2022

Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
🔨 Latest commit ea0400b
🔍 Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/628e758d9f6519000ca08c6d
😎 Deploy Preview https://deploy-preview-6887--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@schetle schetle changed the title add afterPack call after macOS universal package is created fix: add afterPack call after macOS universal package is created May 25, 2022
@mmaietta mmaietta merged commit 4d590d3 into electron-userland:master May 28, 2022
@mmaietta
Copy link
Collaborator

Thank you for your contribution!

@github-actions github-actions bot mentioned this pull request May 28, 2022
@jtbandes
Copy link
Contributor

I can't pre-sign my extension, as electron-builder and by extension, @electron/universal seems to require that my extension is separated out into 2 distinct arch extensions (x64, and arm64) for combining them via makeUniversalApp.

It sounds like this is being fixed in electron/universal#47

I cannot sign these in the current afterPack hooks, because each arch-dependent package gets signed, and then upon the attempted package combination, fails as the packages are different (they'll contain the non-binary signatures, which will be different).

It's strange you say this — I haven't had the same problem (I was using lipo -extract to separate the architectures during afterPack so they could be re-combined successfully, as described in #5552) and now I need to change my code to avoid running this when arch == universal: https://github.com/foxglove/studio/pull/3595

@schetle
Copy link
Contributor Author

schetle commented Jun 14, 2022

I can't pre-sign my extension, as electron-builder and by extension, @electron/universal seems to require that my extension is separated out into 2 distinct arch extensions (x64, and arm64) for combining them via makeUniversalApp.

It sounds like this is being fixed in electron/universal#47

I cannot sign these in the current afterPack hooks, because each arch-dependent package gets signed, and then upon the attempted package combination, fails as the packages are different (they'll contain the non-binary signatures, which will be different).

It's strange you say this — I haven't had the same problem (I was using lipo -extract to separate the architectures during afterPack so they could be re-combined successfully, as described in #5552) and now I need to change my code to avoid running this when arch == universal: foxglove/studio#3595

Having tests inside of afterPack hooks to test architectures, platforms, or other contextual build settings is, I'd imagine, a common thing for many people with more complex builds.

If there's an afterPack call after x64 and a separate call for arm64, then it feels natural for the new, unique universal package to have the same treatment.

Everyone's needs are different, and giving people more power in this way is a benefit.

Happy hacking!

@jtbandes
Copy link
Contributor

If there's an afterPack call after x64 and a separate call for arm64, then it feels natural for the new, unique universal package to have the same treatment.

Agreed! I was just curious to understand what your signing issue was, because I never ran into one with lipo -extract.

@schetle
Copy link
Contributor Author

schetle commented Jun 14, 2022

I am using lipo -extract as well to separate out the .appex into the individual architectures.

The issue then became my signing needs. I don't take the .appex as it is signed on disk as-is or before lipo. If I let electron-builder (and osx-sign) sign my .appex, it will fail notarization. So instead, I opted for afterPack scripts that I can fully control the entitlements (that are different than any of the helper/inherit/login entitlements that electron-builder let's you specify) and signing to resign the embedded .appex before sending it off for notarization.

The issue then became, I can't resign the .appex in afterPack because the separate architectures would individually get signed and then become unique and fail the recombination checks with makeUniversalApp.

@jtbandes
Copy link
Contributor

I'm also re-signing the embedded appex in afterPack: https://github.com/foxglove/studio/blob/a28ffd16ae71fa34a5f2771c7131b86e8a761565/desktop/afterPack.ts#L114-L122

I never had issues with the recombination in makeUniversalApp. I suppose your entitlements are somehow different 🤷

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

Successfully merging this pull request may close these issues.

3 participants