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

Update Android build setup #1228

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

SaschaWillems
Copy link
Collaborator

Description

The Android build setup for our samples is heavily outdated, making it impossible to locally build them with recent Android build tools (e.g. Android Studio) unless you find a way to manually download and setup the exact versions of the build related tools we require,

This makes it nigh impossible to easily build our samples on Android.

This PR fully updates our Android build scripts and setups to support up-to-date Android build tools, including the current version of Android Studio ("Ladybug"). So people wanting to build on Android can simply download Android studio, generate the project files and then easily build the samples.

It also fixes a few issue with our build files that only showed up with more recent build tools. One such change is a deprecation in the native bridge from C++ to Android, which required a fix in the framework (otherwise it would no longer compile).

Note that this is a fairly pervasive change. I did multiple builds, both release and debug from a clean setup to make sure this properly works. I also had to update some third party dependencies to make everything compile with updated compiler toolchains used on Android. I also updated the Android validation layers to the most recent release.

This hopefully fixes all the reported issues people had with trying to build our samples on Android.

Fixes #1188
Fixes #1185
Fixes #1178
Fixes #912

General Checklist:

Please ensure the following points are checked:

  • My code follows the coding style
  • I have reviewed file licenses
  • I have commented any added functions (in line with Doxygen)
  • I have commented any code that could be hard to understand
  • My changes do not add any new compiler warnings
  • My changes do not add any new validation layer errors or warnings
  • I have used existing framework/helper functions where possible
  • My changes do not add any regressions
  • I have tested every sample to ensure everything runs correctly
  • This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

If this PR contains framework changes:

  • [] I did a full batch run using the batch command line argument to make sure all samples still work properly
    Note: It does a famework change for Android, but afaik we don't have batch mode on Android. I did test several samples though.

@SaschaWillems SaschaWillems added build This is relevant to the build system android Issues related to the Android platform labels Nov 16, 2024
Android Studio uses the following plugins/tools to build samples:
=== Android Studio (Recommended)

It is highly recommended to install https://d.android.com/studio[Android Studio] to build, run and trace the sample project. The samples require Android Studio Ladybug.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to lock it down to Ladybug only? Might be better to simply revert that back to highly recommended to install... Also, the samples, don't strictly require Android Studio. I can build / install / run them with just the tools; actually, that's what CI is doing come to think of it.

Copy link
Collaborator Author

@SaschaWillems SaschaWillems Nov 19, 2024

Choose a reason for hiding this comment

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

Yes, LadyBug is currently the only supported Android Studio Version, all others are EOL. Building from CLI still works fine, but most people (see issues) use Android Studio anyway. Aside from that even if you just want to build from CLI, having Android Studio pull all required components for you is much more convenient that trying to manually set them up ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

fair enough, just saying "required" is a strong word ;-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Totally agree, will adjust wording there to not confuse people :)

bldsys/cmake/template/gradle/app.build.gradle.in Outdated Show resolved Hide resolved
bldsys/cmake/template/gradle/app.build.gradle.in Outdated Show resolved Hide resolved
bldsys/cmake/template/gradle/app.build.gradle.in Outdated Show resolved Hide resolved
bldsys/cmake/template/gradle/app.build.gradle.in Outdated Show resolved Hide resolved
@@ -76,5 +76,6 @@ dependencies {
}

project.afterEvaluate{
project.getTasks().getByName("mergeDebugJniLibFolders").dependsOn(unzip)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a note to myself to rethink how this is working. Task dependencies in gradle don't need to be realized completely anymore in order to establish dependencies. This closure forces gradle to completely define each task and then search for the same name. This will slow the build down.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we may look at this at a later point. It took me quite some time to figure out why this would fail until I realized we only hat mergeRelease... in there and I was trying to do a debug build :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

I hear yea, I'm not a fan of gradle's complicated way of task dependency. Definitely NOT a today change.

@gpx1000
Copy link
Collaborator

gpx1000 commented Nov 19, 2024

I'll switch from Changes Requested to approved and leave it up to you on if you'd like to use the PR I made you.

gpx1000
gpx1000 previously approved these changes Nov 19, 2024
@SaschaWillems
Copy link
Collaborator Author

SaschaWillems commented Nov 19, 2024

Awesome. For whatever reason github didn't sent me a notification for that PR, so I didn't realize you already did all those changes :)

Will take a look at it tomorrow and merge. Very much appreciated 👍🏻

bump versions for Android and fix a few things.
@SaschaWillems
Copy link
Collaborator Author

@gpx1000 The builds now sadly fail. It looks like NDK 28 and newer required the user to accept a license:

A problem occurred configuring project ':app'.
> com.android.builder.sdk.LicenceNotAcceptedException: Failed to install the following Android SDK packages as some licences have not been accepted.
     ndk;28.0.12433566 NDK (Side by side) 28.0.12433566
  To build this project, accept the SDK license agreements and install the missing components using the Android Studio SDK Manager.
  All licenses can be accepted using the sdkmanager command line tool:
  sdkmanager.bat --licenses

Running a .bat in a linux based CI obviously doesn't work here. Any ideas?

@gpx1000
Copy link
Collaborator

gpx1000 commented Nov 21, 2024

sdkmanager is an application that we use to download the tooling. In windows it has a .bat in linux it's just a file. I switched us away from using the docker image to using the preinstalled image which already has the NDK setup. https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2004-Readme.md
NDK 26.3.11579264
27.2.12479018 (default)
That looks like the reason for the bug. I guess for now, I'll write a bit more into the build for the CI to grab the 28 NDK version? Or simply we can downgrade since the 28 version is technically in beta? I'll leave the decision up to you Sascha.

…erence to Vulkan Samples, so downgrading for CI purposes is a good idea.
@SaschaWillems
Copy link
Collaborator Author

I'm fine with both :)

In perspective going with NDK 28 is properly a bit more future proof.

@gpx1000
Copy link
Collaborator

gpx1000 commented Nov 21, 2024

I gave you a new PR that simply changes it to 27. 28 is still RC1 and in the future, it will still be asking for RC1 even after 28 is released if we left it the way it is. So probably better to just use 27 for now.

@SaschaWillems
Copy link
Collaborator Author

Awesome 👍🏻

gpx1000 and others added 2 commits November 22, 2024 12:10
Github CI has NDK 27 installed but not yet 28.  It doesn't make a dif…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android Issues related to the Android platform build This is relevant to the build system
Projects
None yet
2 participants