Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MinGW Build Regression #233

Closed
chris-se opened this issue Feb 5, 2024 · 4 comments · Fixed by #238
Closed

MinGW Build Regression #233

chris-se opened this issue Feb 5, 2024 · 4 comments · Fixed by #238
Labels

Comments

@chris-se
Copy link
Contributor

chris-se commented Feb 5, 2024

The fix for #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 f058e5d.

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.

@tbeu
Copy link
Owner

tbeu commented Feb 5, 2024

Thanks for the report. Need to fix it properly.

@seanm @MaartenBent FYI

@MaartenBent
Copy link
Collaborator

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.

@tbeu
Copy link
Owner

tbeu commented Feb 8, 2024

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

@tbeu tbeu added the bug label Feb 8, 2024
chris-se pushed a commit to chris-se/matio that referenced this issue Feb 8, 2024
Issue <tbeu#187> indicates that
OpenBSD requires an explicit specification of -lc on the linker
line when -Wl,--no-undefined is used.

Unfortunately, unconditionally adding -lc is not possible, as that
breaks MinGW builds.

This commit adds a proper CMake test to check whether -lc is
required when using -Wl,--no-undefined. If and only if it is
required, -lc will be added to the linker flags.

This now fixes <tbeu#233>.
chris-se pushed a commit to chris-se/matio that referenced this issue Feb 8, 2024
Issue <tbeu#187> indicates that
OpenBSD requires an explicit specification of -lc on the linker
line when -Wl,--no-undefined is used.

Unfortunately, unconditionally adding -lc is not possible, as that
breaks MinGW builds.

This commit adds a proper CMake test to check whether -lc is
required when using -Wl,--no-undefined. If and only if it is
required, -lc will be added to the linker flags.

This now fixes <tbeu#233>.
@chris-se
Copy link
Contributor Author

chris-se commented Feb 8, 2024

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 pushed a commit to chris-se/matio that referenced this issue Feb 9, 2024
Issue tbeu#187 appears to indicate that
OpenBSD requires an explicit specification of -lc on the linker line
if -Wl,--no-undefined is used.

Unfortunately, -lc is not available on MinGW (due to the C runtime
working different on Windows), so adding it whenever -lm is also linked
against does not work.

This commit reworks the entire CMake linker flag logic:

 - Remove dependency on CMake >= 3.17.0 for some checks -- we need to
   do a manual check_c_source_compiles() anyway (due to
   check_linker_flag() not triggering the potential error when -lc is
   not supplied), so we can drop that dependency.

 - Check for -Wl,--no-undefined both without and with an explicit -lc
   on the command line. If either version works, assume we can pass
   GNU-style linker options. If only the explicit version works, also
   pass -lc explicitly. (If both work don't, because even if -lc works,
   such as on a standard Linux, one would typically not pass it
   explicitly while compiling software.)

Fixes tbeu#233
tbeu pushed a commit to chris-se/matio that referenced this issue Feb 10, 2024
Issue tbeu#187 appears to indicate that
OpenBSD requires an explicit specification of -lc on the linker line
if -Wl,--no-undefined is used.

Unfortunately, -lc is not available on MinGW (due to the C runtime
working different on Windows), so adding it whenever -lm is also linked
against does not work.

This commit reworks the entire CMake linker flag logic:

 - Remove dependency on CMake >= 3.17.0 for some checks -- we need to
   do a manual check_c_source_compiles() anyway (due to
   check_linker_flag() not triggering the potential error when -lc is
   not supplied), so we can drop that dependency.

 - Check for -Wl,--no-undefined both without and with an explicit -lc
   on the command line. If either version works, assume we can pass
   GNU-style linker options. If only the explicit version works, also
   pass -lc explicitly. (If both work don't, because even if -lc works,
   such as on a standard Linux, one would typically not pass it
   explicitly while compiling software.)

Fixes tbeu#233
@tbeu tbeu closed this as completed in #238 Feb 10, 2024
tbeu pushed a commit that referenced this issue Feb 10, 2024
Issue #187 appears to indicate that
OpenBSD requires an explicit specification of -lc on the linker line
if -Wl,--no-undefined is used.

Unfortunately, -lc is not available on MinGW (due to the C runtime
working different on Windows), so adding it whenever -lm is also linked
against does not work.

This commit reworks the entire CMake linker flag logic:

 - Remove dependency on CMake >= 3.17.0 for some checks -- we need to
   do a manual check_c_source_compiles() anyway (due to
   check_linker_flag() not triggering the potential error when -lc is
   not supplied), so we can drop that dependency.

 - Check for -Wl,--no-undefined both without and with an explicit -lc
   on the command line. If either version works, assume we can pass
   GNU-style linker options. If only the explicit version works, also
   pass -lc explicitly. (If both work don't, because even if -lc works,
   such as on a standard Linux, one would typically not pass it
   explicitly while compiling software.)

Fixes #233
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants