Modernize FindZeroMQ.cmake and fix some ZeroMQ related compile bugs#5091
Modernize FindZeroMQ.cmake and fix some ZeroMQ related compile bugs#5091SunBlack wants to merge 1 commit into
Conversation
339ee81 to
9ff93dc
Compare
| endif() | ||
|
|
||
| # pkg-config fallback | ||
| find_package(PkgConfig ${_zmq_quiet} ${_zmq_required}) |
There was a problem hiding this comment.
find_dependency will avoid having to explicitly forward these flags.
There was a problem hiding this comment.
True, always forgetting this command :-/
| if(TARGET libzmq) | ||
| add_library(ZeroMQ::ZMQ ALIAS libzmq) | ||
| return() | ||
| endif() |
There was a problem hiding this comment.
Let's move the CONFIG find this to the detect options, in that manner it remains consistent with the other deps. There if it fails you can invoke this MODULE find.
There was a problem hiding this comment.
Thanks for the changes, what I meant is to move the CONFIG find to the DetectOptions.cmake
There was a problem hiding this comment.
Not sure whether this is a good idea, as there are always more duplication in the DetectOptions.cmake due to the AUTO/ON paths:
if(ADIOS2_USE_ZeroMQ STREQUAL AUTO)
find_package(ZeroMQ 4.1 CONFIG)
if (NOT ZeroMQ_FOUND)
find_package(ZeroMQ 4.1 MODULE)
endif()
elseif(ADIOS2_USE_ZeroMQ)
find_package(ZeroMQ 4.1 CONFIG REQUIRED)
if (NOT ZeroMQ_FOUND)
find_package(ZeroMQ 4.1 MODULE REQUIRED)
endif()
endif()Another downside: The (3) examples could not use the Config code or need also the duplicated paths there
There was a problem hiding this comment.
The reason to have this in detectoptions.cmake is keep that file consistent, the dance of find_package is common there.
Another downside: The (3) examples could not use the Config code or need also the duplicated paths there
Can you develop this point?
Thanks!
There was a problem hiding this comment.
I do think it's poor design to move it to DetectOptions, since that means anyone looking for ZeroMQ has to implement both variants instead of having a central location, but if that's what you want...
The examples don't actually need ZeroMQ, since the target is only linked privately.
There was a problem hiding this comment.
The examples don't actually need ZeroMQ, since the target is only linked privately.
This is true since we find_dependency(zeroMq) in the adios2-config.cmake.
|
|
||
| # If native package was found, stop here | ||
| if(TARGET libzmq) | ||
| add_library(ZeroMQ::ZMQ ALIAS libzmq) |
There was a problem hiding this comment.
lets just use libzmq thru adios2? Looking at libzmq source code it seems that they do not namespace their targets in the config cmake file.
1583f7a to
b42b725
Compare
| find_package(ZeroMQ 4.1 REQUIRED) | ||
| find_package(ZeroMQ 4.1) | ||
| if(NOT ZeroMQ_FOUND) | ||
| find_package(ZeroMQPkg 4.1 REQUIRED) |
There was a problem hiding this comment.
No need for the rename, you can use find_package(... MODULE)
There was a problem hiding this comment.
I know. I put that there on purpose to make it clear that it's only intended for the pkg variant. If the module is ever exported and someone runs find_package(ZeroMQ), they might not realize that it's only meant for that one variant.
There was a problem hiding this comment.
This add extra complexity, please keep it simple using the module is more succinct.
Supersedes #5089 and fixes #5088.
Since the discussion in #5089 showed that
pkg confis necessary, and therefore a separatefindscript is still required, I tested this locally with vcpkg and ran into several other issues with ZeroMQ, so it’s probably worth creating a separate PR after all.Changes:
MODULEandCONFIGto thefind_packagecallsNotes:
scripts/ci/images/Dockerfile.ci-spack-ubuntu22.04-baseor is there really a another legacy path necessary?