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
Bugfix: use CMAKE_FIND_ROOT_PATH_BOTH to locate frameworks #7515
Conversation
Signed-off-by: SSE4 <tomskside@gmail.com>
@@ -850,7 +850,7 @@ class CMakeCommonMacros: | |||
endif() | |||
foreach(_FRAMEWORK ${FRAMEWORKS}) | |||
# https://cmake.org/pipermail/cmake-developers/2017-August/030199.html | |||
find_library(CONAN_FRAMEWORK_${_FRAMEWORK}_FOUND NAME ${_FRAMEWORK} PATHS ${CONAN_FRAMEWORK_DIRS${SUFFIX}}) | |||
find_library(CONAN_FRAMEWORK_${_FRAMEWORK}_FOUND NAME ${_FRAMEWORK} PATHS ${CONAN_FRAMEWORK_DIRS${SUFFIX}} CMAKE_FIND_ROOT_PATH_BOTH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to know when we could use NO_DEFAULT_PATH
, because we fully know this is a framework that lives in the Conan package, and forget about all the system paths, and when it would be a system framework and then it could be found in other locations rather than in the ${CONAN_FRAMEWORK_DIRS${SUFFIX}}
Now that we have the system_libs
, this could be the same for frameworks? system_frameworks
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. It would be a better model. (Not to do it in this PR, but something to do)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Then we should turn this bugfix into a feature for 1.29
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I think having a cpp_info.system_frameworks
makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it makes sense, but it will be much larger change - e.g. update several generators at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I am not saying that it is a change for this PR, but something to consider for a feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly the documentation about find_package
, using CMAKE_FIND_ROOT_PATH_BOTH
is the default behavior, in fact the proposed tests already pass without the source changes.
@jgsogo do you have proof? did you try it to run the test locally with no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my fault. There are two classes with tests in that file and I was running the wrong one.
Indeed the change is needed to find those frameworks in the cross-compiling scenario
closes: #6416
test case adapted from https://github.com/Paultergeist/conan_apple_framework
see find_library documentation.
Changelog: Bugfix: Use
CMAKE_FIND_ROOT_PATH_BOTH
to locate frameworks.Docs: omit
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.