Fix CMake variables #178

dutkalex · 2024-04-25T09:33:58Z

No description provided.

This reverts commit 37fbfea.

This reverts commit 72ec940.

This reverts commit 8c10fc4.

cburstedde · 2024-04-28T21:02:00Z

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 · 2024-04-28T21:16:25Z

Done! @cburstedde

cmake/config.cmake

tim-griesbach · 2024-04-29T17:32:40Z

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 · 2024-04-29T18:05:51Z

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

cburstedde · 2024-04-30T15:52:13Z

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 <PKG>_ENABLE_MPI, which is clean. In the long run, it seems that namespace cleanliness would win, so we will go for the SC_ENABLE_* 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 -DSC_ENABLE_* approach. Also pinging @jmark to be in the loop about upcoming sc changes.

tim-griesbach · 2024-04-30T17:06:32Z

@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 · 2024-04-30T18:04:18Z

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 · 2024-04-30T18:30:21Z

In the long run, it seems that namespace cleanliness would win, so we will go for the SC_ENABLE_* 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 · 2024-04-30T18:48:13Z

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 · 2024-04-30T18:50:27Z

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 · 2024-04-30T18:52:45Z

In the long run, it seems that namespace cleanliness would win, so we will go for the SC_ENABLE_* 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 · 2024-04-30T19:02:45Z

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

doc/author_dutka.txt

cburstedde · 2024-05-16T12:51:00Z

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

dutkalex · 2024-05-16T13:36:59Z

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 · 2024-05-16T14:54:59Z

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 · 2024-05-16T18:10:28Z

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 · 2024-05-17T05:28:14Z

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

dutkalex · 2024-05-17T06:29:59Z

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 · 2024-05-17T08:55:03Z

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 · 2024-05-20T20:22:21Z

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 · 2024-05-24T11:52:46Z

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 · 2024-05-24T12:42:43Z

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 · 2024-05-24T13:33:48Z

Looks good to me @cburstedde 😊

cburstedde · 2024-05-30T13:53:23Z

Welcome to the developer list. :)

elykwilliams · 2024-05-31T05:34:35Z

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

cburstedde · 2024-05-31T09:28:19Z

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

cmake fix

f25bd8e

dutkalex marked this pull request as draft April 25, 2024 09:34

dutkalex added 8 commits April 25, 2024 11:54

remove useless files

8c10fc4

simplifications

72ec940

ci fix

37fbfea

Revert "ci fix"

0c3aee1

This reverts commit 37fbfea.

Revert "simplifications"

02fb578

This reverts commit 72ec940.

Revert "remove useless files"

5514f47

This reverts commit 8c10fc4.

ci fix

919b314

ci fix

12136ab

dutkalex marked this pull request as ready for review April 25, 2024 12:45

dutkalex added 2 commits April 28, 2024 23:11

Create author_dutka.txt

c647fcc

Update release_notes.txt

2ce86fc

tim-griesbach reviewed Apr 29, 2024

View reviewed changes

cmake/config.cmake Show resolved Hide resolved

dutkalex added 3 commits April 29, 2024 19:55

Update config.cmake

a27a36c

Update check_mpiio.cmake

e007584

Internal zlib fix

dbe4bd0

Update config.cmake

a46958a

cburstedde reviewed May 16, 2024

View reviewed changes

doc/author_dutka.txt Outdated Show resolved Hide resolved

dutkalex added 2 commits May 16, 2024 15:25

Update author_dutka.txt

03c806f

weird cmake bug fix

de7c6e2

cburstedde mentioned this pull request May 16, 2024

CI:CMake: add GCC-14 #184

Merged

Merge remote-tracking branch 'origin/develop' into cmake-fix

f29ffee

Tentative: apply part of patch

2e08a6a

cburstedde added 7 commits May 24, 2024 13:59

Set cache variable

c7e079b

CMake: checking functionality of -Dmpi=

ae2b8ee

CMake: test use of simplest -Dmpi option

799d484

Merge branch 'develop' into cmake-fix

cbae99b

Officially use -DSC_ENABLE_MPI:BOOL= in CI

b41fb14

Merge branch 'develop' into cmake-fix

83b780f

White space

ab87ede

cburstedde merged commit 3a00a6a into cburstedde:develop May 30, 2024
36 checks passed

dutkalex deleted the cmake-fix branch May 30, 2024 14:05

dutkalex restored the cmake-fix branch May 31, 2024 06:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CMake variables #178

Fix CMake variables #178

dutkalex commented Apr 25, 2024

cburstedde commented Apr 28, 2024

dutkalex commented Apr 28, 2024

tim-griesbach commented Apr 29, 2024

dutkalex commented Apr 29, 2024

cburstedde commented Apr 30, 2024 via email

tim-griesbach commented Apr 30, 2024

tim-griesbach commented Apr 30, 2024

tim-griesbach commented Apr 30, 2024

dutkalex commented Apr 30, 2024

dutkalex commented Apr 30, 2024

dutkalex commented Apr 30, 2024

dutkalex commented Apr 30, 2024 •

edited

Loading

cburstedde commented May 16, 2024

dutkalex commented May 16, 2024

tim-griesbach commented May 16, 2024

cburstedde commented May 16, 2024

cburstedde commented May 17, 2024

dutkalex commented May 17, 2024

cburstedde commented May 17, 2024

scivision commented May 20, 2024 •

edited

Loading

cburstedde commented May 24, 2024

cburstedde commented May 24, 2024

dutkalex commented May 24, 2024

cburstedde commented May 30, 2024

elykwilliams commented May 31, 2024

cburstedde commented May 31, 2024 via email

Fix CMake variables #178

Fix CMake variables #178

Conversation

dutkalex commented Apr 25, 2024

cburstedde commented Apr 28, 2024

dutkalex commented Apr 28, 2024

tim-griesbach commented Apr 29, 2024

dutkalex commented Apr 29, 2024

cburstedde commented Apr 30, 2024 via email

tim-griesbach commented Apr 30, 2024

tim-griesbach commented Apr 30, 2024

tim-griesbach commented Apr 30, 2024

dutkalex commented Apr 30, 2024

dutkalex commented Apr 30, 2024

dutkalex commented Apr 30, 2024

dutkalex commented Apr 30, 2024 • edited Loading

cburstedde commented May 16, 2024

dutkalex commented May 16, 2024

tim-griesbach commented May 16, 2024

cburstedde commented May 16, 2024

cburstedde commented May 17, 2024

dutkalex commented May 17, 2024

cburstedde commented May 17, 2024

scivision commented May 20, 2024 • edited Loading

cburstedde commented May 24, 2024

cburstedde commented May 24, 2024

dutkalex commented May 24, 2024

cburstedde commented May 30, 2024

elykwilliams commented May 31, 2024

cburstedde commented May 31, 2024 via email

dutkalex commented Apr 30, 2024 •

edited

Loading

scivision commented May 20, 2024 •

edited

Loading