Debian package fails to build with GCC >= 7 due to outdated caf_single

[amckinstry]

Defect/bug report

This applies to 1.7.4+, tested today on 1.8.6 on Debian.
Builds fine on GCC 6.3.0, fails on GCC 7.

Detailed bug log:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=853586

This is in building src/single/caf_single.c where the register and deregister methods don't match the signatures in the headers.
Looking at the src/mpi/caf_mpi.c alternative there is conditional code e.g.

#ifdef GCC_GE_7
void
PREFIX (deregister) (caf_token_t *token, int type, int *stat, char *errmsg,
                     int errmsg_len)
#else
void
PREFIX (deregister) (caf_token_t *token, int *stat, char *errmsg, int errmsg_len)
#endif

but no equivalent code for the caf_single version.

[rouson]

Unless others disagree, I suggest we delete caf_single because GCC already provides its own so having one in OpenCoarrays is redundant and creates unnecessary maintenance work. @afanfa, @tob2 , and @vehre, thoughts?

@amckinstry, is there a reason for choosing the OpenCoarrays caf_single library over the GCC caf_single library?

If no one disagrees, then I'll close this issue and submit a pull request with the deletion.

[zbeekman]

Perhaps a bigger question is: "Why is the build system trying to build this?" I just dug through our CMake logic pretty thoroughly, and I see no indication of it even being able to enter the src/single directory, much less build the single target... my local tests certainly don't attempt to build this... and make help does not list it in any of the targets created by CMake.

@amckinstry do you have any insight into what dark magic the package manager build system might be doing to trigger this build of caf_single.c? There's no reason, as far as I can tell, that the vanilla CMake build would ever enter the src/single directory, much less try to build it.

As long as @tob2, @vehre, @jerryd and @afanfa don't have any objections, removing this will hopefully fix the issue, although I don't understand why it's getting built in the first place.

[amckinstry]

@rouson Ok, I'd be happy to delete caf_single. I'd built it because I thought it was necessary (hadn't tried the GCC internal version).

The "dark magic" in the package build system is a local patch I have for Debian to build both shared and static versions of all libraries, rather than shipping just .a archives. Debian uses shared libraries for everything by default, to ensure that important (eg. security) bugs are fixed on an automated system upgrade. We also ship static libraries that may be of higher optimisation, but the programmer would have to deliberately choose to use them, and so hopefully understands the consequences of their actions
(higher optimisations - eg shared libs would be -fPIC while static were traditionally not. Now with recent gcc changes in 6* and 7* this is less important).

[vehre]

The caf_single.c in OpenCoarrays is unmaintained and to my knowledge a rather old copy of the gcc one. I recommend deleting it.

[zbeekman]

@amckinstry can you adjust the debian package build to not build this asset any longer? (i.e. don't build caf_single

Also, is there anything we can do to ease your package maintainer burdens? Anything that strikes you as a build system anti pattern or just a royal PITA?

Building shared libs should be doable with -DBUILD_SHARED_LIBS:BOOL=true (or =on or =1 etc.) It's possible that we could provide multiple targets, e.g., to build and install both shared and static libraries by default, although I'm not sure this is common and/or considered best practice...

[amckinstry]

Thanks, I've disabled using caf_single in Debian (for the next release, Stretch).
At the moment I have the following patch applied to build in Debian:

Index: open-coarrays-1.8.6/src/mpi/CMakeLists.txt
===================================================================
--- open-coarrays-1.8.6.orig/src/mpi/CMakeLists.txt
+++ open-coarrays-1.8.6/src/mpi/CMakeLists.txt
@@ -36,7 +36,8 @@ if (MPI_Fortran_MODULE_COMPILES)
   set(MPI_CAF_FORTRAN_FILES ../extensions/opencoarrays.F90)
 endif()

-add_library(caf_mpi mpi_caf.c ../common/caf_auxiliary.c ${MPI_CAF_FORTRAN_FILES})
+add_library(caf_mpi SHARED mpi_caf.c ../common/caf_auxiliary.c ${MPI_CAF_FORTRAN_FILES})
+add_library(caf_mpi_static STATIC  mpi_caf.c ../common/caf_auxiliary.c ${MPI_CAF_FORTRAN_FILES})
 target_link_libraries(caf_mpi PRIVATE ${MPI_C_LIBRARIES} ${MPI_Fortran_LIBRARIES})

 set_target_properties ( caf_mpi
@@ -53,9 +54,14 @@ endif()
 include_directories(${CMAKE_BINARY_DIR}/mod)

 install(TARGETS caf_mpi EXPORT OpenCoarraysTargets
+  DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+  LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+)
+install(TARGETS caf_mpi_static EXPORT OpenCoarraysTargets
   ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}"
   LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}"
 )
+set_target_properties(caf_mpi PROPERTIES SOVERSION 1 SONAME "libcaf_mpi.so.${OpenCoarraysVersion}")

 # Install modules to standard include dir, but namespace them with compiler/version
 set (mod_install "OpenCoarrays/${CMAKE_Fortran_COMPILER_ID}/${CMAKE_Fortran_COMPILER_VERSION}")

This does two things:
(1) Build both shared and static versions
(2) Set the SOVERSION and SONAME in the library (libcaf_mpi.so.1) . This is relevant for maintenance later. It would be useful to have these applied maintstream.

[amckinstry]

Just to note, with the disabling of caf_single, the problem is fixed in Debian. Leaving this bug open until the buggy code is removed

[zbeekman]

This does two things:
(1) Build both shared and static versions
(2) Set the SOVERSION and SONAME in the library (libcaf_mpi.so.1) . This is relevant for maintenance later. It would be useful to have these applied maintstream.

We're currently setting the default SOVERSION as

SOVERSION ${PROJECT_VERSION.MAJOR}.${PROJECT_VERSION.MINOR}

If there is a compelling reason to use a manually incremented (i.e., 1, 2, 3, etc.) major SOVERSION then please let us know and we will adopt this convention. Otherwise, we use semantic versioning, so any major changes to the library that cause backwards incompatibilities will cause a major version bump. Off the top of my head I can't think of any other cases where we would generate an ABI incompatible binary...

As a side note, compiling for GCC <= 6 will produce an ABI incompatible library with a version compiled for GCC >= 7 due to changes on the GFortran compiler side.

[amckinstry]

The SOVERSION is ok. The SONAME needs to be set to libcaf_mpi.so.${PROJECT_VERSION.MAJOR}
(the above patch now looks buggy but the resulting SONAME as verified by readelf -a is correct).

On GCC/GFortran version: Yes, Debian 9 (stretch) will be using GCC6 ABI; Debian 10 (buster) is expected to be gcc/gfortran 7. Just testing what needs to be fixed for this ...

Over the next release (1-2 yrs) I plan to write a symbol versioning patch for OpenCoarrays.

[zbeekman]

The SONAME needs to be set to libcaf_mpi.so.${PROJECT_VERSION.MAJOR}
(the above patch now looks buggy but the resulting SONAME as verified by readelf -a is correct).

I was under the impression that CMake did this for you automatically... I'll have to go double check the docs

[zbeekman]

(also, note to self and others: Let's keep this open until I've applied a version of your patch to build both static and so/dynamic libs...)