search

cburstedde / libsc

The "sc" auxiliary library
www.p4est.org/
GNU Lesser General Public License v2.1
36 stars 34 forks source link

Fix CMake variables #178

Closed dutkalex closed 5 months ago

cburstedde commented 6 months ago

Thanks! Unifying the variables is great. Would you mind adding a file doc/author_.txt like the others, releasing your code under the FreeBSD license? And if you could add a line to doc/release_notes.txt?

(Wondering why all of a sudden the autotools ./bootstrap script dies on Darwin (unrelated).)

Any comments @scivision -- thinking about callability from other software besides p4est.

dutkalex commented 6 months ago

Done! @cburstedde

tim-griesbach commented 6 months ago

Thanks for the simplifications!

We tried out the changes and setting SC_USE_INTERNAL_ZLIB in ccmake to ON leads to the following configure error:

 CMake Error at cmake/zlib.cmake:60 (add_library):
   add_library cannot create imported target "ZLIB::ZLIB" because another
   target with the same name already exists.
 Call Stack (most recent call first):
   cmake/config.cmake:36 (include)
   CMakeLists.txt:15 (include)

cc @hannesbrandt

dutkalex commented 6 months ago

@tim-griesbach @hannesbrandt Thanks for letting me know. This should work better now :)

cburstedde commented 6 months ago

Done! @cburstedde

Thanks for the initiative! Indeed it is better to have one variable for one feature, not two. On the other hand, we lose the simplicity of -Dmpi=ON that we were enjoying previously. I am really liking it with autoconf, where the configure option is named --enable-mpi, which is inherited to subpacakges, and the variable set is _ENABLE_MPI, which is clean.

In the long run, it seems that namespace cleanliness would win, so we will go for the SCENABLE* variables. Is there no way at all to have a short CMake setting via "mpi", maybe with presets, on the command/configure line?

Pinging @scivision on comments about the -DSCENABLE* approach.

Also pinging @jmark to be in the loop about upcoming sc changes.

tim-griesbach commented 6 months ago

@tim-griesbach @hannesbrandt Thanks for letting me know. This should work better now :)

Thank you for the quick response! We tried out your recent changes and we get

-- Build files have been written to: /home/griesbac/workspace/sc_cmake/ZLIB-prefix/src/ZLIB-build
[ 10%] Performing build step for 'ZLIB'
[100%] Built target zlib
[ 11%] Performing install step for 'ZLIB'
[100%] Built target zlib
Install the project...
-- Install configuration: "Release"
-- Installing: /usr/local/lib/libz.a
CMake Error at cmake_install.cmake:46 (file):
  file INSTALL cannot copy file
  "/home/griesbac/workspace/sc_cmake/ZLIB-prefix/src/ZLIB-build/libz.a" to
  "/usr/local/lib/libz.a": Permission denied.

Without the changes of this PR, we obtained

-- Build files have been written to: /home/griesbac/workspace/sc_cmake/ZLIB-prefix/src/ZLIB-build
[ 10%] Performing build step for 'ZLIB'
[100%] Built target zlib
[ 11%] Performing install step for 'ZLIB'
[100%] Built target zlib
Install the project...
-- Install configuration: "Release"
-- Up-to-date: /home/griesbac/workspace/sc_cmake/local/lib/libz.a
-- Up-to-date: /home/griesbac/workspace/sc_cmake/local/include/zlib.h
-- Up-to-date: /home/griesbac/workspace/sc_cmake/local/include/zlib_name_mangling.h
-- Up-to-date: /home/griesbac/workspace/sc_cmake/local/include/zconf.h
-- Installing: /home/griesbac/workspace/sc_cmake/local/lib/pkgconfig/zlib.pc
-- Up-to-date: /home/griesbac/workspace/sc_cmake/local/lib/libz.a
-- Up-to-date: /home/griesbac/workspace/sc_cmake/local/lib/cmake/ZLIB/ZLIB.cmake
-- Installing: /home/griesbac/workspace/sc_cmake/local/lib/cmake/ZLIB/ZLIB-release.cmake
[ 13%] Completed 'ZLIB'

Apparently the installation directory of zlib changed to a path outside of the build directory.

tim-griesbach commented 6 months ago

We saw that when one is initially calling cmake without any passed options and then calls ccmake to switch SC_ENABLE_MPI to ON, one gets the warning that MPI I/O is disabled/not found. However, an initial call of cmake with the command line parameter -DSC_ENABLE_MPI=1 works fine.

We tested also the code without the changes of this PR and copied the moved warnings to the same place as in this PR but the code still did not show the warning.

tim-griesbach commented 6 months ago

In the long run, it seems that namespace cleanliness would win, so we will go for the SCENABLE* variables. Is there no way at all to have a short CMake setting via "mpi", maybe with presets, on the command/configure line?

After some further tinkering, we found a possible way. One could add

if (DEFINED mpi)
  set (SC_ENABLE_MPI ${mpi} CACHE BOOL "" FORCE)
endif()

directly before the first appearance of SC_ENABLE_MPI in cmake/config.cmake. While mpi does not show up in ccmake, it is still possible to use -Dmpi.

dutkalex commented 6 months ago

Thank you for the quick response! We tried out your recent changes and we get

-- Build files have been written to: /home/griesbac/workspace/sc_cmake/ZLIB-prefix/src/ZLIB-build
[ 10%] Performing build step for 'ZLIB'
[100%] Built target zlib
[ 11%] Performing install step for 'ZLIB'
[100%] Built target zlib
Install the project...
-- Install configuration: "Release"
-- Installing: /usr/local/lib/libz.a
CMake Error at cmake_install.cmake:46 (file):
  file INSTALL cannot copy file
  "/home/griesbac/workspace/sc_cmake/ZLIB-prefix/src/ZLIB-build/libz.a" to
  "/usr/local/lib/libz.a": Permission denied.

Without the changes of this PR, we obtained

-- Build files have been written to: /home/griesbac/workspace/sc_cmake/ZLIB-prefix/src/ZLIB-build
[ 10%] Performing build step for 'ZLIB'
[100%] Built target zlib
[ 11%] Performing install step for 'ZLIB'
[100%] Built target zlib
Install the project...
-- Install configuration: "Release"
-- Up-to-date: /home/griesbac/workspace/sc_cmake/local/lib/libz.a
-- Up-to-date: /home/griesbac/workspace/sc_cmake/local/include/zlib.h
-- Up-to-date: /home/griesbac/workspace/sc_cmake/local/include/zlib_name_mangling.h
-- Up-to-date: /home/griesbac/workspace/sc_cmake/local/include/zconf.h
-- Installing: /home/griesbac/workspace/sc_cmake/local/lib/pkgconfig/zlib.pc
-- Up-to-date: /home/griesbac/workspace/sc_cmake/local/lib/libz.a
-- Up-to-date: /home/griesbac/workspace/sc_cmake/local/lib/cmake/ZLIB/ZLIB.cmake
-- Installing: /home/griesbac/workspace/sc_cmake/local/lib/cmake/ZLIB/ZLIB-release.cmake
[ 13%] Completed 'ZLIB'

Apparently the installation directory of zlib changed to a path outside of the build directory.

Yes this is intentional. Hardcoding the install path into the CMakeLists.txt is a code smell. The install path is meant to be set on the command line as it depends on the target machine. Enforcing one breaks portability, composability with downstream software, and packaging tools such as CPack or Spack. In order to retrieve the old behavior you just need to specify it with a -DCMAKE_INSTALL_PREFIX=... or using the --prefix option of cmake.

dutkalex commented 6 months ago

We saw that when one is initially calling cmake without any passed options and then calls ccmake to switch SC_ENABLE_MPI to ON, one gets the warning that MPI I/O is disabled/not found. However, an initial call of cmake with the command line parameter -DSC_ENABLE_MPI=1 works fine.

We tested also the code without the changes of this PR and copied the moved warnings to the same place as in this PR but the code still did not show the warning.

Very strange behavior. Maybe some cache issue... Let me investigate and get back to you...

dutkalex commented 6 months ago

In the long run, it seems that namespace cleanliness would win, so we will go for the SCENABLE* variables. Is there no way at all to have a short CMake setting via "mpi", maybe with presets, on the command/configure line?

After some further tinkering, we found a possible way. One could add

if (DEFINED mpi)
  set (SC_ENABLE_MPI ${mpi} CACHE BOOL "" FORCE)
endif()

directly before the first appearance of SC_ENABLE_MPI in cmake/config.cmake. While mpi does not show up in ccmake, it is still possible to use -Dmpi.

Yes this seems like a good tradeoff: existing code would continue working, but new downstream projects would not notice it. I can include it in the PR if this is ok for everyone

dutkalex commented 6 months ago

Is there no way at all to have a short CMake setting via "mpi", maybe with presets, on the command/configure line?

Hardcoding the install path into the CMakeLists.txt is a code smell.

@cburstedde @tim-griesbach Another solution to both of these problems could be to define a custom SCDev (or whatever) build mode which provides the desired defaults and shortcuts...

scivision commented 6 months ago

I didn't try the changes myself, but I agree with all these changes. Nice work!

Note: before incorporating these changes into P4EST, etc. such parent projects need to have their variables updated to match SC new variable names.

The option to set install prefix is like:


cmake -B build --install-prefix /path/to/install
dutkalex commented 6 months ago

@cburstedde @tim-griesbach @elykwilliams Anything more you need to get this merged?

cburstedde commented 5 months ago

Thanks so much! I'd go for the suggestion with if (DEFINED mpi), and not relying on build modes. Same with the debug configure variable.

Will this version be compatible with the present p4est, or do we need to update synchronously?

tim-griesbach commented 5 months ago

We saw that when one is initially calling cmake without any passed options and then calls ccmake to switch SC_ENABLE_MPI to ON, one gets the warning that MPI I/O is disabled/not found. However, an initial call of cmake with the command line parameter -DSC_ENABLE_MPI=1 works fine. We tested also the code without the changes of this PR and copied the moved warnings to the same place as in this PR but the code still did not show the warning.

Very strange behavior. Maybe some cache issue... Let me investigate and get back to you...

@dutkalex We still have this issue with the changes in this PR. Do you have an idea how to avoid this?

dutkalex commented 5 months ago

Ah yes right! Thanks for reminding me! Last time I could not reproduce the issue but I'll give it another try

dutkalex commented 5 months ago

Thanks so much! I'd go for the suggestion with if (DEFINED mpi), and not relying on build modes. Same with the debug configure variable.

Will this version be compatible with the present p4est, or do we need to update synchronously?

I think that it should work fine with the current state of p4est (develop) given that it already queries the prefixed variables: set(P4EST_ENABLE_MPI ${SC_ENABLE_MPI} CACHE BOOL "P4EST MPI support" FORCE)

cburstedde commented 5 months ago

Wondering about the CI in that one instance... and thanks for adding the mpi and debug shortcuts.

dutkalex commented 5 months ago

Ah yes right! Thanks for reminding me! Last time I could not reproduce the issue but I'll give it another try

I finally managed to replicate the problem. I don't quite understand what was the root cause, but it should be fixed now!

tim-griesbach commented 5 months ago

Ah yes right! Thanks for reminding me! Last time I could not reproduce the issue but I'll give it another try

I finally managed to replicate the problem. I don't quite understand what was the root cause, but it should be fixed now!

Thank you for the fix! I tried out your changes and it works for me as well.

cburstedde commented 5 months ago

Wondering about the CI in that one instance... and thanks for adding the mpi and debug shortcuts.

This may be resolved after the latest merge into develop.

cburstedde commented 5 months ago

So close. What's going on on Windows now (unrelated?)?

dutkalex commented 5 months ago

I think so:

CMake Error at C:/Program Files/CMake/share/cmake-3.29/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find MPI (missing: MPI_C_FOUND C)
cburstedde commented 5 months ago 
CMake Error at C:/Program Files/CMake/share/cmake-3.29/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find MPI (missing: MPI_C_FOUND C)

Thanks; I'd think @scivision would know how to repair this.

scivision commented 5 months ago

There was conflicting CMake variable use, fixed by this patch

178.patch

Note that CMAKE_BUILD_TYPE must be set before project() or it has no effect.

cburstedde commented 5 months ago

There was conflicting CMake variable use, fixed by this patch

178.patch

Note that CMAKE_BUILD_TYPE must be set before project() or it has no effect.

Thanks; when I keep using -Dmpi the error persists. The most recent code has the purpose to allow for -Dmpi on the cmake command line while using SC_ENABLE_MPI as the variable that is used in the cmake code. Is there a way to make this work with cmake -Dmpi?

cburstedde commented 5 months ago

Ok I've integrated it all with the latest develop version. Sorry for the merge in between. It got a bit confusing, but at least we're current with everything.

Would @dutkalex check whether this is all correct and I didn't screw up the merge?

dutkalex commented 5 months ago

Looks good to me @cburstedde 😊

cburstedde commented 5 months ago

Welcome to the developer list. :)

elykwilliams commented 5 months ago

@dutkalex Can you please restore your branch, it seems one of your commits has broken the CI.

cburstedde commented 5 months ago

@dutkalex Can you please restore your branch, it seems one of your commits has broken the CI.

It's a mix of disabling the OpenMPI support in Cmake and your code on the SC_ variables. I might have overlooked something in merging. I do not know what though -- do you think you can narrow it down (by bisect if need be)?

Sorry I cannot help until later next week, and thanks! Feel free to reopen.