-
Couldn't load subscription status.
- Fork 1.1k
Add cmake_extra_file_names property for CMakeConfigDeps
#18821
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
base: develop2
Are you sure you want to change the base?
Conversation
test/functional/toolchains/cmake/cmakedeps2/test_cmakeconfigdeps_new.py
Outdated
Show resolved
Hide resolved
test/functional/toolchains/cmake/cmakedeps2/test_cmakeconfigdeps_new.py
Outdated
Show resolved
Hide resolved
cmake_extra_file_names property for CMakeConfigDepscmake_extra_file_names property for CMakeConfigDeps
… into ar/cmake-extra-file-names
| for f in extra_file_names: | ||
| filename = f"{f}-config.cmake" if f == f.lower() else f"{f}Config.cmake" | ||
| ret[filename] = f'include(CMakeFindDependencyMacro)\n' \ | ||
| f"find_dependency({old})" |
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.
Are we sure the find_dependency() approach is the best one?
Why not:
- Use a plain
include(config.filename)? - Why not duplicating the contents of
config.content()in each file?
I am a bit concerned the extra find_dependency() might have corner cases, it might kind of change the actual dependency graph of cmake files?
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.
cc @jcar87 who suggested this approach
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 theres' a few conflicting use cases.
Currently we have a gap between foobar-config.cmake and the ability to locate it via find_package().
The CMake documentation for find_package states that:
In this mode, CMake searches for a file called <lowercasePackageName>-config.cmake or <PackageName>Config.cmake.
That is, if the package is called FooBar, FOOBAR, FooBAR FOOBar, CMake will find it correctly via foobar-config.cmake`, example:
find_package(FooBar)
find_package(FOOBAR)
find_package(FooBAR)
find_package(FOOBar)
all of the above are all satisfied by foobar-config.cmake, and CMake sets <name-you-searched-for>_FOUND) to `True.
However, that is does NOT currently work with CMakeConfigDeps, as we don't place the file in the search path, but define <name>_DIR instead.
The advantage of this approach is that we avoid the search altogether, so we minimise the risk of finding the wrong config file (e.g. when crossbuilding in some instances), the disadvantage is that we lose the ability to use names with different upper/lower case letters than the generated filename.
An example of this is libxml2-config.cmake (generated by the upstream project) which happily works with find_package(LibXml2) (using the name of the legacy built-in module), but does not work with CMakeConfigDeps (yet).
So a possible way to support this, would be for cmake_extra_file_names to ensure that every "alternative" name has a xxx_DIR entry in conan_cmakedeps_paths.cmake.
That would eliminate the need of generating additional files, on the condition that the file generated is all lowercase (<lowercaseName>-config.cmake) and the extra filenames are all variations that match the name but with alternative upper/lower case combinations. I would personally consider always generating the filenames with lower case but we do need to know the intended package name.
if we want to support generating files with different names altogether, that is, GTest and GoogleTest or similar - then yes, we would need to either:
- generate multiple files (with full contents)
or - have files like proposed in this PR that call find_package on the actual filename
I think in the past there were some cases in Conan Center where generating alternative names would have been convenient - however I'm not 100% sure this is justified unless it is to mirror what upstream does (and it is very unlikely that upstream generates the same package with different names) - so I wouldn't consider this unless we had very well motivated use cases. Having 2 different names for the same package is what caused most of the corner case headaches in CMakeDeps.
|
I need this feature for Tesseract package when building for Android: conan-io/conan-center-index#28367 The Teseeract project consumes the Cmake file It regularly generates CpuFeaturesNdkCompatConfig.cmake, but when building for Android, it generates CpuFeaturesNdkCompatConfig too. So far, I can't generate two separated config files in package_info. It would be nice having it available for |
Changelog: Feature: Add
cmake_extra_file_namesproperty forCMakeConfigDeps.Docs: TODO
This comes from a discussion from last week with the team that could solve some shortcomings in a few recipes in CCI