MathisRosenhauer / libaec

libaec - Adaptive Entropy Coding library
https://gitlab.dkrz.de/k202009/libaec
BSD 2-Clause "Simplified" License
12 stars 9 forks source link

Improve cmake for mingw #19

Closed kmilos closed 3 years ago

kmilos commented 3 years ago

Use CTest: it includes enable_testing() within, but also defines BUILD_TESTING option (on by default) that can be used to disable tests externally (could be useful for various CI build automation matrixes)

No need to specify prefixes and suffixes in find_library(), those are handled automatically depending on platform; it does make sense to ensure for the static linking case though

Test for MSVC rather than WIN32: MINGW also defines WIN32, but supports library naming (and a few other things) like UNIX, so no need to add tricks like "_static" (by default, on MINGW you'll get static libaec.a, shared libaec.dll and libaec.dll.a as import library not clashing with the static one)

MathisRosenhauer commented 3 years ago

@jwsblokland could you please check that this works as intended with hdf5 on Windows? Thanks, Mathis

jwsblokland commented 3 years ago

@MathisRosenhauer Yes, everything works as intended. This is clearly an improvement. I knew I needed to do something special for Windows because of the clash of library name but I did not realize this only holds for MSVC.

One of the advantages about CMake Config file is that you can still easily change things without effecting the CMake projects which makes use of libaec.

Would it be an idea to increase the version number to, for example 1.0.6?

MathisRosenhauer commented 3 years ago

Thanks @jwsblokland and thanks for the patch @kmilos. I will wait for a little while to see if any patches from Fedora or other distros come in and make a new release then.

kmilos commented 3 years ago

Btw, I wonder if the "_static" suffix was the best choice, it seems HDF5 looks for "-static"?

jwsblokland commented 3 years ago

Maybe not. However, if you have in the same directory both libsz.so and libsz-static.a (or libsz_static.a) their FindSZIP function will still choose libsz.so instead of the static one.. In my HPC environment we have typically both the static and shared libray installed. This is one of the reasons why I created a pull request https://github.com/HDFGroup/hdf5/pull/703 in which I better integrate libaec into their CMake build system such one can easily switch between the shared and static library of libaec.

kmilos commented 3 years ago

Maybe not. However, if you have in the same directory both libsz.so and libsz-static.a (or libsz_static.a) their FindSZIP function will still choose libsz.so instead of the static one.

Sure, and that's why I left the explicit filename+extension for the libaec_USE_STATIC_LIBS case if one is using libaec-config.cmake, and it then doesn't matter if it's "_static" or "-static" since it's under libaec (packager) control. Btw, it's not just "their FindSZIP", it's how find_library() behaves in general (matches shared first).

I'm just wondering for the legacy FindSZIP case (no libaec-config.cmake, no szip-config.cmake), maybe it's still better libaec switches to "-static" so we don't proliferate yet another naming convention?

jwsblokland commented 3 years ago

Thinking about it, I agree with you it is the better choice.