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.

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

Make CGAL detection robust #13668

Open
luca-heltai opened this issue May 4, 2022 · 6 comments
Open

Make CGAL detection robust #13668

luca-heltai opened this issue May 4, 2022 · 6 comments

Comments

@luca-heltai
Copy link
Member

Currently we support CGAL versions > 5.0. From 5.0 onwards, cgal is a header only library. While this should simplify its detection and installation, there are issues in the ways we currently process our external libraries that do not make it easy to extract information about the linking interface of the external libraries.

In pricinciple, for user codes, we use

find_package(CGAL 5.0)
target_link_libraries(${target}, CGAL::CGAL)

and this works fine.

However, different versions of CGAL behave differently with their CGAL::CGAL target, and in the library we do not use the same mechanism, i.e., we do not (yet?) allow something like (notice the OPTIONAL CGAL::CGAL line)

DEAL_II_PACKAGE_HANDLE(CGAL
  INCLUDE_DIRS
    REQUIRED CGAL_INCLUDE_DIRS
  LIBRARIES
    OPTIONAL CGAL::CGAL
  USER_INCLUDE_DIRS
    REQUIRED CGAL_INCLUDE_DIRS
  CLEAR 
    CGAL_INCLUDE_DIRS
    CGAL_LIBS
  )

since CGAL::CGAL does not resolve to a fully specified collection of library paths, but it recursevly includes targets like Boost::Boost. This currently breaks our build system.

I'm not sure if there is a clean way around this, especially since CGAL 4.0, 5.0, and CGAL 5.4 each have a different behaviour.

I'm not proficient enough with cmake to make this work robustly. Maybe @tamiko can help?

@luca-heltai
Copy link
Member Author

FYI @fdrmrc.

@drwells
Copy link
Member

drwells commented May 4, 2022

Another CGAL problem we have is that it may depend on GMP and MPFR but we don't seem to propagate those dependencies along (I get missing symbol errors since we don't link against GMP). As a result, master currently doesn't work for me unless I disable CGAL.

@luca-heltai
Copy link
Member Author

luca-heltai commented May 4, 2022

This is fixed in #13669. If CGAL uses gmp and mpfr, then its CGAL_LIBRARIES field contains them.

@tamiko
Copy link
Member

tamiko commented May 4, 2022

@luca-heltai I think the time has come to rewrite our external feature setup: I am thinking of something on the line of the following:

  • augment the DEAL_II_PACKAGE_HANDLE macro to recursively resolve targets in the link interface
  • add a new keyword to supply targets directly
  • and switch our build system to create import targets for external dependencies.
  • properly export the link interface for deal.II via fully populated import targets.

This should work by now™ because all major bugs we encountered in the past (3-5 years ago) seem to have been resolved.

This will require a bit of time though - I'll look forward to fixing this in the next two weeks.

@luca-heltai luca-heltai added this to Done in CGAL Support May 12, 2022
@tamiko
Copy link
Member

tamiko commented May 30, 2022

I will postpone this fix to 9.5. This is a major overhaul of the build system that I think should get in after the 9.4 release.

@tamiko tamiko modified the milestones: Release 9.4, Release 9.5 May 30, 2022
@tamiko
Copy link
Member

tamiko commented Jun 20, 2023

@luca-heltai I am postponing this to the 9.6 milestone - but changing this should be very straightforward. I think I am a bit afraid of last minute changes that break something.

@tamiko tamiko modified the milestones: Release 9.5, Release 9.6 Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

3 participants