Update MPI linking, building tests

Rickbude commented 2 years ago

Various improvements (hopefully) to MPI linking. Summary:

Bump CMake version to 3.9, such that find_package(MPI) exports the targets MPI::MPI_C, MPI::MPI_Fortran etc
Remove INCLUDE_DIRECTORIES(${MPI_INCLUDE_PATH}): include path should be tied to the MPI targets described above
Replace target_link_libraries(some_target ${MPI_Fortran_LIBRARIES}) with target_link_libraries(some_target MPI::MPI_Fortran)
Link scalapack with MPI::MPI_C (should fix #66)

Aside from this I noticed two variables were used for building tests: SCALAPACK_BUILD_TESTING and SCALAPACK_BUILD_TESTS. Only the latter had an associated cache variable, so I assumed all instances should be the latter.

Rickbude commented 2 years ago

I just noticed a mistake in the main CMakeLists at line 250-254:

add_library(scalapack ${blacs} ${tools-C} ${pblas} ${ptools} ${redist} ${src-C} MPI::MPI_C)
...
target_link_libraries( scalapack ${LAPACK_LIBRARIES} ${BLAS_LIBRARIES})

which will result in configure errors under MSVC, should instead be:

add_library(scalapack ${blacs} ${tools-C} ${pblas} ${ptools} ${redist} ${src-C})
...
target_link_libraries( scalapack ${LAPACK_LIBRARIES} ${BLAS_LIBRARIES}  MPI::MPI_C)

Is there a way to edit my pull request, or should I just cancel this pull request and create a new one?

drhpc commented 2 years ago

I tried this in my pkgsrc attempt (#66), after adapting the patch to this state:

patch-2.2.0-PR67.txt

The result seems to work in linking to MPI now in the installed library. But the file list changed. I wonder if that is intentional.

$ bmake print-PLIST
@comment $NetBSD$
lib/cmake/scalapack-2.1.0/scalapack-config-version.cmake
lib/cmake/scalapack-2.1.0/scalapack-config.cmake
lib/cmake/scalapack-2.1.0/scalapack-targets-noconfig.cmake
lib/cmake/scalapack-2.1.0/scalapack-targets.cmake
lib/libscalapack.so
lib/pkgconfig/scalapack.pc

So this is only the shared lib and strangely a directory versioned with 2.1.0, not 2.2.0. The old list of files looked like that:

$ cat PLIST 
@comment $NetBSD$
lib/cmake/${PKGNAME}/scalapack-config-version.cmake
lib/cmake/${PKGNAME}/scalapack-config.cmake
lib/cmake/${PKGNAME}/scalapack-targets-noconfig.cmake
lib/cmake/${PKGNAME}/scalapack-targets.cmake
lib/libscalapack.a
lib/libscalapack.so
lib/pkgconfig/scalapack.pc

PKGNAME is scalapack-2.2.0 in this case. I am not sure if there's some interaction with the build system around it to cause this. Where does the version number come from? And also: Is it intentional that the static library is not built anymore?

Rickbude commented 2 years ago

. Where does the version number come from? And also: Is it intentional that the static library is not built anymore?

To be honest, I don't know how the changes I made to the CMakeLists could result in the differences you mentioned. Are you building in both cases from a clean git clone, empty CMake cache, and identical build flags (so e.g. BUILD_SHARED_LIBS=ON|OFF in both cases)? If you give me your exact configure and build commands I can give it a try and try to replicate the observed behavior.

drhpc commented 2 years ago

(I am not building from a git checkout, but trying to package a released version of ScaLAPACK, so it is 2.2.0 + patches.)

Well, I am comparing with the previously working (somewhat) build of scalapack 2.1.0. There, the version number of the cmake files matches. Now, in release 2.2.0, I see this in CMakeLists.txt:

set(SCALAPACK_VERSION 2.1.0)
set(CPACK_PACKAGE_VERSION_MAJOR 2)
set(CPACK_PACKAGE_VERSION_MINOR 1)
set(CPACK_PACKAGE_VERSION_PATCH 0)

Is it intentional to have that not in sync with the package release version? Regarding the shared/static libs, it's probably a surprise that the static lib was built before, too, with 2.1.0. What is the intention of the upstream project here? I used to care a lot for keeping static and shared libs installed for LAPACK, as static was the old-school default, actually tried to submit changes to that build to enable both shared and static lib build in one go.

I am now tending towards not giving a **** and just getting on with life, so only the shared lib is fine by me. My only question, then, is if the SCALAPACK_VERSION in the build should match the released version.

Rickbude commented 2 years ago

Hmm okay I think you are looking at slightly outdated code then. This versioning issue (I think #46 is related) seems to have been addressed by commit 31e82e869cfbee0466a55dc1af546a69aba90293.

drhpc commented 2 years ago

Sure I am looking at outdated code — the current official release;-) I see that #46 fixed that. I wonder if your intended ABI policy is appropriate, though. If the SONAME is always libscalapack.so.MAJOR.MINOR … are you intending for version 2.3 not to be backwards compatible to version 2.2? I have a project where I took care to only add API and keep the old symbols working, over many dozen minor version increases now. The SONAME is still libfoo.so.0, with the full version being libfoo.so.0.47.0 (the 47th ABI update without patchlevel. A binary built with the first library release still works with the freshest one. I consider that being nice to users.

Is the ABI of 2.2.x supposed to be a subset of the ABI of 2.3.x, hence symlinking the library would work for old binaries? Or is it considered a full ABI break? Would be nice to clarify that in the docs. Then, you please be careful in increasing the minor version of ScaLAPACK to avoid too frequent program breakage with updates;-)

But back to the original issue: Yes, MPI linking seems fine now with the changes of this PR ported to 2.2.0 release for my purposes.

drhpc commented 2 years ago

I cannot reproduce the case of shared and static liblapack being built now. Maybe that was an artifact of the build debugging with some inconsistent state. I'll default to only the shared lib now.

weslleyspereira commented 2 years ago

Well, I am comparing with the previously working (somewhat) build of scalapack 2.1.0. There, the version number of the cmake files matches. Now, in release 2.2.0, I see this in CMakeLists.txt:
set(SCALAPACK_VERSION 2.1.0)
set(CPACK_PACKAGE_VERSION_MAJOR 2)
set(CPACK_PACKAGE_VERSION_MINOR 1)
set(CPACK_PACKAGE_VERSION_PATCH 0)
Is it intentional to have that not in sync with the package release version?

Not intentional. I think we could (github-)release ScaLAPACK with the current master + this PR. It would solve this and a couple of other issues.

If the SONAME is always libscalapack.so.MAJOR.MINOR … are you intending for version 2.3 not to be backwards compatible to version 2.2? I have a project where I took care to only add API and keep the old symbols working, over many dozen minor version increases now. The SONAME is still libfoo.so.0, with the full version being libfoo.so.0.47.0 (the 47th ABI update without patchlevel. A binary built with the first library release still works with the freshest one. I consider that being nice to users.

Is the ABI of 2.2.x supposed to be a subset of the ABI of 2.3.x, hence symlinking the library would work for old binaries? Or is it considered a full ABI break? Would be nice to clarify that in the docs. Then, you please be careful in increasing the minor version of ScaLAPACK to avoid too frequent program breakage with updates;-)

We had no policy, so not good. Then we adopted one in #48. This policy says that libscalapack.so.2.3 may not be ABI-compatible to libscalapack.so.2.2. This is similar to what they are using for Debian. Please, feel free to open a new issue to address this. We may, for instance, modify the policy a bit and use libscalapack.so.MAJOR.MINOR.PATCH. I just think this thread is not ideal to start this discussion.

Regarding the shared/static libs, it's probably a surprise that the static lib was built before, too, with 2.1.0. What is the intention of the upstream project here? I used to care a lot for keeping static and shared libs installed for LAPACK, as static was the old-school default, actually tried to submit changes to that build to enable both shared and static lib build in one go.

I am now tending towards not giving a **** and just getting on with life, so only the shared lib is fine by me. My only question, then, is if the SCALAPACK_VERSION in the build should match the released version.

In #61, there was a change in the behavior of BUILD_STATIC_LIBS. However, I am not sure when ScaLAPACK had a build with shared and static libs. I tried v2.2.0 in my Linux machine with BUILD_STATIC_LIBS=ON and BUILD_SHARED_LIBS=ON and obtained only libscalapack.so. Actually, I don't know how BUILD_STATIC_LIBS is used in the building process. This is not standard in CMake. Also, I recommend we continue this discussion in another thread.

Reference-ScaLAPACK / scalapack

Update MPI linking, building tests #67