-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[cgal] compatibility with Conan 2.0, and cgal/5.5.2 #17858
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
That is for compatibility with Conan 1.x.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit cdb7052cgal/5.3.2
|
This comment has been minimized.
This comment has been minimized.
... to backport part of CGAL CMake logic for CGAL::CGAL
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit f12596dcgal/5.3.2
cgal/5.5.1
cgal/5.5
cgal/5.5.2
|
The error is on MacOS, with Apple-clang 13 and C++17:
Does anybody know what I can do? Maybe @uilianries, @prince-chrismc? You helped me with my previous PRs #12249 and #13443. |
@lrineau I started an internal CI job to build |
This comment has been minimized.
This comment has been minimized.
@uilianries Houra! The last CI set of tests eventually succeeded! Thanks for your help. |
os.path.join(self.package_folder, self._cmake_module_file_rel_path) | ||
) | ||
|
||
def _create_cmake_module_variables(self, module_file): |
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.
Why do you need to create it only now? What error are you trying to fix?
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 fixing a lot of issues, at least for CMake consumers. See #10011 for example.
CGAL requires C++14, and specific compilers flags to enable the possibility to set FPU rounding modes. That CMake function, from CGAL pull-request CGAL/cgal#7512, takes care of all the known compilers CGAL has ever supported.
Everything is probably doable adding compilers flags using cpp_info
, except for target_compile_features(${target} INTERFACE cxx_decltype_auto)
for which Conan has currently no equivalent.
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 would be great adding this detailed in description as a comment in the code. Usually, we receive this kind of change as a patch, over the current version, but I see applying it to each past version would require another patch too. Another option is you adding it as a cmake file side-by-side with the conanfile.py, and exporting it.
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. Done in 010075b.
generators = "CMakeDeps", "CMakeToolchain" | ||
|
||
def requirements(self): | ||
self.requires("eigen/3.4.0") |
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.
why do you need to include Eigen too? Is there a public header or something missing?
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.
Some parts of CGAL require Eigen3, but not CGAL core parts.
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.
Is it something new? We didn't have it before and cgal was working. If it's a new dependency, it would require a if Version(self.version) < "5.5.2"
for instance.
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 should be solved as cgal dependency directly then, and could be added as dependency based on an option.
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.
CGAL is a header-only library, with dependencies and optional dependencies. That is not new. What is new is that I have upgraded the test_package
with a less trivial test.
During the build of the CGAL Conan package, the dependencies will not change anything in the produced package (that is header-only). I do not think Conan package options are a good way to deal with that. There are only three dependencies: boost, gmp, and mpfr. But currently 18 optional dependencies. That is not new.
If a user has a program with several compilation units using CGAL (like the CGAL 3D demo, with all its plugins) but only some of them using optional dependencies, then the use of Conan option would pull the dependencies for all the compilation units, and not only for the one really needing them.
Co-authored-by: Uilian Ries <[email protected]>
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit e968c3bcgal/5.5
cgal/5.3.2
cgal/5.5.1
cgal/5.5.2
|
That will avoid a warning: ``` post_package_info(): WARN: [CMAKE FILE NOT IN BUILD FOLDERS (KB-H019)] The *.cmake files have to be placed in a folder declared as `cpp_info.builddirs`. Currently folders declared: {'/home/conan/w/prod/BuildSingleReference/.conan/data/cgal/5.5/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/lib/cmake/CGAL', '/home/conan/w/prod/BuildSingleReference/.conan/data/cgal/5.5/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/'} post_package_info(): WARN: [CMAKE FILE NOT IN BUILD FOLDERS (KB-H019)] Found files: ./lib/cmake/conan-official-cgal-variables.cmake ```
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.
LGTM
Conan v1 pipeline ✔️All green in build 17 (
Conan v2 pipeline ✔️
All green in build 17 ( |
@uilianries, @prince-chrismc: is there anything more I should do with this pull-request? Or just wait? I am sorry for my questions: I am not completely familiar with the Conan community and its practices. |
* [cgal] compatibility with Conan 2.0, and cgal/5.5.2 * cleanup for the linter * Fix linter errors for Conan 1.0 * Use patch directly to avoid `strip` in conandata That is for compatibility with Conan 1.x. * remove version before 5.3 (header only) * backport the patch * remove CGAL CMake Config files * Fix the licenses/ directory * fix linter error * reuse patches with versions, for Conan 1.x * fix a final linter warning * handle that CGAL requires C++14 * CGAL is a header-only library * add back a CMake build module ... to backport part of CGAL CMake logic for CGAL::CGAL * Update to Boost 1.82 Co-authored-by: Uilian Ries <[email protected]> * place the Conan specific CMake module in lib/cmake/CGAL That will avoid a warning: ``` post_package_info(): WARN: [CMAKE FILE NOT IN BUILD FOLDERS (KB-H019)] The *.cmake files have to be placed in a folder declared as `cpp_info.builddirs`. Currently folders declared: {'/home/conan/w/prod/BuildSingleReference/.conan/data/cgal/5.5/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/lib/cmake/CGAL', '/home/conan/w/prod/BuildSingleReference/.conan/data/cgal/5.5/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/'} post_package_info(): WARN: [CMAKE FILE NOT IN BUILD FOLDERS (KB-H019)] Found files: ./lib/cmake/conan-official-cgal-variables.cmake ``` * Add a comment about CGAL_setup_CGAL_flags --------- Co-authored-by: Uilian Ries <[email protected]>
Specify library name and version: cgal/5.5.2
This pull-request adds cgal/5.5.2, but its major contribution is the compatibiity with Conan 2.0.
Fixes #10011.