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

Feature: Sign macOS builds #8561

Merged
merged 1 commit into from Jan 13, 2021
Merged

Feature: Sign macOS builds #8561

merged 1 commit into from Jan 13, 2021

Conversation

@orudge
Copy link
Contributor

@orudge orudge commented Jan 13, 2021

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.

@orudge orudge requested a review from TrueBrain Jan 13, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

Minor stuff; can't wait for this to be merged :D


dmg_filename=(bundles/*.dmg)

if [ "${dmg_filename}" = "bundles/*.dmg" ]; then

This comment has been minimized.

@TrueBrain

TrueBrain Jan 13, 2021
Member

dmg_filename=$(ls bundles/*.dmg)

if [ -z "${dmg_filename}" ]; then ...

Might look a bit better.

This comment has been minimized.

@orudge

orudge Jan 13, 2021
Author Contributor

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.

This comment has been minimized.

@TrueBrain

TrueBrain Jan 13, 2021
Member

Oops:

$(ls bundles/*.dmg 2>/dev/null || true)

(it is about being explicit you expect failure, and handle it, basically :D)

This comment has been minimized.

@orudge

orudge Jan 13, 2021
Author Contributor

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


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.

This comment has been minimized.

@TrueBrain

TrueBrain Jan 13, 2021
Member

Please quote everything after echo. Also means you don't need to escape.

This comment has been minimized.

@orudge

orudge Jan 13, 2021
Author Contributor

Fixed (this did occur to me after I'd pushed!)

#!/bin/bash
set -e

# This script attempts to notarize the OpenTTD DMG generated by cpack.

This comment has been minimized.

@TrueBrain

TrueBrain Jan 13, 2021
Member

nit: CPack

This comment has been minimized.

@orudge

orudge Jan 13, 2021
Author Contributor

Fixed.

- name: Notarize
env:
AC_USERNAME: ${{ secrets.APPLE_DEVELOPER_USERNAME }}
AC_PASSWORD: ${{ secrets.APPLE_DEVELOPER_PASSWORD }}

This comment has been minimized.

@TrueBrain

TrueBrain Jan 13, 2021
Member

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

This comment has been minimized.

@orudge

orudge Jan 13, 2021
Author Contributor

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.

This comment has been minimized.

@TrueBrain

TrueBrain Jan 13, 2021
Member

Yeah, but I know our audience a bit too well sadly :P

APPLE_DEVELOPER_SITE_USERNAME ?

This comment has been minimized.

@orudge

orudge Jan 13, 2021
Author Contributor

Have replaced with APPLE_DEVELOPER_APP_x, hopefully that's OK.

@orudge orudge force-pushed the orudge:sign-macos branch from 1a9eefe to cb16f76 Jan 13, 2021
@orudge orudge force-pushed the orudge:sign-macos branch from cb16f76 to d67af78 Jan 13, 2021
@orudge orudge merged commit 60851ef into OpenTTD:master Jan 13, 2021
8 checks passed
8 checks passed
Emscripten
Details
Commit checker
Details
Check preview needs update Check preview needs update
Details
Linux (clang, clang++)
Details
Linux (gcc, g++)
Details
Mac OS (x64, x86_64)
Details
Windows (x86)
Details
Windows (x64)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.