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

Remove CMake from the build process #227

Merged
merged 1 commit into from
Jan 23, 2025
Merged

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Nov 21, 2024

Presently, we're using CMake to build the GDExtension (aka the C++ bits) on Android, but using scons to build for every other platform.

This causes a couple of problems:

  • We need to recreate what scons is already doing in CMake. This led to some issues in the past, where I was expecting certain #defines to exist, because I know godot-cpp adds them, only to realize they weren't there when doing Android builds because they weren't in our custom CMake configuration here
  • To add a build option, we need to do it in both scons and CMake. I started working on this PR because I wanted to remove Meta's headers and provide a build option to point to a local copy of the Meta headers (like we discussed at a recent XR Team meeting), only to realize there'd be added complexity for adding the option to both scons and CMake
  • CI can't cache the build artifacts from CMake leading to slower Android builds on CI. Our current CI has caching for both scons and Gradle artifacts, but it can't cache the CMake artifacts. Perhaps there is a way to do it, but I couldn't figure it out initially, and if we switch to building with scons we'll get the already working scons caching

This PR switches to always using scons to build everything C++-related, and then Gradle just pulls those .so's into the .aar files. This is similar to how Godot is built for Android.

This has some follow on effects:

  • We are no longer building the GDExtension multiple times for each vendor. Previously, we rebuilt the GDExtension for each vendor with special defines, ie META_VENDOR_ENABLED, PICO_VENDOR_ENABLED, etc. We never ended up using those defines for anything, though. This will just save us build time :-)
  • We are no longer linking the libopenxr_loader.so to our libgodotopenxrvendors.so. We never really needed to do this - we just need to make sure that libopenxr_loader.so gets loaded early enough. This PR switches to loading it explicitly in the Android plugin, which is working in my testing. (Note: the libopenxr_loader.so may be a different actual loader depending on the vendor, but it always has that same file name.)
  • The binary directories got shuffled around a bit. If Gradle isn't building the native code itself via CMake, then abiFilters doesn't seem to work for excluding certain architectures in different flavors. So, I had shuffle around the directories for those binaries, so I could instead use jniLibs.srcDirs to point at the architectures we want. I also just removed the armeabi-v7a binaries for the Meta loader, because I think that's for like GearVR or Oculus Go or some other unsupported platform.

TODO:

  • Add a non-functional CMake configuration that can be used to configure Android Studio for debugging. We do this same thing for Godot.
  • Test the plugin with other vendors and make sure it's still working! I've tested builds from this PR on Meta Quest, and it works great. :-) I need to test on the HTC Vive XR Elite, and ensure it still works there. But that's the only other Android headset I have - it'd be great to get some help with testing this from other folks with other headsets.

Marking as a DRAFT until those TODO's are completed.

UPDATE: I've tested with both Meta and HTC Vive XR Elite (which uses the Khronos loader), and it's now working on both of those. It'd be great if folks who have Pico, MagicLeap, Lynx, etc were able to test too, but I've done as much as I can personally.

@dsnopek dsnopek added the enhancement New feature or request label Nov 21, 2024
@dsnopek dsnopek added this to the 3.1.0 milestone Nov 21, 2024
@dsnopek dsnopek requested a review from m4gr3d November 21, 2024 21:19
@dsnopek dsnopek marked this pull request as draft November 21, 2024 21:19
@dsnopek dsnopek force-pushed the remove-cmake branch 3 times, most recently from 0f64bef to 36ea5d6 Compare November 21, 2024 22:17
@dsnopek dsnopek modified the milestones: 3.1.0, 4.x Nov 21, 2024
@dsnopek dsnopek force-pushed the remove-cmake branch 2 times, most recently from 54744ad to 345e91d Compare December 17, 2024 20:29
@dsnopek dsnopek changed the title [DRAFT] Remove CMake from the build process Remove CMake from the build process Dec 17, 2024
@dsnopek
Copy link
Collaborator Author

dsnopek commented Dec 17, 2024

I've got this working with both Meta Quest and HTC Vive XR Elite now. I ended up having to include the Khronos loader .so files in the repo because the package from gradle is setup to be a "prefab" to use when building with CMake, and I couldn't get gradle to just pull in the .so files. Personally, I think this is fine - we're including the .so's for all the other loaders already, so this is no different.

Unfortunately, that's all the headsets I have to test! It'd be great if other folks could test the builds on other headsets too, but there isn't any more that I can do personally.

I've also added a non-functional CMakeLists.txt which should allow debugging via Android Studio, but I'm not super experienced with Android Studio, so I'd love feedback on if I got that right (@m4gr3d could you give it a try?)

Anyway, I'm taking this out of draft now :-)

PS: Look at how fast the fully cached builds are! The Android build finishes in ~2 minutes, compared to ~20 minutes before this PR.

@dsnopek dsnopek marked this pull request as ready for review December 17, 2024 20:41
.github/workflows/build-addon-on-push.yml Outdated Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
@rpavlik
Copy link

rpavlik commented Dec 18, 2024

Regarding the Khronos loader...

I ended up having to include the Khronos loader .so files in the repo because the package from gradle is setup to be a "prefab" to use when building with CMake, and I couldn't get gradle to just pull in the .so files.

I am confused how Gradle could pull them if they are in jniLibs but not if they are in e.g. prefab/modules/openxr_loader/libs/android.arm64-v8a/ . Is there some Gradle thing using jniLibs that I wasn't aware of? (The only way I've seen publicly to use AARs with jniLibs in them is this: https://github.com/googlevr/cardboard/blob/master/hellocardboard-android/build.gradle#L55-L67 )

I'm happy to put them also in e.g. jniLibs/arm64-v8a if that helps. (it's a zip file, making a second copy of something in it doesn't take up more space.) Does this tree look OK to you?

.
├── jniLibs
│   ├── arm64-v8a
│   │   └── libopenxr_loader.so
│   ├── armeabi-v7a
│   │   └── libopenxr_loader.so
│   ├── x86
│   │   └── libopenxr_loader.so
│   └── x86_64
│       └── libopenxr_loader.so
└── openxr
    ├── AndroidManifest.xml
    ├── META-INF
    │   └── LICENSE
    └── prefab
        ├── modules
        │   ├── headers
        │   │   └── include
        │   │       └── openxr
        │   │           ├── openxr.h
        │   │           ├── openxr_loader_negotiation.h
        │   │           ├── openxr_platform_defines.h
        │   │           ├── openxr_platform.h
        │   │           ├── openxr_reflection.h
        │   │           ├── openxr_reflection_parent_structs.h
        │   │           └── openxr_reflection_structs.h
        │   └── openxr_loader
        │       ├── libs
        │       │   ├── android.arm64-v8a
        │       │   │   ├── abi.json
        │       │   │   └── libopenxr_loader.so
        │       │   ├── android.armeabi-v7a
        │       │   │   ├── abi.json
        │       │   │   └── libopenxr_loader.so
        │       │   ├── android.x86
        │       │   │   ├── abi.json
        │       │   │   └── libopenxr_loader.so
        │       │   └── android.x86_64
        │       │       ├── abi.json
        │       │       └── libopenxr_loader.so
        │       └── module.json
        └── prefab.json

19 directories, 23 files

@dsnopek
Copy link
Collaborator Author

dsnopek commented Dec 18, 2024

@rpavlik:

I am confused how Gradle could pull them if they are in jniLibs but not if they are in e.g. prefab/modules/openxr_loader/libs/android.arm64-v8a/ . Is there some Gradle thing using jniLibs that I wasn't aware of?

If the files were in jniLibs, then you should be able to do this to pull in the .so files:

dependencies {
    khronosApi "org.khronos.openxr:openxr_loader_for_android:$versions.openxrVersion"
}

(The only way I've seen publicly to use AARs with jniLibs in them is this: https://github.com/googlevr/cardboard/blob/master/hellocardboard-android/build.gradle#L55-L67 )

I suppose we could do something like that here too, and manually unzip the .aar file to get the files...

I'm happy to put them also in e.g. jniLibs/arm64-v8a if that helps. (it's a zip file, making a second copy of something in it doesn't take up more space.) Does this tree look OK to you?

I think within the AAR file, they should actually be under jni/ (rather than jniLibs/ which is the variable used within Gradle). I'm not a Gradle expert, though :-)

@rpavlik
Copy link

rpavlik commented Dec 18, 2024

I mean, that's approx the experience that you get when using prefab with cmake or android.mk. I didn't think it worked without turning on prefab.

does this aar work? (just renamed to zip to be able to upload)

It's 1.1.43 with the patches from me to also populate jniLibs (which does look like the right dirs per android codesearch)
openxr_loader_for_android-1.1.43.zip

@rpavlik
Copy link

rpavlik commented Dec 18, 2024

OK, guess that's my bad... it does appear that jni/ is the magic dir name in an aar. LMK if this one works for you, it appears to automatically trigger copying the native lib without even touching cmake or prefab.
openxr_loader_for_android-1.1.43.zip
(rename to aar, use like implementation files("${project.rootDir}/libs/openxr_loader_for_android-1.1.43.aar"))

@dsnopek
Copy link
Collaborator Author

dsnopek commented Dec 19, 2024

@rpavlik Thanks! Unfortunately, I wasn't able to make it work. When using a local AAR file, I got errors like:

Direct local .aar file dependencies are not supported when building an AAR. The resulting AAR would be broken because the classes and Android resources from any local .aar file dependencies would not be packaged in the resulting AAR. Previous versions of the Android Gradle Plugin produce broken AARs in this case too (despite not throwing this error). The following direct local .aar file dependencies of the :plugin project caused this error: /home/dsnopek/Sync/Projects/default/godot_openxr_vendors/plugin/aars/openxr_loader_for_android-1.1.43.aar

It's possible that it could work with a non-local AAR from Maven, or maybe configuring it in a different way (I also tried using a flat file repository, which also didn't work, but maybe I'm missing some magic incantation).

Anyway, I ended up changing the PR to manually extract the AAR and copy the .so files, rather than depending on Gradle's built-in dependency handling (similar to the hellocardboard-android sample you linked earlier). It's not pretty, but it works, and means we don't have to keep the .so files in the repo. :-) So, I think this is probably good enough.

.github/workflows/build-addon-on-push.yml Outdated Show resolved Hide resolved
plugin/build.gradle Outdated Show resolved Hide resolved
plugin/build.gradle Show resolved Hide resolved
plugin/build.gradle Outdated Show resolved Hide resolved
plugin/build.gradle Outdated Show resolved Hide resolved
plugin/build.gradle Outdated Show resolved Hide resolved
@dsnopek dsnopek force-pushed the remove-cmake branch 4 times, most recently from 37f3bab to 5295889 Compare January 14, 2025 20:50
@dsnopek dsnopek force-pushed the remove-cmake branch 7 times, most recently from 5bce260 to 7373963 Compare January 14, 2025 22:14
@dsnopek
Copy link
Collaborator Author

dsnopek commented Jan 14, 2025

I think have all review addressed now, and CI doesn't fail, however, the artifacts don't look quite right and I need to figure that before this is ready.

For some reason, in the artifact built on CI, all the AARs have the Khronos loader. Locally, this doesn't happen, I can unzip the AARs and see the correct loader in there. So, I need to figure out what's going on with that.

Also, the binaries are way smaller, like 10% of their previous size. For example, the Khronos AAR goes from 28mb to 2.2mb. The prefab stuff isn't there any more, but the biggest difference is just from the libgodotopenxrvendors.so getting much smaller. It's possible that the scons configuration is just more optimized for size than the cmake one, and everything is fine. But I need to investigate and make sure that's true.

@dsnopek dsnopek force-pushed the remove-cmake branch 2 times, most recently from 5943f45 to 92f4913 Compare January 14, 2025 23:18
@dsnopek
Copy link
Collaborator Author

dsnopek commented Jan 14, 2025

Alright, I think this is ready now!

Removing the Gradle caching from CI fixed the problem with all the AARs including the Khronos loader. I unzipped and inspected them all, and they all have the correct loader in the artifact from CI. I don't think the Gradle caching really did much for us anyway. With the caching disabled, the job that runs gradle completed in ~1 minute, so that seems fine.

Regarding the size: I think the cmake builds were so big because they couldn't use the build_profile feature in the scons configuration. In fact, I'm not sure why we thought it was reasonable for the relatively small amount of C++ code in this extension to result in a ~20mb binary! With the amount of code here, ~2mb sounds about right.

I also tested the binaries in the artifact from CI with:

  • Meta Quest 3
  • Pico 4 Ultra
  • HTC Vive XR Elite
  • Linux (for doing the export from the Godot editor)

They all worked fine!

Unfortunately, that's only testing two loaders: the Meta one and the Khronos one. But I don't have any hardware that uses any of the other loaders.

@dsnopek dsnopek requested a review from m4gr3d January 14, 2025 23:38
@dsnopek dsnopek force-pushed the remove-cmake branch 3 times, most recently from 025f7b7 to e2facb9 Compare January 21, 2025 21:07
@dsnopek
Copy link
Collaborator Author

dsnopek commented Jan 21, 2025

I've just rebased this on latest master, however, I've also updated godot-cpp in order to pull in PR godotengine/godot-cpp#1628 which was just merged today.

That PR fixes a compile time regression in godot-cpp, and since one of the major advantages of this PR is to improve build times, I think it makes sense to pull it in here as well.

@dsnopek dsnopek force-pushed the remove-cmake branch 2 times, most recently from 19a721e to f61af33 Compare January 21, 2025 21:17
plugin/CMakeLists.txt Outdated Show resolved Hide resolved
plugin/CMakeLists.txt Outdated Show resolved Hide resolved
plugin/CMakeLists.txt Outdated Show resolved Hide resolved
plugin/build.gradle Outdated Show resolved Hide resolved
@dsnopek dsnopek force-pushed the remove-cmake branch 2 times, most recently from 48dff97 to 73e7b62 Compare January 22, 2025 16:56
@dsnopek dsnopek requested a review from m4gr3d January 22, 2025 18:55
plugin/build.gradle Outdated Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
@dsnopek
Copy link
Collaborator Author

dsnopek commented Jan 23, 2025

@m4gr3d Thanks for the review! I've addressed both points in my latest push

Copy link
Collaborator

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks great and works as expected! Awesome job!

@dsnopek dsnopek merged commit ebb58a0 into GodotVR:master Jan 23, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants