matio icon indicating copy to clipboard operation
matio copied to clipboard

MinGW Build Regression

Open chris-se opened this issue 1 year ago • 3 comments

The fix for https://github.com/tbeu/matio/issues/187 caused a build regression in MinGW for me (amd64, CMake Buildsystem):

  • MinGW (for compatibility w/ build systems that expect glibc) allows the user to specify -lm for the math functions, and automatically maps them to the correct CRT. But it does not provide a -lc, because normally nobody explicitly specifies that.
  • The fix for the OpenBSD issue caused the build system of matio to automatically add -lc to the compiler line due to the commit https://github.com/tbeu/matio/commit/f058e5dfbaa8bc49fbe76631b6dbf7637be04b10.

This causes MinGW builds to fail, since HAVE_LIBM is true on those systems, but -lc doesn't work there.

The least wrong simple fix I could come up with (without testing on OpenBSD, which I don't use) was to simply also check for -lc and then only link to that if it's available. The following patch works for me on Linux, macOS and Windows:

--- a/cmake/compilerOptions.cmake       2023-11-12 13:25:19.000000000 +0100
+++ b/cmake/compilerOptions.cmake       2024-02-05 16:29:48.804984158 +0100
@@ -70,6 +70,13 @@ endforeach()
 
 include(CheckLibraryExists)
 check_library_exists(m pow "" HAVE_LIBM)
+# OpenBSD also requires -lc (See
+# <https://github.com/tbeu/matio/issues/187> for
+# details), but that doesn't exist on all platforms
+# that do have libm (most notably not MinGW), so
+# explicitly check for the existence of -lc before
+# linking against in in src.cmake.
+check_library_exists(c fopen "" HAVE_LIBC)
 
 include(CheckCSourceCompiles)
 set(TEST_CODE_DECIMAL_POINT "
--- a/cmake/src.cmake   2023-11-12 13:25:19.000000000 +0100
+++ b/cmake/src.cmake   2024-02-05 16:26:10.244019256 +0100
@@ -68,7 +68,10 @@ if(STDINT_MSVC)
 endif()
 
 if(HAVE_LIBM)
-    target_link_libraries(${PROJECT_NAME} PUBLIC m c)
+    target_link_libraries(${PROJECT_NAME} PUBLIC m)
+    if(HAVE_LIBC)
+        target_link_libraries(${PROJECT_NAME} PUBLIC c)
+    endif()
 endif()
 
 if(MSVC)

While this will solve the issue, the most appropriate fix would probably be to only do that if OpenBSD requires this, for example performing a --no-undefined link check, and only add -lc if that is required to avoid the linker error with that flag. However, I can also open a pull request for my workaround if that's more the route you'd want to go in.

chris-se avatar Feb 05 '24 15:02 chris-se

Thanks for the report. Need to fix it properly.

@seanm @MaartenBent FYI

tbeu avatar Feb 05 '24 16:02 tbeu

Looks good to me, it works fine on Ubuntu. I also don't have OpenBSD to check.

One small remark, you could move the if(HAVE_LIBC) outside the if(HAVE_LIBM). For those very rare systems that don't have m but do need c.

MaartenBent avatar Feb 05 '24 22:02 MaartenBent

I've setup a MinGW build CI that confirms the regression: https://github.com/tbeu/matio/actions/runs/7822032818/job/21340240188

tbeu avatar Feb 08 '24 06:02 tbeu

I've now attempted to write a proper generic test that doesn't depend on the OS but doesn't also add -lc on operating systems where that isn't necessary. See PR #238.

Since I don't have an OpenBSD system, I can't test that case, but I did take care when writing the CMake test. If something comes up on OpenBSD, this should hopefully be easily fixable.

chris-se avatar Feb 08 '24 14:02 chris-se