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.

Sign up for GitHub

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

Jump to bottom

Ensure that Find<PackageName>.cmake modules work #536

Merged
merged 3 commits into from
Jul 18, 2023
Merged

Ensure that Find<PackageName>.cmake modules work #536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6a258d
Ensure find_package() supports Find<PackageName>.cmake
ssrobins Jul 17, 2023
4a4794a
Only try to delete the build directory if it exists
ssrobins Jul 17, 2023
7a76da3
Add test coverage
ssrobins Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion conan_provider.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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.

Copy link
Contributor

@jcar87 jcar87 Jul 18, 2023 •
edited
Loading

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.

Copy link
Contributor Author

@ssrobins ssrobins Jul 18, 2023 •
edited
Loading

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.

Copy link
Contributor

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!

Copy link
Contributor Author

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.

Copy link
Contributor

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 :)

find_package(${ARGN} BYPASS_PROVIDER)
endmacro()


Expand Down
5 changes: 5 additions & 0 deletions tests/resources/cmake_module/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
cmake_minimum_required(VERSION 3.24)
project(MyApp CXX)

set(CMAKE_CXX_STANDARD 17)
find_package(Threads REQUIRED)
17 changes: 16 additions & 1 deletion tests/test_smoke.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,20 @@ def test_reconfigure_on_conanfile_changes(self, capfd, chdir_build):
assert all(expected in out for expected in expected_conan_install_outputs)


class TestCmakeModule:
@pytest.fixture(scope="class", autouse=True)
def cmake_module_setup(self):
src_dir = Path(__file__).parent.parent
shutil.copytree(src_dir / 'tests' / 'resources' / 'cmake_module', ".", dirs_exist_ok=True)
yield

def test_cmake_module(self, capfd, chdir_build):
"Ensure that the Find<PackageName>.cmake modules from the CMake install work"
run("cmake .. -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES=conan_provider.cmake -DCMAKE_BUILD_TYPE=Release")
out, _ = capfd.readouterr()
assert "Found Threads: TRUE" in out


class TestSubdir:
@pytest.fixture(scope="class", autouse=True)
def subdir_setup(self):
Expand Down Expand Up @@ -177,7 +191,8 @@ def test_no_os_version(self, capfd, chdir_build):
class TestAndroid:
@pytest.fixture(scope="class", autouse=True)
def android_setup(self):
shutil.rmtree("build")
if os.path.exists("build"):
shutil.rmtree("build")
yield

def test_android_armv8(self, capfd, chdir_build):
Expand Down