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.

Sign up for GitHub

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

Jump to bottom

Added the option not to install the package #353

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Added the option not to install the package #353

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b0968b
Added the option not to install the package
triskeldeian Oct 2, 2019
fc8e07c
Added a conditional to switch between searching zeromq variables or t…
triskeldeian Oct 8, 2019
085ec87
Merge branch 'master' of github.com:zeromq/cppzmq
triskeldeian Oct 8, 2019
1372088
Merge branch 'master' of github.com:zeromq/cppzmq
triskeldeian Nov 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 46 additions & 33 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ include (DetectCPPZMQVersion)

project(cppzmq VERSION ${DETECTED_CPPZMQ_VERSION})

option(CPPZMQ_ENABLE_INSTALL "Generate the install target" ON )

find_package(ZeroMQ QUIET)

# libzmq autotools install: fallback to pkg-config
Expand Down Expand Up @@ -49,41 +51,52 @@ foreach (target cppzmq cppzmq-static)
$<INSTALL_INTERFACE:include>)
endforeach()

target_link_libraries(cppzmq INTERFACE libzmq)
if(NOT TARGET libzmq)
target_link_libraries(cppzmq INTERFACE ${ZeroMQ_LIBRARY} )
else()
target_link_libraries(cppzmq INTERFACE libzmq )
endif()

if(NOT TARGET libzmq-static)
target_link_libraries(cppzmq-static INTERFACE ${ZeroMQ_STATIC_LIBRARY})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are not directly related to the addition of CPPZMQ_ENABLE_INSTALL. In addition, this seems to break all CI builds. Could you please give this a look, possibly revert the changes?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I corrected what you mentioned in the new pull request.

About the link part: the issue is that linking the target is typically the correct thing to do but you may want to be able to revert to the old style cmake variables. In fact I had that if/else but I forgot to commit it, sorry.
The deeper problem that requires that change is that you perform a search for ZeroMQ first which could trigger a FindModule in the user system which would set those two variables and set ZeroMQ to found but will not set the target appropriately. This is what happened to me on Windows installing ZeroMQ with vcpkg, as far as I remember and would happen anyway if one has his own FindModule in the search path. This way you should able to cope with that case as well

Copy link

@ferdnyc ferdnyc Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@triskeldeian has a point here. I struggled with the same issue on Windows. I ended up writing my own FindZeroMQ.cmake and intentionally calling it before the find_package(cppzmq). Basically, I'm taking advantage of the mentioned ordering issue to thwart the problematic side-effects of the cppzmq config. I'm not sure this PR is quite how I'd address the problem, personally... but the issue at the heart of this is real.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the problem is the non-standard way FindZeroMQ.cmake probes for the shared vs. static library, since they're separate-but-equal which means neither of them can really be considered "required". I ended up dropping the static lib alternative, in my own find module, since it isn't needed for any of my builds and getting rid of it makes everything so much simpler. Like, I can use FindPackageHandleStandardArgs since ZeroMQ_LIBRARY is now one of the REQUIRED_VARS.

(Not using FPHSA in FindZeroMQ.cmake is the reason the REQUIRED argument has no effect in the find_package() call, BTW... just to address this source comment:)

cppzmq/CMakeLists.txt

Lines 16 to 22 in 18db456

find_package(ZeroMQ REQUIRED)
endif()
# TODO "REQUIRED" above should already cause a fatal failure if not found, but this doesn't seem to work
if(NOT ZeroMQ_FOUND)
message(FATAL_ERROR "ZeroMQ was not found, neither as a CMake package nor via pkg-config")
endif()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, coming back around to this issue, the real problem is a quirk of how CMake's target_link_libraries() works. Since it's a heavily-overloaded call that's had a lot of different jobs over time, it's probably the least strict of all the CMake commands.

When a value is passed to it that isn't a target (say in a case of unconditional linking, when the target may or may not exist), it's just assumed to be an argument for the linker. CMake processes whatever it's given as a string arg that it will now happily stuff onto the linker command line when targets get linked to this one.

And if there's no target for it, then we can already be pretty certain that libzmq is not a valid input for the linker, so having it there's gonna break some stuff.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's one of the reasons for namespacing IMPORTED targets, in fact. Because, while CMake can't be sure that libzmq isn't a linker argument, it CAN be sure that e.g. ZeroMQ::ZeroMQ must be a target, and otherwise it's not valid. (Same for ZeroMQ::libzmq, or any other permutation you like.) As the cmake-developer(7) doc says:

When providing imported targets, these should be namespaced (hence the Foo:: prefix); CMake will recognize that values passed to target_link_libraries that contain :: in their name are supposed to be imported targets (rather than just library names), and will produce appropriate diagnostic messages if that target does not exist (see policy CMP0028).

else()
target_link_libraries(cppzmq-static INTERFACE libzmq-static)
endif()

include(GNUInstallDirs)
include(CMakePackageConfigHelpers)

install(TARGETS cppzmq cppzmq-static
EXPORT ${PROJECT_NAME}-targets)

install(FILES ${CPPZMQ_HEADERS}
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})

# GNUInstallDirs "DATADIR" wrong here; CMake search path wants "share".
set(CPPZMQ_CMAKECONFIG_INSTALL_DIR "share/cmake/${PROJECT_NAME}" CACHE STRING "install path for cppzmqConfig.cmake")

configure_file(libzmq-pkg-config/FindZeroMQ.cmake
libzmq-pkg-config/FindZeroMQ.cmake
COPYONLY)

export(EXPORT ${PROJECT_NAME}-targets
FILE "${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Targets.cmake")
configure_package_config_file(${PROJECT_NAME}Config.cmake.in
"${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.cmake"
INSTALL_DESTINATION ${CPPZMQ_CMAKECONFIG_INSTALL_DIR})
write_basic_package_version_file(${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}ConfigVersion.cmake
VERSION ${CPPZMQ_VERSION}
COMPATIBILITY AnyNewerVersion)
install(EXPORT ${PROJECT_NAME}-targets
FILE ${PROJECT_NAME}Targets.cmake
DESTINATION ${CPPZMQ_CMAKECONFIG_INSTALL_DIR})
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.cmake
${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}ConfigVersion.cmake
DESTINATION ${CPPZMQ_CMAKECONFIG_INSTALL_DIR})
install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/libzmq-pkg-config/FindZeroMQ.cmake
DESTINATION ${CPPZMQ_CMAKECONFIG_INSTALL_DIR}/libzmq-pkg-config)
if(CPPZMQ_ENABLE_INSTALL)
include(GNUInstallDirs)
include(CMakePackageConfigHelpers)

install(TARGETS cppzmq cppzmq-static
EXPORT ${PROJECT_NAME}-targets)

install(FILES ${CPPZMQ_HEADERS}
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})

# GNUInstallDirs "DATADIR" wrong here; CMake search path wants "share".
set(CPPZMQ_CMAKECONFIG_INSTALL_DIR "share/cmake/${PROJECT_NAME}" CACHE STRING "install path for cppzmqConfig.cmake")

configure_file(libzmq-pkg-config/FindZeroMQ.cmake
libzmq-pkg-config/FindZeroMQ.cmake
COPYONLY)

export(EXPORT ${PROJECT_NAME}-targets
FILE "${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Targets.cmake")
configure_package_config_file(${PROJECT_NAME}Config.cmake.in
"${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.cmake"
INSTALL_DESTINATION ${CPPZMQ_CMAKECONFIG_INSTALL_DIR})
write_basic_package_version_file(${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}ConfigVersion.cmake
VERSION ${CPPZMQ_VERSION}
COMPATIBILITY AnyNewerVersion)
install(EXPORT ${PROJECT_NAME}-targets
FILE ${PROJECT_NAME}Targets.cmake
DESTINATION ${CPPZMQ_CMAKECONFIG_INSTALL_DIR})
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.cmake
${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}ConfigVersion.cmake
DESTINATION ${CPPZMQ_CMAKECONFIG_INSTALL_DIR})
install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/libzmq-pkg-config/FindZeroMQ.cmake
DESTINATION ${CPPZMQ_CMAKECONFIG_INSTALL_DIR}/libzmq-pkg-config)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation here is inconsistent, the parameter lists are no longer aligned. Could you indent everything by 2 or 4 spaces? (This is not entirely consistent in the rest of the file unfortunately)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's corrected in the new pull request. There was some mix of tabs and white spaces

endif()

option(CPPZMQ_BUILD_TESTS "Whether or not to build the tests" ON)

Expand Down