-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Update Mac packaging to work without platypus #18522
Conversation
-mkdir -p $@/Contents/Resources/julia | ||
make -C $(JULIAHOME) binary-dist | ||
tar zxf ../../../julia-*.tar.gz -C $@/Contents/Resources/julia --strip-components 1 | ||
rm -f $@/Contents/Resources/julia/lib/*.{a,la} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get rid of this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ViralBShah this was your contribution: is this still necessary? Do we still build .a or .la files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they get built by deps and we don't want to install them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I didn't see any in my build, that's why I was wondering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh there might not be any under lib/julia at the moment misread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this line is necessary anymore. I recently checked and noticed that we do not install the .a files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misread that path initially, but yeah we should be removing static libs and libtool junk during make install
and/or make binary-dist
which is before it gets here
I've also gotten rid of some of the old cruft (like references to Winston and Cairo). |
@simonbyrne I'll look over this tonight when I have some free time. |
cp julia.icns $@/Contents/Resources/ | ||
plutil -replace CFBundleName -string "Julia" $@/Contents/Info.plist | ||
plutil -replace CFBundleIconFile -string "julia.icns" $@/Contents/Info.plist | ||
plutil -insert CFBundleVersion -string "$(JULIA_VERSION_OPT_COMMIT)" $@/Contents/Info.plist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CFBundleVersion: "build version number should be a string comprised of three non-negative, period-separated integers with the first integer being greater than zero—for example, 3.1.2" ... "While developing a new version of your app, you can include a suffix after the number that is being updated; for example 3.1.3a1"
I believe the JULIA_VERSION_OPT_COMMIT
variable does not always precisely follow these rules. Maybe it doesn't matter too much in the case of nightlies which are not tagged commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should always be of the form X.Y.Z
, X.Y.Z-aaa
or X.Y.Z-aaa-hash
. At the moment the first integer is zero, but there's not much we can do about that.
For reference, platypus would use X.Y.Z
or X.Y.Z-aaa
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried this out, but the changes look good to me. I didn't know that osacompile could output an app bundle! That simplifies it all greatly.
Might consider making sure all the recommended Info.plist keys are added.
plutil -replace CFBundleName -string "Julia" $@/Contents/Info.plist | ||
plutil -replace CFBundleIconFile -string "julia.icns" $@/Contents/Info.plist | ||
plutil -insert CFBundleVersion -string "$(JULIA_VERSION_OPT_COMMIT)" $@/Contents/Info.plist | ||
plutil -insert CFBundleShortVersionString -string "$(JULIA_VERSION)" $@/Contents/Info.plist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For release versions (e.g. 0.5.0), JULIA_VERSION
is probably fine for CFBundleShortVersionString, which should just be three period separated integers. Not sure how important it is to strictly adhere to docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me either. It will be of that form for release versions, but for nightlies it can be 0.5.0-dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed now.
plutil -replace CFBundleIconFile -string "julia.icns" $@/Contents/Info.plist | ||
plutil -insert CFBundleVersion -string "$(JULIA_VERSION_OPT_COMMIT)" $@/Contents/Info.plist | ||
plutil -insert CFBundleShortVersionString -string "$(JULIA_VERSION)" $@/Contents/Info.plist | ||
plutil -insert CFBundleIdentifier -string "org.julialang.julia" $@/Contents/Info.plist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This app identifier might be too generic. The prefix org.julialang
is good. But the suffix of .julia
is too generic. I would suggest something like org.julialang.julia.terminallauncherapp
.
There are two related issues I'm thinking of here.
- Bundle/app identifiers should be unique across the system. Suppose later on there are multiple apps published that all related to Julia in some way. Maybe one app just launches Terminal and the repl like we have here. But another app launches a native Cocoa repl with inline graphics support. Both apps need a unique bundle ID. So it doesn't hurt to be specific with the bundle ID in order to leave future changes/growth open.
- Code signing ties the bundle/app ID to the code signature. And code signing really wants unique code signing IDs.
I'd suggest changing the app id to something more specific now while the app doesn't do anything of substance than have to deal with chaning the app id in the future when it could be more difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had just recently changed it for this reason (it used to be just org.julialang
), but we can be more specific here.
Does this mean that we can't later switch the code signer for the same ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, as of now, there are two points where the code signature comes into play:
- GateKeeper checks the identity of the signer, which by default must be Apple or a registered Developer ID. If the signing identity doesn't pass GateKeeper checks, the code isn't allowed to execute. So as long as the signing identity is a Developer ID or Apple, it can be changed in the future without affecting the default behavior.
- The macOS application (network) firewall records its rules base on the code signature. I have not tested this, but I think that if the signing identity changes, then the firewall rules may need to be updated. This is probably not a big deal.
plutil -insert CFBundleVersion -string "$(JULIA_VERSION_OPT_COMMIT)" $@/Contents/Info.plist | ||
plutil -insert CFBundleShortVersionString -string "$(JULIA_VERSION)" $@/Contents/Info.plist | ||
plutil -insert CFBundleIdentifier -string "org.julialang.julia" $@/Contents/Info.plist | ||
plutil -insert NSHumanReadableCopyright -string "© 2016 The Julia Project" $@/Contents/Info.plist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking: Most NSHumanReadableCopyright statements I see begin with the actual word "copyright" including the default one Xcode generates.
IANAL, but I think I read once that international copyright conventions/rules stipulate that the word "copyright" must be used. Of course, maybe I have it backwards...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, Wikipedia says just using the symbol should be fine.
|
||
all: clean $(DMG_NAME) | ||
|
||
$(DMG_NAME): dmg/$(APP_NAME) | ||
-cp -f julia.icns dmg/.VolumeIcon.icns | ||
-ln -fs /Applications ./dmg/Applications | ||
-chmod 755 ./dmg/$(APP_NAME)/Contents/MacOS/Julia | ||
-chmod 755 $</Contents/MacOS/applet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect osacompile to set the permissions on the applet correctly. Is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. The background for this is #9010, and as I said in that issue it's a pretty weird edge case which most apps don't bother with. Maybe we should take this opportunity to get rid of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I say we see if we can get rid of it as we iterate on this PR. I'd like to simplify as much as possible, and the spirit of #9010 was the chown
on line 30 anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this now.
@@ -0,0 +1,16 @@ | |||
Julia OS X packaging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OS X -> macOS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it's still OS X up to Sierra, which is still unreleased
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if Apple provides official marketing guidelines on the OS X vs macOS naming issue.
plutil -insert NSHumanReadableCopyright -string "© 2016 The Julia Project" $@/Contents/Info.plist | ||
-mkdir -p $@/Contents/Resources/julia | ||
make -C $(JULIAHOME) binary-dist | ||
tar zxf ../../../julia-*.tar.gz -C $@/Contents/Resources/julia --strip-components 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe strict adherence to code signing rules would disallow putting executable code (e.g. julia executable and deps libraries) in the Resources directory. But this is a whole different issue...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the updated Resource Rules
, it looks like all files are signed, there is no distinction made between "Resources" and "Binaries" anymore, so the "security" side of this distinction should now be moot.
As far as adherence to protocol goes, Apple states:
...putting code into other places will cause it to be sealed as data (resource) files. This causes this code to be sealed twice; once in the nested code, and once in the outer signature. This wastes both signing and verification time and storage space. Also, this can break the outer signature of apps that use their own update mechanisms to replace nested code. If this nested code is being treated as resources, the outer signature doesn't know that this nested content is actually code.
Always put code and data into their proper places. This applies to all signed code and is enforced by the code signing machinery regardless of how the code is distributed.
They don't threaten us with codesigning breaking if we don't follow the rules, but they do say that the Mac App Store may reject submissions that don't follow these rules without warning:
Note: While strict compliance with these rules may not affect your app today, anything that doesn't meet these requirements note may be rejected by code signing verification (and the Mac App Store validator, in the case of Mac App Store apps) at any point in the future without notice.
All this being said, I'm not at all against moving things around, so if you have a better layout in mind I'd love to hear it. We can move things around pretty flexibly now by setting make flags such as these to place our binaries in the proper location, and our non-binaries in others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing structure is just the same as what we did with platypus, but there's no real reason we can't change it. As a first step, it would probably make sense to structure it as a app-local framework.
I don't think we need to move individual files around though. The R and Python frameworks both just keep the usual unix directory structure buried somewhere, and then symlink everything as necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, maybe this would be better as a different pull request.
make -C ../../.. binary-dist | ||
tar zxf ../../../julia-*.tar.gz | ||
mv julia-* julia | ||
-mkdir -p ./julia/libexec ./julia/share |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these created in the tarball now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They seem to be. Also, I think these were leftover from the days when we included Tk and Cairo libs.
I'm fairly happy with this now. I think we can leave the framework structure for another issue. |
I'm running the tip of this branch through the buildbots to see what it comes up with. |
Looks to work just fine, here's the resultant binary, grab it while it's hot! |
Binary seems to work well. |
Will this change the internal structure of the dmg in any way that we'd need to worry about on Travis for |
It shouldn't, from what I can tell. |
It shouldn't, I intentionally kept the directory structure the same. |
@tkelman I'll leave this up to you to pull the trigger on. |
I don't use or care about supporting this OS, but ok. |
Should we backport to 0.5? |
We can do, as it should be pretty much identical for users, but I'll leave this call to the release maintainers. |
if it doesn't cause any issues on the nightlies, it would be good for future-proofing to avoid accidentally upgrading to broken versions of things and releasing broken builds again |
(cherry picked from commit 329e7e1) ref #18522 include fix for #14427 (cherry picked from commit 259e174) expand README (cherry picked from commit 399710e) remove use of sudo from OS X app makefile (cherry picked from commit 55fa673) add recommended Info.plist keys (cherry picked from commit aeadb11)
Fixes #18518