Descent3 icon indicating copy to clipboard operation
Descent3 copied to clipboard

Enable cross-compiling of HogMaker

Open tophyr opened this issue 1 year ago • 12 comments
trafficstars

Pull Request Type

  • [ ] GitHub Workflow changes
  • [ ] Documentation or Wiki changes
  • [x] Build and Dependency changes
  • [ ] Runtime changes
    • [ ] Render changes
    • [ ] Audio changes
    • [ ] Input changes
    • [ ] Network changes
    • [ ] Other changes

Description

HogMaker is a build tool that must run on the host. It must not, then, be compiled as part of the overall target build. For builds that must cross-compile, like Android (and eventually iOS/visionOS), we need to be able to reference an already-built HogMaker project.

Checklist

  • [x] I have tested my changes locally and verified that they work as intended.
  • [x] I have documented any new or modified functionality.
  • [x] I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
  • [x] I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

tophyr avatar Jun 24 '24 07:06 tophyr

@Lgt2x ah, from your comments it's clear I needed to add some documentation about the process. When cross-compiling, there have to be two CMake invocations: the first builds HogMaker using the host toolchain (and exports the HogMakerConfig.cmake file) and then the second, using the target toolchain, gets pointed to the first build.

# creates HogMakerConfig.cmake
cmake -B builds/host
# creates the host HogMaker binary
cmake --build builds/host --target HogMaker

# now, do the actual android (or target) build
cmake -B builds/target -DCMAKE_TOOLCHAIN_FILE=/path/to/android/toolchain.cmake -DHogMaker_DIR=$(pwd)/builds/host
cmake --build builds/target

So -

HogMakerConfig.cmake was not included in this PR. Where is the HogMaker package created in case we're building for Android

That file gets created by the first CMake invocation

in this else(), we're not targeting Android, which means that HogMaker will not be looked up using find_package. So why exporting the target?

correct. we have to export it from this one, so that the android build can subsequently reference it.

tophyr avatar Jun 25 '24 04:06 tophyr

Conditionals improved and README updated.

tophyr avatar Jun 26 '24 01:06 tophyr

ping @Lgt2x for re-review

tophyr avatar Jun 30 '24 02:06 tophyr

On a second thought, this solution of running cmake twice with different options to load the native toolchain to build HogMaker, and then running the cross-compilation toolchain for the game adds complexity to our build system. I experimented with a solution using ExternalProject cmake logic, which allows running two different toolchains in one build for my WASM build, and considers HogMaker a different project. The change to cross compile HogMaker looks like this: https://github.com/Lgt2x/Descent3/commit/db3a886236b495a38ef29acf76e5886a98db7786

Do you think this could work for Android too?

Lgt2x avatar Jul 21 '24 15:07 Lgt2x

Reworked PR to use @Lgt2x's idea of ExternalProject_Add instead of manually prebuilding and then using find_package.

tophyr avatar Aug 04 '24 21:08 tophyr

@Lgt2x @winterheart how's this one looking review-wise? I'll get the merge conflicts updated tonight, but are there any other structural/functional/stylistic changes you'd like to see?

tophyr avatar Aug 20 '24 18:08 tophyr

Well I'm fine with the change as I mostly wrote the patch. Is it fine for you @winterheart ?

Lgt2x avatar Aug 20 '24 19:08 Lgt2x

Look good for me, but I'm not tested it.

winterheart avatar Aug 21 '24 20:08 winterheart

Can we try this approach for cross-compile purposes? This snippet before add_custom_command should do trick:

if(CMAKE_CROSSCOMPILING)
   find_package(HogMaker)
else()
   # Build HogMaker executable
   add_executable(HogMaker ...)
   # Export executable as target
   export(TARGETS HogMaker FILE
          "${CMAKE_BINARY_DIR}/HogMakerConfig.cmake")
endif()
# Call add_custom_command with HogMaker involved
...

And then first build native tools, after that configure cross-compiled environment:

# Build native tools
mkdir build-native; cd build-native
cmake ..
make HogMaker
# Cross-platform build
cd ..
mkdir build-cross; cd build-cross
cmake -DCMAKE_TOOLCHAIN_FILE=MyToolchain.cmake \
      -DHogMaker_DIR=~/src/build-native/ ..
make

With this approach you'll need to first build host tools and then reuse them on cross-compile environment.

winterheart avatar Sep 01 '24 22:09 winterheart

With this approach you'll need to first build host tools and then reuse them on cross-compile environment.

This was the original approach with this PR, a few months ago. See above: https://github.com/DescentDevelopers/Descent3/pull/465#issuecomment-2187953682

It worked alright, but @Lgt2x noted:

this solution of running cmake twice with different options to load the native toolchain to build HogMaker, and then running the cross-compilation toolchain for the game adds complexity to our build system.

The currently-proposed solution adds no complexity to the build process.

tophyr avatar Sep 02 '24 06:09 tophyr

@Lgt2x this is blocked on your changes requested btw; seems like you're ok with it so just flagging that

tophyr avatar Sep 02 '24 06:09 tophyr

sorry, all good :)

Lgt2x avatar Sep 02 '24 06:09 Lgt2x

Looks like #637 has accomplished this and more in the meantime! <3

tophyr avatar Dec 27 '24 06:12 tophyr

Actually, I'm gonna reopen this one - @winterheart / @Lgt2x I still think it'd be overall better to use @Lgt2x's approach of treating HogMaker as an external (rather than sub-) project (https://github.com/DescentDevelopers/Descent3/commit/cc835ce09c2be65406b2dc4403e4a532f7aab185). This would let us eliminate the two-step build process.

I'll get this branch rebased, but in the meantime - thoughts?

tophyr avatar Dec 27 '24 18:12 tophyr

Hey @tophyr , I'm glad you're back on the project! I'm quite satisfied by the solution we adopted in #637 , I don't think the 2-step build process is a big limitation, and it has proven to be successful for the ARM64 builds introduced in #642 . Does this solution fall short for your use cases? I'm not too keen on refactoring all of it just for the sake of it

Lgt2x avatar Dec 27 '24 20:12 Lgt2x

It's good to be back again :)

The benefit to me of having CMake treat HogMaker as an external project is that the overall process is better-encapsulated into how Android's build process sees things. I will play around again with getting the two-step build working in Android (I did it once before, I'm pretty sure) to see how easy that'd be.

I do also think it just makes the build process simpler to reason about, in terms of usage across platforms: Note that the Github CI for Configure cross-compiled build and Configure CMake are now actually nearly-identical, only differing in the BUILD_TESTING and BUILD_EDITOR variables. By treating HogMaker as an external project, we don't actually have to treat cross-compilation specially at the top level. (Which, again, is why this ends up helping me for Android.)

tophyr avatar Dec 27 '24 23:12 tophyr

~Ok, I've got Android working honestly pretty smoothly with the HogMaker cross-compilation as-is (without this PR).~

I do still think it's a nicer build to encapsulate it, as I like the idea of linux-cross-arm64 not needing to be "special" in CI, but my Android stuff is no longer blocked on this.

tophyr avatar Dec 28 '24 07:12 tophyr

Update: I am blocked on this. The Android build additions needed to configure a second, host-only, native build - correctly - are janky and complex. It will be much better to have a singular CMake invocation that encapsulates this dependency.

To exemplify, here is the change this branch enables on my in-progress Android build: https://github.com/tophyr/Descent3/commit/a5dc7329ffcea443abefd1537030416ea40da0a4 The Android Gradle build knows how to integrate one CMake build, but basically it does not have a very clean way to integrate a second native build as a dependency to the first. The deleted tasks in that diff get the job done, kinda, but they also do not support any other "production-level" build responsibilities like cleaning or introspection. With the HogMaker build encapsulated into the top-level CMake command, everything integrates into the Android side of things very nicely.

@Lgt2x @winterheart

What downsides to this approach do you guys see? I understand the "if it ain't broke don't fix it" concern, but aside from just inertial resistance is there a problem that ExternalProject_Add causes? To me, this actually looks simpler because we're not maintaining/referencing a custom MakeHog function.

tophyr avatar Dec 29 '24 07:12 tophyr

Actually, I've just merged this into https://github.com/DescentDevelopers/Descent3/pull/459 - that PR is now ready for review, and this change makes more sense in the context of that one overall.

tophyr avatar Dec 30 '24 07:12 tophyr