Skip to content

ci: Add standalone macOS build support#7581

Merged
jamilbk merged 11 commits into
mainfrom
chore/update-macos-ci
Dec 28, 2024
Merged

ci: Add standalone macOS build support#7581
jamilbk merged 11 commits into
mainfrom
chore/update-macos-ci

Conversation

@jamilbk
Copy link
Copy Markdown
Member

@jamilbk jamilbk commented Dec 24, 2024

The CI swift workflow needs to be updated to accommodate the macOS standalone build. This required a decent amount of refactoring to make the Apple build process more maintainable.

Unfortunately this PR ended up being a giant ball of yarn where pulling on one thread tended to unravel things elsewhere, since building the Apple artifacts involve multiple interconnected systems. Combined with the slow iteration of running in CI, I wasn't able to split this PR into easier to digest commits, so I've annotated the PR as much as I can to explain what's changed.

The good news is that Apple release artifacts can now be easily built from a developer's machine with simply scripts/build/macos-standalone.sh. The only thing needed is the proper provisioning profiles and signing certs installed.

Since this PR is so big already, I'll save the swift/apple/README.md updates for another PR.

@vercel
Copy link
Copy Markdown

vercel Bot commented Dec 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 28, 2024 10:14pm

@jamilbk jamilbk requested a review from a team December 24, 2024 20:15
@jamilbk jamilbk marked this pull request as draft December 24, 2024 20:57
@jamilbk jamilbk removed the request for review from a team December 24, 2024 20:57
@jamilbk jamilbk force-pushed the chore/update-macos-ci branch from 3a44ebc to 0a07ac0 Compare December 25, 2024 12:32
@jamilbk jamilbk changed the title ci: Bump macOS runner and Xcode versions ci: Add standalone macOS build support Dec 25, 2024
@jamilbk jamilbk force-pushed the chore/update-macos-ci branch from 0a07ac0 to 494ec47 Compare December 25, 2024 13:11
@jamilbk jamilbk force-pushed the chore/update-macos-ci branch from 494ec47 to c1c55cf Compare December 25, 2024 13:22
@jamilbk jamilbk force-pushed the chore/update-macos-ci branch 2 times, most recently from 77ea1cc to 0ddaa11 Compare December 25, 2024 13:28
@jamilbk jamilbk force-pushed the chore/update-macos-ci branch from 0ddaa11 to af801b9 Compare December 25, 2024 13:33
@jamilbk jamilbk force-pushed the chore/update-macos-ci branch from af801b9 to b36d45c Compare December 25, 2024 14:17
@jamilbk jamilbk force-pushed the chore/update-macos-ci branch from 6ecae4e to 1fdb04e Compare December 25, 2024 14:38
@jamilbk jamilbk force-pushed the chore/update-macos-ci branch from 0f28470 to 9dc8548 Compare December 25, 2024 14:45
@jamilbk jamilbk force-pushed the chore/update-macos-ci branch from 322094a to ce58771 Compare December 25, 2024 14:55
name: build-${{ matrix.sdk }}
runs-on: ${{ matrix.runs-on }}
name: ${{ matrix.job_name }}
runs-on: macos-15
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was updated to match our development machines which reduces the build configuration drift between the two.

contents: read
id-token: "write"
env:
XCODE_VERSION: "16.2"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated to be a recent version. There were a few subtle differences in how provisioning profiles and certs are stored/loaded in Xcode 16 that will be noted where appropriate.

Comment thread .github/workflows/_swift.yml Outdated
Comment on lines +19 to +39
- job_name: build-ios
rust-targets: aarch64-apple-ios
build-script: scripts/build/ios-appstore.sh
ARTIFACT_PATH: "./Firezone.ipa"
upload-script: scripts/upload/app-store-connect.sh

- job_name: build-macos-appstore
rust-targets: aarch64-apple-darwin x86_64-apple-darwin
build-script: scripts/build/macos-appstore.sh
ARTIFACT_PATH: "./Firezone.pkg"
upload-script: scripts/upload/app-store-connect.sh

- job_name: build-macos-standalone
rust-targets: aarch64-apple-darwin x86_64-apple-darwin
build-script: scripts/build/macos-standalone.sh
# mark:next-apple-version
ARTIFACT_PATH: "./firezone-macos-client-1.4.0.dmg"
NOTARIZE: "${{ (github.event_name == 'workflow_dispatch' || github.ref == 'refs/heads/main') && 'true' || 'false' }}"
# mark:next-apple-version
upload-script: scripts/upload/github.sh
RELEASE_NAME: macos-client-1.4.0
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was refactored to be a matrix for each distributable, with the upload process carved out into another script.

secrets are not available from the strategy context, otherwise I would have added them here and not in the build steps below.

exit 1
fi
- name: Upload build to App Store Connect
IOS_APP_PROVISIONING_PROFILE: "${{ secrets.APPLE_IOS_APP_PROVISIONING_PROFILE }}"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not every matrix job requires every secret, but doing it like this simplifies the build scripts.

The overall goal here was to not require any ENV vars when building locally, and override appropriate ones from CI if we're building on a runner.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Embedding this was somewhat of a pain, but I think it's important. Many Mac users simply try to (mistakenly) run their apps directly from the readonly disk image.

For many apps that works just fine, but for ours it won't due to the network extension. So I spent some time educating the user just in case, to avoid future support burden here.

Comment thread scripts/upload/app-store-connect.sh Outdated
# xcrun altool requires private keys to be files in a specific naming format
API_PRIVATE_KEYS_DIR=${API_PRIVATE_KEYS_DIR:-$RUNNER_TEMP}
PRIVATE_KEY_PATH="$API_PRIVATE_KEYS_DIR/AuthKey_$API_KEY_ID.p8"
echo -n "$API_KEY" | base64 --decode -o "$PRIVATE_KEY_PATH"
Copy link
Copy Markdown
Member Author

@jamilbk jamilbk Dec 28, 2024

Choose a reason for hiding this comment

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

Slight change here to use a base64-encoded version of the API key. It is already in a PEM format, but the downstream build-rust.sh script was barfing on its contained newlines, so this fixes that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could also reuse that utility decode function used in lib.sh when building. Not sure if it is worth consolidating though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

Comment thread scripts/upload/github.sh
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We do this from quite a few other places in CI. I'll look into reusing this script for those.

Comment thread swift/apple/.gitignore
xcuserdata/
**/*.xcuserstate

Firezone/xcconfig/config.xcconfig
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok so this change simplifies things nicely. Instead of defining multiple xcconfigs and moving them into place just before build, we simply specify the build vars on the command directly. So we can now commit a single xcconfig and use that for development and the release build scripts will override what vars they need.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Simplifications from the removal of xcconfigs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This file has been inlined in the ios build script.

Copy link
Copy Markdown
Member

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Great job! I think the structure makes a lot of sense. Can we extend swift/apple/README.md to document how to use the scripts?

Edit: Just read your PR description.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've seen it for several MacOS apps to have a somewhat playful thing here. I think JetBrains for example has something like:

(<logo.png>).install()

It would probably be quite easy to just once-off hire a designer to come up with a cool idea here. Maybe a tunnel that you can drag the Firezone logo through?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hm yeah, though generating this is a little tricky and requires some trial and error since the window coordinates didn't seem to line up to Figma's pixel coordinates. I.e. getting the icons placed onto the PNG in the right place in the script might make the process a little cumbersome for an external designer.

I'll spend a few more minutes to see if I can make it a little snazzier.

Comment thread scripts/build/lib.sh Outdated
Comment on lines +54 to +56
local profiles_path="$1"
local profile="$2"
local profile_file="$3"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
local profiles_path="$1"
local profile="$2"
local profile_file="$3"
local profile="$1"
local profiles_path="$2"
local profile_file="$3"

I think this order would make more sense, perhaps also don't build the path within the function?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

Comment thread scripts/build/lib.sh
-t cert \
-f pkcs12 \
-k "$keychain_path"
security set-key-partition-list \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps add this information as a source-code comment as to why we are doing this.

Comment thread scripts/build/lib.sh Outdated
rm "$cert_path"
}

function insert_build_timestamp() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
function insert_build_timestamp() {
function set_current_project_version() {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Apple calls these build version, updated this to set_project_build_version.

Comment on lines +33 to +39
GIT_SHA="$git_sha" \
CODE_SIGN_STYLE=Manual \
CODE_SIGN_IDENTITY="$code_sign_identity" \
CONFIGURATION_BUILD_DIR="$temp_dir" \
APP_PROFILE_ID="$app_profile_id" \
NE_PROFILE_ID="$ne_profile_id" \
ONLY_ACTIVE_ARCH=NO \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are these ENV variables? It is odd that they are defined as part of the build command and not at the very front like ENV vars for regular commands.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes and no. Xcode will pass them as env vars to downstream scripts when appropriate, but they won't persist beyond that.

I thought this way reduced the chance these env vars can affect the productbuild command just below and others.

I'm probably a little traumatized from my experience with env vars and Xcode / cargo downstream effects 😆

Comment thread scripts/build/macos-standalone.sh Outdated
# Notarize disk image and embedded app
if [ "$notarize" = "true" ]; then
private_key_path="$temp_dir/firezone-api-key.p8"
echo -n "$API_KEY" | base64 --decode -o "$private_key_path"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This could reuse the other function in lib if you could just pass a single output path.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah I made this a base64_decode function for reuse. Probably a good idea since it deals with secrets and we don't want to fudge that up.

Comment thread scripts/build/macos-standalone.sh Outdated
private_key_path="$temp_dir/firezone-api-key.p8"
echo -n "$API_KEY" | base64 --decode -o "$private_key_path"

# Submit app bundle to be notarized. Can take a few minutes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few minutes? What does this do? Wait for manual approval of an Apple support person?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, the API is quite unfriendly: https://developer.apple.com/documentation/security/customizing-the-notarization-workflow#Check-the-status-of-your-request

the notarization process typically takes less than an hour.

I was thinking we can start with this and see how long the waits are. If they really take an hour we might need the notarization step to happen in a separate workflow or job.

Comment thread scripts/upload/app-store-connect.sh Outdated
# xcrun altool requires private keys to be files in a specific naming format
API_PRIVATE_KEYS_DIR=${API_PRIVATE_KEYS_DIR:-$RUNNER_TEMP}
PRIVATE_KEY_PATH="$API_PRIVATE_KEYS_DIR/AuthKey_$API_KEY_ID.p8"
echo -n "$API_KEY" | base64 --decode -o "$PRIVATE_KEY_PATH"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could also reuse that utility decode function used in lib.sh when building. Not sure if it is worth consolidating though.

Comment thread scripts/upload/github.sh
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe call this script github-release because we also upload stuff as GitHub artifacts.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

Comment thread .github/workflows/_swift.yml Outdated
- job_name: build-ios
rust-targets: aarch64-apple-ios
build-script: scripts/build/ios-appstore.sh
ARTIFACT_PATH: "./Firezone.ipa"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So all env variables here are by convention upper-cased? Don't feel strongly about it but the convention seems a bit unnecessary.

Also would prefer all "scripts" to be grouped together.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

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.

2 participants