Don't replace CMake's find_package

[bernhardmgruber]

• [x] Checked for duplicates

Describe the bug

ROOT overrides CMake's find_package in cmake/modules/SearchInstalledSoftware.cmake, which is not uncommon, but relies on undocumented behavior of CMake. See e.g.: https://crascit.com/2018/09/14/do-not-redefine-cmake-commands/

This however breaks other tools, that also rely on this behavior, resulting in an infinite recursion. My concrete example is vcpkg, a popular package manager (not only for Windows), that pulls itself into a CMake project via a toolchain file to inject dependencies. Unfortunately, vcpkg needs to hook find_package itself to point it to the installed dependencies.

Expected behavior

ROOT can be configured with vcpkg and other tools sensitive to overriden find_package. This requires ROOT to not override find_package.

To Reproduce

Clone ROOT, create build_win folder, run:

PS C:\dev\root\build_win> cmake -DCMAKE_TOOLCHAIN_FILE=D:\dev\vcpkg\scripts\buildsystems\vcpkg.cmake ..
-- Building for: Visual Studio 16 2019
-- Selecting Windows SDK version 10.0.19041.0 to target Windows 10.0.19042.
-- The C compiler identification is MSVC 19.29.30037.0
-- The CXX compiler identification is MSVC 19.29.30037.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30037/bin/Hostx64/x64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30037/bin/Hostx64/x64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found Git: C:/Program Files/Git/cmd/git.exe (found version "2.30.0.windows.2")
-- Detected ROOT_VERSION 6.25.01
-- Looking for Python
-- Could NOT find Python3 (missing: Python3_NumPy_INCLUDE_DIRS NumPy) (found version "3.9.2")
-- Could NOT find Python2 (missing: Python2_EXECUTABLE Python2_LIBRARIES Python2_INCLUDE_DIRS Python2_NumPy_INCLUDE_DIRS Interpreter Development NumPy Development.Module Development.Embed)
    Reason given by package:
        Interpreter: Wrong major version for the interpreter "C:/Program Files/Python39/python.exe"

-- Looking for pthread.h
-- Looking for pthread.h - not found
-- Found Threads: TRUE
-- Performing Test GLIBCXX_USE_CXX11_ABI
-- Performing Test GLIBCXX_USE_CXX11_ABI - Failed
-- ROOT Platform: win32
-- ROOT Compiler: MSVC 19.29.30037.0
-- ROOT Processor: AMD64
-- ROOT Architecture: win32
-- Build Type: '' (flags = '')
-- Compiler Flags: -nologo -IC:/dev/root/build/win -FIw32pragma.h -FIsehmap.h -Zc:__cplusplus -MD -GR -EHsc- -W3 -wd4141 -wd4291 -wd4244 -wd4049 -D_WIN32  -D_XKEYCHECK_H -DNOMINMAX -D_CRT_SECURE_NO_WARNINGS
-- ROOT default compression algorithm: zlib
-- PyROOT will be built for version 3.9.2
-- Looking for ZLib
CMake Error at D:/dev/vcpkg/scripts/buildsystems/vcpkg.cmake:693 (set):
  Maximum recursion depth of 1000 exceeded
Call Stack (most recent call first):
  D:/dev/vcpkg/scripts/buildsystems/vcpkg.cmake:771 (_find_package)
... // many stack frames
  D:/dev/vcpkg/scripts/buildsystems/vcpkg.cmake:771 (_find_package)
  cmake/modules/SearchInstalledSoftware.cmake:16 (_find_package)
  cmake/modules/SearchInstalledSoftware.cmake:55 (find_package)
  CMakeLists.txt:220 (include)

Setup

ROOT master cloned today on Windows 10.

[Axel-Naumann]

What would be the downside of renaming ROOT's find_package (say to ROOT_FIND_PACKAGE) and using that throughout ROOT? Would there be e.g. any side-effects for say package X used by ROOT which depends on package Y, which ROOT has already find, but now package X will find a different one? And what about users configuring against ROOT, what would be the side-effect they see? pulling @amadio in for opinions and expertise!

[amadio]

ROOT's approach of turning find_package to a no-op was based on the talk "Effective CMake". It only works with subprojects of ROOT (i.e. LLVM) if we override find_package without renaming it, otherwise LLVM will not use the builtin zlib, for example, when that's enabled. I don't think ROOT is at fault here for relying on this feature of CMake.

[bernhardmgruber]

I don't think ROOT is at fault here for relying on this feature of CMake.

As described in the blob post by a CMake maintainer (emphasis is mine):

Even if find_package() were only redefined once though, it would still be relying on undocumented CMake behavior which may be modified or removed completely in a future version. Reliance on such behavior should be discouraged and as the above discussion shows, the technique is not safe to use in general.

It's mostly sad that vcpkg did the same trick, making ROOT incompatible with it.

Given the circumstances I think this cannot be fixed at the moment, since neither ROOT nor vcpkg can easily change and I also don't expect CMake to make such behavior defined and allow overriding build-ins multiple times. So I guess we need to close this is won't-fix?

[amadio]

Here is a relevant issue where this is discussed more in depth. One of the comments in this issue explains the rationale behind overriding find_package quite well. If there is a better solution that can work the way the current solution does (i.e. works also for LLVM without having to change its calls to find_package(ZLIB), for example), we can implement it. However, so far I have not found a another way to do it. Maybe now that the required version of CMake is newer than 3.11, we may be able to improve things by using the FetchContent module. It may also be possible to use CMAKE_DISABLE_FIND_<PackageName> to skip checking for packages if that doesn't force packages to be considered not found when <PackageName_FOUND> is then also set by hand. The reality, though, is that CMake still kinda sucks for managing optionally bundled dependencies like ROOT wants to support.

As for the undocumented nature of the feature, it's sad but, like the CDash test measurements that were undocumented for a long time, if you offer it, people will try to use it.

[guitargeek]

Thanks a lot for the discussion, @bernhardmgruber and @amadio!

Given what I read here, I think a good compromise is to replace find_package only in the case of using builtins. If you use vcpkg, I'm sure you don't intend to use any builtins in ROOT, so this approach should enable the vcpkg usecase.

• https://github.com/root-project/root/pull/16274

[guitargeek]

Reopening because there was some problem with my fix in the nightlies.

[github-actions[bot]]

Hi @pcanal, @bellenot,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely, :robot:

[pcanal]

I think a good compromise is to replace find_package only in the case of using builtins.

It turns out that it needs to be defined 'as soon' as the first built-in is processed (in particular if it is zlib)

For example, an error without it:

CMake Error at /usr/share/cmake/Modules/SelectLibraryConfigurations.cmake:64 (set):
  set given invalid arguments: FORCE specified without CACHE
Call Stack (most recent call first):
  /usr/share/cmake/Modules/FindZLIB.cmake:172 (select_library_configurations)
  /usr/share/cmake/Modules/FindPNG.cmake:68 (find_package)
  cmake/modules/SearchInstalledSoftware.cmake:457 (find_Package)
  CMakeLists.txt:272 (include)

and so we would need to add after each builtin selection something like:

if(builtin_zlib)
  if(_has_not_yet_replace_find_package)
     set(_has_not_yet_replace_find_package true)
     macro(find_package) 
         ...
     endmacro()
 endif()
  list(APPEND ROOT_BUILTINS ZLIB)
  add_subdirectory(builtins/zlib)
endif()

and without finding a way to factor out the boiler plate, this would be hard to maintain.

Another option would be to have a ROOT_NO_BUILTINS that disallow any builtins (maybe excluding the required one: LLVM, Clang, AfterImage).