halide / Halide

a language for fast, portable data-parallel computation
https://halide-lang.org
Other
5.92k stars 1.07k forks source link

Different Halide targets needed with try_compile depending on cmake version #5781

Open cimes-isi opened 3 years ago

cimes-isi commented 3 years ago

The library specified with the LINK_LIBRARIES option in try_compile requires a different value depending on the user's cmake version. For example, the following works with cmake 3.17:

CMakeLists.txt:

cmake_minimum_required(VERSION 3.16)
project(halide-try-compile-test)

# Set up language settings
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED YES)
set(CMAKE_CXX_EXTENSIONS NO)

find_package(Halide REQUIRED)

try_compile(HALIDE_WORKS ${CMAKE_BINARY_DIR}
            SOURCES ${PROJECT_SOURCE_DIR}/test_halide.cpp
            LINK_LIBRARIES Halide::Halide)
message(STATUS "Halide works: ${HALIDE_WORKS}")

test_halide.cpp:

#include <Halide.h>

int main(void) {
  Halide::Func f;
  return 0;
}

With cmake 3.17, I get:

-- Halide detected current CMake target:  powerpc-64-linux
-- Halide using default generator target: host
-- Found ZLIB: /usr/lib64/libz.so (found version "1.2.7") 
-- Found PNG: /usr/lib64/libpng.so (found version "1.5.13") 
-- Found JPEG: /usr/lib64/libjpeg.so (found version "62") 
-- Halide works: TRUE
-- Configuring done
-- Generating done

but with cmake 3.18, I get:

-- Halide detected current CMake target:  powerpc-64-linux
-- Halide using default generator target: host
-- Found ZLIB: /usr/lib64/libz.so (found version "1.2.7") 
-- Found PNG: /usr/lib64/libpng.so (found version "1.5.13") 
-- Found JPEG: /usr/lib64/libjpeg.so (found version "62") 
CMake Error at /ccs/home/cimes/casper/halide-bug-reproducers/halide-try-compile/build/CMakeFiles/CMakeTmp/CMakeLists.txt:18 (add_executable):
  Target "cmTC_c281c" links to target "Halide::Halide" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?

CMake Error at CMakeLists.txt:11 (try_compile):
  Failed to generate test project build system.

-- Configuring incomplete, errors occurred!
See also "/ccs/home/cimes/casper/halide-bug-reproducers/halide-try-compile/build/CMakeFiles/CMakeOutput.log".
See also "/ccs/home/cimes/casper/halide-bug-reproducers/halide-try-compile/build/CMakeFiles/CMakeError.log".

The issue is resolved if I use Halide::shared::Halide instead (and also works for the older cmake version), but this seems like the wrong thing to have to do. I can link other targets against Halide::Halide without issues in all cmake versions that are supported, so something is different with try_compile. I played around with different cmake policies discussed in the try_compile documentation, but they did not resolve the issue.

Eventually I noticed that HalideConfig.cmake behaves differently depending on the cmake version, though I'm not entirely sure what the proper behavior should be or what the fix would look like, or even if there's anything that can reasonably be done about it: https://github.com/halide/Halide/blob/1c0f8245ee152d8e4a0a03bfe1ff6f0037d4c390/packaging/HalideConfig.cmake#L66-L76

alexreinking commented 3 years ago

I think this will be fixed when #5754 lands. We're dropping support for shared and static in the same directory (different directories in the same project still ok) precisely because of edge cases like this.

The documentation for try_compile's LINK_LIBRARIES argument reads:

LINK_LIBRARIES <libs>...

Specify libraries to be linked in the generated project. The list of libraries may refer to system libraries and to Imported Targets from the calling project.

Since the Halide::Halide alias is not itself an imported target, that might be the problem. Seems like try_compile ought to resolve aliases, but doesn't.

OTOH you very likely do not need to use try_compile in the first place...

cimes-isi commented 3 years ago

Thanks for the update. We're using try_compile to test for a distributed feature we have in a Halide branch, and only enabling it in our application code if that capability is available. This is a common approach in cmake when codebases need to support multiple versions of dependencies with API differences.

alexreinking commented 3 years ago

This is a common approach in cmake when codebases need to support multiple versions of dependencies with API differences.

There's a lot of common practice in CMake that is sub-optimal. Your configure times would be shorter if you just added set(Halide_HAS_DISTRIBUTED 1) in HalideConfig.cmake (you are in a branch, after all). That would turn into a find_package version argument after upstreaming (if that is your intention).

cimes-isi commented 3 years ago

Yes, eventually we will make the feature more discoverable like that, but it's just not implemented yet and isn't high on the priority list. There are several challenges in getting the code into a state where it can be upstreamed, which we'll be reaching out about - hopefully soon. In the meantime, the try_compile approach gets the job done. Thanks.

alexreinking commented 3 years ago

it can be upstreamed, which we'll be reaching out about - hopefully soon.

Happy to hear it! 🙂