Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[cgal] compatibility with Conan 2.0, and cgal/5.5.2 #17858
Changes from all commits
0b7139f
e72d442
b7130d2
dec18e4
cdb3775
4dec274
4f81ad1
06ee378
0d3310e
113539c
cdb7052
8bb4592
583e0e7
f12596d
2d94480
fc183e7
e968c3b
fa86468
010075b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 fortarget_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.
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.