CMake options without prefix #302
Comments
|
**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``, ``json``and ``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.
|
Should I branch from the master or the develop? |
|
Some problems originate from the |
|
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.
|
|
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... |
|
The PR for |
This sounds good, thanks for keeping at it! |
|
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 |
|
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)? |
Description
The
mpi,openmp,json,enable-file-deprecated,enable_p6est,enable_p8est,vtk_binaryandzlibCMake options can easily collide with other projects. I think that it is a significant problem especially for thempi,openmp,jsonandzlib, which should be renamed to something likeP4EST_ENABLE_MPI,P4EST_ENABLE_OPENMP,P4EST_ENABLE_JSONandP4EST_ENABLE_ZLIBrespectively. 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
The text was updated successfully, but these errors were encountered: