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

Improve cmake for mingw #19

Merged
merged 1 commit into from
Jun 28, 2021

Conversation

kmilos
Copy link
Contributor

@kmilos kmilos commented Jun 25, 2021

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)

Signed-off-by: Miloš Komarčević <miloskomarcevic@aim.com>
@MathisRosenhauer
Copy link
Owner

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

@jwsblokland
Copy link
Contributor

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

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.

@MathisRosenhauer MathisRosenhauer merged commit deaaefa into MathisRosenhauer:master Jun 28, 2021
@kmilos kmilos deleted the cmake_mingw branch June 28, 2021 06:54
@kmilos
Copy link
Contributor Author

kmilos commented Jun 29, 2021

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

@jwsblokland
Copy link
Contributor

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 HDFGroup/hdf5#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
Copy link
Contributor Author

kmilos commented Jun 29, 2021

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants