Skip to content

Install the Android NDK before all steps that need it #754

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

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

kendalharland
Copy link
Contributor

@kendalharland kendalharland commented Jun 27, 2024

Ensure android NDK is installed regardless of runner.

This fixes the workflow on runner images where the NDK is not
installed. It bundles together a few other changes to ensure
CMake is executed correctly and make it easier to diff CMake
log in the GHA UI with the content in swift-toolchain.yml

Changes

  • Always run gha-setup-vsdevenv even if runners.os != 'Windows'
    • It provides us with the path to CMake
  • Switch to nttdl/setup-ndk to install the Android NDK.
  • Always specify Android-specific MAKE defines, but enable others
    iff matrix.os == Android.
    • CMAKE_SYSTEM_NAME=${{ matrix.os }} controls which system we're
      building for, so there's no need to use matrix.extra_flags for
      Android-specific defines. Inlining them ensures powershell
      variable expansion works, and makes it easier to copy paste
      CMake commands for for local reproduction.

Test

@kendalharland kendalharland marked this pull request as draft June 27, 2024 23:28
@kendalharland kendalharland force-pushed the kendal/install-android-ndk branch from 77db6c3 to d013ae3 Compare June 27, 2024 23:32
@kendalharland kendalharland marked this pull request as ready for review June 27, 2024 23:33
@kendalharland kendalharland force-pushed the kendal/install-android-ndk branch 3 times, most recently from 403b6bb to 7ad589e Compare June 28, 2024 20:21
Copy link
Owner

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

This has multiple changes, including whitespace changes. Can you please split this up into focused changes?

@kendalharland kendalharland force-pushed the kendal/install-android-ndk branch from 7ad589e to 23a4634 Compare June 28, 2024 20:32
@kendalharland
Copy link
Contributor Author

kendalharland commented Jun 28, 2024

This has multiple changes, including whitespace changes. Can you please split this up into focused changes?

Removed the whitespace changes.

Re the rest of the changes: Unfortunately these are all necessary to fix the Cirun builds :/ :

  • Without unconditionally running gha-setup-vsdevenv, we can't find CMake
  • Without inlining CMAKE_ANDROID_NDK, $NDKPATH is not expanded from matrix.extra_flags and CMake configure fails.
  • Without switching to nttdl/setup-ndk I'll need to duplicate the powershell code to download the NDK before each cmake configure (Cirun images don't have the NDK)

But, as you mentioned above, I could put the non-powershell, Android -specific CMake defines back until a later PR if you'd like.

@compnerd
Copy link
Owner

I don't think I understand what you are suggesting. I don't see why you would not be able to extract just the installation of the NDK into a separate change that can go in first - that should have no impact on the GHA builds.

Separately, changing gha-setup-devenv could go in a separate change with no impact on GHA builders but further prepare the rules for supporting additional runners.

At that point, you could make just the changes needed to pass the extra require flags.

@kendalharland kendalharland force-pushed the kendal/install-android-ndk branch 2 times, most recently from 3869ff2 to 819b124 Compare June 28, 2024 20:56
Copy link
Owner

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

This still conflates 2-3 different changes, but I suppose it is fine. The ninja changes are one item, the NDK installation and use are a separate item, and the PIC code is a third item. Please split changes into distinct issues being addressed in the future.

This fixes the workflow on runner images where the NDK is not
installed. It bundles together a few other changes to ensure
CMake is executed correctly and make it easier to diff CMake
log in the GHA UI with the content in swift-toolchain.yml

* Always run gha-setup-vsdevenv even if runners.os != 'Windows'
  * It provides us with the path to CMake
* Switch to nttdl/setup-ndk to install the Android NDK.
* Always specify Android-specific MAKE defines, but enable others
  iff matrix.os == Android.
  * CMAKE_SYSTEM_NAME=${{ matrix.os }} controls which system we're
    building for, so there's no need to use matrix.extra_flags for
    Android-specific defines. Inlining them ensures powershell
    variable expansion works, and makes it easier to copy paste
    CMake commands for for local reproduction.
@kendalharland kendalharland force-pushed the kendal/install-android-ndk branch from 819b124 to 7350ae7 Compare June 28, 2024 20:58
@kendalharland
Copy link
Contributor Author

I suppose that's true. Given how GitHub doesn't make it easy to stack PRs, it's just harder to test as 3 separate PRs. But you're correct, those individual changes would independently work on top of the existing GH runners. Will do in the future.

@compnerd compnerd merged commit c3463b8 into compnerd:main Jun 28, 2024
@kendalharland kendalharland deleted the kendal/install-android-ndk branch June 28, 2024 22:25
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