Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop mutating CMAKE_PREFIX_PATH in config file generated by CMakeDeps #12464

Merged
merged 6 commits into from
Nov 7, 2022

Conversation

jcar87
Copy link
Contributor

@jcar87 jcar87 commented Nov 4, 2022

Changelog: Bugfix: Fix issue where find_program does not work for a tool requirement in the build context, when the dependency is also a regular requirement in the host context.
Docs: Omit

Close #12237

Notes

This fixes a bug where the following two invocations were dependent on the order of the calls. As investigated in #12237, there does not appear to be a case where the generated config files by CMakeDeps need to alter the value of CMAKE_PREFIX_PATH - where needed, that is handled by CMakeToolchain.

For the following, when cross building, in both instances the following two invocations should find the binary executable from the package in the build context. Before this fix, calling find_program after find_package would result in the binary executable being found in the host context (which may not run at all).

        find_package(foobar CONFIG REQUIRED)
        find_program(FOOBIN_EXECUTABLE foobin)

and this

        find_program(FOOBIN_EXECUTABLE foobin)
        find_package(foobar CONFIG REQUIRED)


host_profile = textwrap.dedent("""
[settings]
os=Linux
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a functional test, this will probably be broken in Win/OSX? need a pytest.skip?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really test functionality that is platform specific - it's "cmake only" - figured that by fixing the profiles and setting CMAKE_CXX_COMPILER_WORKS in CMake (I've seen this done in other tests, this would enable us to runt this test on all platforms, or at least to write it in a way that is more platform agnostic

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that providing a profile like that for Windows would fail at some point, like the armv8 architecture, or something like that. Probably can be simplified and left mostly with the platform specific ones for testing, but I am ok with it as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, the host profile could be listing a random architecture here that is not x86_64 - this test is not exercising anything that is platform specific, hence I feel it's safe to "fix" the profiles like many other tests do. A different approach could be tried if we want the profiles to match something that makes more sense on the machine the test is running on.

def layout(self):
pass
def package(self):
self.copy(pattern="lib*", dst="lib")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use modern copy() for merging to 2.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do! Will update shortly. The test as it is needs other changes to work on 2.0

@memsharded memsharded added this to the 1.54 milestone Nov 4, 2022
@SpaceIm
Copy link
Contributor

SpaceIm commented Nov 5, 2022

Should I close #12229?

@@ -88,7 +88,6 @@ def template(self):

# FIXME: What is the result of this for multi-config? All configs adding themselves to path?
set(CMAKE_MODULE_PATH {{ '${' }}{{ pkg_name }}_BUILD_DIRS{{ config_suffix }}} {{ '${' }}CMAKE_MODULE_PATH})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is harmless but honestly a config file manipulating CMAKE_MODULE_PATH is unusual, I think it's too intrusive.

@czoido czoido merged commit eb923de into conan-io:develop Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] self.cpp_info.builddirs default value causes CMAKE_PREFIX_PATH to be mutated during find_package
5 participants