Fix Halide/HalideHelpers CMake package dependency inversion#9234
Open
alexreinking wants to merge 2 commits into
Open
Fix Halide/HalideHelpers CMake package dependency inversion#9234alexreinking wants to merge 2 commits into
alexreinking wants to merge 2 commits into
Conversation
Halide previously depended on HalideHelpers, backwards from the intended cross-compiling story where a target-side build should load only the lightweight, arch-independent helpers and lazily pull in the compiler binaries only when actually needed. Swap the roles: Halide is now the always-available helpers package (find_package(Halide) never touches a compiled binary while cross-compiling unless the new JIT or Python component is explicitly requested), and HalideCompiler is the new name for the platform-specific compiled package it can lazily load. The static/shared preference recorded by Halide's static/shared components is a plain, directory-scoped variable, so independent find_package(Halide ...) calls in unrelated directories of the same project don't stomp on each other. HalideCompilerConfig.cmake detects a genuine conflict -- an explicit component that contradicts an already-recorded preference, or an ancestor directory that already loaded the other flavor -- and fails cleanly instead of silently keeping whichever flavor loaded first. An explicit static/shared request is also forwarded as a real component into HalideConfig.cmake's own find_dependency(HalideCompiler) call, since find_dependency() otherwise skips its underlying find_package() call for arguments identical to an earlier call in the same directory (or a descendant), and the preference variable alone isn't part of those arguments -- without forwarding, that CMake-level optimization would silently defeat the conflict detection above. Also fixes two pre-existing, unrelated packaging bugs: a casing mismatch in the FlatBuffers pkgdep heuristic, and a missing CMAKE_MODULE_PATH entry for the bundled Halide_LLVM/V8 Find modules. Adds test/integration coverage to testing-ecosystem.yml: shared + static + serialization builds, a real aarch64 cross-compile, a forced-JIT-without-compiler negative case, and independent per-directory static/shared selection including a conflict-detection negative case. Fixes #7344 Fixes #7037 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
alexreinking
added
the
release_notes
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9234 +/- ##
=======================================
Coverage ? 70.25%
=======================================
Files ? 255
Lines ? 78929
Branches ? 18879
=======================================
Hits ? 55451
Misses ? 17879
Partials ? 5599 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Halide's installed CMake package previously depended on
HalideHelpers, backwards from the intended cross-compiling story where a target-side build should load only the lightweight, arch-independent helpers and lazily pullin the compiled compiler binaries only when actually needed. This PR swaps the roles:
Halideis now the always-available helpers package.find_package(Halide)never touches a compiled binary while cross-compiling unless the newJITorPythoncomponent is explicitly requested.HalideCompileris the new name for the platform-specific compiled package thatHalide(oradd_halide_generator, etc.) can lazily load.The static/shared preference recorded by
Halide'sstatic/sharedcomponents is a plain, directory-scoped variable, so independentfind_package(Halide ...)calls in unrelated directories of the same project don't stomp on each other.HalideCompilerConfig.cmakedetects a genuine conflict -- an explicit component that contradicts an already-recorded preference, or an ancestor directory that already loaded the other flavor -- and fails cleanly instead of silently keeping whichever flavor loaded first.Also fixes two pre-existing, unrelated packaging bugs: a casing mismatch in the FlatBuffers pkgdep heuristic, and a missing
CMAKE_MODULE_PATHentry for the bundledHalide_LLVM/V8Find modules.Adds
test/integrationcoverage totesting-ecosystem.yml: shared + static + serialization builds, a real aarch64 cross-compile, a forced-JIT-without-compiler negative case, and independent per-directory static/shared selection including a conflict-detection negative case.Fixes #7344
Fixes #7037
Test plan
test/integrationsuite passes locally (shared + static builds, JIT/AOT matrix, multi-directory static/shared independence and conflict-detection tests)testing-ecosystem.ymlCI workflow updated to build and exercise this surface (shared build, static+serialization build, cross-compile to aarch64, forced-JIT-without-compiler negative case)tools/check_cmake_style.pyandmdformatpass on all changed files