CMake configuration error with user provided OpenVDB

[FabienPean-Virtonomy]

If the user provides OpenVDB package prior including lagrange, it will trigger an error during CMake configuration step (version 3.31.6)

[cmake] CMake Error at build/_deps/lagrange-src/cmake/lagrange/lagrange_find_package.cmake:48 (target_link_libraries):
[cmake]   Cannot specify link libraries for target "OpenVDB::openvdb" which is not
[cmake]   built by this project.

from lines

lagrange/cmake/lagrange/lagrange_find_package.cmake

Lines 44 to 49 in b3f4080

	# https://github.com/AcademySoftwareFoundation/openvdb/issues/1630
	if(NOT BUILD_SHARED_LIBS)
	find_package(blosc CONFIG REQUIRED)
	find_package(ZLIB REQUIRED)
	target_link_libraries(OpenVDB::openvdb INTERFACE blosc_static ZLIB::ZLIB)
	endif()

because it seems GLOBAL target allows for modifying properties of the imported target.

https://cmake.org/cmake/help/latest/command/find_package.html#basic-signature
Added in version 3.24: Specifying the GLOBAL keyword will promote all imported targets to a global scope in the importing project. Alternatively, this functionality can be enabled by setting the CMAKE_FIND_PACKAGE_TARGETS_GLOBAL variable.

The minimal reproducing example is

cmake_minimum_required(VERSION 3.31)

if(DEFINED ENV{VCPKG_ROOT})
    set(CMAKE_TOOLCHAIN_FILE "$ENV{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake" CACHE STRING "")
endif()

project(test_lagrange VERSION 0.1.0 LANGUAGES C CXX)

find_package(OpenVDB CONFIG REQUIRED)
# find_package(OpenVDB CONFIG REQUIRED GLOBAL) # Workaround

include(FetchContent)
FetchContent_Declare(lagrange GIT_REPOSITORY https://github.com/adobe/lagrange.git GIT_TAG v6.34.0 GIT_SHALLOW TRUE)
FetchContent_MakeAvailable(lagrange)

lagrange_include_modules(volume)

Actually, I wonder what is the "right" CMake way around find_package with GLOBAL or not. It seems that there is even a CMake default option to put them all GLOBAL but off by default.

To make is work without the user knowing might be wrapping (all) find_package in lagrange_find_package by if(NOT TARGET) ... endif()

[jdumas]

Thanks for the report. Since we are already special-casing this library in our find_package() proxy, I would simply skip the find_package() call and following block if OpenVDB::openvdb exists prior to L43.

We can't reasonably wrap all find_package() calls like that though, since there is no clear mapping between the name used in find_package() and the imported target names being created after a successful find. (And I'd rather not add a manual table mapping the two, I don't think it would scale well?)

[FabienPean-Virtonomy]

My thought on that would be to dispatch the content of lagrange_find_package(XX) into respective dependency XXXX.cmake. it collocates everything related to a single dependency together and make clearer the steps of what is search how/where. It would feel easier to reason about, but I do not know if it generalizes well to such large project or with CMake dependency provider feature.

# recipes/external/OpenVDB.cmake
if(TARGET OpenVDB::openvdb) # Already provided, validity of target is caller responsibility
    return()
endif()

# System provided but fixed as needed
if(LAGRANGE_USE_FIND_PACKAGE AND openvdb IN_LIST maybe_external_packages)
  find_package(OpenVDB 12 CONFIG REQUIRED GLOBAL COMPONENTS openvdb)
  # https://github.com/AcademySoftwareFoundation/openvdb/issues/1630
  if(NOT BUILD_SHARED_LIBS)
      find_package(blosc CONFIG REQUIRED)
      find_package(ZLIB REQUIRED)
      target_link_libraries(OpenVDB::openvdb INTERFACE blosc_static ZLIB::ZLIB)
  endif()
  return()
endif()

# Else locally fetched
message(STATUS "Third-party (external): creating target 'OpenVDB::openvdb'")
# etc

[jdumas]

That's not a bad idea. So far I've been reluctant to insert any Lagrange-specific logic and variables in our recipes/*.cmake files, with the idea that those can be copy-pasted to other projects and reused easily.

Ultimately my goal would be to have every dependency supported via vcpkg as well as CPM/FetchContent, and make the "allowlist" in maybe_external_packages unnecessary. But we're not quite there yet and it'll take a while 😅

[FabienPean-Virtonomy]

That's not a bad idea. So far I've been reluctant to insert any Lagrange-specific logic and variables in our recipes/*.cmake files, with the idea that those can be copy-pasted to other projects and reused easily.

I can understand the motivation, though adding those wouldn't impact current copy-pastability as long as it it is bounded within a single if(LAGRANGE_USE_FIND_PACKAGE AND...) since an unset variable evaluates to OFF iirc, unless LAGRANGE_USE_FIND_PACKAGE is in global scope.

Ultimately my goal would be to have every dependency supported via vcpkg as well as CPM/FetchContent, and make the "allowlist" in maybe_external_packages unnecessary. But we're not quite there yet and it'll take a while 😅

Great to hear and good luck with that ! 😄