cppzmq icon indicating copy to clipboard operation
cppzmq copied to clipboard

Added the option not to install the package

Open triskeldeian opened this issue 4 years ago • 4 comments

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

triskeldeian avatar Oct 02 '19 14:10 triskeldeian

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

coveralls avatar Oct 08 '19 15:10 coveralls

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.

sigiesec avatar Oct 14 '19 10:10 sigiesec

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.

triskeldeian avatar Oct 14 '19 14:10 triskeldeian

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.

sigiesec avatar Oct 14 '19 14:10 sigiesec