CMakeDeps: no more modification of CMAKE_MODULE_PATH/CMAKE_PREFIX_PATH

[SpaceIm]

Changelog: (Fix): Remove CMAKE_MODULE_PATH/CMAKE_PREFIX_PATH modification in files generated by CMakeDeps
Docs: https://github.com/conan-io/docs/pull/XXXX

closes #12237

Basically these modifications of CMAKE_MODULE_PATH/CMAKE_PREFIX_PATH in CMakeDeps can defeat the logic of CMakeToolchain implemented in #10186.
For example: conan-io/conan-center-index#13269 (comment)

It means that if:

• a recipeA provides both a lib & an executable foo
• user depends on recipeA both in requires & tool_requires
• user relies on CMakeToolchain & CMakeDeps, and has find_package(recipeA) before find_program(FOO NAMES foo)

then, find_program() will find executable foo from recipeA "requires" package instead of recipeA "tool_requires" package.

So this PR removes this side effect.

Anyway it's not the responsibility of a config file to modify these variables (canonical files generated by a install(EXPORT ...) command don't do that).

• 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.}

[jcar87]

Hi @SpaceIm - thanks for your contribution.

I'm looking into this and trying to understand in which scenarios this is a problem.

I have a feeling that if these lines were to be removed, we would be impacting the functionality of cmake_build_modules if declared in cpp_info.

This:

        set(CMAKE_MODULE_PATH {{ '${' }}{{ pkg_name }}_BUILD_DIRS{{ config_suffix }}} {{ '${' }}CMAKE_MODULE_PATH})
        set(CMAKE_PREFIX_PATH {{ '${' }}{{ pkg_name }}_BUILD_DIRS{{ config_suffix }}} {{ '${' }}CMAKE_PREFIX_PATH})

will result result in something like:

set(CMAKE_MODULE_PATH ${xxx_BUILD_DIRS_yyy} ${CMAKE_MODULE_PATH})
set(CMAKE_PREFIX_PATH ${xxx_BUILD_DIRS_yyy} ${CMAKE_PREFIX_PATH})

and the variable xxx_BUILD_DIRS_yyy will typically be empty unless the package_info() defines cmake_build_modules in cpp_info. So these two lines should already be a no-op in a great number of cases.

When we have this situation:

    requires = "xxx/9.9"
    tool_requires = "xxx/9.9"

The CMakeDeps generator will only generate the files for the requires (not the tool_requires). Unless one uses build_context_activated (docs here). In this case, we would also need build_context_suffix (docs here) to disambiguate the name duplication.

So I would imagine that for CMAKE_PREFIX_PATH/CMAKE_MODULE_PATH to be actually manipulated in the situation you describe, we would need the following conditions:

• package_info() sets cmake_build_modules
• a consumer lists this as a requires and a tool_requires
• for the tool_requires, build_context_activated is explicitly set
• as well as build_context_suffix to disambiguate the names
• the consumer would need to have two separate find_package() calls, one for the actual name and the suffixed name.

What should happen otherwise is that the CMakeToolchain + CMakeDeps generators should be aware of build/host profiles and requires/tool_requires and result in the conan_toolchain.cmake having something like this in this scenario:

# Definition of CMAKE_PREFIX_PATH, CMAKE_XXXXX_PATH
# The Conan local "generators" folder, where this toolchain is saved.
list(PREPEND CMAKE_PREFIX_PATH ${CMAKE_CURRENT_LIST_DIR} )
list(PREPEND CMAKE_PROGRAM_PATH "/Users/luisc/.conan/data/libtasn1/4.16.0/_/_/package/e98cc99c78f8ae352930d807a85fc7d6df891023/bin")
list(PREPEND CMAKE_LIBRARY_PATH "/Users/luisc/.conan/data/libtasn1/4.16.0/_/_/package/299aaabcb5fb1385c096ccf2c550a653c4438173/lib")
list(PREPEND CMAKE_INCLUDE_PATH "/Users/luisc/.conan/data/libtasn1/4.16.0/_/_/package/299aaabcb5fb1385c096ccf2c550a653c4438173/include")

where CMAKE_PROGRAM_PATH already points to the location of the tool_requires package in the build context.

Is there a chance that the issue is not with CMakeToolchain and CMakeDeps, but with the legacy cmake + cmake_find_package_multi ? In that scenario I can see that the only generated .cmake files the build is exposed to are the host files, which would explain the failure.

[SpaceIm]

I have a feeling that if these lines were to be removed, we would be impacting the functionality of cmake_build_modules if declared in cpp_info.

Why? cmake_build_modules files are included with full path in config file, so CMAKE_MODULE_PATH doesn't matter.

This:

        set(CMAKE_MODULE_PATH {{ '${' }}{{ pkg_name }}_BUILD_DIRS{{ config_suffix }}} {{ '${' }}CMAKE_MODULE_PATH})
        set(CMAKE_PREFIX_PATH {{ '${' }}{{ pkg_name }}_BUILD_DIRS{{ config_suffix }}} {{ '${' }}CMAKE_PREFIX_PATH})

will result result in something like:

set(CMAKE_MODULE_PATH ${xxx_BUILD_DIRS_yyy} ${CMAKE_MODULE_PATH})
set(CMAKE_PREFIX_PATH ${xxx_BUILD_DIRS_yyy} ${CMAKE_PREFIX_PATH})

and the variable xxx_BUILD_DIRS_yyy will typically be empty unless the package_info() defines cmake_build_modules in cpp_info. So these two lines should already be a no-op in a great number of cases.

You mean "unless builddirs is defined in cpp_info" instead?

Anyway, try conan-io/conan-center-index@9569dbf with conan 1.52.0, 2 profiles, and cross-build with different arch, you'll see that ${xxx_BUILD_DIRS_yyy} is not empty in config file of libtasn1 in test package.

When we have this situation:

    requires = "xxx/9.9"
    tool_requires = "xxx/9.9"

The CMakeDeps generator will only generate the files for the requires (not the tool_requires). Unless one uses build_context_activated (docs here). In this case, we would also need build_context_suffix (docs here) to disambiguate the name duplication.

So I would imagine that for CMAKE_PREFIX_PATH/CMAKE_MODULE_PATH to be actually manipulated in the situation you describe, we would need the following conditions:

• package_info() sets cmake_build_modules
• a consumer lists this as a requires and a tool_requires
• for the tool_requires, build_context_activated is explicitly set
• as well as build_context_suffix to disambiguate the names
• the consumer would need to have two separate find_package() calls, one for the actual name and the suffixed name.

Unrelated

What should happen otherwise is that the CMakeToolchain + CMakeDeps generators should be aware of build/host profiles and requires/tool_requires and result in the conan_toolchain.cmake having something like this in this scenario:

# Definition of CMAKE_PREFIX_PATH, CMAKE_XXXXX_PATH
# The Conan local "generators" folder, where this toolchain is saved.
list(PREPEND CMAKE_PREFIX_PATH ${CMAKE_CURRENT_LIST_DIR} )
list(PREPEND CMAKE_PROGRAM_PATH "/Users/luisc/.conan/data/libtasn1/4.16.0/_/_/package/e98cc99c78f8ae352930d807a85fc7d6df891023/bin")
list(PREPEND CMAKE_LIBRARY_PATH "/Users/luisc/.conan/data/libtasn1/4.16.0/_/_/package/299aaabcb5fb1385c096ccf2c550a653c4438173/lib")
list(PREPEND CMAKE_INCLUDE_PATH "/Users/luisc/.conan/data/libtasn1/4.16.0/_/_/package/299aaabcb5fb1385c096ccf2c550a653c4438173/include")

where CMAKE_PROGRAM_PATH already points to the location of the tool_requires package in the build context.

Is there a chance that the issue is not with CMakeToolchain and CMakeDeps, but with the legacy cmake + cmake_find_package_multi ? In that scenario I can see that the only generated .cmake files the build is exposed to are the host files, which would explain the failure.

No, it's an issue with CMakeDeps. CMakeToolchain is fine I think, I've made the PR allowing to properly find executables of build context with find_program() even when executable is available both from host & build context, so I know quite well how it works.

[jcar87]

Hi @SpaceIm - I see what you mean, I can reproduce this and can see how the files generated by CMakeDeps modify the contents of CMAKE_PREFIX_PATH.

I have opened an issue for us to track this: #12237 - as first we'd like to make sure that removing this does not impact existing functionality.

It may as well be that this PR fixes the issue without any negative consequences - however, we would typically include a functional test that minimally reproduces the issue, fails on develop, and then passes after the merge.

[SpaceIm]

It may as well be that this PR fixes the issue without any negative consequences - however, we would typically include a functional test that minimally reproduces the issue, fails on develop, and then passes after the merge.

I agree. I'll do when I have time (probably not this week).

[SpaceIm]

@jcar87 I've updated one of the functional test, this test fails without the fix in this PR. Maybe it should be a dedicated integration test instead since it tests an interaction between CMakeToolchain & CMakeDeps?

(by the way conftest.py is broken on my mac, I have to disable completely add_tool, otherwise I can't run tests based on CMake, it was working fine few months ago. Seems to come from #11941)

[czoido]

Thanks for the contribution, closing this in favor of #12464 that implements part of these changes.