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

CMake revived #150

Merged
merged 6 commits into from
Sep 4, 2020
Merged

CMake revived #150

merged 6 commits into from
Sep 4, 2020

Conversation

MaartenBent
Copy link
Collaborator

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
Copy link

coveralls commented Aug 30, 2020

Coverage Status

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

@tbeu tbeu linked an issue Aug 31, 2020 that may be closed by this pull request
@tbeu
Copy link
Owner

tbeu commented Aug 31, 2020

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.

.travis.yml Outdated Show resolved Hide resolved
@tbeu
Copy link
Owner

tbeu commented Aug 31, 2020

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

cmake/src.cmake Outdated Show resolved Hide resolved
cmake/src.cmake Outdated Show resolved Hide resolved
cmake/thirdParties.cmake Outdated Show resolved Hide resolved
cmake/matioConfig.cmake.in Outdated Show resolved Hide resolved
cmake/matio_pubconf.cmake.in Outdated Show resolved Hide resolved
cmake/matio_pubconf.cmake.in Outdated Show resolved Hide resolved
cmake/test.cmake Outdated Show resolved Hide resolved
cmake/options.cmake Outdated Show resolved Hide resolved
cmake/options.cmake Outdated Show resolved Hide resolved
cmake/options.cmake Outdated Show resolved Hide resolved
cmake/options.cmake Outdated Show resolved Hide resolved
cmake/test.cmake Outdated Show resolved Hide resolved
cmake/packaging.cmake Outdated Show resolved Hide resolved
@tbeu
Copy link
Owner

tbeu commented Sep 1, 2020

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

cmake/thirdParties.cmake Outdated Show resolved Hide resolved
@MaartenBent
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Owner

tbeu commented Sep 2, 2020

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
Copy link
Owner

tbeu commented Sep 2, 2020

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

CMakeLists.txt Outdated Show resolved Hide resolved
cmake/thirdParties.cmake Outdated Show resolved Hide resolved
@tbeu
Copy link
Owner

tbeu commented Sep 3, 2020

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.

.appveyor.yml Show resolved Hide resolved
cmake/thirdParties.cmake Outdated Show resolved Hide resolved
cmake/thirdParties.cmake Outdated Show resolved Hide resolved
@MaartenBent
Copy link
Collaborator Author

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

@tbeu
Copy link
Owner

tbeu commented Sep 3, 2020

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

OK, I'll wait then.

@MaartenBent
Copy link
Collaborator Author

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

@MaartenBent
Copy link
Collaborator Author

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

@tbeu
Copy link
Owner

tbeu commented Sep 3, 2020

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
Copy link
Owner

tbeu commented Sep 3, 2020

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

@MaartenBent
Copy link
Collaborator Author

Thanks.

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

Copy link
Owner

@tbeu tbeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very valuable contribution. Thanks a lot. It always is a pleasure to work with experts of Git, CI and CMake.

@tbeu tbeu merged commit 98c613f into tbeu:master Sep 4, 2020
@emmenlau
Copy link

emmenlau commented Sep 4, 2020

Awesome work guys!!!

@MaartenBent MaartenBent deleted the cmake branch October 27, 2020 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMake
5 participants