search

maxmind / libmaxminddb

C library for the MaxMind DB file format
https://maxmind.github.io/libmaxminddb/
Apache License 2.0
912 stars 239 forks source link

build: cmake: fix installation of .dll files on Windows #261

Closed fcelda closed 3 years ago

fcelda commented 3 years ago

The .dll files are not installed on Windows when libmaxminddb is built as a shared library.

A shared on Windows consist of the .dll with the actual code and .lib with the description of symbols. Weirdly enough, the .dlls are installed into runtime directory and .libs into the libraries directory.

The install target in CMake installs only ARCHIVE and PUBLIC_HEADER objects. To support Windows, it either needs to include RUNTIME as well or the install command doesn't need to list the objects types. This PR uses the later approach.

oschwald commented 3 years ago

Thanks! I tested this on Linux and it doesn't seem to affect the installation paths there.

fcelda commented 3 years ago

You are welcome.

I also don't have access to the Windows build environment. I discovered this and the few other recent issues when attempting to create a Conan package for libmaxminddb. The build was failing in Conan's CI so it took me a few iterations to get it working on Windows because the CI was my only feedback.

I suspect there might be another problem with the symbol visibility on Windows. This is what I had to include in the CMake wrapper for the Conan recipe:

if(WIN32 AND BUILD_SHARED_LIBS)
    set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)
endif()

However, I don't know if it's always needed and I cannot reason about that change well so I'm not opening a pull request.

oschwald commented 3 years ago

Hmm, from reading the docs, I think enabling CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS might be needed when building a DLL given that we don't explicitly declare things for DLL export.