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

Added the option not to install the package #353

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

Conversation

triskeldeian
Copy link

This can be useful to use cppzmq directly as a subproject. Allows to import this repository as a whole as a subtree or submodule and use the generated targets without exporting it in the main project install target

This can be useful to use this as a direct subproject
Copy link
Member

@sigiesec sigiesec left a comment

Choose a reason for hiding this comment

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

I am not sure if these changes are really necessary to achieve the goal. Isn't it sufficient to use the EXCLUDE_FROM_ALL option for add_subdirectory when including a cppzmq subdirectory to achieve this, without requiring any changes to cppzmq?

install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/libzmq-pkg-config/FindZeroMQ.cmake
DESTINATION ${CPPZMQ_CMAKECONFIG_INSTALL_DIR}/libzmq-pkg-config)
target_link_libraries(cppzmq INTERFACE ${ZeroMQ_LIBRARY} )
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

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).

${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

@coveralls
Copy link

coveralls commented Oct 8, 2019

Pull Request Test Coverage Report for Build 303

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.5%) to 86.404%

Totals Coverage Status
Change from base Build 301: 0.5%
Covered Lines: 680
Relevant Lines: 787

💛 - Coveralls

@sigiesec
Copy link
Member

Sorry for getting back to this only now. I would still like to know if using EXCLUDE_FROM_ALL wouldn't allow to skip installing as well.

Please edit the commits such that there is:

  • One commit that adds the CPPZMQ_ENABLE_INSTALL option (if necessary by the answer to the question above)
  • One commit that modifies the target_link_libraries commands (but admittedly I still don't understand what problem that solves)

Each commit should focus on a single issue and should not transition to a non-working state. Please also rebase the branch on the upstream master and remove the merge commit. There shouldn't be any merge commits within a PR.

These conditions are required to have an understandable commit history, and to allow for bisecting any issues without hazzles in the future.

@triskeldeian
Copy link
Author

I'm not sure what you mean. How do I change commits that are already pushed? Should I create a new branch from the baseline with the modification committed in the order you proposed?
About the other questions:
About EXCLUDE_FROM_ALL I will have to verify whether this won't additional side effects. From the CMake documentation:

Note that inter-target dependencies supersede this exclusion. If a target built by the parent project depends on a target in the subdirectory, the dependee target will be included in the parent project build system to satisfy the dependency.

I don't know if this will also add the target dependency to the install target. I will test it as soon as possible. As a side note I never used EXCLUDE_FROM_ALL but I used a few project using this additional option to customize this aspect.

The other change on the target linking was necessary, in my case, when using vcpkg on Windows, as far as I can remember. The issue is that at the beginning of the script you check whether the zeroMQ was found but that just checks that the variable ZeroMQ_FOUND was set. Now if one uses the modern approach to CMake, that is to export targets all is well and the following checks are ignored and when time comes to link the cppzmq target, that is linked correctly. Unfortunately the find module that was integrated in vcpkg doesn't set the target but the traditional CMake variable ZeroMQ_LIBRARY. With that switch you are just covering the possibility of the find module using the old CMake approach of exporting variables rather than targets.

@sigiesec
Copy link
Member

You can change commits, regardless of whether they were pushed or not. A guide is available here: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History#_changing_multiple When pushing, you need to add the --force-with-lease option (or an equivalent in a git GUI tool). You shouldn't do this on a "shared" branch where more than one persons commits to (or at least be extra careful that everyone you share it with is aware of that), but this branch is in your personal fork and therefore not shared in this sense.

If you don't want to delve into that now, I can do this for you. Just let me know.

Just a side note for the future (be it in this or in other repos): I'd recommend not pushing anything to the master branch of your fork, but create a named branch from which you create a PR. Technically, this makes no difference, but avoids confusion, since the PR is bound to a branch, not a particular commit.

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 this pull request may close these issues.

4 participants