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

Add extend-info and extra-resource arguments for creating the Mac application package. #253

Merged
merged 1 commit into from Feb 29, 2016

Conversation

erkyrath
Copy link

@erkyrath erkyrath commented Feb 5, 2016

It is useful to be able to set arbitrary fields in the plist file. MacOS documents many keys, and it's unreasonable to define a separate electron-packager argument for each one. Some keys also need large array or dict structures, which can't comfortably be specified on the command line or in a package script.

Therefore, we want to be able to specify a plist file and merge its contents into the app Info.plist.

We may also need to install files directly into the app's Contents/Resources directory. For example, UTExportedTypeDeclarations may specify an icon for a document type; the icon should be an .icns file in Contents/Resources.

Therefore, two new arguments:

extend-info: Take the filename of a plist file and extend Info.plist with its contents. Other explicit arguments such as app-bundle-id override this.

extra-resource: Take a filename and copy that file into the app's Contents/Resources directory. May be used multiple times.

@max-mapper
Copy link
Contributor

I don't see anything wrong this this PR @erkyrath, thanks for making it and adding tests.

However, on a meta note, I really feel like this module is doing too much plist specific stuff that ideally would live externally. If I could do it over again I would defer all plist operations to the user and have them require plist parsing code etc. I'd be interested to hear if anyone has thoughts on a better overall solution to working with plists that doesn't involve implementing lots and lots of code in this module (hence making it hard to maintain)

@erkyrath
Copy link
Author

erkyrath commented Feb 5, 2016

Yeah, I can see it's getting messy. But if this logic didn't live here, it would have to live somewhere.

If a "user" is an app developer (e.g. me), then the user doesn't want to handle it all. The package.json for my app is at https://github.com/erkyrath/lectrote/blob/master/package.json . As you see, the scripts section isn't short, but it's tidy, and it's all I need to package my thing.

(I chose to have five separate script lines rather than using "all". That's on me.)

@malept
Copy link
Member

malept commented Feb 5, 2016

Thinking out loud, perhaps a separate node module that provides a nice API to merge plist files?

@malept
Copy link
Member

malept commented Feb 18, 2016

We just merged a new feature, which unfortunately means that your PR now has merge conflicts. Could you please rebase?

@erkyrath
Copy link
Author

I'll take a look tonight.

… Mac

application package.

`extend-info`: Take the filename of a plist file and extend Info.plist with its contents. Other explicit arguments such as `app-bundle-id` override this.

`extra-resource`: Take a filename and copy that file into the app's Contents/Resources directory. May be used multiple times.
@erkyrath
Copy link
Author

Sorry, the merge message got duplicated there. I tried to fix it and it got worse, so I'm not touching it any more.

@malept
Copy link
Member

malept commented Feb 19, 2016

Hmm. It looks like you did git merge instead of git rebase?

@erkyrath
Copy link
Author

I did a rebase, and then git told me that origin had a new commit that I had to pull, so I did, then I rebased that, then git told me that origin had a new commit... then I realized I was stuck in a time loop so I gave up.

@malept
Copy link
Member

malept commented Feb 19, 2016

I might have some time to take a crack at it this weekend.

@erkyrath
Copy link
Author

The code is right (and the pull didn't change it). Feel free to rearrange the commits however you like.

@erkyrath
Copy link
Author

(Or I could just force-discard that last commit entirely)

@malept
Copy link
Member

malept commented Feb 19, 2016

(Or I could just force-discard that last commit entirely)

Did a quick check to see if I could rebase easily, looks like the answer is no. When I try this weekend, I'm likely going to branch master, cherry-pick your commit, and hand-merge any conflicts (yay for a pretty comprehensive testsuite).

@erkyrath
Copy link
Author

I tried git reset --hard HEAD^, git push -f and I think it worked. Sorry about the mess. I am not one of nature's rebasers.

@erkyrath
Copy link
Author

Yah, I don't know. Travis checks just succeeded on exactly the same code as the checks that just failed. Looks like a timeout error? A transient problem? I hope so because it's bedtime here. :/

@adam-lynch
Copy link

I just tried this out (while trying to solve electron/electron#4403) and weirdly, I had to change https://github.com/erkyrath/electron-packager/blob/featureplist/mac.js#L116 from using writeFileSync to writeFile (and nested all of the following code into the writeFile callback). Not sure what's up with that.

@adam-lynch
Copy link

This PR seems good to me btw 👍

@erkyrath
Copy link
Author

What went wrong when you had the writeFileSync lines?

(Those calls are not me; it's been writeFileSync for many releases, looks like.)

@adam-lynch
Copy link

What went wrong when you had the writeFileSync lines?

The Info.plist didn't have my additions. I only had to change the one line I linked to, not all of the writeFileSyncs.

(Those calls are not me; it's been writeFileSync for many releases, looks like.)

Yeah saw that.

malept added a commit that referenced this pull request Feb 29, 2016
Add `extend-info` and `extra-resource` arguments for creating the Mac application package.
@malept malept merged commit 1cf5f04 into electron:master Feb 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants