cburstedde / p4est

The "p4est" forest-of-octrees library
www.p4est.org/
GNU General Public License v2.0
253 stars 114 forks source link

3d examples are not built by cmake #206

Open pkestene opened 1 year ago

pkestene commented 1 year ago

Description

Currently in example/CMakeLists.txt, 3d examples are only built when target P4EST::P8EST is defined. The problem is that this target is never defined in p4est, because p8est sources are compiled as an object library, and then directly added to p4est target sources. So currently, 3d example can't be built by cmake.

This commit 523e73668abbae5a428824c8fc8a0478942c1994 removed target P4EST::P8EST

Proposed solution

As enabling / disabling p8est / p6est are features that can be enabled/disabled at top-level via cmake options, I think we should also export this information in cmake/config.cmake.in so that e.g. variable P4EST_ENABLE_P8EST is defined (true or false) when a downstream cmake project (like example pcmake project) is doing find_package(P4EST CONFIG)and use that variable to decide if 3d examples can be built.

cburstedde commented 1 year ago

With autoconf, we have --enable-2d and --enable-3d. These are default on but can be switched independently. Shall we add the analogous options to CMake?

pkestene commented 1 year ago

currently, as I understand, 2d is always built, 3d and p6est can be enabled/disabled on the cmake configure command line via option enable_p6est and enable_p8est which are also default ON. I thinks it is ok.

Another point is that, now a project that use p4est (via cmake find_package) is aware if p4est was or wasn't built with p8est enabled, because P4EST_ENABLE_P8EST is always defined.

Additionnally, in PR #207 I modified also the generated pkgconfig, so that users that rely on pkgconfig to find p4est can also easily have access to the information, e.g.

pkg-config --variable p4est_enable_p8est p4est

currently this variable is a boolean, that can be either ON/OFF, but it could be 0/1; what do you think ? The pkgconfig file generated by autotools should also defined those variables, I think.

cburstedde commented 1 year ago

This sounds nice. Would you be able to put this logic into the pkgconfig file, independent of the build system used? The ultimate goal would be a p4est built with CMake to use a libsc built by autotools and vice versa.

pkestene commented 1 year ago

This is currently possible, but a few minor adjustments need to be made for enabling that in a robust way.

pkestene commented 1 year ago

Just to let you know, I checked if it was possible e.g. to build p4est with cmake, using libsc build with autotools, already installed.

One problem emerged: if libsc is built with mpi enabled, but the use tries to build p4est with mpi OFF, currently the build fails, because the mpi.h header is not found. The reason is that, p4est top-level CMakeList.txt only activate MPI ipon upon option "mpi", but it should also activate MPI if the libsc found in environment was itself built with MPI activated.

I'll make a PR about that in libsc and p4est soon.

Finally, this illustrates the need for libsc to expose its build features in the pkg-config file, as well as in SCConfig.cmake file

cburstedde commented 1 year ago

One problem emerged: if libsc is built with mpi enabled, but the use tries to build p4est with mpi OFF, currently the build fails, because the mpi.h header is not found. The reason is that, p4est top-level CMakeList.txt only activate MPI ipon upon option "mpi", but it should also activate MPI if the libsc found in environment was itself built with MPI activated.

I'll make a PR about that in libsc and p4est soon.

We check that the _ENABLE_MPI and _ENABLE_DEBUG options are the same. I think it is ok to require this, it just needs to be well documented.

pkestene commented 1 year ago

What I meant, is that:

cburstedde commented 1 year ago

I like the proposition to put the MPI and DEBUG status into pkgconfig (both build systems) and SCConfig.cmake.

I would prefer that no options are auto-enabled. If there is a mismatch, we can print a helpful message and suggest to the user how to reconfigure their build.

cburstedde commented 1 year ago

currently, as I understand, 2d is always built, 3d and p6est can be enabled/disabled on the cmake configure command line via option enable_p6est and enable_p8est which are also default ON. I thinks it is ok.

It is possible to build only 3d with autoconf. It's a nice experiment for consistency. The --enable-p6est switch (default true) in p4est only builds p6est if both 2d and 3d are enabled. So should the pkgconfig variable for p6est reflect this logic and be true only if all three switches are true? With CMake, 2d can be hardwired to true since there is no switch for it.

Another point is that, now a project that use p4est (via cmake find_package) is aware if p4est was or wasn't built with p8est enabled, because P4EST_ENABLE_P8EST is always defined.

Additionnally, in PR #207 I modified also the generated pkgconfig, so that users that rely on pkgconfig to find p4est can also easily have access to the information, e.g.

pkg-config --variable p4est_enable_p8est p4est

currently this variable is a boolean, that can be either ON/OFF, but it could be 0/1; what do you think ? The pkgconfig file generated by autotools should also defined those variables, I think.

I'd use whatever is the standard form of a boolean in pkgconfig. The values from autoconf are yes/no, is that compatible? Otherwise, we may need to translate to true/false or 0/1.

Shall we thus add p4est_enable_p4est=yes in CMake, and with autoconf p4est, p8est as appropriate? With autoconf, p6est is automatically chosen as the logical "and" of enable-2d, enable-3d, and enable-p6est, and with CMake it becomes equal to enable_p8est. This way the pkgfiles converge even more between the build systems.

cburstedde commented 1 year ago

Hi may I ask on whether we are with this issue? I know we had quite a few merges towards this.

cburstedde commented 2 months ago

Let me ask @mkirilin who has been looking into the example build rules of CMake. Is there anything that remains to be done, or shall we close this issue?

mkirilin commented 2 months ago

At the current state of the develop branch 8dfa831, with cmake, all the 2d examples always build by default and 3d examples build depending on enable_p8est flag which is ON by default as well. I think the issue might be closed.

cburstedde commented 2 months ago

At the current state of the develop branch 8dfa831, with cmake, all the 2d examples always build by default and 3d examples build depending on enable_p8est flag which is ON by default as well. I think the issue might be closed.

Thanks for adding this update. Shall we look at the pkgconfig files and verify that the contents are what we want for both autoconf and cmake?