-
Notifications
You must be signed in to change notification settings - Fork 993
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
CMakeDeps/CMakeToolchain: Several improvements for open issues #9455
CMakeDeps/CMakeToolchain: Several improvements for open issues #9455
Conversation
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'd say that overall, it looks good, lets talk about the comments.
Breaking them into smaller pieces would be nice, yes, some things like the cmake_value
template function is fine and can be merged already, but not necessary, the PR is not that long, so as you prefer.
So far I think this kind of flexibility will be needed if we want to support all the scenarios out there and adapt to the existing [nightmare] ecosystem. Probably we need a decision about the use-cases that we won't cover and how to workaround them: build-modules, patches to consumer sources, duplicated files for Meson,... that can help us to understand the full picture.
There can be different files that one consumer will always think it is the best one:
Depending on how intrusive Conan wants to be, we will need more or fewer config parameters in the recipe to fit each scenario.
I don't like to have too many config parameters... but sometimes the same library mixes several of the previous approaches. You need some files provided by the library and for some others Conan generated ones are better. In this scenario you can remove selected ones in the recipe: for package in ("Fontconfig", "Freetype", "GDAL", "GIFLIB", "GTA", "Jasper", "OpenEXR"):
# Prefer conan's find package scripts over osg's
os.unlink(os.path.join(self._source_subfolder, "CMakeModules", "Find{}.cmake".format(package)))
Life is hard 😢 This weekend I've been fighting with DART, I think it is a library with good build-system files (CMake) but with subtle details that make it a bit complicated: library-provided module files that use pkg_config to find dependencies, problems with multi-config generators, casing and names conflicts,... It would be very challenging to provide something that just is a drop-in replacement. |
@SpaceIm @prince-chrismc @madebr WDYT? will it make your life easier/harder/same? any comments are welcomed. |
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 template looks really really complete! 🚀 Great work
WDYT? will it make your life easier/harder/same? any comments are welcomed.
Gut reaction, the same, but it's hard to say only reviewing the code.... Some of the property names are a little ambiguous for me.
Some questions that might be outside of the diff.
- Could config be the default? that's what the majority of projects install (or they are supposed to)
- Maybe an example of setting the package name, namespace and a target?
- How about namespaces per target?
I also have a few questions about the code
|
||
ret = req.new_cpp_info.get_property("cmake_target_name", "CMakeDeps") | ||
if not ret: | ||
ret = req.cpp_info.get_name("cmake_find_package_multi", default_name=False) |
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.
No fallback to "cmake_find_package"
for 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.
I've been greping conan-center-index
and the name is always set for both of them:
self.cpp_info.names["cmake_find_package"] = "Crc32c"
self.cpp_info.names["cmake_find_package_multi"] = "Crc32c"
So the fallback looks enough. We introduce this fallback mostly for Conan center.
FIND_MODE_MODULE = "module" | ||
FIND_MODE_CONFIG = "config" | ||
FIND_MODE_NONE = "none" | ||
FIND_MODE_BOTH = "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.
I am curious why a "both"
option?
AFAIK official CMake are modules (And there are a few projects the provide their own) and most project install targets + follow https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html which are your config packages
but I've not come across a project that does both 🤔 with the old generators it was the costume who decided what to install but now it's defined by the recipe... we'll need to be more careful to define it correctly
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.
both
cover the case where the library has a module in cmake, typical FindXXX.cmake
but also the author's provided config (irrespective if we package it or not). In that case, you don't know how the consumer will expect to consume the package, maybe find_package(XXX MODULE)
maybe find_package(xxx CONFIG)
but we want Conan to be as "ready" as possible to consume any conan center package. So CMakeDeps
will generate both. Note that the config
namespace and target name should be declared typically following the author's one.
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.
That makes sense, I understand the motivation but I am not aware of any examples.
There are a few project out there that provide their own module files even though the upstream project provides config files. I wonder if this bonus feature will help there 🤔
self.cpp_info.set_property("cmake_target_name", "MyDepTarget") | ||
|
||
self.cpp_info.set_property("cmake_module_file_name", "mi_dependencia") | ||
self.cpp_info.set_property("cmake_module_target_name", "mi_dependencia_target") |
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.
What about modules without targets? some of the older only use variables... how can we provide those?
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 issue with these modules without targets is, Which variables should we declare? probably nothing to be automated. This might be another reason to enable a property to introduce custom cmake code that would be declared when consuming the module/config but we didn't agree yet about that.
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.
Good to me, can be merged and continue working and improving over it (like #9511)
Changelog: Feature: CMakeToolchain new member
find_builddirs
defaulted toTrue
to add thecpp_info.builddirs
from the requirements to theCMAKE_PREFIX_PATH/CMAKE_MODULE_PATH
. That would allow finding the config files packaged and to be able toinclude()
them from the consumerCMakeLists.txt
.Changelog: Bugfix: CMakeToolchain. Fixed a bugfix whereby a variable declared at the
.variables
containing a boolean ended at CMake with a quoted"True"
or"False"
values, instead ofON
/OFF
Changelog: Feature: CMakeDeps. Added a new property
cmake_find_mode
with possible values toconfig
(default),module
,both
ornone
to control the files to be generated from a package itself. Thenone
replaces the currentskip_deps_file
property.Changelog: Feature: CMakeDeps: Added two new properties
cmake_module_file_name
andcmake_module_target_name
, analog tocmake_file_name
andcmake_target_name
, but to configure the name ofFindXXX.cmake
file and the target declared inside.Docs: conan-io/docs#2209