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

Platform: Mac App Store support #223

Merged
merged 6 commits into from Feb 18, 2016
Merged

Platform: Mac App Store support #223

merged 6 commits into from Feb 18, 2016

Conversation

sethlu
Copy link
Contributor

@sethlu sethlu commented Dec 20, 2015

  • 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

@sethlu sethlu changed the title Master MAS build support Dec 20, 2015
@@ -18,7 +18,8 @@ var supportedPlatforms = {
// Maps to module ID for each platform (lazy-required if used)
darwin: './mac',
linux: './linux',
win32: './win32'
win32: './win32',
mas: './mac' // it is almost the same as the mac version
Copy link
Member

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 malept added the docs-needed 📝 Pull request needs documentation label Dec 20, 2015
@malept
Copy link
Member

malept commented Dec 20, 2015

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

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

malept commented Dec 20, 2015

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.

@malept malept added the needs manual testing 🏗️ Pull request needs to be tested outside of the testsuite to be validated label Dec 20, 2015
@max-mapper
Copy link
Contributor

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
Copy link
Member

malept commented Dec 20, 2015

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
Copy link
Contributor

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
Copy link
Contributor Author

sethlu commented Dec 21, 2015

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 sethlu changed the title MAS build support Platform: Mac App Store support Dec 21, 2015
@sethlu
Copy link
Contributor Author

sethlu commented Dec 21, 2015

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
Copy link
Contributor

@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
Copy link

cjb commented Dec 21, 2015

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

@max-mapper
Copy link
Contributor

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

@sethlu
Copy link
Contributor Author

sethlu commented Dec 21, 2015

@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
Copy link
Contributor

@sethlu excellent, sounds good, thanks!

@malept
Copy link
Member

malept commented Dec 21, 2015

Regarding signing packages on Windows: #32

@sethlu
Copy link
Contributor Author

sethlu commented Dec 27, 2015

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.

@@ -27,7 +27,8 @@
"plist": "^1.1.0",
"rcedit": "^0.3.0",
"rimraf": "^2.3.2",
"run-series": "^1.1.1"
"run-series": "^1.1.1",
"electron-osx-sign": "^0.1.4"
Copy link
Member

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.

@max-mapper
Copy link
Contributor

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
Copy link
Contributor Author

sethlu commented Dec 29, 2015

@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 sethlu closed this Dec 29, 2015
@sethlu
Copy link
Contributor Author

sethlu commented Dec 29, 2015

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 sethlu force-pushed the master branch 4 times, most recently from a22e476 to 5bb0c9a Compare February 16, 2016 23:38
@sethlu
Copy link
Contributor Author

sethlu commented Feb 17, 2016

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

@malept malept added this to the Next major or minor version after 5.2.1 milestone Feb 17, 2016
@malept malept removed the needs manual testing 🏗️ Pull request needs to be tested outside of the testsuite to be validated label Feb 17, 2016
@malept malept mentioned this pull request Feb 17, 2016
@malept
Copy link
Member

malept commented Feb 17, 2016

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

@sethlu
Copy link
Contributor Author

sethlu commented Feb 17, 2016

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

@@ -137,6 +153,10 @@ packager(opts, function done (err, appPath) { })

Valid values are listed in [Apple's documentation](https://developer.apple.com/library/ios/documentation/General/Reference/InfoPlistKeyReference/Articles/LaunchServicesKeys.html#//apple_ref/doc/uid/TP40009250-SW8).

`app-copyright` - *String*

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

Choose a reason for hiding this comment

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

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

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

malept commented Feb 17, 2016

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
Copy link
Contributor Author

sethlu commented Feb 18, 2016

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

@malept
Copy link
Member

malept commented Feb 18, 2016

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!

malept added a commit that referenced this pull request Feb 18, 2016
Platform: Mac App Store support

Fixes #104, #163, #261.
@malept malept merged commit 3a64e65 into electron:master Feb 18, 2016
@jasonhinkle
Copy link
Contributor

Fantastic work guys!

@positlabs
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-target:mac 🍎 Bundling an Electron app specifically for macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants