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

Add super-build for cross-compiling HANNK #6374

Merged
merged 2 commits into from
Nov 3, 2021
Merged

Conversation

alexreinking
Copy link
Member

@alexreinking alexreinking commented Nov 1, 2021

After #6361 the cross-compiling workflow requires building HANNK twice: once for the host-run generators (with a host toolchain) and again with the cross toolchain. This PR adds an opt-in superbuild that orchestrates running both builds and correctly updates the world with a single call to Ninja.

The configure_cmake.sh script learned how to call get_host_target to determine whether the given HL_TARGET requires cross compiling. When not cross-compiling, it invokes the build directly. When cross compiling, it invokes the super-build. It doesn't actually run the build (it's called "configure", after all) in either case. It checks a few basic sanity conditions on the env-vars it reads, now, too.

The superbuild has a few special features to make using it more convenient:

  • Variables matching HANNK_HOST_<VAR> are forwarded to the host build as <VAR>
  • Variables matching HANNK_CROSS_<VAR> are forwarded to the cross build as <VAR>
  • All other cache variables that are explicitly set by either the user or the superbuild toolchain are forwarded to the cross build
  • The super-build writes shims to make cmake --install and ctest work from the top-level build directory.

@alexreinking alexreinking added the buildbot_test_nothing Buildbots ignore this PR entirely. Takes precedence over all other buildbot_test_* labels. label Nov 1, 2021
@alexreinking
Copy link
Member Author

Stopping buildbots because these changes are not tested by the buildbots.

@alexreinking
Copy link
Member Author

alexreinking commented Nov 2, 2021

@steven-johnson raised a very valid concern, which is that super-builds are complex. Here is why I believe they're worth the pain setting up (I don't actually think this is very hard to use).


Let me summarize the prior design:

  1. Check CMAKE_CROSSCOMPILING to determine whether we are indeed cross-compiling
  2. If we are, then execute_process to call the host build.
  3. The host build is responsible for compiling the generators and calling them to produce the cross-compiled libraries.
  4. The cross build manually creates IMPORTED targets for those libraries.
  5. It creates a custom target to re-run the the host build every time

Here are a few deficiencies with the prior implementation:

  1. CMAKE_CROSSCOMPILING is unreliable when producing fat binaries on macOS (and in other cases... sometimes it's set when compiling to 32-bit Linux instead of 64-bit).
  2. Running the host configure step on every cross-configure isn't necessary after the first time, unless one of the cache variables changes. There is no way to pass arguments to the host build, or override its toolchain.
  3. IMPORTED targets are awkward to work with, especially with install()... I'm assuming one day we might want to install Hannk somehow.
  4. Manually creating IMPORTED targets for our own binaries is complex when a standard feature for that exists (export())
  5. Manually creating custom targets to run other builds is complex when a standard feature for that exists (ExternalProject)

The brittleness I'm most concerned about in this design is the inability to pick the host toolchain that will be used. Setting something like CC or CXX will at best (worst?) affect the cross-build and will likely be ignored by the host build.

But the rabbit hole of toolchain nonsense doesn't end with the compiler selection, sadly... people have many reasons to pass custom flags (to work around bugs, try a special compiler, etc.). So unless CMake suddenly develops features to support multiple active toolchains (the devs are very resistant to this), it's better for end users to have more loosely coupled projects.

That's why I went for a "build twice with export + find_package" approach.

On the other hand, this is kind of a pain for people actually working on the project in a cross-compiling setting since they're too de-coupled; the cross-build doesn't know how to call the host-build. That's what the super-build solves.

The way I designed the super-build:

  1. Sidesteps CMAKE_CROSSCOMPILING misbehavior because you're explicitly asking to use two separate toolchains for the generators and for everything else
  2. Lets ExternalProject correctly handle calling the configure and re-build steps
  3. Creates native targets (not IMPORTED) from the generators, so that CMake will treat them as first-class citizens.
  4. Avoids manually creating imported targets by using export(). Wires the projects via (2)
  5. See (2)

It's also opt-in, so if there's something wrong with the super-build, a user can still call the build twice manually to get un-stuck, using standard CMake features, without patching our build sources.

Finally, the script has some knowledge about how to set up the super-build for common scenarios (e.g. Android and WASM) and also knows when a super-build isn't required. It is also opt-in.

Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

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

LGTM, but going to see if Andrew or Dillon have any thoughts

apps/hannk/cmake/superbuild/CMakeLists.txt Outdated Show resolved Hide resolved
@alexreinking
Copy link
Member Author

alexreinking commented Nov 2, 2021

The single failure is the custom cuda context failure... unrelated.

@abadams
Copy link
Member

abadams commented Nov 2, 2021

Doesn't look like custom_cuda_context. Looks like a deadlock?! https://buildbot.halide-lang.org/master/#/builders/181/builds/935

@alexreinking
Copy link
Member Author

Doesn't look like custom_cuda_context. Looks like a deadlock?!

Whoops! Bad assumption... regardless, this PR has nothing to do with it.

@alexreinking
Copy link
Member Author

Buildbots are green!

@steven-johnson
Copy link
Contributor

I guess no one else has opinions, so LGTM, let's land this

@alexreinking alexreinking merged commit ac2673b into master Nov 3, 2021
@alexreinking alexreinking deleted the alex/hannk-superbuild branch November 3, 2021 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buildbot_test_nothing Buildbots ignore this PR entirely. Takes precedence over all other buildbot_test_* labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants