- 
                Notifications
    
You must be signed in to change notification settings  - Fork 32
 
De-matrix Android triples #176
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -139,13 +139,13 @@ on: | |
| type: string | ||
| description: "Command to use when building the package with the Swift SDK for Android" | ||
| default: "swift build" | ||
| android_sdk_triple: | ||
| android_sdk_triples: | ||
| type: string | ||
| description: "The triple to use when building with the Swift SDK for Android" | ||
| default: "[\"x86_64-unknown-linux-android28\"]" | ||
| android_ndk_version: | ||
| description: "The triples to use when building with the Swift SDK for Android" | ||
| default: "[\"aarch64-unknown-linux-android28\", \"x86_64-unknown-linux-android28\"]" | ||
| android_ndk_versions: | ||
| type: string | ||
| description: "The NDK version to use when building with the Swift SDK for Android" | ||
| description: "The NDK versions to use when building with the Swift SDK for Android" | ||
| default: "[\"r27d\"]" | ||
| windows_pre_build_command: | ||
| type: string | ||
| 
          
            
          
           | 
    @@ -511,8 +511,7 @@ jobs: | |
| fail-fast: false | ||
| matrix: | ||
| swift_version: ${{ fromJson(inputs.android_sdk_versions) }} | ||
| sdk_triple: ${{ fromJson(inputs.android_sdk_triple) }} | ||
| ndk_version: ${{ fromJson(inputs.android_ndk_version) }} | ||
| ndk_version: ${{ fromJson(inputs.android_ndk_versions) }} | ||
| os_version: ${{ fromJson(inputs.linux_os_versions) }} | ||
| exclude: | ||
| - ${{ fromJson(inputs.android_exclude_swift_versions) }} | ||
| 
        
          
        
         | 
    @@ -529,6 +528,26 @@ jobs: | |
| - name: Checkout repository | ||
| uses: actions/checkout@v1 | ||
| if: ${{ matrix.os_version == 'amazonlinux2' }} | ||
| - name: Checkout swiftlang/github-workflows repository | ||
| if: ${{ matrix.os_version != 'amazonlinux2' && github.repository != 'swiftlang/github-workflows' }} | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: swiftlang/github-workflows | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we pin this to a specific revision, like  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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After re-reading the security policy about approving workflow runs 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. '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.  | 
||
| with: | ||
| repository: swiftlang/github-workflows | ||
| path: github-workflows | ||
| - name: Determine script-root path | ||
| id: script_path | ||
| run: | | ||
| if [ "${{ github.repository }}" = "swiftlang/github-workflows" ]; then | ||
| echo "root=$GITHUB_WORKSPACE" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "root=$GITHUB_WORKSPACE/github-workflows" >> $GITHUB_OUTPUT | ||
| fi | ||
| - name: Provide token | ||
| if: ${{ inputs.needs_token }} | ||
| run: | | ||
| 
          
            
          
           | 
    @@ -559,8 +578,8 @@ jobs: | |
| echo "Unknown package manager (tried apt-get, dnf, yum)" >&2 | ||
| exit 1 | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the benefit of the cloning logic here as opposed to the curl command? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.  | 
||
| bash -s -- --android --flags="$BUILD_FLAGS" --build-command="${{ inputs.android_sdk_build_command }}" --android-sdk-triple=${{ join(fromJson(inputs.android_sdk_triples), ' --android-sdk-triple=') }} --android-ndk-version="${{ matrix.ndk_version }}" ${{ matrix.swift_version }} | ||
| 
     | 
||
| windows-build: | ||
| name: Windows (${{ matrix.swift_version }} - windows-2022) | ||
| 
          
            
          
           | 
    ||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swift buildsupports multi-arch builds in some contexts (today, with thexcodebackend 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, thanks for explaining 👍