feeback after testing on Fedora 30

[halbux]

After some more testing I am piling up issues:

1. there is not even a slepc-devel nor slepc package in Fedora repos at all
2. cblas.h is hard to find
3. mpi has always been and here again is a pain to make work

Anyways the best thing to do is to use the libs and headers created in ~/SLlibs after running the install_petsc.sh script, not the standard packaged ones available in the repos

[halbux]

Wouldn't it just be easier and more robust to use the static lib available online for sparselizard, that's one single package with external lib versions that have been tested with sparselizard, I right now don't see how cmake is making anything more simple or robust compared to that single .a package

[halbux]

I just managed to build sparselizard with cmake after removing all MPI stuff for cmake as well as copying the whole petsc folder that includes the mpiuni folder to /usr/include

After the build however it still is a mystery for me how to build the example executables, I cannot find any binary file (I once build the bin of paraview with cmake but that was 6 month ago and cmake is far from intuitive enough to remember it after 6 month)

[halbux]

@matsievskiysv
All fixed. It's working for me, but only when providing all the appropriate path in ~/SLlibs

[matsievskiysv]

Damn github notifications - didn't see all these messages.

only when providing all the appropriate path in ~/SLlibs

cmake-gui is very convenient for setting the variables.

[halbux]

@matsievskiysv Yes it is. But that one I could not use the gui to do it, there is a header in the petsc source code that explicitly looks for mpi.h inside a folder:

include "petsc/mpiuni/mpi.h"

and except for copying that petsc folder to /usr/include I didn't find a way to set the path in the cmake-gui... the folder structure needs to be there and not just "mpi.h" (BTW: mpi is not used with petsc, petsc is explicitly compiled without mpi, but for that it uses a dummy "mpiuni" that bypasses actual mpi calls)

Otherwise: I just fixed the -Wunused warnings in master For the -Wno-return I don't feel responsible for every complaint the compiler can make, they all are not relevant

[matsievskiysv]

Did you try setting SLEPC_INCLUDE_PATH? I'm away from my machine right now and cannot test this right now.

I don't feel responsible for every complaint the compiler can make

Personally I prefer my programs to be warning-free. If you have a different opinion, that's fine. It's your code, after all..

[halbux]

@matsievskiysv Ok, finally found the motivation to fix all -Wno return warnings, there are now no more warnings on my machine when cmake --build .

[halbux]

And it seems to work just fine without copying the petsc/mpiuni folder to /usr/include! I will check on a fresh linux install to be sure

[matsievskiysv]

there are now no more warnings on my machine when cmake --build .

You can probably get rid of those by changing PRIVATE to PUBLIC in https://github.com/matsievskiysv/sparselizard/blob/cmake/src/CMakeLists.txt#L61-L62

[halbux]

@matsievskiysv Just ran the default example: I am amazed by just how low performance the repo packaged combo openblas/mumps is... the default example runs... 10x slower than with the openblas/mumps compiled by petsc in the ~/SLlibs folder. I think that's a good enough reason to entirely forget about that option, at least for all appllications that I target I need high performance

[matsievskiysv]

When running top, I see that static lib from the website uses all CPUs for the whole calculation, and .so version uses only one thread after mesh loading.

[halbux]

Yes indeed. After linking with the libs created by the petsc configure script in "install_external_libs" I get the same performance again as usual, so definitely use these and not the standard packages!

@matsievskiysv Otherwise: I have made quite a lot of changes (including removing docker) from your commits and I am slowly preparing to bring the cmake to master before deleting the stable branch.

I would like to thank you in the cmake file for your help and actually making cmake happen in sparselizard. Maybe best is if you agree that I use

Yourfirstnamefirstletter. YOURLASTNAME to mention you. If yes could you please provide these? :)

[matsievskiysv]

Added compiler options -fopenmp and -no-pie. Now I get almost the same simulation time. I'll create a PR

[matsievskiysv]

Yourfirstnamefirstletter. YOURLASTNAME to mention you. If yes could you please provide these? :)

S.V. Matsievskiy or S. Matsievskiy

[matsievskiysv]

Added compiler options -fopenmp and -no-pie. Now I get almost the same simulation time. I'll create a PR

@halbux Oh, I see you made a lot of changes. I don't want to compile libraries from source, so I won't create a PR. Just add the line

target_compile_options(sparselizard PUBLIC -fopenmp -no-pie)

somewhere in src/CMakeLists.txt.

PS, some lines in https://github.com/halbux/sparselizard/blob/stable/src/CMakeLists.txt#L35-L49 are not needed. You only add those if you want to expose them to the user in https://github.com/halbux/sparselizard/blob/stable/src/config.h.in#L5-L7

PPS, you removed docker folder, but didn't remove it from readme

[halbux]

Yes, the readme still has to be updated. I guess also the optimization flags are needed so I ll add:

-fopenmp -fPIC -no-pie -O3

For the not needed lines: It will be nice to have e.g. the has_slepc flag available if in the future I/a user wants to deactivate slepc in the source code. Or is it a problem to make all flags visible to the user?

Also: thanks added

[matsievskiysv]

Or is it a problem to make all flags visible to the user

No, not a problem.

I guess also the optimization flags are needed

cmake automatically adds -fPIC to library builds and -O3 when building a release version.

[halbux]

Allright, now the version is final, I will move it manually to master file by file to be sure I am aware of all files that have changed (tomorrow latest it's done) Thanks again for your help, I like your code, it was cleanly written @matsievskiysv And you are thanked in Cmakelist.txt (and still will be when in master :))

[halbux]

Done. All in master now, working. I am however wondering: in src

custom_add_library_from_dir(sparselizard "${SRC_DIR_LIST}") is ok

custom_add_library_from_dir(sparselizard ${SRC_DIR_LIST}) is not ok

The " seem of importance there. Is there a danger that they are missing somewhere else where it matters?

Could you also please kindly have a quick look at master to ensure I did not miss anything of importance?

Thanks again! I very much appreciate your code and help!

Alex

[matsievskiysv]

The " seem of importance there

It is only needed when argument is a list [1]

[halbux]

Thanks!

[matsievskiysv]

Could you also please kindly have a quick look at master to ensure I did not miss anything of importance?

Not sure about removing search paths from cMake/SetupXXX.cmake files. Otherwise looks ok

[halbux]

The REQUIRED OFF) or ON does not seem to work with cmake 3.17

[halbux]

@matsievskiysv There is some user request to have the libsparselizard.so in build/ (and not build/src otherwise they don't see it and think the .so has not been created) as well as having a folder "build/include" that contains all headers. Since I don't understand 100% of your functions would you agree to kindly add this? If you don't have time that's obviously fine as well and I will dig deeper, let me know in either case.

Thanks again,

Alex

[matsievskiysv]

I'll see what I can do.

[halbux]

@matsievskiysv lib is moved one up cleanly (I think) with at the beginning: set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})

I will collect the headers and dump them in a folder tomorrow, but I am afraid cmake will not be aware of that and not update the headers when the are changed in src?

[matsievskiysv]

I think you should use configure_file(<input> <output> COPYONLY) if you want cMake to watch for file changes.

[matsievskiysv]

Ok, you can slightly modify function in cMake/functions.cmake:

# Copy other files
function(custom_copy_file TARGET FROMDIRS TODIR GLOBS)
    foreach(d IN LISTS FROMDIRS)
      foreach(g IN LISTS GLOBS)
        file(GLOB SRC "${d}/${g}")
        foreach(f IN LISTS SRC)
          string(REGEX REPLACE "^.*/" "" DEST ${f})
          # message(FATAL_ERROR "${f} ${TODIR}/${DEST} COPYONLY")
          configure_file(${f} ${TODIR}/${DEST} COPYONLY)
        endforeach()
      endforeach()
    endforeach()
endfunction(custom_copy_file)

and then in src/CMakeLists.txt just add

custom_copy_file(sparselizard "${SRC_DIR_LIST}" "${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/include" "*.h")

And don't forget about simulation/default/CMakeLists.txt:

custom_copy_file(default ${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR} "*.msh;*.geo")

[halbux]

Done. Thank you, that's cleaner than what I was about to do :)

[halbux]

@matsievskiysv It seems that when I copy paste the main.cpp and .geo and .msh from another example folder into the simulations/default folder cmake does not notice that they have been changed. Is there a way to always copy when building, not matter if it has changed or not?

[matsievskiysv]

cmake allows making symbolic link instead of copying the file. Maybe this would work better?

[halbux]

@matsievskiysv So in build/simulations/default there would be a symbolic link to the files in simulations/default? What if the file is changed, does the symbolic link stay on the file itself or only on its name? If on the name that would solve the problem indeed. How would that be implemented? I will give it a try to see how that works.

[matsievskiysv]

Cmake could make both hard links (different file names point to the same file on disk. Unix only, restricted to the one file system) and symbolic links (point to the file name). Symbolic links would work better. In any case, if original file is modified, file in build dir will also be modified. Function is almost the same:

function(custom_symlink_file TARGET FROMDIRS TODIR GLOBS)
    foreach(d IN LISTS FROMDIRS)
        foreach(g IN LISTS GLOBS)
            file(GLOB SRC "${d}/${g}")
            foreach(f IN LISTS SRC)
              get_filename_component(filename ${f} NAME)
              if(NOT ${filename} STREQUAL "CMakeLists.txt")
                string(REGEX REPLACE "^.*/" "" DEST ${f})
                file(CREATE_LINK ${f} ${TODIR}/${DEST} SYMBOLIC)
              endif()
            endforeach()
        endforeach()
    endforeach()
endfunction(custom_symlink_file)

So you just add it to cMake/functions.cmake and replace custom_copy_file with custom_symlink_file in simulations/default/CMakeLists.txt

[halbux]

wonderfull! it's added, thanks! @matsievskiysv It fixed most of the problem, and all the danger of not using the mesh one thinks he is using is gone.

However one last issue remains: the main.cpp used is now the correct one but cmake still thinks it has not changed when the file is replaced, so the main is not recompiled

[matsievskiysv]

I use command cmake --build build -j$(nproc) and everything seems to work as expected. What command do you use for build?

[halbux]

@matsievskiysv Yes I do the same. But when I copy the files from another example and paste them in the ones in simulations/default then cmake does not even recompile the main.cpp (probably because it thinks it has not changed): I get:

[ 99%] Built target sparselizard [100%] Built target default

whereas if I then do any editing and saving of the main.cpp then I get

[ 99%] Building CXX object simulations/default/CMakeFiles/default.dir/main.cpp.o [100%] Linking CXX executable default [100%] Built target default

so the main.cpp in this case is recompiled.

--> I don't always want to open the main.cpp, add a space, save just to make cmake understand that it should recompile the main... + with this behavior I know there will be cases in the future where I will start not understanding what's going on because results do not change although I have replaced the main.cpp...

[matsievskiysv]

If you move example, then the modification time is preserved and make looks at the modification time to determine if file is stale or not. Try touch main.cpp before compilation.