Repository navigation
Conversation
461a63a to
1f2f08d
Compare
LecrisUT
left a comment
LecrisUT
left a comment
There was a problem hiding this comment.
Just a quick comment to ack that I can help on this review.
1714d13 to
194ba2a
Compare
LecrisUT
left a comment
LecrisUT
left a comment
There was a problem hiding this comment.
Overall LGTM. Some followups if possible
| if(CMAKE_Fortran_COMPILER_ID STREQUAL "GNU") | ||
| if("${CMAKE_Fortran_FLAGS}" MATCHES "-ffpe-trap=[izoupd]") | ||
| set(FPE_EXIT TRUE) | ||
| endif() |
There was a problem hiding this comment.
If possible, would rather this go away in this form. It should instead be explicitly linked to the target that uses it, make them generator expressions, opt-in variables etc.
But would also be hard to review all of the changes of what actually makes sense, e.g. "Ensure that Release builds use O2 instead of O3". Let's split this up for a later PR?
There was a problem hiding this comment.
That would be a tricky one I think 🤔
| try_run( | ||
| xintface_res xintface_compile_res SOURCES ${BLACS_INSTALL_SRC}/Fintface.f | ||
| ${BLACS_INSTALL_SRC}/Cintface.c NO_CACHE | ||
| COMPILE_OUTPUT_VARIABLE xintface_compile_output | ||
| RUN_OUTPUT_VARIABLE xintface_output) |
There was a problem hiding this comment.
I am pretty sure this thing can die? 15 years later, there must be a fixed format each compiler uses now.
There was a problem hiding this comment.
I wonder if CMake's FortranCInterface would be of help. If possible maybe do it in a dedicated PR.
There was a problem hiding this comment.
👍 for dedicated PR as usual.
| if(MSVC) | ||
| add_library(scalapack-F) | ||
| add_library(ScaLAPACK::scalapack-F ALIAS scalapack-F) | ||
| set(SCALAPACK_FORTRAN_TARGET scalapack-F) | ||
| list(APPEND _scalapack_library_targets scalapack-F) | ||
| endif() |
There was a problem hiding this comment.
Instead of this, make a common INTERFACE target for this and add it either to scalapack or a new library.
Although, how relevant is this anymore? What Fortran compiler is there anymore for MSVC?
There was a problem hiding this comment.
Honestly, I'm also confused; just didn't check it as I rarely open Windows now :(
| - name: Check CMake package | ||
| run: | | ||
| mkdir consumer | ||
| cat > consumer/CMakeLists.txt <<'EOF' | ||
| cmake_minimum_required(VERSION 3.26) | ||
| project(consumer LANGUAGES C Fortran) | ||
| find_package(ScaLAPACK CONFIG REQUIRED) | ||
| if(NOT TARGET ScaLAPACK::scalapack) | ||
| message(FATAL_ERROR "ScaLAPACK::scalapack target not found") | ||
| endif() | ||
| EOF | ||
| cmake -S consumer -B consumer/build \ | ||
| -DCMAKE_PREFIX_PATH="$PWD/scalapack_install" |
There was a problem hiding this comment.
This should be a file and added in the testing infrastructure, but let's do that later
75feacc to
8aa9f45
Compare
3 participants
Modernize and simplify the ScaLAPACK CMake build by using standard imported targets for MPI/LAPACK, a canonical
ScaLAPACK::scalapacktarget, target-based source aggregation, and standardinstall(EXPORT)package configuration. This PR also makes the library unconditionally PIC, fixes broken pkg-config path generation, removes obsolete custom dependency/download logic, and aligns testing and version compatibility with standard CMake mechanisms.Closes #109. Closes #124. Closes #169.