search

tbeu / matio

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

Don't create HDF5::HDF5 target if it already exists + require zlib/hdf5 if enabled #155

Closed madebr closed 3 years ago

madebr commented 3 years ago

Hey,

These changes should make matio build on cci without any patches. (apart from the fix to the linker flags)

By requiring the dependencies, you are 100% sure that matio has them enabled. Because it might always not be found.

Kind regards

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 82.314% when pulling fb199d31e18d7e6496d1269eed84a1a2822500c9 on madebr:cmake_conan_transparant into 882178d3943b815d47084a8f19e55797b1580ad3 on tbeu:master.

madebr commented 3 years ago

For the sake of build system consistency: I'd actually prefer to build Hdf5 with CMake if Matio is built by CMake and use Autotools consistently otherwise.

Ok, but why is zlib built with autotools instead of cmake? Isn't requiring this an arbitrary requirement?

tbeu commented 3 years ago

For the sake of build system consistency: I'd actually prefer to build Hdf5 with CMake if Matio is built by CMake and use Autotools consistently otherwise.

Ok, but why is zlib built with autotools instead of cmake? Isn't requiring this an arbitrary requirement?

Right. In that case, zlib should be built by CMake, too. Or use system-wide zlib.

MaartenBent commented 3 years ago

zlib is not build with autotools in the Travis script, it is/was enclosed with if [[ "${USE_CMAKE:-no}" == "no" ]].

It is built together with the HDF5 libraries when executing ctest.

And in this case find_package(ZLIB) is not used at all, since the HDF5 config file defines the hdf5::zlib-static or hdf5::zlib-shared target.

madebr commented 3 years ago

zlib is not build with autotools in the Travis script, it is/was enclosed with if [[ "${USE_CMAKE:-no}" == "no" ]].

It is built together with the HDF5 libraries when executing ctest.

And in this case find_package(ZLIB) is not used at all, since the HDF5 config file defines the hdf5::zlib-static or hdf5::zlib-shared target.

Correct. I wasn't expecting this complicated configuration because I am used to always use system libraries (or conan packages). Personally, I try to avoid private zlib libraries because this introduces duplicate symbols. But it's totally fine for ci :smile: .

Anyhow, I reverted the hdf5-cmake-ci change.

MaartenBent commented 3 years ago

I'm getting this same error now as well, because FindHDF5 in CMake 3.19 defines a HDF5::HDF5 target. I propose a different solution, see #159.

The other change in this PR (adding REQUIRED) is also still open for discussion. Since the CMake project tries to mirror the behaviour of configure, I suggest to do the same here as well: if hdf5 or zlib is not found, continue without supporting it (i.e don't add REQUIRED).

tbeu commented 3 years ago

@MaartenBent Not being a CMake expert myself, I think I'll go with your alternate PR #159.

The other change in this PR (adding REQUIRED) is also still open for discussion. Since the CMake project tries to mirror the behaviour of configure, I suggest to do the same here as well: if hdf5 or zlib is not found, continue without supporting it (i.e don't add REQUIRED).

Yes, sounds reasonable.