-
Notifications
You must be signed in to change notification settings - Fork 250
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
Ensure that Find<PackageName>.cmake modules work #536
Conversation
ab52cb4
to
2cd1d35
Compare
2cd1d35
to
7730aa9
Compare
7730aa9
to
7a76da3
Compare
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.
Many thanks for checking it and contributing this so quickly!
@@ -275,7 +275,8 @@ macro(conan_provide_dependency package_name) | |||
if(${index} EQUAL -1) | |||
list(PREPEND CMAKE_PREFIX_PATH "${CONAN_GENERATORS_FOLDER}") | |||
endif() | |||
find_package(${ARGN} BYPASS_PROVIDER CMAKE_FIND_ROOT_PATH_BOTH) | |||
set(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE 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.
The provider captures every find_package()
, so I don't see how restoring the value after the call would make any difference, so I think it is good as-is.
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.
I think it's probably best to leave CMAKE_FIND_ROOT_PATH_MODE_PACKAGE
untouched, regardless of whether we set it in the call to find_package
, or before as in this PR.
When cross building, the BOTH
value gives priority and re-roots the entire search order around CMAKE_FIND_ROOT_PATH
and CMAKE_SYSROOT
if defined. So if the Sysroot contains CMake configuration files, it will look for those FIRST, before looking in the CONAN_GENERATORS_FOLDER
which is what we want. This is likely to happen on sysroots for Linux distros, like Raspberry Pi or NVIDIA Jetson.
The issue with the Android NDK which motivated this workaround, as reported in this previous PR: #521 (comment) - has to do with the fact that the Android NDK toolchain sets this to ONLY
, which causes find_package to never consider anything outside of the sysroot: android/ndk#517
When it comes to the specific variable CMAKE_FIND_ROOT_PATH_MODE_PACKAGE
, this is probably a defect/bug or oversight in the Android NDK toolchain: there aren't any packages in the sysroot - so while this approach may be valid for libraries and headers, it is not for CMake packages, unless the expectation is that users add files to the Android sysroot:
/opt/android_ndk_r25b/toolchains/llvm/prebuilt/darwin-x86_64/sysroot/usr
$ find . -name "*.cmake"
^ (the following returns nothing)
If we set this to BOTH
- when crossbuilding we would be in essence recreating the problems described in this issue: conan-io/conan#12478 - giving more priority to the sysroot than to Conan.
We also cannot set this to NEVER
, because it's not entirely uncommon to expect some packages to be provided by the SDK/sysroot.
So taking into consideration that we do indeed capture all calls to find_package
, we probably need a logic that ensures Conan is searched first, and if the package is not found, then it follows normal procedure. CMake's documentation does suggest an option:
find_package (<PackageName> PATHS paths... NO_DEFAULT_PATH)
find_package (<PackageName>)
which in our case could be translated to:
find_package (<PackageName> ${ARGN} PATHS "${CONAN_GENERATORS_FOLDER}" NO_DEFAULT_PATH)
find_package (<PackageName> ${ARGN})
The first call, in essence, prevents CMake from looking anywhere BUT CONAN_GENERATORS_FOLDER
- and only if it is not found there, would the second call come into play.
We have a secondary issue, which Im not sure it's been reported yet, but our dependency provider will fail for any dependency where the package_info()
in the recipe is set to ONLY generate a find_module.
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.
This isn't just an Android toolchain issue, iOS/tvOS/watchOS has the same issue. Before posting this fix, I undid the CMAKE_FIND_ROOT_PATH_BOTH
setting on #528 and got a similar error as we did on Android:
https:/conan-io/cmake-conan/actions/runs/5581115133/jobs/10198913086
From here, I think we just have to try solutions against the existing test coverage like your suggestion above, keep adding any missing test coverage, and see what solution satisfies them all. I'm happy to help with reviewing and testing.
I do think this merge did get us closer to working for all scenarios so, in my opinion it should stay on develop2
while further improvements are made.
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.
The situation for iOS/tvOS and watchOS is exactly the same as Android - that variable is set to ONLY
and that's what it wouldn't work - https:/Kitware/CMake/blob/master/Modules/Platform/Darwin.cmake#L13-L15
Interestingly enough, when iOS support was added to CMake, the variable CMAKE_FIND_ROOT_PATH_MODE_PACKAGE
was not touched at all - Kitware/CMake@11da882#diff-657a0bc2e0a97ee9c1f85f20e7e4d57dbd40b053642de9c7da5f62dedc81f908
and it was added later to cover for a scenario where the wrong libraries may be picked up: Kitware/CMake@5f5e306
This highlights an interesting limitation of CMake which is hinted at in conan-io/conan#12478: the CMake defaults for iOS/watchOS/tvOS mean that find_package
will never actually locate a package in config mode (with a -config.cmake
) at all - But this also points at the solution, either just prepend the generators folder to CMAKE_FIND_ROOT_PATH
- or invoke find_package twice.
My concern with focusing only on known test coverage is that applying fixes without addressing or understanding the root causes may actually cause breakages in those scenarios that are not currently covered by tests, but are perfectly valid otherwise, like cross-building for a Linux distro when the sysroot contains cmake packages. So if we have enough knowledge to prevent those issues irrespective of the testing level, the better!
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.
Thanks for the additional info!
I said add test coverage not just use existing coverage and I certainly agree on proactively solving problems. I'm even willing to work on it though I'd prefer to finish up the two other pending PRs first, if possible.
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.
Oh, thank you for discovering and bringing attention to these issues in the first place! It's a great motivator to move this tool forward :D
The other two pending PRs are on my priority list this week :)
This addresses #532.
In the process of supporting
find_package()
for cross-compile scenarios, we accidentally excludedFind<PackageName>.cmake
modules from the search. According to CMake docs, only 'module mode' will look for these files, which is only available in the basic command signature offind_package()
. By usingCMAKE_FIND_ROOT_PATH_BOTH
infind_package()
we switched to the full command signature.The fix does set
CMAKE_FIND_ROOT_PATH_MODE_PACKAGE
globally like it did before this pull request comment. I suppose I could save the value of the variable and put it back to avoid a global influence, if you want.I added test coverage for this scenario. While investigating, I realized the Android tests were failing if the build folder wasn't already there, making it impossible to run
pytest -k android
so I fixed that too!