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

Windows MSYS discovery of libzmq breaks due to library names #361

Closed
ferdnyc opened this issue Nov 2, 2019 · 5 comments · Fixed by #510
Closed

Windows MSYS discovery of libzmq breaks due to library names #361

ferdnyc opened this issue Nov 2, 2019 · 5 comments · Fixed by #510

Comments

@ferdnyc
Copy link

ferdnyc commented Nov 2, 2019

Post-#267, I find that the FindZeroMQ.cmake from this repo is still failing on Windows, when invoked in a CMake project using the "MSYS Makefiles" generator, with ZeroMQ 4.3.1 installed from the MYS-official mingw32/mingw-w64-i686-zeromq (32-bit) and mingw64/mingw-w64-x86_64-zeromq (64-bit) packages via pacman.

(This also appears to be a different issue from #270, where I believe libzmq was self-built by the reporter. I'm not building libzmq, just installing the standard MSYS packages that include a libzmq.pc file.)

A little background. Also, apologies for the length of the forthcoming coredump:

zmq.hpp is actually included in the MSYS packages, so technically cppzmq isn't needed on (our) Windows at all, and find_package(cppzmq) will end up being a NOTFOUND.

However, since ZeroMQ doesn't install CMake targets for itself, and since our project uses ZeroMQ in cross-platform builds on other systems where cppzmq is useful (various Linux distros like Fedora, where cppzmq is a separate package and not bundled with libzmq), I wanted to replace our home-grown FindZMQ.cmake with the FindZeroMQ.cmake from cppzmq. My goal was to have ZeroMQ itself always be detected the same way, instead of getting configured twice in different ways when cppzmq is present. Plus, FindZeroMQ.cmake created IMPORTED targets, where our old find module was still stuck in the results-variables dark ages.

The switchover worked a treat, on Linux and MacOS. But on our Windows builders, it broke in really weird, subtle ways.

The issue, I believe, comes down to this note, from the "Imported Libraries" section of the CMake add_library() docs:

An `UNKNOWN` library type is typically only used in the implementation of Find Modules. It allows the path to an imported library (often found using the `find_library()` command) to be used without having to know what type of library it is. This is especially useful on Windows where a static library and a DLL’s import library both have the same file extension.

That point is relevant to these sections of the FindZeroMQ.cmake code:

find_library(ZeroMQ_LIBRARY NAMES libzmq.so libzmq.dylib libzmq.dll
PATHS ${PC_LIBZMQ_LIBDIR} ${PC_LIBZMQ_LIBRARY_DIRS})
find_library(ZeroMQ_STATIC_LIBRARY NAMES libzmq-static.a libzmq.a libzmq.dll.a
PATHS ${PC_LIBZMQ_LIBDIR} ${PC_LIBZMQ_LIBRARY_DIRS})

add_library(libzmq-static STATIC IMPORTED ${PC_LIBZMQ_INCLUDE_DIRS})
set_property(TARGET libzmq-static PROPERTY INTERFACE_INCLUDE_DIRECTORIES ${PC_LIBZMQ_INCLUDE_DIRS})
set_property(TARGET libzmq-static PROPERTY IMPORTED_LOCATION ${ZeroMQ_STATIC_LIBRARY})

...The problem is that libzmq.dll.a is NOT a static library. As the CMake documentation notes, despite the extension it's actually the import library for libzmq.dll. So, searching for and configuring libzmq.dll.a as a static library appears to break — and not in obvious ways, in really confusing ones.

What I was finding was that FindZeroMQ.cmake would report success:

-- Found ZeroMQ: C:/msys64/mingw64/lib/libzmq.dll.a (found version "4.3.1") 

The libzmq target would be created, but the build would fail before the linking step, with:

mingw32-make[2]: *** No rule to make target 'libzmq-NOTFOUND', needed by 'src/library.dll'.  Stop.

Attempting to use the libzmq-static target would also fail, since the file isn't a static library, and causes a build dependency for libzmq.dll to be created which then goes unsatisfied.

For the moment I've switched to removing the libzmq-static target entirely, and creating libzmq as an UNKNOWN IMPORTED library. I'm also detecting the library by name, instead of by filename:

find_path(ZeroMQ_INCLUDE_DIR zmq.h
          PATHS ${ZeroMQ_DIR}
                ${PC_LIBZMQ_INCLUDE_DIRS})

find_library(ZeroMQ_LIBRARY
             NAMES zmq
             PATHS ${ZeroMQ_DIR}
                   ${PC_LIBZMQ_LIBDIR}
                   ${PC_LIBZMQ_LIBRARY_DIRS})
if(ZeroMQ_LIBRARY)
    set(ZeroMQ_FOUND ON)
endif()

set ( ZeroMQ_LIBRARIES ${ZeroMQ_LIBRARY} )
set ( ZeroMQ_INCLUDE_DIRS ${ZeroMQ_INCLUDE_DIR} )

if(NOT TARGET libzmq)
    add_library(libzmq UNKNOWN IMPORTED)
    set_target_properties(libzmq PROPERTIES
        IMPORTED_LOCATION ${ZeroMQ_LIBRARIES}
        INTERFACE_INCLUDE_DIRECTORIES ${ZeroMQ_INCLUDE_DIRS})
endif()

That seems to work on all platforms, at least for our purposes.

I'd like to restore the shared/static library split in a way that works for all platforms, and if I can figure out a way to do it I'll submit a PR. It might be as simple as moving libzmq.dll.a to the SHARED detection, rather than the STATIC detection. But I have to do a lot more testing to confirm that.

@sigiesec
Copy link
Member

sigiesec commented Nov 4, 2019

@ferdnyc Thanks a lot for bringing this up! I will be happy to include a fix to the CMakeLists.txt, but I fear I won't be able to bring up one myself, given that I am not at all familiar with MSYS.

If you want it add an MSYS CI job (on Appveyor?) that checks that this works as expected, that would also be great to ensure that it doesn't break again in the future!

@ferdnyc
Copy link
Author

ferdnyc commented Nov 5, 2019

The ONLY way I've found to make it work is by changing the SHARED IMPORTED library target to UNKNOWN IMPORTED, but that does work on all platforms. Everything else can stay the same, and CMake's find_library() discovers the libzmq.dll.a file almost no matter what arguments are supplied to it, but unless the libzmq target is an UNKNOWN IMPORTED library, it becomes libzmq-NOTFOUND when passed to target_link_libraries() in the parent CMakeLists.txt.

The idea of being less explicit about the library type in the find module makes me a little nervous, and I think I'm going to install and try a couple more Windows compilers to confirm before I submit a PR to that effect, but so far it's absolutely the only thing that works, and no other changes need to be made on all platforms I've tried so far. (Though I will probably also move libzmq.dll.a to the shared-library side of the discovery, for good measure.)

If you want it add an MSYS CI job (on Appveyor?) that checks that this works as expected, that would also be great to ensure that it doesn't break again in the future!

That makes sense as well, I'll definitely take you up on that. AppVeyor have a ready-made MSYS2 setup pre-installed on their Windows images, so it's also quite painless. (Travis' Windows setup is a disaster for doing anything other than MSVC builds... and even for those, it's not great.)

@Cazadorro
Copy link

@sigiesec I ran into this issue today and was surprised this was still an issue, indeed replacing your FindZeroMQ.cmake with ferdnyc's fixes the issue. But beyond this, it isn't really a MSYS issue, and it isn't really a Mingw issue. Trying to use VCPKG or Conan to get ZMQ as a dependency, regardless if whether MSVC was used or not, was similarly painful with the default FindZeroMQ.cmake.

@sigiesec
Copy link
Member

Please provide a PR that fixes it. Having a test that covers it with or without MSYS would be nice, but that's optional.

Cazadorro pushed a commit to Cazadorro/cppzmq that referenced this issue Jul 28, 2021
…penshot, previously cppzmq was not looking for the correct names, and rejected valid static library names when looking for zmq on windows, this fix aims to include those other names
@Cazadorro
Copy link

@sigiesec We are using this change on both windows and linux and it appears to work (windows using vcpkg, linux using normal repository install of zmq), is it possible you could take a look at this? we currently depend on this change to use your library.

gummif added a commit that referenced this issue Aug 20, 2021
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 a pull request may close this issue.

3 participants