Electron 8 & fix App Store submission #120
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 11 commits
May 11, 2020 13:14
minor code changes required for uuid, eslint-config-scratch, and uglifyjs-webpack-plugin
Hopefully these will help solve the crash that Mac App Store reviewers are encountering...
Skipping notarization isn't strictly wrong, but needs a big warning since the build probably won't run on Catalina.
apparently the `com.apple.security.app-sandbox` entitlement and the
`hardenedRuntime: true` setting are mututally exclusive with
`electron-builder`, so the hardened runtime is now only used by non-MAS
builds ('mac' but not 'mas' or 'mas-dev')
kchadha
reviewed
Jun 3, 2020
| * | ||
| * @param {string} mediaType - one of Electron's media types, like 'microphone' or 'camera' | ||
| * @returns {boolean} - true if permission granted, false otherwise. | ||
| * @returns {boolean|Promise.<boolean>} - true if permission granted, false otherwise. |
There was a problem hiding this comment.
We talked about this separately but just to record it here: functionally this is fine because it's called with await, which hides the difference between returning a promise and returning a raw Boolean. To avoid possible future problems I will make a followup change where this function always returns a promise, especially since there's basically no down-side to doing so in this context. (This isn't performance-critical, no synchronous work is happening around this call, etc.)
kchadha
approved these changes
Jun 3, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I wasn't sure who would be best to review this. Please feel free to reassign if you don't have time!
Resolves
Resolves #118
Resolves #119
Proposed Changes
This PR represents a few different changes which ideally would be separated, but because test builds were made with these commits I'm presenting them here as-is.
The most important changes are:
hardenedRuntime: falseto themassection ofelectron-builder.yamlto resolve Crashing / no main window for Mac App Store reviewers #118masDevsettings toelectron-builder.yamlto resolvemas-devbuild won't run due to invalid code signature #119mas-devtarget requires a "Mac Developer" provisioning profile to be placed in thescratch-desktopdirectory and namedmacDeveloperThisDevice.provisionprofile. I chose that file name in the hope that it would be somewhat self-documenting.*.provisionprofileto.gitignoreOther changes include:
com.apple.security.cs.allow-dyld-environment-variablesandcom.apple.security.device.microphoneto the entitlements for bothmacandmasbuilds.allow-dyld-environment-variablesand Apple documentation seems to suggest thatcom.apple.security.device.microphoneshould be present ifcom.apple.security.device.audio-inputis in use.miniloginstead ofconsole.logfor logginguglifyjsto fix a security complaint fromnpm auditReason for Changes
Fixing the
mas-devbuild allowed me to test locally. Primarily this was about adding themasDevsection inelectron-builder.yaml, but I also needed to buildapp-builder-binlocally with this as-yet-unreleased change: develar/app-builder#34. These two changes fixed #119, allowing me to reproduce the issue that the Mac App Store reviewers have been reporting.Fixing #118 boiled down to just setting
hardenedRuntime: falsefor themasbuild, which is sort of embarrassing given how long it has taken to figure out the problem. In essence, it turns out thatelectron-builder(or one of its dependencies) doesn't work correctly ifhardenedRuntimeis enabled and the app's entitlements list includescom.apple.security.app-sandbox. The sandbox entitlement is required for Catalina compatibility, so the solution for now (and maybe forever) is to disable the hardened runtime for Mac App Store builds. See electron/electron#22656 for further discussion on this.Test Coverage
There are effectively three things to test:
mactarget): this can be tested withnpm startor, for a more strict test,npm run dist.mastarget): we can't really test this locally. We can build it withnpm run distto create the build but only Apple can run it.mas-devtarget): adjustscripts/electron-builder-wrapper.jsto buildmas-devinstead ofmasand runnpm run dist, then run the application created indist/mas-dev.The build process for each of these requires various digital certificates and provisioning profiles, so practically speaking they can only really be done by a member of the Scratch Team.
I have done all three of these things locally and they all work for me. If you'd like to try this out yourself (and you're a Scratch Team member) I'd be happy to help.
Everyone else: hopefully this PR will help us get the Mac App Store version up to date soon!