cgal CGAL: Modernize CMakeLists.txt

Summary of Changes

Changed PUBLIC to PRIVATE in the target_link_libraries()
~~Link against Boost::<COMPONENT>~~ as we do more than just linking and must use the CGAL::_.._support
Use target_compile_definitions() and add_compile_definitions()

More to be done.

Release Management

Affected package(s): all
Issue(s) solved (if any): fix one item of #4815
License and copyright ownership: unchanged

Oct 08 '24 16:10 afabri

Why do we have this cmake if-then-else here? Is it for backward compatibility for older versions of cmake? @lrineau

Oct 09 '24 07:10 afabri

Why do we have this cmake if-then-else here? Is it for backward compatibility for older versions of cmake? @lrineau

Yes, I think so. Maybe it can be removed.

Oct 09 '24 07:10 lrineau

Why do we have this cmake if-then-else here? Is it for backward compatibility for older versions of cmake? @lrineau

Yes, I think so. Maybe it can be removed.

I guess we have a test platform where we use the "minimal required" version of cmake?

Also can we then remove the line add_definitions("-DCGAL_USE_BOOST_PROGRAM_OPTIONS") ??

Oct 09 '24 07:10 afabri

Also can we then remove the line add_definitions("-DCGAL_USE_BOOST_PROGRAM_OPTIONS") ??

It is needed. Does that need "modernization" @lrineau ?

Oct 09 '24 08:10 afabri

Why do we have this cmake if-then-else here? Is it for backward compatibility for older versions of cmake? @lrineau

Yes, I think so. Maybe it can be removed.

I guess we have a test platform where we use the "minimal required" version of cmake?

My guess is that it will depend how boost is found (config mode or with FindBoost.cmake).

Also can we then remove the line add_definitions("-DCGAL_USE_BOOST_PROGRAM_OPTIONS") ??

IIRC, this is to disable something at runtime in the example. So if you remove it you need to update the example and the cmake script to not compile it.

Oct 09 '24 08:10 sloriot

CGAL_USE_BOOST_PROGRAM_OPTIONS

No. That preprocessor macro is used in Ridges_3 examples.

Oct 09 '24 08:10 lrineau

Also can we then remove the line add_definitions("-DCGAL_USE_BOOST_PROGRAM_OPTIONS") ??

It is needed. Does that need "modernization" @lrineau ?

https://github.com/CGAL/cgal/blob/4305f01384659ecc200db2c41f41aa48d43e92d2/Ridges_3/examples/Ridges_3/CMakeLists.txt#L21-L30

It swaps three lines of target_compile_definitions against one line add_definitions that is for the three programs after. I think it is all right. But, as for all the other cases, that is a matter of taste. If the other way around looks better for you, change the code.

Oct 09 '24 08:10 lrineau

TBB seems also to use keywords like emit(), as can be seen in the CI

/usr/include/oneapi/tbb/profiling.h:229:15: error: expected unqualified-id before ‘)’ token
  229 |     void emit() { }

Maybe we can tell moc not to parse TBB headers. Would even accelerate compilation. Or we just keep QT_NO_KEYWORDS

Oct 09 '24 09:10 afabri

TBB seems also to use keywords like emit(), as can be seen in the CI
/usr/include/oneapi/tbb/profiling.h:229:15: error: expected unqualified-id before ‘)’ token
  229 |     void emit() { }

Bless the CI!

And I thanks myself for that initiative (started by #4234).

Oct 09 '24 09:10 lrineau

There is a disussion on github/TBB. There I even saw @mglisse. My conclusion is that we just keep QT_NO_KEYWORDS

Oct 09 '24 10:10 afabri

Why do we have this cmake if-then-else here? Is it for backward compatibility for older versions of cmake? @lrineau

Yes, I think so. Maybe it can be removed.

I guess we have a test platform where we use the "minimal required" version of cmake?

My guess is that it will depend how boost is found (config mode or with FindBoost.cmake).

Also can we then remove the line add_definitions("-DCGAL_USE_BOOST_PROGRAM_OPTIONS") ??

IIRC, this is to disable something at runtime in the example. So if you remove it you need to update the example and the cmake script to not compile it.

Starting cmake 3.5, Boost::<COMPONENT> are also defined by FindBoost.cmake so we can get rid of the else case.

Oct 09 '24 11:10 sloriot

@lrineau what is CGAL::Data in target_link_libraries() about ?

Oct 10 '24 05:10 afabri

@lrineau what is CGAL::Data in target_link_libraries() about ?

it is used to set the path to the data directory

Oct 10 '24 08:10 sloriot

Concering VTK it seems that we still have to link against ${VTK_LIBRARIES}

Oct 10 '24 08:10 afabri

@lrineau what is CGAL::Data in target_link_libraries() about ?

That is the CMake support for the function CGAL::data_file_path. As for CGAL-6.0, it only adds a compile definition -DCGAL_DATA_DIR=</path/to/Data/data>.

Oct 10 '24 09:10 lrineau

/build:doc

Oct 16 '24 22:10 lrineau

The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/8528/doc/Manual/index.html

Oct 16 '24 22:10 github-actions[bot]

Successfully tested in CGAL-6.1-Ic-8 (CGAL-6.1-Ic-7 for Rubens)

Oct 31 '24 09:10 sloriot