cmake: add a proper test to determine if explicit linking against -lc is required #238

chris-se · 2024-02-08T14:26:35Z

This pull request hopefully contains the proper way to fix the -lc issue raised in #187, without breaking MinGW as in #233.

This was tested on Linux and Windows/MinGW, but not on OpenBSD (I don't have a system running). However, since the check is operating-system agnostic, it should hopefully work there as well.

chris-se · 2024-02-08T14:36:47Z

Note: the windows-mingw test in this PR fails because the test execution doesn't work correctly, probably a minor CI setup issue. (See https://github.com/tbeu/matio/actions/runs/7831127379/job/21366733842?pr=238) Compare this to a different PR where the MinGW build wasn't yet fixed, and it failed already at the build stage in the windows-mingw test. (See https://github.com/tbeu/matio/actions/runs/7827524982/job/21355503568?pr=237)

cmake/compilerOptions.cmake

tbeu · 2024-02-08T14:41:39Z

@seanm Can you please cross-check in OpenBSD? Thanks!

tbeu · 2024-02-08T14:42:22Z

Note: the windows-mingw test in this PR fails because the test execution doesn't work correctly, probably a minor CI setup issue. (See https://github.com/tbeu/matio/actions/runs/7831127379/job/21366733842?pr=238) Compare this to a different PR where the MinGW build wasn't yet fixed, and it failed already at the build stage in the windows-mingw test. (See https://github.com/tbeu/matio/actions/runs/7827524982/job/21355503568?pr=237)

Yes, my bad. Can you fix it on the fly. $MATDUMP is just different.

tbeu · 2024-02-08T20:46:44Z

There now is a OpenBSD CI available. Feel free to rebase.

seanm · 2024-02-08T20:58:49Z

There now is a OpenBSD CI available. Feel free to rebase.

Oh, nice! I didn't think github had that. Where can I RTFM on that?

tbeu · 2024-02-08T21:02:55Z

What excatly? OpenBSD CI or rebase button?

seanm · 2024-02-08T21:10:02Z

The former. Thanks for sharing.

MaartenBent

Looks good. But I would prefer message(VERBOSE ...).
This status messages should not be relevant for end users.

CMake also has some macros to handle the CMAKE_REQUIRED_ variables, that you could use instead of manually resetting them: https://cmake.org/cmake/help/latest/module/CMakePushCheckState.html

chris-se · 2024-02-09T07:58:26Z

I've now updated this PR to include all of the previous feedback, including using VERBOSE for specific messages, as well as reworking some other things. (+ rebasing on current master)

The OpenBSD CI has now run successfully, and the CMake checks work as intended:

  -- Performing Test HAVE_LINK_NO_UNDEFINED_IMPLICIT_LIBC
  -- Performing Test HAVE_LINK_NO_UNDEFINED_IMPLICIT_LIBC - Failed
  -- Performing Test HAVE_LINK_NO_UNDEFINED_EXPLICIT_LIBC
  -- Performing Test HAVE_LINK_NO_UNDEFINED_EXPLICIT_LIBC - Success

This means that -lc is added on OpenBSD because that is needed there, but it's not added on other platforms, where it either isn't required (Linux) or even breaks things (MinGW).

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 · 2024-02-10T17:54:58Z

Thanks for fixing! Very appreciated.

tbeu · 2024-02-10T19:14:39Z

I've just noticed that the merge breaks the linking of matdump.exe on my Cygwin setup.

The relevant configure output is:

-- Performing Test HAVE_LINK_NO_UNDEFINED_IMPLICIT_LIBC
-- Performing Test HAVE_LINK_NO_UNDEFINED_IMPLICIT_LIBC - Success
-- Performing Test HAVE_LINK_NO_UNDEFINED_EXPLICIT_LIBC
-- Performing Test HAVE_LINK_NO_UNDEFINED_EXPLICIT_LIBC - Success
-- Performing Test HAVE_LINK_RETAIN_SYMBOLS_FILE
-- Performing Test HAVE_LINK_RETAIN_SYMBOLS_FILE - Success
-- Performing Test HAVE_LINK_UNDEFINED_ERROR
-- Performing Test HAVE_LINK_UNDEFINED_ERROR - Success
-- Using GNU-style linker flags

The link command is

/usr/bin/cc -O3 -DNDEBUG -Wl,--enable-auto-import CMakeFiles/matdump.dir/tools/matdump.c.o CMakeFiles/matdump.dir/snprintf/snprintf.c.o -o matdump.exe -Wl,--out-implib,libmatdump.dll.a -Wl,--major-image-version,0,--minor-image-version,0  libmatio.a -lm "/cygdrive/c/Program Files/HDF_Group/HDF5/1.14.3/lib/libhdf5.lib" "/cygdrive/c/Program Files/HDF_Group/HDF5/1.14.3/lib/libszaec.lib" "/cygdrive/c/Program Files/HDF_Group/HDF5/1.14.3/lib/libaec.lib" "/cygdrive/c/Program Files/HDF_Group/HDF5/1.14.3/lib/libzlib.lib" -lshlwapi

which results in about thousands lines of error messages.

The previous link command was

/usr/bin/cc -O3 -DNDEBUG -Wl,--enable-auto-import CMakeFiles/matdump.dir/tools/matdump.c.o CMakeFiles/matdump.dir/snprintf/snprintf.c.o -o matdump.exe -Wl,--out-implib,libmatdump.dll.a -Wl,--major-image-version,0,--minor-image-version,0  libmatio.a -lm -lc "/cygdrive/c/Program Files/HDF_Group/HDF5/1.14.3/lib/libhdf5.lib" "/cygdrive/c/Program Files/HDF_Group/HDF5/1.14.3/lib/libszaec.lib" "/cygdrive/c/Program Files/HDF_Group/HDF5/1.14.3/lib/libaec.lib" "/cygdrive/c/Program Files/HDF_Group/HDF5/1.14.3/lib/libzlib.lib" -lshlwapi

with -lc being the only difference.

I tried to setup a Cygwin CI yesterday but failed.

MaartenBent · 2024-02-10T22:12:51Z

So cygwin does need libc, even though the implicit check succeeds.

It can't find printf. Can you modify the test code to use printf instead of (or in addition to) malloc and free?

MaartenBent · 2024-02-10T23:41:17Z

Also are you linking with msvc hdf5 libraries? I've never used cygwin myself, but mixing gcc and msvc is usually not a good idea.

tbeu · 2024-02-11T06:35:58Z

I tried a lot - even forcing -lc, but it only works if the merge is reverted.

Yes, I also wondered why it links with MSVC static HDF5 libs.

chris-se · 2024-02-11T12:29:15Z

Most of the messages for undefined symbols you posted come from linking against MSVC static HDF5 libs, which obviously don't use the same C runtime as Cygwin, which is why those fail. (What I don't get is why they succeed if you revert the patch? They shouldn't work at all.)

My guess is that -lc isn't actually what's causing this here, but something probably related to linking against static MSVC libraries (perhaps the header of the wrong C runtime is used during compilation?). I haven't used cygwin in more than a decade, but I can take a look. Could you describe your setup a bit more? Did you just install cygwin + the official Windows binaries of HDF5?

chris-se · 2024-02-11T15:28:32Z

So interestingly, in the constellation you describe (HDF5 installed via the official binary installer) and building from within Cygwin, I get the following error already compiling in cygwin before the CMake change:

In file included from /cygdrive/c/Program Files/HDF_Group/HDF5/1.14.3/include/hdf5.h:21,
                 from /home/christian/old.matio/src/matio_private.h:76,
                 from /home/christian/old.matio/src/endian.c:31:
/cygdrive/c/Program Files/HDF_Group/HDF5/1.14.3/include/H5public.h:285:19: error: conflicting types for ‘ssize_t’; have ‘long long int’
  285 | typedef long long ssize_t;
      |                   ^~~~~~~
In file included from /home/christian/old.matio/src/matio.h:37,
                 from /home/christian/old.matio/src/matio_private.h:36,
                 from /home/christian/old.matio/src/endian.c:31:
/usr/include/stdio.h:81:18: note: previous declaration of ‘ssize_t’ with type ‘ssize_t’ {aka ‘long int’}
   81 | typedef _ssize_t ssize_t;
      |                  ^~~~~~~

However, when I install the hdf5-devel Cygwin package, I have the following behavior:

If I just call CMake without additional options, it still finds the HDF5 version installed in C:\Program Files, not the HDF5 version that is part of Cygwin.
If I specify -DHDF5_ROOT=/usr when invoking CMake (build directory has to be clean, if there's already a CMake configuration there, this won't work), then everything compiles successfully on Cygwin, both with and without my patch, and ./matdump ../share/test_file_hdf5.mat appears to work as expected.

Since my commit only touches the build system, but not the source files, could it be that the object files from your previous build were still seen as current (because when using make instead of e.g. ninja, compiler flag changes, such as different include paths, aren't detected, and don't cause a rebuild), which is why you don't see the ssize_t error in your case (because it only tried to re-link, not re-compile). And that because you now installed the official HDF5 installer on your system (but previously hadn't) and CMake detects the Visual Studio one over the Cygwin on, that this broke the Cygwin build, and not my change? And that because you didn't start with a fresh build directory, you didn't see the issues? (Only guessing here.)

tbeu · 2024-02-15T10:17:33Z

OK, after reinstalling libhdf5-devel in CygWin, unsinstalling the MSVC static libs and setting -DMATIO_SHARED=ON I can successfully link in CygWin. (It would also work if -lc is set, but not necessary.) CygWin does not install the static hdf5 libs, which is the reason why configuration with -DMATIO_SHARED=OFF fails: Could NOT find HDF5 (missing: HDF5_LIBRARIES) (found version "1.12.3")

Finally, we can leave this as is. Thanks again for fixing and investigating.

chris-se mentioned this pull request Feb 8, 2024

MinGW Build Regression #233

Closed

tbeu reviewed Feb 8, 2024

View reviewed changes

cmake/compilerOptions.cmake Outdated Show resolved Hide resolved

tbeu requested a review from MaartenBent February 8, 2024 14:40

tbeu force-pushed the master branch 3 times, most recently from 6911ba8 to d78a523 Compare February 8, 2024 20:31

MaartenBent requested changes Feb 8, 2024

View reviewed changes

tbeu linked an issue Feb 9, 2024 that may be closed by this pull request

MinGW Build Regression #233

Closed

chris-se force-pushed the proper-libc-link-test branch from dd46722 to 58e52d6 Compare February 9, 2024 07:51

tbeu requested a review from MaartenBent February 9, 2024 08:23

tbeu force-pushed the master branch 12 times, most recently from a91ab85 to 9893d4c Compare February 9, 2024 21:13

tbeu force-pushed the master branch 4 times, most recently from f249429 to 8442fbc Compare February 9, 2024 21:46

MaartenBent approved these changes Feb 10, 2024

View reviewed changes

Christian Seiler added 2 commits February 10, 2024 18:02

Github CI: fix windows-mingw test suite invocation

3a336cf

tbeu force-pushed the proper-libc-link-test branch from 58e52d6 to d39f93f Compare February 10, 2024 17:02

tbeu merged commit b0b3140 into tbeu:master Feb 10, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: add a proper test to determine if explicit linking against -lc is required #238

cmake: add a proper test to determine if explicit linking against -lc is required #238

chris-se commented Feb 8, 2024

chris-se commented Feb 8, 2024

tbeu commented Feb 8, 2024

tbeu commented Feb 8, 2024 •

edited

Loading

tbeu commented Feb 8, 2024

seanm commented Feb 8, 2024

tbeu commented Feb 8, 2024

seanm commented Feb 8, 2024

MaartenBent left a comment

chris-se commented Feb 9, 2024

tbeu commented Feb 10, 2024

tbeu commented Feb 10, 2024 •

edited

Loading

MaartenBent commented Feb 10, 2024

MaartenBent commented Feb 10, 2024

tbeu commented Feb 11, 2024

chris-se commented Feb 11, 2024

chris-se commented Feb 11, 2024

tbeu commented Feb 15, 2024

cmake: add a proper test to determine if explicit linking against -lc is required #238

cmake: add a proper test to determine if explicit linking against -lc is required #238

Conversation

chris-se commented Feb 8, 2024

chris-se commented Feb 8, 2024

tbeu commented Feb 8, 2024

tbeu commented Feb 8, 2024 • edited Loading

tbeu commented Feb 8, 2024

seanm commented Feb 8, 2024

tbeu commented Feb 8, 2024

seanm commented Feb 8, 2024

MaartenBent left a comment

Choose a reason for hiding this comment

chris-se commented Feb 9, 2024

tbeu commented Feb 10, 2024

tbeu commented Feb 10, 2024 • edited Loading

MaartenBent commented Feb 10, 2024

MaartenBent commented Feb 10, 2024

tbeu commented Feb 11, 2024

chris-se commented Feb 11, 2024

chris-se commented Feb 11, 2024

tbeu commented Feb 15, 2024

tbeu commented Feb 8, 2024 •

edited

Loading

tbeu commented Feb 10, 2024 •

edited

Loading