search

tbeu / matio

MATLAB MAT File I/O Library
https://matio.sourceforge.io
BSD 2-Clause "Simplified" License
338 stars 97 forks source link

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

Closed chris-se closed 7 months ago

chris-se commented 7 months ago

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 commented 7 months ago

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)

tbeu commented 7 months ago

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

tbeu commented 7 months ago

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 commented 7 months ago

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

seanm commented 7 months ago

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 commented 7 months ago

What excatly? OpenBSD CI or rebase button?

seanm commented 7 months ago

The former. Thanks for sharing.

chris-se commented 7 months ago

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).

tbeu commented 7 months ago

Thanks for fixing! Very appreciated.

tbeu commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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:

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 commented 7 months ago

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.