CMakeToolchain: proper support of find_*() & include() commands #10186

SpaceIm · 2021-12-16T16:27:32Z

Changelog: Fix: Improved CMakeToolchain robustness regarding find_file, find_path and find_program commands allowing better cross-build scenarios and better differentiation of the right context where to get, for example, executables (build vs host).
Docs: conan-io/docs#2383

In find_program(), avoid to find executables of host context before executables of build context.
support all cross-build scenario (hopefully).
support include() of CMake modules coming from build context.
remove CMakeToolchain.find_builddirs (doesn't make sense anymore).
Refer to the issue that supports this Pull Request.
If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
I've read the Contributing guide.
I've followed the PEP8 style guides for Python code.
I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

^{Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.}

closes #9427
closes #9735
closes #10167

- avoid to find host executables in find_program() - support cross-build scenario - support include() of CMake modules coming from build context

CLAassistant · 2021-12-16T16:27:38Z

All committers have signed the CLA.

SpaceIm · 2021-12-16T16:40:04Z

Two comments regarding existing code I didn't change:

I don't understand what is the purpose of android part, it seems to specific and dangerous since it manipulates CMAKE_FIND_ROOT_PATH which is the top precedence in all paths checked by find_*() commands
=> ok, I've removed this part, it's the usual issue due to CMAKE_FIND_ROOT_PATH_MODE_* set to ONLY.
CMAKE_FIND_PACKAGE_PREFER_CONFIG can be removed (another PR?) now that CMakeDeps can generate Find files (cmake_find_mode property).

SpaceIm · 2021-12-16T17:11:13Z

tests fail because:

they want to be able to find some config files in the root package folder, which is something I don't want find_package() can do, because adding root package folder to CMAKE_PREFIX_PATH has too many side effects.
Actually, there should be 2 more properties in cpp_info of recipes: cmake_module_path (folders of packaged CMake module and Find files) & cmake_prefix_path (folders of packaged pkgconfig & CMake Config files), empty by default, and CMAKE_MODULE_PATH & CMAKE_PREFIX_PATH should be populated with these paths respectively (+ CMAKE_CURRENT_LIST_DIR). Indeed builddirs is too generic and semantically unrelated to CMAKE_MODULE_PATH & CMAKE_PREFIX_PATH.
Anyway, here I expect folders of packaged CMake module and config files to be added explicitly in builddirs.
I have removed find_builddir logic, but not related test. I think this switch won't make sense after this PR.

Maybe I should have added the tests I want to succeed in the first place (same recipe in requirements & build requirements with libs & tools packaged, discovery of module files from build requirements).

memsharded

I have my concerns about the extension of this to "dependencies.build". Things liks libs, headers and frameworks probably shouldn't be added by default. And the executables and shared libs don't need to be added to the path, because they will already be in the PATH and LD_LIBRARY_PATH, because they are put there by the VirtualBuildEnv (which is a bit more general solution, as it works for every build system)

conan/tools/cmake/toolchain.py

memsharded · 2021-12-17T00:54:52Z

conan/tools/cmake/toolchain.py

@@ -425,34 +439,64 @@ def context(self):
        if prefer_config is not None and prefer_config.lower() in ("false", "0", "off"):
            find_package_prefer_config = "OFF"

-        os_ = self._conanfile.settings.get_safe("os")
-        android_prefix = "${CMAKE_CURRENT_LIST_DIR}" if os_ == "Android" else None


This was necessary at some point, to have the Android build finding the xxx-config.cmake by Conan. Maybe it is no longer needed, but it is difficult to know, because we are not running yet Android build tests in our CI

Yes, it's the same issue than cross-build to iOS/tvOS/watchOS, but 2 different solutions have been implemented. Here it's fixed by forcing CMAKE_FIND_ROOT_PATH_MODE_PACKAGE to "BOTH" (same fix than iOS/tvOS/watchOS).

Of course, the best solution would be to not have to manipulate CMAKE_FIND_ROOT_PATH_MODE_* but other solutions raise other issues (conflicts between host & build context), and I think this one is harmless compare to other solutions.

CMAKE_MODULE_PATH has no side effects in find_program()/find_library()/find_file()/file_path(), so it's fine. What should be avoided is to add root package folder to CMAKE_PREFIX_PATH, otherwise executables of host context would have precedence over executables of build context in find_program() calls.

SpaceIm · 2021-12-17T09:23:26Z

TODO:

add test to check that executables of build context are found before executables of host context in find_program() (native & cross build, recipe in build requirement only & recipe in requirements and build requirements).
add test to check that CMake modules of build context are accessible with include() (native & cross build).

lasote

I understand these issues (and agree with them) and I see a lot of value in this PR to understand the issues we currently have. But still not convinced about the way to resolve them. Should we use more properties to declare specific things like config file paths? and maybe more things?

Should we do this in Conan 1.X even if maybe the better fix (not either sure) is to change the cppinfo model or defaults?

Also, I think this PR does some "tricks" like changing the ROOT_PATH_MODE variables and filling CMAKE_FIND_ROOT_PATH, probably there is not single answer for all cases, I'm afraid it might depend on every project/package/toolchain, in some cases, I would need to find some things in my packages, sometimes I don't, sometimes from build, sometimes from host...All these questions should be answered by the user, the challenge is how to do it understandable and maintainable.

conan/tools/cmake/toolchain.py

lasote · 2021-12-22T15:32:05Z

conan/tools/cmake/toolchain.py

+        endif()
+
+        if(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE STREQUAL "ONLY")
+            set(CMAKE_FIND_ROOT_PATH ${CMAKE_FIND_ROOT_PATH} {{ generators_folder }} {{ host_build_paths_noroot }})


is this trick necessary? why not set BOTH? I'm sure there is an explanation.

Sure, the toolchain could force CMAKE_FIND_ROOT_PATH_MODE_PACKAGE to BOTH. But I would like to avoid to override this variable if set by user toolchain.

I'm wondering if the user toolchain is setting CMAKE_FIND_ROOT_PATH_MODE_PACKAGE STREQUAL "ONLY", wouldn't be mostly the same modifying the CMAKE_FIND_ROOT_PATH?. Why is that ok but not overriding the CMAKE_FIND_ROOT_PATH_MODE_PACKAGE variable?

Not sure to understand the questions. Would you like to set CMAKE_FIND_ROOT_PATH with generator folder and non-root builddirs of requires, regardless of CMAKE_FIND_ROOT_PATH_MODE_PACKAGE value?

No, just saying that you are protecting the user set value from CMAKE_FIND_ROOT_PATH_MODE_PACKAGE because I would like to avoid to override this variable if set by user toolchain. but then you are modifying the CMAKE_FIND_ROOT_PATH, so I'm wondering if it is not kind of "cheating".

In other words, why is so important to keep the user value of CMAKE_FIND_ROOT_PATH_MODE_PACKAGE (and others)?

Ok, so when CMAKE_FIND_ROOT_PATH_MODE_* is ONLY, you have no other choice than manipulating CMAKE_FIND_ROOT_PATH because all other variables are ignored. CMAKE_FIND_ROOT_PATH is like CMAKE_PREFIX_PATH but with higher precedence, so no it's not cheating.

Why is it important to honor CMAKE_FIND_ROOT_PATH_MODE_* of users? Principle of least surprise.
But as I said before, if ONLY is overridden by BOTH, it should not break consumers (other proposal in #10186 (comment)), because system paths have the lowest priority in search paths of find_*() commands, and BOTH still look into CMAKE_FIND_ROOT_PATH first if the user has injected some stuff not managed by conan.

Then I would simplify and just override to BOTH.

Ok, so when CMAKE_FIND_ROOT_PATH_MODE_* is ONLY, you have no other choice than manipulating CMAKE_FIND_ROOT_PATH because all other variables are ignored. CMAKE_FIND_ROOT_PATH is like CMAKE_PREFIX_PATH but with higher precedence, so no it's not cheating.

Yes, I got that, but:

Why is it important to honor CMAKE_FIND_ROOT_PATH_MODE_* of users? Principle of least surprise.

Not sure that changing CMAKE_FIND_ROOT_PATH is least surprise either, so I vote for simplifying and just overriding to BOTH when cross_build.

simplified in 1e89fe9

CMAKE_FIND_ROOT_PATH_MODE_* of user honored unless it's ONLY.

lasote · 2021-12-22T15:33:29Z

conan/tools/cmake/toolchain.py

+        if(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE STREQUAL "ONLY")
+            set(CMAKE_FIND_ROOT_PATH ${CMAKE_FIND_ROOT_PATH} {{ generators_folder }} {{ host_build_paths_noroot }})
+        endif()
+        if(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM STREQUAL "NEVER")


Same question for all "ifs". Sounds very complex :(

Yes it's complex, I'll try to explain the all logic when I have time, it's not obvious. I try to address #10186 (comment).
If you want something simple, I can fallback to something like this:

template = textwrap.dedent(""" ... {% if cross_building %} if(NOT DEFINED CMAKE_FIND_ROOT_PATH_MODE_PACKAGE OR CMAKE_FIND_ROOT_PATH_MODE_PACKAGE STREQUAL "ONLY") set(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE "BOTH") endif() if(NOT DEFINED CMAKE_FIND_ROOT_PATH_MODE_PROGRAM OR CMAKE_FIND_ROOT_PATH_MODE_PROGRAM STREQUAL "ONLY") set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM "BOTH") endif() if(NOT DEFINED CMAKE_FIND_ROOT_PATH_MODE_LIBRARY OR CMAKE_FIND_ROOT_PATH_MODE_LIBRARY STREQUAL "ONLY") set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY "BOTH") endif() {% if is_apple %} if(NOT DEFINED CMAKE_FIND_ROOT_PATH_MODE_FRAMEWORK OR CMAKE_FIND_ROOT_PATH_MODE_FRAMEWORK STREQUAL "ONLY") set(CMAKE_FIND_ROOT_PATH_MODE_FRAMEWORK "BOTH") endif() {% endif %} if(NOT DEFINED CMAKE_FIND_ROOT_PATH_MODE_INCLUDE OR CMAKE_FIND_ROOT_PATH_MODE_INCLUDE STREQUAL "ONLY") set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE "BOTH") endif() {% endif %} """)

conan/tools/cmake/toolchain.py

SpaceIm · 2022-01-14T08:40:30Z

some conflicts to solve

memsharded · 2022-01-19T18:13:41Z

I am contributing a fix to the broken test in 6bb0be8.

The test was failing because we added this new test to run CMake with -werror -warn, and this was detecting a bunch of warnings because of usage of uniniatilized variables. I am changing those lines to list(PREPEND ...) to avoid that warning, and I also took a chance and splitted the CMAKE_MODULE_PATH and CMAKE_PREFIX_PATH in multiple lines, adding a comment to each one. I think it will be useful for readers trying to understand what the toolchain is doing and why is adding some folders to those variables (the explanation in each line comment might be poor, it would be great to add the full explanation: "adding to CMAKE_PREFIX_PATH so we can locate the ....config.cmake inside, etc, etc")

memsharded · 2022-01-19T19:52:35Z

I had to push a few changes more, after merging latest "develop" state in 5d19b4f.

The most important thing that changed is that now the cppinfo directories are absolute, everything was almost good, except the comparison of builddirs == "", that was broken. Now it seems to be fixed.

CMakeToolchain: proper support of find_*() & include() commands

f2e977d

- avoid to find host executables in find_program() - support cross-build scenario - support include() of CMake modules coming from build context

SpaceIm added 6 commits December 16, 2021 20:41

CMakeToolchain: remove find_builddirs

f04fb1b

don't test include() not in toolchain file

ff262ba

test builddirs other that root package folder

f21ed65

unconditionally force CMAKE_FIND_ROOT_PATH_MODE_*

16c941a

CMakeToolchain: android workaround not needed anymore

dba1989

CMakeToolchain: fix comments

be4b91d

SpaceIm mentioned this pull request Dec 16, 2021

[bug] CMakeToolchain does not include cpp_info.builddirs from build requirements in CMAKE_PREFIX_PATH CMAKE_MODULE_PATH #10167

Closed

memsharded reviewed Dec 17, 2021

View reviewed changes

conan/tools/cmake/toolchain.py Show resolved Hide resolved

memsharded requested a review from lasote December 17, 2021 00:08

memsharded added this to the 1.44 milestone Dec 17, 2021

SpaceIm added 2 commits December 17, 2021 01:17

cmaketoolchain: allow to find host executables if not cross-building

5204b73

CMakeToolchain: intermediate module for host & build context

9aa71cf

memsharded reviewed Dec 17, 2021

View reviewed changes

SpaceIm added 2 commits December 17, 2021 09:31

factorize

029dfc0

rename variables to distinguish host & build

ab11029

SpaceIm force-pushed the feature/cmake-toolchain-robust-find-commands branch from f2a242d to 66a93d8 Compare December 17, 2021 08:56

SpaceIm force-pushed the feature/cmake-toolchain-robust-find-commands branch from 66a93d8 to 75812f4 Compare December 17, 2021 08:57

lasote requested a review from franramirez688 December 17, 2021 09:53

SpaceIm mentioned this pull request Dec 17, 2021

[question] Unable to find_package with cmake when cross compiling #9735

Closed

1 task

SpaceIm added 2 commits December 18, 2021 23:06

Merge branch 'develop' into feature/cmake-toolchain-robust-find-commands

858fab8

test include() of cmake modules from requires & build_requires

e34655a

SpaceIm force-pushed the feature/cmake-toolchain-robust-find-commands branch from 132f23f to e34655a Compare December 19, 2021 01:23

SpaceIm added 2 commits December 19, 2021 03:59

tests of find_file()/find_path()/find_library()/find_program()

f9978e8

fix test_cmaketoolchain_path_find_program test on windows

e020731

lasote added the type: look into label Dec 22, 2021

lasote reviewed Dec 22, 2021

View reviewed changes

lasote modified the milestones: 1.44, 1.45 Dec 28, 2021

lasote removed the type: look into label Dec 28, 2021

SpaceIm mentioned this pull request Dec 28, 2021

[grpc] Allow cross to M1/iOS conan-io/conan-center-index#8560

Closed

4 tasks

SpaceIm added 3 commits December 29, 2021 19:43

simplify

1e89fe9

Merge branch 'develop' into feature/cmake-toolchain-robust-find-commands

8a5ee2b

cleanup

c244e30

lasote reviewed Jan 11, 2022

View reviewed changes

conan/tools/cmake/toolchain.py Outdated Show resolved Hide resolved

lasote approved these changes Jan 14, 2022

View reviewed changes

SpaceIm and others added 3 commits January 14, 2022 09:43

Merge branch 'develop' into feature/cmake-toolchain-robust-find-commands

fd72d4a

Merge branch 'develop' into feature/cmake-toolchain-robust-find-commands

121b61a

using CMake list(APPEND instead of set with unitialized variables

6bb0be8

memsharded approved these changes Jan 19, 2022

View reviewed changes

fixed absolute paths issues

5d19b4f

fix iOS tests missing os.sdk

b1d69a7

lasote self-assigned this Jan 20, 2022

lasote merged commit 861170c into conan-io:develop Jan 20, 2022

SpaceIm deleted the feature/cmake-toolchain-robust-find-commands branch January 20, 2022 13:12

SpaceIm mentioned this pull request Jan 23, 2022

grpc: fix cross-build + provide all CMake executable imported targets + add shared option + modernize conan-io/conan-center-index#9044

Merged

4 tasks

lasote mentioned this pull request Feb 1, 2022

Docs for 10186 conan-io/docs#2383

Merged

puetzk mentioned this pull request Feb 4, 2022

Avoid setting CMAKE_FIND_ROOT_PATH_MODE_LIBRARY BOTH in CMakeToolchain #10513

Closed

SpaceIm mentioned this pull request Feb 23, 2022

[question] Conan 1.45.0 does not add root folders to CMAKE_PREFIX_PATH #10648

Closed

lasote mentioned this pull request Aug 16, 2022

[bug] FMT missing builddirs for the component #11870

Closed

This was referenced Oct 2, 2022

libtasn1: conan v2 support conan-io/conan-center-index#13269

Merged

CMakeDeps: no more modification of CMAKE_MODULE_PATH/CMAKE_PREFIX_PATH #12229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMakeToolchain: proper support of find_*() & include() commands #10186

CMakeToolchain: proper support of find_*() & include() commands #10186

SpaceIm commented Dec 16, 2021 •

edited by lasote

Loading

CLAassistant commented Dec 16, 2021 •

edited

Loading

SpaceIm commented Dec 16, 2021 •

edited

Loading

SpaceIm commented Dec 16, 2021 •

edited

Loading

memsharded left a comment

memsharded Dec 17, 2021

SpaceIm Dec 17, 2021 •

edited

Loading

SpaceIm commented Dec 17, 2021 •

edited

Loading

lasote left a comment

lasote Dec 22, 2021

SpaceIm Dec 22, 2021

lasote Dec 22, 2021 •

edited

Loading

SpaceIm Dec 23, 2021

lasote Dec 28, 2021

lasote Dec 28, 2021

SpaceIm Dec 28, 2021 •

edited

Loading

lasote Dec 28, 2021

SpaceIm Dec 29, 2021

lasote Dec 22, 2021

SpaceIm Dec 23, 2021 •

edited

Loading

SpaceIm commented Jan 14, 2022

memsharded commented Jan 19, 2022

memsharded commented Jan 19, 2022

CMakeToolchain: proper support of find_*() & include() commands #10186

CMakeToolchain: proper support of find_*() & include() commands #10186

Conversation

SpaceIm commented Dec 16, 2021 • edited by lasote Loading

CLAassistant commented Dec 16, 2021 • edited Loading

SpaceIm commented Dec 16, 2021 • edited Loading

SpaceIm commented Dec 16, 2021 • edited Loading

memsharded left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

SpaceIm commented Dec 17, 2021 • edited Loading

lasote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasote Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Dec 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

SpaceIm commented Jan 14, 2022

memsharded commented Jan 19, 2022

memsharded commented Jan 19, 2022

SpaceIm commented Dec 16, 2021 •

edited by lasote

Loading

CLAassistant commented Dec 16, 2021 •

edited

Loading

SpaceIm commented Dec 16, 2021 •

edited

Loading

SpaceIm commented Dec 16, 2021 •

edited

Loading

SpaceIm Dec 17, 2021 •

edited

Loading

SpaceIm commented Dec 17, 2021 •

edited

Loading

lasote Dec 22, 2021 •

edited

Loading

SpaceIm Dec 28, 2021 •

edited

Loading

SpaceIm Dec 23, 2021 •

edited

Loading