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

Fix android builds requiring external (cgo) linking, but cgo is not enabled #56

Conversation

BagToad
Copy link
Contributor

@BagToad BagToad commented Aug 29, 2024

Fixes #50

This PR proposes:

  • Extension authors will opt-in to releasing to Android / releasing to Android will be optional in future gh-extension-precompile versions.
  • Support for building and releasing to Android targets will be best effort but cannot block the release to platforms officially supported by gh core.
  • Extensions using Go < 1.22 and wish to release to Android can continue to leverage older versions of gh-extension-precompile to avoid this new logic.
  • New action inputs:
    • release_android
    • android_sdk_version
    • android_ndk_home
  • These changes will prompt a version bump to gh-extension-precompile.

Building on Android and change details

We will set CGO_ENABLED=1 and CC=${ANDROID_NDK_HOME}/toolchains/llvm/prebuilt/linux-x86_64/bin/aarch64-linux-android${ANDROID_SDK_VERSION}-clang for Android architectures, if both release_android and android_sdk_version action inputs are provided. If release_android is enabled but android_sdk_version is not set, the action will fail.

The android_ndk_home action input is used if the ANDROID_NDK_HOME environment variable is not set. The $ANDROID_NDK_HOME environment variable will be set automatically on GitHub hosted runners.

When release_android is not set, the action will not attempt to build for Android targets, avoiding the Android specific requirements completely - this is the proposed default behavior.

I just want my extension to continue building for Android - what should I do?

Along with bumping the action version, modify your workflow that calls gh-extension-precompile to include at least release_android and android_sdk_version.

Example:

- uses: cli/gh-extension-precompile@v2
  with:
    release_android: true
    android_sdk_version: 30
    generate_attestations: true
    go_version_file: go.mod

If you are running the workflow on a self-hosted runner, you should confirm that either the ANDROID_NDK_HOME environment variable is set or that you provide the android_ndk_home action input.

Testing

Build and release no android [email protected]

Build and release with android [email protected]

Extension authors will now need to opt-in to building for android targets. When opted-in to building on android, extension authors will also be required to define at least the Android SDK build-tools version.

If opted-in to building for android targets, we will attempt to build assuming Go 1.22 or newer. Previous action versions will be required to build for android using Go 1.22 and older.
Copy link

@CleanCut CleanCut left a comment

Choose a reason for hiding this comment

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

👍🏻 This would help my use. I have no need of android builds in the first place.

In fact, I'd love it if you could opt-in or opt-out of all the platforms. I only need one.

@BagToad BagToad marked this pull request as ready for review September 9, 2024 19:31
@BagToad BagToad requested a review from a team as a code owner September 9, 2024 19:31
@BagToad BagToad requested a review from andyfeller September 9, 2024 19:31
Copy link
Contributor

@andyfeller andyfeller left a comment

Choose a reason for hiding this comment

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

These changes look good, however I think there's more we need to do here before merging it especially since this will be a breaking change (@v2)

action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
build_and_release.sh Show resolved Hide resolved
Add heading to readme for customizing the build process for Go extensions that refers users to "extensions written in other compiled languages" section. This removes duplicating information that is the same for either case.
Slim down the action input description. Lean on the readme to document this in more depth instead.
README.md Show resolved Hide resolved
Copy link
Contributor

@andyfeller andyfeller left a comment

Choose a reason for hiding this comment

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

99% there! everything is looking good with minor suggestions on docs; let me know if you feels strongly and let's knock this out today

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@BagToad
Copy link
Contributor Author

BagToad commented Sep 10, 2024

Thank you @andyfeller 🙏 I have committed your suggestions and it's ready for another review pass ✨

README.md Outdated Show resolved Hide resolved
@BagToad BagToad requested a review from andyfeller September 10, 2024 18:53
Copy link
Contributor

@andyfeller andyfeller left a comment

Choose a reason for hiding this comment

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

shipit-squirrel-fireworks

Sincere thanks for unblocking our Go extension developer community while putting significant efforts to verifying these changes within Android simulator ✨

After merging this PR, I believe the only things that remain are:

  1. Tagging the repo with v2.0.0 tag + release, which will cause our GitHub Actions to create the v2 branch; for more info, see below

  2. Updating the release notes within the v2 release to explain the breaking change and how users will get more information to adopt the latest changes

jobs:
release:
runs-on: ubuntu-latest
steps:
- name: Update major release branch
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
tag="${GITHUB_REF#refs/tags/}"
major=${tag%%.*}
gh api -X PATCH /repos/"$GITHUB_REPOSITORY"/git/refs/heads/"$major" -f sha="$GITHUB_SHA"

@BagToad BagToad merged commit 561b19d into trunk Sep 11, 2024
Copy link

@Cinderella252 Cinderella252 left a comment

Choose a reason for hiding this comment

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

Seems to work

heaths added a commit to heaths/gh-codeowners that referenced this pull request Sep 28, 2024
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.

Error: android/amd64 requires external (cgo) linking, but cgo is not enabled
4 participants