Use add_halide_generator() everywhere in apps/ #6554
Conversation
Existing CMake code uses add_executable + target_link_libraries to create a Generator, but the somewhat-recently-added add_halide_generator() helper is a cleaner way to do this. Move to using it everywhere in apps/ instead. (Note that add_halide_generator()) doesn't yet work for in-tree builds, unfortunately.)
We currently disable deprecation warnings inside Halide. This re-enables them there, and also inside add_halide_generator(). (Note, additive to PR #6554)
apps/bilateral_grid/CMakeLists.txt
Outdated
| # Generator | ||
| add_halide_generator(bilateral_grid.generator SOURCES bilateral_grid_generator.cpp) | ||
| target_link_libraries(bilateral_grid.generator PRIVATE Halide::Tools) | ||
| target_link_libraries(bilateral_grid::halide_generators::bilateral_grid.generator PRIVATE Halide::Tools) |
There was a problem hiding this comment.
Does this actually work? I thought you couldn't set properties through aliases (and for imported targets, PRIVATE makes no sense)
There was a problem hiding this comment.
...I thought that's what you wanted me to do.
I thought the whole point of this was to make it work for crosscompiling. The if TARGET approach won't because there apparently won't be a non-decorated version of the library in that case.
There was a problem hiding this comment.
I'm sorry, I must not have explained well. if (TARGET ${unqualified_name}) is the correct way to check if you're in the host-build scenario, which is the one that matters if you're going to set build options on ${unqualified_name}, like which libraries to link to the generator executable.
- In the host build scenario, these targets exist:
${unqualified_name}is a normal CMake target that will create an executable. It is linked toHalide::Generatorby default.${qualified_name}is anALIAStarget to${unqualified_name}.
- In the cross-build scenario, this target exists:
${qualified_name}is anIMPORTEDtarget that points to the executable in the host build.${unqualified_name}does not exist.
In the cross-build, you are specifically importing the host-built tools. Since they have already been built, it does not make sense to set build properties on them.
|
I didn't re-add Halide::Tools to anything that didn't require it for compilation. My guess is that we had a lot that were adding it from copy-paste codegen. |
|
Now that this is merged, we should actually try cross-compiling some of these. |
Existing CMake code uses add_executable + target_link_libraries to create a Generator, but the somewhat-recently-added add_halide_generator() helper is a cleaner way to do this. Move to using it everywhere in apps/ instead.
(Note that add_halide_generator()) doesn't yet work for in-tree builds, unfortunately.)