-
-
Notifications
You must be signed in to change notification settings - Fork 355
ci: parallelize xcframework slice builds #5218
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
base: main
Are you sure you want to change the base?
Conversation
That would be super nice to get done. Thanks for starting this @armcknight. |
Got the slices all building in parallel, working on downloading the artifacts for assembly together. It's looking like the total job time will be around 5-6 minutes now based on https://github.com/getsentry/sentry-cocoa/actions/runs/15080146739 |
Fiiiiinally got all the things in the right path locations to get it all passing again. Final result: 6m17s. |
The only CI checks I validated were in the workflow build-xcframework.yml. The failing tests are due to having removed the old build-xcframework.sh script but not yet updating benchmarking.yml or ui-tests.yml to use the new reusable workflows that parallelize the steps from it; #5277 fixes that and should be merged into this branch before merging this one to |
.github/workflows/release.yml
Outdated
@@ -1,6 +1,8 @@ | |||
name: Release | |||
run-name: Release ${{ github.event.inputs.version }} | |||
|
|||
# TODO: change this to dispatch the build-xcframework.yml workflow, and then depend on it here with the workflow dependency setup described at https://stackoverflow.com/a/64733705 |
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.
done and removed in #5277
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
bfe863d | 1223.19 ms | 1236.23 ms | 13.04 ms |
d33f2c8 | 1233.67 ms | 1246.92 ms | 13.24 ms |
ebfe678 | 1234.63 ms | 1254.52 ms | 19.89 ms |
06548c0 | 1226.71 ms | 1252.37 ms | 25.66 ms |
216bdf9 | 1193.69 ms | 1217.90 ms | 24.21 ms |
154f795 | 1225.53 ms | 1231.04 ms | 5.51 ms |
2095ae0 | 1238.20 ms | 1251.37 ms | 13.17 ms |
27f970b | 1234.21 ms | 1243.98 ms | 9.77 ms |
bdd896e | 1211.19 ms | 1239.06 ms | 27.87 ms |
0589699 | 1233.49 ms | 1249.09 ms | 15.60 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
bfe863d | 21.58 KiB | 414.57 KiB | 392.99 KiB |
d33f2c8 | 22.31 KiB | 820.52 KiB | 798.20 KiB |
ebfe678 | 22.31 KiB | 775.27 KiB | 752.95 KiB |
06548c0 | 20.76 KiB | 427.36 KiB | 406.59 KiB |
216bdf9 | 21.58 KiB | 418.13 KiB | 396.54 KiB |
154f795 | 20.76 KiB | 435.25 KiB | 414.49 KiB |
2095ae0 | 21.90 KiB | 709.06 KiB | 687.16 KiB |
27f970b | 21.58 KiB | 706.97 KiB | 685.39 KiB |
bdd896e | 22.31 KiB | 780.75 KiB | 758.43 KiB |
0589699 | 21.58 KiB | 656.60 KiB | 635.02 KiB |
Previous results on branch: armcknight/ci/parallelize-xcframework-build
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
cd452df | 1229.82 ms | 1254.49 ms | 24.67 ms |
abb8f1a | 1232.08 ms | 1245.48 ms | 13.40 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
cd452df | 23.76 KiB | 824.89 KiB | 801.13 KiB |
abb8f1a | 23.76 KiB | 824.89 KiB | 801.13 KiB |
fee2b24
to
f928af0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5218 +/- ##
=============================================
- Coverage 92.900% 92.899% -0.002%
=============================================
Files 688 688
Lines 86303 86299 -4
Branches 31201 31192 -9
=============================================
- Hits 80176 80171 -5
- Misses 6025 6028 +3
+ Partials 102 100 -2 see 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
d76e93b
to
517598f
Compare
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.
Great progress on this, thanks for working on it! I left a couple of comments to discuss.
@@ -1,5 +1,5 @@ | |||
#!/bin/bash | |||
set -euo pipefail | |||
|
|||
echo "{ \"1.0\": \"file:///$(pwd)/Carthage/Sentry.framework.zip?alt=file:///$(pwd)/Carthage/Sentry.xcframework.zip\" }" > ./Samples/Carthage-Validation/Sentry.Carthage.json | |||
echo "{ \"1.0\": \"file:///$(pwd)/Carthage/SentrySwiftUI.framework.zip?alt=file:///$(pwd)/Carthage/SentrySwiftUI.xcframework.zip\" }" > ./Samples/Carthage-Validation/SentrySwiftUI.Carthage.json | |||
echo "{ \"1.0\": \"file://$(pwd)/Carthage/Sentry.framework.zip?alt=file://$(pwd)/Carthage/Sentry.xcframework.zip\" }" > ./Samples/Carthage-Validation/Sentry.Carthage.json |
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.
m
: The change makes sense to me as pwd
will return absolute values. Do we have a way to confirm this change does not break Carthage integration?
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.
There is a validation step that builds a project using Carthage and the local assets.
|
||
set -eou pipefail | ||
|
||
sdk="${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.
m
: As you are adding new parameters, it might be worth the extra effort to replace positional parameter with named ones like in sentry-xcodebuild.sh
. This has no priority for me, I'll leave it to you.
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 code was previously written, just in a file with a different name. I want to keep this change set small and focused just on refactoring the existing behavior. We can perform further improvements on the actual implementation in subsequent change sets. FWIW I agree with you here.
xcodebuild_cmd="xcodebuild -create-xcframework" | ||
|
||
for slice_dir in "$search_path"/xcframework-*; do | ||
framework_path=$(find "$slice_dir" -name '*.framework' -print -quit) |
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.
h
: While it's more verbose, there might be value in explicitly naming the framework slices and reference them here by name, rather than using find
, because if any framework is absent (e.g. due to configuration error) we would have incomplete frameworks and not even detect it (CI wouldn't fail). WDYT?
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.
We definitely need some kind of validation of the set of slices we expect. For instance, this CI is fully green, yet the built product is actually still missing a Mac Catalyst slice.
@@ -240,7 +240,6 @@ jobs: | |||
path: | | |||
raw-build-output.log | |||
raw-build-for-testing-output.log | |||
raw-test-output.log |
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.
h
: Why are you removing this? The command ./scripts/sentry-xcodebuild.sh test-without-building
used above is writing to raw-test-output.log
# Since Carthage 0.38.0 we need to create separate .framework.zip and .xcframework.zip archives. | ||
# After creating the zips we create a JSON to be able to test Carthage locally. | ||
# For more info check out: https://github.com/Carthage/Carthage/releases/tag/0.38.0 | ||
build-xcframework: |
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.
m
: Having a build script locally is convenient for testing. Would it be possible to keep the command and call build-xcframework-slice.sh
and assemble-xcframework.sh
similar to CI here? I know it's duplicate logic, so we should add a comment explaining this is just for convenience while doing local development, but executing the GitHub Actions locally is quite complicated.
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 agree. I kept a version of it around about halfway through this PR and then gave up after some point due to the flux of changes. I'll work on adding it back in.
run: ./scripts/build-xcframework-slice.sh ${{matrix.sdk}} ${{inputs.name}} "${{inputs.suffix}}" ${{inputs.macho-type}} ${{inputs.configuration-suffix}} | ||
shell: bash | ||
|
||
- name: Remove Sentry.framework from SentrySwiftUI build |
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.
l
: It would be great to have a comment here why we need to do this.
In general it would be great if our scripts have more documentation. As a fairly new maintainer it's not so easy to understand the previous decisions in configuration, e.g. why the minimum OS version needs to be 100.0 for static libraries. |
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.
Thanks for tackling this @armcknight. This will make CI way more faster.
The GH actions summary in the PR is a bit hard to read for me. It would be great to shorten the names, so you know which job is which by looking at these titles:
run: ./scripts/assemble-xcframework.sh "${{github.workspace}}/xcframework-slices" "${{inputs.name}}${{inputs.suffix}}" | ||
shell: bash | ||
|
||
- name: Archive XCFramework |
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.
l
: This uploads and doesn't archive the XCFramework, I believe.
- name: Archive XCFramework | |
- name: Upload XCFramework |
|
||
variant-id: | ||
description: |- | ||
The ID of the variant to build an XCFramework slice for. Used to collect appropriate slices for final deliverable assembly. |
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.
l
: An example for a possible value would be nice.
The job to build the xcframework builds all the slices in serial, which takes upwards of 30 minutes. There are at least 6 slices, so if we parallelize these, we should be able to cut that time down to a small fraction.
Before: 30 minutes 52 seconds
After: 6 minutes 17 seconds
One thing I noticed but haven't yet addressed is that the SentrySwiftUI build actually produced Sentry.xcframework as well, because to archive the SentrySwiftUI target, Sentry target must be built as well as it's a dependency. So, we could avoid the duplicated tests with some tweaks to the workflow logic. I just haven't figured out how to do that yet.
Another future task could be caching individual slices and assembled variants, as not all will always need to be rebuilt (think, if uikit code is modified, then the UIKitless variant won't need to be rebuilt).
#skip-changelog
Fixes GH-4925