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

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

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

fcelda
Copy link
Contributor

@fcelda fcelda commented Apr 28, 2021

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

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

@oschwald oschwald merged commit 9ec72a1 into maxmind:main Apr 28, 2021
@fcelda fcelda deleted the remove-hard-coded-install-paths branch April 28, 2021 20:40
@fcelda
Copy link
Contributor Author

fcelda commented Apr 28, 2021

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

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.

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

Successfully merging this pull request may close these issues.

2 participants