tbeu / matio

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

CMake revived #150

Closed MaartenBent closed 3 years ago

MaartenBent commented 3 years ago

I though I'd give it a try as well. It is based on #107. I tried to fix the issues mentioned there.

I moved the commands for Travis CI into separate scripts, to make them easier to read and modify.

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 82.574% when pulling a1741c39b807971ef208a40e1dbf8f0698d93140 on MaartenBent:cmake into 0ae5353c0a89072b2bdf5bcbbedfb2ee3b6b1e3d on tbeu:master.

tbeu commented 3 years ago

Thanks a lot. This looks very promising and I am going to check it.

Please do add yourself to the Acknowledgements section of both README and README.md.

I am currently working on the conan support, but not yet based on cmake generator.

tbeu commented 3 years ago

Can you please check that -no-undefined -export-symbols @srcdir@/matio.sym is set for the LD_FLAGS (CMAKE_SHARED_LINKER_FLAGS), too.

tbeu commented 3 years ago

Do not worry about AppVeyor CI: I cancelled the jobs to check out CMake support there.

MaartenBent commented 3 years ago

I have made the config input files an exact copy of the files that are used as input for configure (but with CMake defines). I'm aware that this probably breaks building with older Visual Studio versions because it uses a modified matio_pubconf.h. I'll integrate these modifications later.

MaartenBent commented 3 years ago

I think these are all my changes for now.

If you need help with AppVeyor, let me know. I'd suggest to not run CMake in the main directory, but create and go into e.g. cmake_build. In build_script you can use something like:

cmake --build . --config $env:configuration -- /logger:"C:\Program Files\AppVeyor\BuildAgent\Appveyor.MSBuildLogger.dll"
tbeu commented 3 years ago

I think these are all my changes for now.

💯 Thank you.

If you need help with AppVeyor, let me know

Yes, something is broken, but I have no clue how to fix the env variable check for %build_with%=="cmake".

tbeu commented 3 years ago

Do you know why PACKAGE_VERSION cannot be set on Linux?

[ 75%] Built target matio
[ 87%] Built target matdump
[ 93%] Building C object CMakeFiles/test_mat.dir/test/test_mat.c.o
/home/tbeu/matio-cmake/test/test_mat.c: In function ‘main’:
/home/tbeu/matio-cmake/test/test_mat.c:3840:34: error: ‘PACKAGE_VERSION’ undeclared (first use in this function); did you mean ‘H5_PACKAGE_VERSION’?
                        prog_name,PACKAGE_VERSION);
                                  ^~~~~~~~~~~~~~~
                                  H5_PACKAGE_VERSION
/home/tbeu/matio-cmake/test/test_mat.c:3840:34: note: each undeclared identifier is reported only once for each function it appears in
CMakeFiles/test_mat.dir/build.make:62: recipe for target 'CMakeFiles/test_mat.dir/test/test_mat.c.o' failed
make[2]: *** [CMakeFiles/test_mat.dir/test/test_mat.c.o] Error 1
CMakeFiles/Makefile2:141: recipe for target 'CMakeFiles/test_mat.dir/all' failed
make[1]: *** [CMakeFiles/test_mat.dir/all] Error 2
Makefile:129: recipe for target 'all' failed
make: *** [all] Error 2
tbeu@laptop:~/matio-cmake$ cmake --version
cmake version 3.10.2

CMake suite maintained and supported by Kitware (kitware.com/cmake).
tbeu@laptop:~/matio-cmake$

It is missing in lines 199, 208 and 244 of matioConfig.h.

MaartenBent commented 3 years ago

Maybe it is a protected name, its also used by https://cmake.org/cmake/help/latest/manual/cmake-packages.7.html. VERSION is also not set. I'll work around it.

I see Travis does not build test_snprintf, because it has a native snprintf implementation, but it still tries to tun test_snprintf. I'll always enable the test_snprintf project on unix.

MaartenBent commented 3 years ago

The environment value build_with did not change for different jobs. I worked around it by extracting it from APPVEYOR_JOB_NAME environment variable.

tbeu commented 3 years ago

Thank you again so far. What is left for me to do is to update the NEWS (requires a rebase of this branch) and to mention CMake support in the two README files.

MaartenBent commented 3 years ago

I'll rebase it and smash all these small commits into one.

tbeu commented 3 years ago

I'll rebase it and smash all these small commits into one.

OK, I'll wait then.

MaartenBent commented 3 years ago

I first need to fix Travis. It doesn't like the version number: https://travis-ci.org/github/tbeu/matio/jobs/723872609#L846

MaartenBent commented 3 years ago

Changing find_package(HDF5) to find_package(HDF5 1.8) broke it. Setting CMAKE_PREFIX_PATH correctly seems to have fix it.

tbeu commented 3 years ago

Changing find_package(HDF5) to find_package(HDF5 1.8) broke it. Setting CMAKE_PREFIX_PATH correctly seems to have fix it.

Ah, now I see. The line number L846 was not correctly displayed when clicking the link. hence my first confusion.

tbeu commented 3 years ago

I added you as a contributor such that you should be able to cancel or restart the Travis jobs.

MaartenBent commented 3 years ago

Thanks.

CMake jobs are building successful now, so I'll rebase, squash and force-push. Then I think it can be merged.

emmenlau commented 3 years ago

Awesome work guys!!!