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
Feature: Sign macOS builds #8561
Conversation
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.
Minor stuff; can't wait for this to be merged :D
|
||
dmg_filename=(bundles/*.dmg) | ||
|
||
if [ "${dmg_filename}" = "bundles/*.dmg" ]; then |
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.
dmg_filename=$(ls bundles/*.dmg)
if [ -z "${dmg_filename}" ]; then ...
Might look a bit better.
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 results in:
ls: bundles/*.dmg: No such file or directory
Taking out the set -e
gives the same error followed by our custom error. Although it's not the most elegant, the code that's there seems to work without too much fuss.
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.
Oops:
$(ls bundles/*.dmg 2>/dev/null || true)
(it is about being explicit you expect failure, and handle it, basically :D)
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.
Nope, that doesn't then split the string up into an array if multiple DMGs happen to be in the folder. I'm sure there will be some further way around that, but it feels it's getting a bit more convoluted again. :)
(Though we do say to clear out the bundles directory first.)
os/macosx/notarize.sh
Outdated
|
||
if [ "${dmg_filename}" = "bundles/*.dmg" ]; then | ||
echo No .dmg found in the bundles directory, skipping notarization. Please read this | ||
echo script\'s source for execution instructions. |
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.
Please quote everything after echo
. Also means you don't need to escape.
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.
Fixed (this did occur to me after I'd pushed!)
os/macosx/notarize.sh
Outdated
#!/bin/bash | ||
set -e | ||
|
||
# This script attempts to notarize the OpenTTD DMG generated by cpack. |
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.
nit: CPack
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.
Fixed.
.github/workflows/release.yml
Outdated
- name: Notarize | ||
env: | ||
AC_USERNAME: ${{ secrets.APPLE_DEVELOPER_USERNAME }} | ||
AC_PASSWORD: ${{ secrets.APPLE_DEVELOPER_PASSWORD }} |
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 rather avoid people being confused that we user the developers username and password. You mention the username and password is app specific? Maybe better to name it as such? APPLE_APPLICATION_USERNAME
?
This just to avoid people misunderstanding what this is, and complaining about us not being secure or what-ever :)
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.
Hm, it's a username/password for the Apple Developer site, so it seemed a reasonable name to me. Can change if you would like but I don't think personally it's a cause for concern. I have mentioned in the comments in the .sh that it should ideally be an app-specific password that's used.
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.
Yeah, but I know our audience a bit too well sadly :P
APPLE_DEVELOPER_SITE_USERNAME
?
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.
Have replaced with APPLE_DEVELOPER_APP_x
, hopefully that's OK.
Fixes #7826.
Motivation / Problem
User has to jump through hoops to use a downloaded build on Intel Macs on recent versions of macOS.
User has to jump through even more hoops running Terminal commands to use a downloaded build on an Apple Silicon Mac.
Description
OpenTTD Distribution Ltd has applied for membership of the Apple Developer Program and now has a valid code signing certificate for distribution of Mac builds. This has been added into GitHub Secrets for OpenTTD and this pull request enables the creation of signed Mac builds. The builds are then notarized by Apple with the notarization ticket stapled to the .dmg. This prevents any GateKeeper prompts from appearing when the user has downloaded a build.
Limitations
Anybody who has forked the repository and runs the actions in their own branch will not be able to produce signed builds unless they provide their own certificate and developer ID. The builds can however be run successfully without signing or notarization.