search

cburstedde / p4est

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

CMake options without prefix #302

Open dutkalex opened 7 months ago

dutkalex commented 7 months ago

Description The mpi, openmp, json, enable-file-deprecated, enable_p6est, enable_p8est, vtk_binary and zlib CMake options can easily collide with other projects. I think that it is a significant problem especially for the mpi, openmp, jsonand zlib, which should be renamed to something like P4EST_ENABLE_MPI, P4EST_ENABLE_OPENMP, P4EST_ENABLE_JSON and P4EST_ENABLE_ZLIB respectively. It would also disambiguate the exact behavior associated to each option. This is a very low-hanging fruit in terms of hardening of the CMake build system, and improving the interoperability with external projects. I can easily propose a small PR to fix this.

To Reproduce

mkdir build
cd build
ccmake ..
cburstedde commented 7 months ago

Description The mpi, openmp, json, enable-file-deprecated, enable_p6est, enable_p8est, vtk_binary and zlib CMake options can easily collide with other projects. I think that it is a significant problem especially for the mpi, openmp, jsonand zlib, which should be renamed to something like P4EST_ENABLE_MPI, P4EST_ENABLE_OPENMP, P4EST_ENABLE_JSON and P4EST_ENABLE_ZLIB respectively. It would also disambiguate the exact behavior associated to each option.

To Reproduce

 mkdir build; cd build
ccmake ..

This is a very low-hanging fruit in terms of hardening of the CMake build system. I can easily propose a small PR to fix this.

Thanks; we've recently stumbled upon this phenomenon of having two variables for the same setting. These should be unified to the variable names used in autoconf and as #define, that is, go for the P4EST_ uppercase versions.

While at it, we may symmetrize cmake to add P4EST_ENABLE_BUILD_2D on top of P4EST_ENABLE_BUILD_3D and P4EST_ENABLE_P6EST. Please go ahead if you like.

dutkalex commented 7 months ago

Please go ahead if you like.

Should I branch from the master or the develop?

dutkalex commented 7 months ago

Some problems originate from the sc library, so fixing this will require a PR on the sc repo too.

cburstedde commented 7 months ago

Some problems originate from the sc library, so fixing this will require a PR on the sc repo too.

Yes, both libraries would need similar treatment. If it would be possible to reuse some CMake code between them, that would be great. It would be good if the Presets structure remains functional, since third parties rely on them for their builds, especially about setting debug and mpi modes.

Please base of develop, and thanks! It may be that we address sc first and then I'd update the submodule in p4est, before the PR can be posted there -- but it may also be that the two PRs interdepend, please comment on this.

dutkalex commented 7 months ago

Yes I agree. I will start investigating the question. It is not obvious to me at the time whether the two can be decoupled or not...

dutkalex commented 7 months ago

The PR for libsc is ready. Waiting on your feedback for integration. Once this is merged in the develop, I'll be able to see what needs to be adapted on the p4est side...

cburstedde commented 6 months ago

The PR for libsc is ready. Waiting on your feedback for integration. Once this is merged in the develop, I'll be able to see what needs to be adapted on the p4est side...

This sounds good, thanks for keeping at it!

cburstedde commented 5 months ago

Once we get the libsc CMake/CI to go through, I'll update the sc submodule in p4est. Then we may proceed along the same lines. We can use the CMake variables P4EST_ENABLE_* throughout and leave the one -Dmpi shortcut as with libsc.

cburstedde commented 4 months ago

The sc updates are merged. Would you see some room for yourself to align the p4est behavior to sc (assuming the sc behavior is to your liking/consistent, please let us know)?