conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.23k stars 980 forks source link

[bug] Conan setting `CMAKE_FIND_ROOT_PATH_MODE_*=BOTH` fundamentally breaks cross-compilation #16324

Closed tttapa closed 21 hours ago

tttapa commented 5 months ago

Describe the bug

When cross-compiling, it is essential that the build system and the host system are strictly separated. In CMake, this can be ensured by setting CMAKE_FIND_ROOT_PATH_MODE_{LIBRARY,INCLUDE,PACKAGE}=ONLY, which tells the corresponding find_* command to only look for libraries, headers and packages in the host system's sysroot or in the CMAKE_FIND_ROOT_PATH.

However, when tools.build.cross_building:cross_build=true, Conan silently (!) sets these variables to BOTH, and in doing so it allows CMake to look for libraries, headers and packages in the build system's root filesystem as well. As a result, packages cross-compiled using Conan can silently link to wrong versions of libraries it found in /usr/lib, headers of different versions than intended, etc.
In the best case, this results in linker errors because of incompatible architectures, in the worst case, the resulting binaries are subtly broken because they depend on libraries (or versioned symbols in those libraries) that are unavailable on the host system.

If a user explicitly sets CMAKE_FIND_ROOT_PATH_MODE_{LIBRARY,INCLUDE,PACKAGE}=ONLY in their toolchain file, it is often for good reasons, and IMHO silently changing those values inside of Conan's toolchain file is not an acceptable solution.
I can help with a PR, but wanted to discuss here first. In the meantime, I'd suggest adding a very clear warning to Conan's toolchain file if it detects that the user set these variables to ONLY. (It will still be broken, but at least then the people know that it's broken.)

Here are some previous discussions on the same topic:

Addressing some of the comments there:

https://github.com/conan-io/conan/issues/10513 I see the problems with using CMAKE_FIND_ROOT_PATH, though - conan doesn't really want to re-root everybody else's path's either.

I don't believe that this is a problem. Re-rooting all paths is exactly what you want when cross-compiling. If a user wants to search the build system's paths as well, they should not set CMAKE_FIND_ROOT_PATH_MODE_*=ONLY in the first place. But that's for the user to decide, Conan should not silently change this.

https://github.com/conan-io/conan/issues/9427#issuecomment-995642284 Here is the problem, let's take CMAKE_FIND_ROOT_PATH_MODE_INCLUDE set to ONLY as an example: find_file() doesn't look into CMAKE_INCLUDE_PATH (or CMAKE_PREFIX_PATH) in this case, it iterates in paths of CMAKE_FIND_ROOT_PATH, but not the paths themselves, only <path>/include.

Exactly, that's what you want when cross-compiling.

What does it mean? It means that you must add the root package folder to CMAKE_FIND_ROOT_PATH. But by doing this you may also make tools (in <path>/bin) of host context discoverable, and they may be found by find_program() before executables of build context.

Not if you set CMAKE_FIND_ROOT_PATH_MODE_PROGRAM=NEVER, which is the common use case when cross-compiling.

If you think a little bit about this, you'll see that you have no other choice than populating CMAKE_FIND_ROOT_PATH with root package folders of requirements AND build requirements.

No, this is not the correct conclusion. They should stay separate, and CMAKE_FIND_ROOT_PATH and CMAKE_FIND_ROOT_PATH_MODE_* are exactly the tools that CMake provides to allow you to keep them separate.

Actually CMAKE_FIND_ROOT_PATH_MODE_PROGRAM set to ONLY even breaks the discovery of Ninja.

Which is why you usually don't see people setting CMAKE_FIND_ROOT_PATH_MODE_PROGRAM=ONLY when cross-compiling :)


How to reproduce it

Tested on Linux, you can find a reproducible version with Docker here: Script: https://github.com/tttapa/conan-find-mode-bug/blob/main/.github/workflows/test.yml
Output: https://github.com/tttapa/conan-find-mode-bug/actions/runs/9197591352/job/25298402017

conanfile.txt

[generators]
CMakeDeps
CMakeToolchain
[layout]
cmake_layout

toolchain.cmake

# System information
set(CMAKE_SYSTEM_NAME "Linux")
set(CMAKE_SYSTEM_PROCESSOR "x86_64")

# Search path configuration
set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER)
set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY)
set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY)
set(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE ONLY)

# These are the only places where CMake is allowed to look for libraries etc.
set(CMAKE_FIND_ROOT_PATH ${CMAKE_CURRENT_LIST_DIR}/find_root)
set(CMAKE_SYSROOT ${CMAKE_CURRENT_LIST_DIR}/sysroot)

CMakeLists.txt

cmake_minimum_required(VERSION 3.20)
project(conan_find_mode)

set(CMAKE_FIND_DEBUG_MODE On)
# libreadline.so is in /usr/lib/x86_64-linux-gnu on the build system (not in the sysroot)
find_library(LIBREADLINE "libreadline.so")
if (LIBREADLINE)
    message(SEND_ERROR "Should not have found this library: ${LIBREADLINE}")
endif()

Let's first build this project using only CMake:

cmake -B build-cmake -S . --toolchain toolchain.cmake

This works as expected, libreadline.so is not found, because it does not exist in the (empty) sysroot.
In the output, you'll see that CMake only searched the allowed locations: the host system's sysroot, and the directories explicitly allowed by CMAKE_FIND_ROOT_PATH.

Now using Conan:

conan install . -c tools.cmake.cmaketoolchain:user_toolchain="[\"$PWD/toolchain.cmake\"]" -c tools.build.cross_building:cross_build=true
cmake --preset conan-release

This fails. In the output, you can see that CMake is looking in folders like /usr/lib on the build system. Eventually, it finds the file in /usr/lib/x86_64-linux-gnu/libreadline.so. If you're cross-compiling, this file is not likely to be compatible with your host system (e.g. different architecture, different glibc version, different ABI, different compilation flags, etc.), and the resulting binaries are now broken.

memsharded commented 5 months ago

Hi @tttapa

Thanks for your report.

Can you please clarify what Conan version are you using?

Conan 2 sets only:

        {% if build_paths %}
        # The explicitly defined "builddirs" of "host" context dependencies must be in PREFIX_PATH
        list(PREPEND CMAKE_PREFIX_PATH {{ build_paths }})
        {% endif %}
        {% if cmake_program_path %}
        list(PREPEND CMAKE_PROGRAM_PATH {{ cmake_program_path }})
        {% endif %}

from the build context, should this 2 variables CMAKE_PREFIX_PATH or CMAKE_PROGRAM_PATH make the find_library() find a library in the build context?

tttapa commented 5 months ago

Hi, I'm using version 2.3.1.

At first sight, the code you posted looks fine, the problematic lines are further down:

https://github.com/conan-io/conan/blob/95da083793960b74f362a0db6323df37e31e58e9/conan/tools/cmake/toolchain/blocks.py#L484-L502

Looking at the git blame, the last change to this bit of code was two years ago in 861170cdf2959942828f6db33c488c1b823522c2.

jcar87 commented 5 months ago

If I remember correctly, if CMAKE_FIND_ROOT_PATH_MODE_{LIBRARY,INCLUDE,PACKAGE} is set to ONLY, and there is a CMAKE_SYSROOT defined, none of the calls to find_package/find_library/find_program will work to locate anything provided by Conan - the entire search path, incl. CMAKE_PREFIX_PATH and so on, will be re-rooted to the sysroot), and none of the relevant locations will actually be considered.

If a user explicitly sets CMAKE_FIND_ROOT_PATHMODE{LIBRARY,INCLUDE,PACKAGE}=ONLY in their toolchain file, it is often for good reasons, and IMHO silently changing those values inside of Conan's toolchain file is not an acceptable solution.

I could agree that if the variable is already defined with an ONLY, we could warn - either honour it and expect CMake to not find anything provided by conan (the user can provide their own toolchain to work that out if they wish), or warn the user to set it to both.

The only case where I could see this working properly is when the only package provided by conan is the sysroot itself, and the sysroot contains everything and no individual libraries are provided as conan packages.

The provided example is fine and valid, but in the situation described, both are an error for find_library(LIBREADLINE "libreadline.so")

Obviously I can see how erroring out early is much preferable, as it would point you to arrive at the solution sooner - but once the underlying problem is addressed (libreadline.so is present in the sysroot), it will work correctly even with the current Conan behaviour.

tttapa commented 5 months ago

If I remember correctly, if CMAKE_FIND_ROOT_PATH_MODE_{LIBRARY,INCLUDE,PACKAGE} is set to ONLY, and there is a CMAKE_SYSROOT defined, none of the calls to find_package/find_library/find_program will work to locate anything provided by Conan - the entire search path, incl. CMAKE_PREFIX_PATH and so on, will be re-rooted to the sysroot), and none of the relevant locations will actually be considered.

This is true, but only because Conan does not currently set CMAKE_FIND_ROOT_PATH.

The provided example is fine and valid, but in the situation described, both are an error for find_library(LIBREADLINE "libreadline.so")

  • if it is not in the sysroot, error out early (the desired outcome)
  • if it found in the build machine, it will error out late (and attempt a link against the wrong one)

No, this is not a given. For example, I'm in a situation where I'm cross-compiling with build=x86_64-linux-gnu and host=x86_64-linux-gnu, but they're different systems with different Linux distributions, different toolchains, and different versions of glibc etc. Even though it may happen to link without errors, linking to binaries from the build system will still result in binaries that do not run once deployed on the host system. Or even worse: it may appear to run correctly in most cases, but ABI incompatibilities or ODR violations cause it to crash sporadically or make it exploitable to bad actors.

Second, when cross-compiling, one often prepares a sysroot with specific versions of packages. If for some reason, CMake is unable to find the intended package in the sysroot (e.g. because of a typo in the paths provided by the user), or it is unable to find the Conan-provided package, CMake should should fail, not silently link to a random version of the package it picked up in a /opt/side-projects/thirdparty/libfoo-master folder that happened to be in the build system's PATH.

memsharded commented 1 week ago

The new CMakeDeps generator in https://github.com/conan-io/conan/pull/16964 will propose a new ay of locating config files.

tttapa commented 1 week ago

Thanks! I plan to look into this some more and propose some changes, but I haven't found the time yet, unfortunately.

memsharded commented 20 hours ago

We are releasing in Conan 2.9 a completely new CMakeDeps generator in https://github.com/conan-io/conan/pull/16964 that has closed this ticket, with many pending features and fixes:

Current known pending functionality (to be added soon):

The new CMakeDeps generator is intended for testing and validation only, being a transparent replacement of the old one, so it is behind a new conf. To use it, use the -c tools.cmake.cmakedeps:new=will_break_next, and that will use the new generator instead of the old one. Note the will_break_next value means exactly that, that value will change in next release to force a break, so no one can depend on this generator in production yet.

Your feedback is very important

As this is a major change, we will only remove the conf gate when we get confirmation from users that it works and solve the issues. Please try the new generator for your project, and let us know if it works. If it doesn't, please re-open this ticket and let us know what failed. Thanks very much!