Add version suffix to DLL when compiling for MinGW target

[manisandro]

Autotools creates DLL with a version suffix. This PR makes also cmake add a version suffix to the DLL.

[rouault]

Do we really need to perpetuate this specific behaviour of mingw support in autoconf ? If by default cmake doesn't add this suffix, maybe we shouldn't ? What's the impact of keeping things like they are currently ?

[manisandro]

From a packaging perspective, the suffix is really useful for generating versioned requires, i.e.

$ rpm -q --provides mingw64-gdal
mingw64(libgdal-30.dll)
mingw64-gdal = 3.4.3-1.fc37

Versioned libraries from my experience (maintaining quite a large number of mingw packages for Fedora) are pretty common, indeed most coming from the autotools days and then being implemented also for cmake or other build systems. Meson actually also adds suffixes by default.

[eli-schwartz]

Huh, fascinating. You're telling cmake to override the .dll / .so / .exe / .dylib component of a target filename, in order to manually insert the SOVERSION property again into the filename? Does cmake really not do that by default? What is the point of adding the SOVERSION property to a target then if it's just going to ignore it half the time? Weird.

This does seem like a (design) bug in cmake's Windows support.

Meson actually also adds suffixes by default.

Part of the documented behavior contract, in fact.

[manisandro]

Googling a bit, I found https://gitlab.kitware.com/cmake/cmake/-/issues/21716 - looks like while being acknowledged as a reasonable request, it was not pursued further.

[rouault]

@mwtoews it looks like the post install checks would need some tuning to accomodate for the libgdal-XY.dll pattern. Can you help with that ? But I'm not sure to understand what the goal of the check_ldd() function is exactly

[mwtoews]

Sure, I can take a look. The check_ldd function checks if the path of the dynamically loaded library is expected.

As a side note, I've been meaning to eventually refresh GDAL's post-install tests to be more in-line with PROJ's tests. Not sure when, however.

[dg0yt]

A version suffix to DLLs doesn't hurt, but IMO it also contributes very little to actual usability. Import lib and static lib shouldn't carry such a suffix.

[dg0yt]

Overriding SUFFIX in this way breaks import lib (.dll.a) and static lib (.a).

[manisandro]

Uhm where do you see this? The suffix isn't added to the import library here from what I can see, and generally I've always seen it added only to the runtime library even in projects which build both static and shared libraries.

[manisandro]

I've reworked this to only set RUNTIME_OUTPUT_NAME which should be safer - I've verified building with both BUILD_SHARED_LIBS=ON|OFF that only the .dll gets the suffix this way.

[dg0yt]

Maybe I was wrong about the import lib but not about the static lib.

RUNTIME_OUTPUT_NAME is an alternative. However, this will put the version before CMAKE_<CONFIG>_POSTFIX...

[manisandro]

POSTFIXES like d etc though are only used in the MSVC world? Alternatively I could go back to setting SUFFIX, but conditionally on BUILD_SHARED_LIBS, so that static libraries are not affected.

[dg0yt]

POSTFIXES like d etc though are only used in the MSVC world?

Possibly yes, but it is a user option (passed to CMake at configuration time). And with the Ninja multi-config generator and exported config, there may be valid use cases outside the MSVC world.

This just illustrates the key conflict with re-implementing autotools features in CMake: It often works against CMake features (here: multi-config support, user/toolchain control). Most often I meet this conflict with simultaneous builds of static and shared.

To be clear, I'm not against adding the version suffix. I just want to make sure it doesn't cause trouble for other reasonable (CMake) use cases.

[manisandro]

Do you see any potential issues with overriding SUFFIX just for BUILD_SHARED_LIBS=TRUE?

[mwtoews]

@rouault here's a working fix (borrowed from here) which will match (e.g.) /path/to/gdal/install-gdal/bin/libgdal, leaving the remainder to be -31.dll or .dll. There is no big deal matching the remainder, as this could potentially change?

diff --git a/autotest/postinstall/test_pkg-config.sh b/autotest/postinstall/test_pkg-config.sh
index 5e0cd76d20..30417ac687 100755
--- a/autotest/postinstall/test_pkg-config.sh
+++ b/autotest/postinstall/test_pkg-config.sh
@@ -31,8 +31,7 @@ case "$UNAME,$2" in
     export LD_LIBRARY_PATH=$prefix/lib
     ;;
   MINGW*,)
-    alias ldd="sh -c 'objdump -x \$1.exe' --"
-    LDD_SUBSTR="DLL Name: libgdal.dll"
+    LDD_SUBSTR=$(cygpath -u "$prefix/bin/libgdal")
     export PATH="$prefix/bin:$PATH"
     ;;
   *,--static)

[dg0yt]

-    alias ldd="sh -c 'objdump -x \$1.exe' --"
-    LDD_SUBSTR="DLL Name: libgdal.dll"
+    LDD_SUBSTR=$(cygpath -u "$prefix/bin/libgdal")

Wait, you need objdump for DLLs, and there is no DLL path stored in an EXE, only the name. What are you testing in PROJ then?

[mwtoews]

@dg0yt it seems that ldd.exe is a good Windows port to show the shared DLL dependencies. The path to the DLL is resolved using the PATH environment variable. I've opted to recommend ldd here instead of objectdump since it shows the full path to the DLL.

[dg0yt]

@mwtoews a) I'm not convinced that the test's point is to verify the environment (PATH) when the test is run. b) I use objdump because it also works on Linux. Linux ldd can't handle Windows executables. (not a dynamic executable).

The minimal change which would fix the current issue is trivial:

--- a/autotest/postinstall/test_pkg-config.sh
+++ b/autotest/postinstall/test_pkg-config.sh
@@ -32,7 +32,7 @@ case "$UNAME,$2" in
     ;;
   MINGW*,)
     alias ldd="sh -c 'objdump -x \$1.exe' --"
-    LDD_SUBSTR="DLL Name: libgdal.dll"
+    LDD_SUBSTR="DLL Name: libgdal"
     export PATH="$prefix/bin:$PATH"
     ;;
   *,--static)

[mwtoews]

@dg0yt sure, that simple fix will work too.

[rouault]

This PR is stalled

[manisandro]

I've pushed an update overriding SUFFIX only for BUILD_SHARED_LIBS=TRUE and adjusting test_pkg-config.sh as suggested above.