CGAL: Modernize CMakeLists.txt

[afabri]

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 Finish the modernization of CGAL CMake scripts #4815
• License and copyright ownership: unchanged

[afabri]

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

[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.

[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?

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

[afabri]

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

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

[sloriot]

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.

[lrineau]

CGAL_USE_BOOST_PROGRAM_OPTIONS

No. That preprocessor macro is used in Ridges_3 examples.

[lrineau]

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

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

cgal/Ridges_3/examples/Ridges_3/CMakeLists.txt

Lines 21 to 30 in 4305f01

	add_definitions("-DCGAL_USE_BOOST_PROGRAM_OPTIONS")
	if(TARGET Boost::program_options)
	target_link_libraries(Compute_Ridges_Umbilics PRIVATE Boost::program_options)
	target_link_libraries(Ridges_Umbilics_SM PRIVATE Boost::program_options)
	target_link_libraries(Ridges_Umbilics_LCC PRIVATE Boost::program_options)
	else()
	target_link_libraries(Compute_Ridges_Umbilics PRIVATE ${Boost_PROGRAM_OPTIONS_LIBRARY})
	target_link_libraries(Ridges_Umbilics_SM PRIVATE ${Boost_PROGRAM_OPTIONS_LIBRARY})
	target_link_libraries(Ridges_Umbilics_LCC PRIVATE ${Boost_PROGRAM_OPTIONS_LIBRARY})
	endif()

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.

[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() { }

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

[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() { }

Bless the CI!

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

[afabri]

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

[sloriot]

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.

[afabri]

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

[sloriot]

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

it is used to set the path to the data directory

[afabri]

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

[lrineau]

@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>.

[lrineau]

/build:doc

[github-actions]

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