De-matrix Android triples#176
Conversation
jakepetroules
force-pushed
the
eng/PR-dematrix-android
branch
7 times, most recently
from
8223e1c to
9658355
Compare
Building for multiple ABIs at once is something the swift-build frontend does for macOS and should eventually do for other platforms. Don't "lift" the triple into a new matrix axis. This also renames the android_sdk_triple and android_ndk_version properties to plurals, since they support multiple versions. This is breaking, but the Android workflow is only a couple of days old, so this should be fine.
jakepetroules
force-pushed
the
eng/PR-dematrix-android
branch
from
9658355 to
1eba3bf
Compare
| fi | ||
| curl -s --retry 3 https://raw.githubusercontent.com/swiftlang/github-workflows/refs/heads/main/.github/workflows/scripts/install-and-build-with-sdk.sh | \ | ||
| bash -s -- --android --flags="$BUILD_FLAGS" --build-command="${{ inputs.android_sdk_build_command }}" --android-sdk-triple="${{ matrix.sdk_triple }}" --android-ndk-version="${{ matrix.ndk_version }}" ${{ matrix.swift_version }} | ||
| cat ${{ steps.script_path.outputs.root }}/.github/workflows/scripts/install-and-build-with-sdk.sh | \ |
There was a problem hiding this comment.
What is the benefit of the cloning logic here as opposed to the curl command?
There was a problem hiding this comment.
I was just discussing this with @etcwilde. This allows changes to the scripts to be reflected on PRs to github-workflows, rather than having to temporarily change the URL to your PR branch and then back again before merging -- makes development on github-workflows itself much smoother.
I'd like to apply this to the other workflows as well, afterwards.
| fail-fast: false | ||
| matrix: | ||
| swift_version: ${{ fromJson(inputs.android_sdk_versions) }} | ||
| sdk_triple: ${{ fromJson(inputs.android_sdk_triple) }} |
There was a problem hiding this comment.
Question about removing the triple from the matrix axis: does this make it harder for end users to diagnose build issues in a particular architecture?
Just thinking from the concept of a CI job, if a dev were testing their package and wanted to test against the Swift SDK for Android, I would think it would create a job run per triple, but admittedly I haven't though too deeply about it: So I guess the outstanding question is just what the benefit is from a CI users perspective
There was a problem hiding this comment.
swift build supports multi-arch builds in some contexts (today, with the xcode backend and when targeting the macOS platform). I'd like to extend that to Android as well so that you can pass multiple triples to a single invocation and get a multi-arch build. This is why makes more sense from a logical perspective for the triple NOT to fan out, because our build system recognizes architecture as a list rather than a single value.
As far as "does this make it harder for end users to diagnose build issues in a particular architecture?" - I don't think so, this is how we've done builds for Apple platforms for forever, and I don't think that's ever come up as a concern.
There was a problem hiding this comment.
makes sense, thanks for explaining 👍
There was a problem hiding this comment.
What is the overhead in build times?
There was a problem hiding this comment.
This changes the default from x86_64 only to x86_64 + aarch64, and the overhead will of course be proportional to the package size. Mainly I wanted to ensure the multiarch support worked correctly in testing this PR, but I think it's beneficial to have both on by default since aarch64 is what's used for the vast majority of devices, and x86_64 for the majority of at-desk testing in the emulator.
Could also revert to a single arch if you strongly prefer.
| path: github-workflows | ||
| - name: Checkout swiftlang/github-workflows repository | ||
| if: ${{ matrix.os_version == 'amazonlinux2' && github.repository != 'swiftlang/github-workflows' }} | ||
| uses: actions/checkout@v1 |
There was a problem hiding this comment.
could we pin this to a specific revision, like main?
ref: 'main'
I don't want to run the risk of running the workflow in this repository (say as part of a CI test) and having this check out a users branch which could contain a malicious version of the script being executed
There was a problem hiding this comment.
After re-reading the security policy about approving workflow runs
https://docs.github.com/en/actions/how-tos/manage-workflow-runs/approve-runs-from-forks
This might not be a real concern, since we have to approve all PR workflow runs from non-write-access-contributors.
I'll defer to @shahmishal on this one
There was a problem hiding this comment.
'ref' defaults to the default branch of the repository, so I think this would not do anything interesting. Furthermore, someone could just put the malicious script inline in the YAML instead of referencing an external one by URL. You're right that workflow approvals are the real security tool here.
|
@shahmishal Does this look good to go? |
|
FWIW, LGTM |
4 participants
Building for multiple ABIs at once is something the swift-build frontend does for macOS and should eventually do for other platforms. Don't "lift" the triple into a new matrix axis.
This also renames the android_sdk_triple and android_ndk_version properties to plurals, since they support multiple versions. This is breaking, but the Android workflow is only a couple of days old, so this should be fine.