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

Binary sign default plugins #8040

Closed
wants to merge 21 commits into from
Closed

Binary sign default plugins #8040

wants to merge 21 commits into from

Conversation

palerdot
Copy link
Contributor

@palerdot palerdot commented Apr 11, 2023

Better version of #8016

Problem

Mac app signing fails when extra binaries are included in the app bundle. Simple Backup plugin uses 7zip binary for its functionality. When bundling this plugin as a default plugin, mac app signing fails because the 7zip binary is not signed by current build process. Related to #7934

Solution

This PR tries to solve the issue by doing the following.

Current joplin app has [email protected] as a transitive/indirect dependency included by electron-builder@v23. As per this github issue comment - electron-userland/electron-builder#4656 (comment), electron-osx-sign made a patch previously addressing the binary signing issue. This PR upgrades to electron-builder@v24, which bundles the latest package/namespace @electron/[email protected] as its dependency.

After upgrading the package, the listed osx-sign package dependency listed by yarn why is

➜ yarn why @electron/osx-sign
└─ app-builder-lib@npm:24.1.2
   └─ @electron/osx-sign@npm:1.0.4 (via npm:^1.0.4)

Testing

After upgrading the package, package was built on a mac machine manually and it succeded. It is not possible to locally sign the app package because of lack of a valid developer credentials. For more details, please check the log below

  • skipped macOS application code signing  reason=cannot find valid "Developer ID Application" identity or custom non-Apple code signing certificate, it could cause some undefined behaviour, e.g. macOS localized description not visible, see https://electron.build/code-signing allIdentities=     0 identities found
                                                Valid identities only
     0 valid identities found

This PR has to be tested with existing CI setup or a local machine, which has valid mac developer credentials.

Miscellaneous

  • [email protected] is currently published with next tag on npm - https://www.npmjs.com/package/electron-builder?activeTab=versions. The download count suggests that people are actively using 24.x. In case this PR works, but only latest tag version has to be used, this PR has to wait till v24 is bumped to latest tag.
  • @electron/notarize is the new recommended namespace and this PR upgrades to this new namespace from electron-notarize.
  • If this PR succeeds, then future additional binaries may have to be included in the mac.binaries field in the package.json.
  • @electron/rebuild is the new recommended namespace and this PR upgrades to this new namespace from electron-rebuild. One of the warnings when building with electron-builder is @electron/rebuild is already incorporated into electron-builder, please consider to remove excess dependency from devDependencies. Maybe, this dependency has to be removed in the future. For now, the package is upgraded to latest version matching the internal dependency of electron-builder.

@palerdot
Copy link
Contributor Author

Mac signing is passing/success if we pass unzipped contents for signing - https://github.com/laurent22/joplin/actions/runs/4685750187/jobs/8303152561?pr=8048

@palerdot palerdot marked this pull request as ready for review April 19, 2023 05:59
@tessus tessus added the desktop All desktop platforms label Apr 26, 2023
@laurent22
Copy link
Owner

We will do it differently

@laurent22 laurent22 closed this May 29, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants