Platform: Mac App Store support #223

sethlu · 2015-12-20T10:04:14Z

MAS platform support
Entitlements (optional) are supported to sign apps, not just with a certificate
Made it easier to sign for MAS distribution, may fallback to default entitlements file

malept · 2015-12-20T19:01:25Z

index.js

This map should be sorted alphabetically.

malept · 2015-12-20T19:20:40Z

Please update the rest of the "darwin platform only" references to be "darwin/mas platforms only".

malept · 2015-12-20T19:27:34Z

This would solve #163 (or at least part of it). I'm going to wait on @maxogden's opinion on this before merging, as he had some concerns in the linked issue.

If someone with a Mac could test this, it would be appreciated.

max-mapper · 2015-12-20T21:35:43Z

Here's my opinion:

Looks pretty solid + straightforward, however:
I think there needs to be some mention somewhere in the readme that mas means "Mac App Store" just to help user friendliness
The fact that we're adding a bunch of code signing specific code with no tests makes me really nervous. I would really prefer it at this point if all the codesigning stuff got moved into a separate standalone module that we could depend on. This kind of patch is a big red flag to me

malept · 2015-12-20T22:26:05Z

I think there needs to be some mention somewhere in the readme that mas means "Mac App Store" just to help user friendliness

👍 - also, linking to the Electron docs would be helpful.

The fact that we're adding a bunch of code signing specific code with no tests makes me really nervous.

Yeah, me too. The problem I have is that I don't know what a reasonable way to programatically test this looks like.

I would really prefer it at this point if all the codesigning stuff got moved into a separate standalone module that we could depend on. This kind of patch is a big red flag to me

👍

max-mapper · 2015-12-20T22:34:32Z

The problem I have is that I don't know what a reasonable way to programatically test this looks like.

Agreed, it's a hard one. I think in place of proper tests the next best thing would be to put all the signing code into a new module, e.g. electron-sign, and then at least it can have a dedicated issue tracker, version number, external API, and maintainers.

Is anyone up for writing + maintaining a electron-sign module?

sethlu · 2015-12-21T04:20:42Z

Putting the codesign into a separated module for building and testing could be a nice track, as testing online proves hard for signing with certificates, needing manual help after all. I'll see if I could start a module integrating the OS X codesign command later.
Here's a list of things I guess might be confusing for those packing the mac apps:

The certificates for packing an app for distributing outside/in the Mac App Store are different.
Distributing outside the Mac App Store doesn't necessarily need the entitlements, as they are for sandboxing as the main purpose (to my perspective).
From my reading online, I found code signing each file in the project is recommended(?) by Apple as Xcode does so, with --deep ignored.
For distribution within the Mac App Store, an installer package should be required for uploading the app to iTunesConnect for reviewing (so I may put up a electron-sign with installer)

sethlu · 2015-12-21T14:20:44Z

Here's what I've got from today: https://github.com/sethlu/electron-sign
Tbh, only those with the dev program could have the code tested.

max-mapper · 2015-12-21T18:24:49Z

@sethlu Wow!! Amazing work. That looks really good so far. I looked at the code and didn't see any obvious problems. It's great that you have a test suite and standard hooked up too :)

When you are ready, do you want to ~~make a PR to electron-packager that~~ update this PR to integrate with electron-sign? I can also add you to the electron-packager collaborators so you can help maintain the integration in the future.

cjb · 2015-12-21T18:41:51Z

(Maybe electron-mac-sign or something, since Windows Electron apps also require signing?)

max-mapper · 2015-12-21T18:58:48Z

@cjb is there any existing code for that? if if's not too hairy maybe it could live in electron-sign too?

sethlu · 2015-12-21T19:08:11Z

@maxogden @cjb I've renamed the project to electron-osx-sign and it should avoid some issues with understanding from simply reading the command, as it is OS X only now. Seems that there is as well some sorts of code signing with Windows, but I wouldn't get into that too much now for Electron as distribution to OS X's a bit more complex in following the steps, and it's quite needing.
An updated repo: https://github.com/sethlu/electron-osx-sign
I'll see how I could have my electron-packager fork updated with code-sign and make several tests before doing anything further.

max-mapper · 2015-12-21T19:12:51Z

@sethlu excellent, sounds good, thanks!

malept · 2015-12-21T19:13:01Z

Regarding signing packages on Windows: #32

sethlu · 2015-12-27T09:41:48Z

I'm not quite sure about the code signing for the Windows platform as I do most of my work on my Mac.
I haven't replaced the testing procedures for codesign around test/mac.js:181, not sure if to remove it as the code signing's been moved to a separate module.

malept · 2015-12-27T17:07:32Z

package.json

Please keep the dependency list alphabetized.

max-mapper · 2015-12-28T18:04:45Z

protip, if you use npm install --save some-dependency then npm will automatically add it to the dependencies in package.json and alphabetize it for you :)

sethlu · 2015-12-29T16:35:25Z

@maxogden Wow, thanks! I'll have it a try the next time I need to have the package description sorted.
On the travis build, I'm thinking about pulling the timeout longer for testing as it frequently does.

sethlu · 2015-12-29T16:37:35Z

Oops, sorry I accidentally closed this.

Indented for clarity in commits

Fix: ix electron/packager#261 Fix: electron/packager#163 Added filterCFBundleIdentifier function Properties to set in app plist - CFBundleDisplayName - CFBundleIdentifier - CFBundleName Properties to set in helper plist - CFBundleDisplayName - CFBundleIdentifier - CFBundleName - CFBundleExecutable In testing script: - Added testing with special characters

sethlu · 2016-02-17T03:30:22Z

@malept Just separated the commits to make each change clearer. Mind having another check?

malept · 2016-02-17T04:06:28Z

Once the app-copyright docs are updated, I'm going to merge this. (Unless one of the other contributors raises concerns soon - @kfranqueiro's concern about whether adding this feature constitutes a major or minor version bump will be addressed in #266. Hopefully I didn't miss anything.)

Fix: maxogden#163

The latest version of Electron to best support `mas` build

…atforms

sethlu · 2016-02-17T08:10:52Z

@malept The readme.md has been updated with app-copyright.
I'm not sure but there's really quite a lot of working in changing the plist properties from my commits.

malept · 2016-02-17T16:06:27Z

readme.md


+`app-copyright` - *String*
+
+  The copyrights string to use in the app plist, will be displayed in the application About box (OS X only).


"copyright", not "copyrights string". I'll fix this post-merge. (I'm doing #265 anyway.)

malept · 2016-02-17T16:11:25Z

Excellent, I'm going to merge this later today.

I'm not sure but there's really quite a lot of working in changing the plist properties from my commits.

Yes, @maxogden had a similar observation in a different PR: #253 (comment)

sethlu · 2016-02-18T10:35:04Z

@malept cool! After this I could join their discussions on plists, just to minimize the amount of code based on file modifications.

malept · 2016-02-18T16:03:52Z

Oops, forgot to merge this last night. I will do so now.

Thanks @sethlu for all the work you've done on this pull request!

Platform: Mac App Store support Fixes #104, #163, #261.

jasonhinkle · 2016-02-18T20:19:31Z

Fantastic work guys!

positlabs · 2016-02-18T21:15:42Z

@sethlu @malept, if you're ever in Oakland, CA, I owe you a beer!

sethlu changed the title ~~Master~~ MAS build support Dec 20, 2015

malept reviewed Dec 20, 2015
View reviewed changes

index.js Outdated

Copy link

Member

malept Dec 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This map should be sorted alphabetically.

malept added the docs-needed 📝 Pull request needs documentation label Dec 20, 2015

malept added the build-target:mac 🍎 Bundling an Electron app specifically for macOS label Dec 20, 2015

malept added the needs manual testing 🏗️ Pull request needs to be tested outside of the testsuite to be validated label Dec 20, 2015

malept mentioned this pull request Dec 20, 2015

Add ability to package OSX build as an App Store compliant app #163

Closed

sethlu changed the title ~~MAS build support~~ Platform: Mac App Store support Dec 21, 2015

malept reviewed Dec 27, 2015
View reviewed changes

package.json Outdated

Copy link

Member

malept Dec 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the dependency list alphabetized.

sethlu closed this Dec 29, 2015

sethlu added 2 commits February 16, 2016 20:46

Indent test/mac

cf1810e

Indented for clarity in commits

sethlu force-pushed the master branch 4 times, most recently from a22e476 to 5bb0c9a Compare February 16, 2016 23:38

malept mentioned this pull request Feb 17, 2016

Deprecate version-string.LegalCopyright and make app-copyright set it #265

Closed

malept added this to the Next major or minor version after 5.2.1 milestone Feb 17, 2016

malept removed the needs manual testing 🏗️ Pull request needs to be tested outside of the testsuite to be validated label Feb 17, 2016

malept mentioned this pull request Feb 17, 2016

Release 6.0.0 #266

Closed

malept mentioned this pull request Feb 17, 2016

Adding NSHumanReadableCopyright to plist #104

Closed

sethlu and others added 3 commits February 17, 2016 14:49

Opts app-copyright (NSHumanReadableCopyright on OS X)

50eb49a

Fix: maxogden#163

Use Electron v0.35.6 for testing

ad1e9f7

The latest version of Electron to best support `mas` build

Add basic Mac App Store docs + differentiate between host & target pl…

9212919

…atforms

sethlu force-pushed the master branch from 5bb0c9a to 9212919 Compare February 17, 2016 06:51

sethlu mentioned this pull request Feb 17, 2016

Expand wiki electron/osx-sign#10

Closed

malept reviewed Feb 17, 2016
View reviewed changes

malept removed the docs-needed 📝 Pull request needs documentation label Feb 17, 2016

malept added a commit that referenced this pull request Feb 18, 2016

Merge pull request #223 from sethlu/master

3a64e65

Platform: Mac App Store support Fixes #104, #163, #261.

malept merged commit 3a64e65 into electron:master Feb 18, 2016

malept mentioned this pull request Feb 21, 2016

Entitlements for saving to user-selected dir #271

Closed

eugirdor mentioned this pull request Apr 27, 2016

Mac OS X: How do you handle migrating the app if you need to change the Bundle Identifier? electron/electron#5321

Closed


		`app-copyright` - String

		The copyrights string to use in the app plist, will be displayed in the application About box (OS X only).

Uh oh!

Platform: Mac App Store support #223

Platform: Mac App Store support #223

Uh oh!

Conversation

sethlu commented Dec 20, 2015

Uh oh!

malept Dec 20, 2015

Choose a reason for hiding this comment

Uh oh!

malept commented Dec 20, 2015

Uh oh!

malept commented Dec 20, 2015

Uh oh!

max-mapper commented Dec 20, 2015

Uh oh!

malept commented Dec 20, 2015

Uh oh!

max-mapper commented Dec 20, 2015

Uh oh!

sethlu commented Dec 21, 2015

Uh oh!

sethlu commented Dec 21, 2015

Uh oh!

max-mapper commented Dec 21, 2015

Uh oh!

cjb commented Dec 21, 2015

Uh oh!

max-mapper commented Dec 21, 2015

Uh oh!

sethlu commented Dec 21, 2015

Uh oh!

max-mapper commented Dec 21, 2015

Uh oh!

malept commented Dec 21, 2015

Uh oh!

sethlu commented Dec 27, 2015

Uh oh!

malept Dec 27, 2015

Choose a reason for hiding this comment

Uh oh!

max-mapper commented Dec 28, 2015

Uh oh!

sethlu commented Dec 29, 2015

Uh oh!

sethlu commented Dec 29, 2015

Uh oh!

sethlu commented Feb 17, 2016

Uh oh!

malept commented Feb 17, 2016

Uh oh!

sethlu commented Feb 17, 2016

Uh oh!

malept Feb 17, 2016

Choose a reason for hiding this comment

Uh oh!

malept commented Feb 17, 2016

Uh oh!

sethlu commented Feb 18, 2016

Uh oh!

malept commented Feb 18, 2016

Uh oh!

jasonhinkle commented Feb 18, 2016

Uh oh!

positlabs commented Feb 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants