Add package dependency checking to install_basic_package_files()

[mbeutel]

install_basic_package_files() makes package installation a breeze by saving us from writing pages of boilerplate code. Yet I think we all agree that it is a crutch: partly because it ought to be provided by CMake itself (cf. cmake/#18634, "Simplify exporting of CMake Packages"), but also because it necessitates duplicating the package dependencies of the exported targets, something CMake should be able to track but currently doesn't (cf. cmake/#17006, "Add new properties to better track what find_package command was used to import the target").

For example, the exported targets of my library depend on gsl-lite, which I express in the target definition using find_package() and target_link_libraries():

# slowmath/src/CMakeLists.txt
...
find_package(gsl-lite 0.36 REQUIRED) # <--
...
add_library(slowmath INTERFACE)
...
target_link_libraries(slowmath
    INTERFACE
        gsl::gsl-lite-v1) # <--
...
install(
    TARGETS slowmath
    EXPORT slowmath
    INCLUDES)

But to install the library package I additionally need to write:

# slowmath/CMakeLists.txt
include(InstallBasicPackageFiles)
install_basic_package_files(slowmath
    COMPATIBILITY SameMajorVersion
    DEPENDENCIES
        "gsl-lite 0.36") # <--

This is clearly suboptimal, but as far as the install_basic_package_files() abstraction goes, I believe this is the best we can do. We cannot really track the packages of target dependencies unless cmake/#17006 is resolved. Unfortunately, for me this duplication has proven error-prone with more complicated packages: I often forget adding packages to the DEPENDENCIES section and only notice after I broke someone who depended on my package.

Now, we may not be able to automate away this duplication, but I think install_basic_package_files() can do some extra work to validate the package dependencies. The idea is as follows:

When installing a package with exports, install_basic_package_files() generates a subproject which consumes the config file in the build directory as a package and defines an executable target which links to all targets our package exports. It then uses ExternalProject_Add() to generate an extra target check-package-dependencies which configures the subproject.

Thus, if one of the exported targets has a dependency that was forgotten in the DEPENDENCIES argument, building the package (with testing enabled) causes the check-package-dependencies target to fail:

  CMake Error at CMakeLists.txt:4 (add_executable):
    Target "slowmath-check-package-dependencies" links to target
    "gsl::gsl-lite-v1" but the target was not found.  Perhaps a find_package()
    call is missing for an IMPORTED target, or an ALIAS target is missing?

I attempted an implementation of this idea in PR #316. I'm not quite happy with some of the details though:

• To trigger CMake's transitive target search, the subproject needs a target with a link step, e.g. an executable target. I currently look at the enabled languages in the host project to select a linker language for the subproject. As far as I can tell, CMake imported targets are not intrinsically language-specific, but this might still cause trouble e.g. if I have a package that exports libraries for languages with incompatible linkage semantics such as C++ and C#. (So far I only used CMake with C++ and CUDA, and I have no idea how linkage works for, say, C# or Swift.)
• I don't want the validation checks if the package is configured for consumption only (e.g. by a package manager). To achieve this, I currently check if CMAKE_TESTING_ENABLED is set, which is the case if enable_testing() has previously been called. Unfortunately CMAKE_TESTING_ENABLED is currently undocumented; in cmake/#20301 "Document CMAKE_TESTING_ENABLED" I request that it be documented.
• CMake doesn't currently support enumerating the targets in an export set (cf. cmake/#19333, "Proposal: get_targets"), so I parse the <package>Targets.cmake file for add_library(<target> ... IMPORTED) statements instead.
• ExternalProject_Add() has an argument BUILD_ALWAYS, but there is no CONFIG_ALWAYS; I work around by passing a timestamp as a config argument.

Perhaps some ugliness in the implementation is acceptable because this is just a sanity check and doesn't alter the semantics of install_basic_package_files(). And if something goes wrong, dependency checking can be suppressed by setting the option NO_PACKAGE_DEPENDENCIES_CHECK in my draft.

What do you think?

[traversaro]

Fyi @GiulioRomualdi

[traversaro]

I did not review this in detail, but thanks a lot for this! This functionality is quite useful and would avoided us several problems for exaple the one listed in robotology/unicycle-footstep-planner#33 .

* I don't want the validation checks if the package is configured for consumption only (e.g. by a package manager). To achieve this, I currently check if `CMAKE_TESTING_ENABLED` is set, which is the case if [`enable_testing()`](https://cmake.org/cmake/help/latest/command/enable_testing.html) has previously been called. Unfortunately `CMAKE_TESTING_ENABLED` is currently undocumented; in [cmake/#20301 "Document `CMAKE_TESTING_ENABLED`"](https://gitlab.kitware.com/cmake/cmake/issues/20301) I request that it be documented.

Just to understand, why you don't use the BUILD_TESTING variable for this?

[traversaro]

And if something goes wrong, dependency checking can be suppressed by setting the option NO_PACKAGE_DEPENDENCIES_CHECK in my draft.

I am a bit afraid (even if I am not sure what failure could cause) in silently adding tests in a CMake function call. Do you think it could make sense to make this opt-in instead of opt-out?

[mbeutel]

• I don't want the validation checks if the package is configured for consumption only (e.g. by a package manager). To achieve this, I currently check if CMAKE_TESTING_ENABLED is set, which is the case if enable_testing() has previously been called. Unfortunately CMAKE_TESTING_ENABLED is currently undocumented; in cmake/#20301 "Document CMAKE_TESTING_ENABLED" I request that it be documented.

Just to understand, why you don't use the BUILD_TESTING variable for this?

BUILD_TESTING is defined by the CTest module, which is useful but not mandatory for unit tests in CMake. Many projects define tests with enable_testing() and add_test() depending on some custom build option (example) and without including CTest; for these projects BUILD_TESTING will not be defined even if testing is enabled. CTest also calls enable_testing(), so detecting enable_testing() covers both scenarios.

I am a bit afraid (even if I am not sure what failure could cause) in silently adding tests in a CMake function call. Do you think it could make sense to make this opt-in instead of opt-out?

I believe this would significantly diminish the usefulness of the extra check: we would have to teach people to enable it explicitly (which costs time and effort and makes their build scripts noisier), and it wouldn't be enabled on old projects. Personally I'm strongly in favor of everything that reduces the noise level in CMake files.

If you are worried about inadvertent breakage, I guess we could first make it opt-in, then gain some experience in the field, and later make it opt-out if no problems occur.

Note that my extension defines a custom target (through ExternalProject_Add()) but does not define additional tests (i.e. I don't call add_test()). The custom target will just fail to build if the dependency checks fail.

[drdanz]

I haven't seen the code yet (sorry, I'm reading this from the phone), but having an easy way to test a package and it's targets seems like a very good idea (also it would help a lot in creating unit tests for this module) , but in my opinion it goes beyond the scope of install_basic_package_files, what about making a separate module? Something like add_package_test maybe?

I'll just dump a few random ideas and comments in random order here

• If you create a target, it doesn't necessarily mean that you can actually use that target, for example you could be installing a header only library as interface that links to something that you don't have on your system.
• add_test instead of custom target?
• I'm not sure if ExternalProject is a good idea, I'd rather add a test that runs CMake in a clean project just to be sure that the package doesn't taint current project.
• Just linking might not be enough, it could be useful to force including a header in the main file
• Instead of linking it would be possible to iterate over the target interface link targets and ensure that the target exists
• Maybe, instead of finding the targets in the export set, it could accept the package name and one target in the command
• Perhaps try_compile could be used here.

I will have a look at the code next week, sorry, but I don't think I'll be able to do it earlier.

[mbeutel]

Thank you both for the feedback, much appreciated.

I haven't seen the code yet (sorry, I'm reading this from the phone), but having an easy way to test a package and it's targets seems like a very good idea (also it would help a lot in creating unit tests for this module) , but in my opinion it goes beyond the scope of install_basic_package_files, what about making a separate module? Something like add_package_test maybe?

This would boil down to

do_thing(args...)
validate_thing_args(args...)

i.e. we would have an optional extra step just for checking that the arguments to install_basic_package_files() were consistent.

• If you create a target, it doesn't necessarily mean that you can actually use that target, for example you could be installing a header only library as interface that links to something that you don't have on your system.

You mean like this?

project(Foo ...)
add_library(Foo INTERFACE)
# no calls to `find_package(Bar)` or `target_link_libraries(Foo PRIVATE Bar::Bar)` here!
install_basic_package_files(Foo
    COMPATIBILITY SameMajorVersion
    DEPENDENCIES
        "Bar") # not actually available on this system

That would indeed break my check. But is this really a sensible scenario? It would also lead to inconsistency; consuming the targets of Foo through add_subdirectory() would cause Foo to have transitive dependencies different from what one would get through find_package().

• add_test instead of custom target?
• I'm not sure if ExternalProject is a good idea, I'd rather add a test that runs CMake in a clean project just to be sure that the package doesn't taint current project.

I agree with @traversaro that add_test() should not silently be called by install_basic_package_files() itself. But add_test() is probably fine if we put the check in a separate function.

Why do you dislike generating a custom target for the check? CMake already generates some custom targets by itself (e.g. list_install_components, edit_cache, cmake_check_build_system), so I don't think this is necessarily inappropriate.

• Just linking might not be enough, it could be useful to force including a header in the main file

Note that the subproject is only ever configured, not built, and hence neither compiled nor linked. I only want the transitive dependency search which CMake runs during configuration.

• Instead of linking it would be possible to iterate over the target interface link targets and ensure that the target exists

But what would be the benefit of duplicating CMake's target resolution logic?

• Maybe, instead of finding the targets in the export set, it could accept the package name and one target in the command

But install_basic_package_files() used to have a TARGETS argument which was deprecated in favor of EXPORTS (I think). It would feel odd to introduce a companion function add_package_test() which expects a TARGETS argument.

[mbeutel]

• Unfortunately CMAKE_TESTING_ENABLED is currently undocumented

That should no longer be a concern:

I think it will be fine to document CMAKE_TESTING_ENABLED. It's not going anywhere so you can use it safely too.

(https://gitlab.kitware.com/cmake/cmake/issues/20301#note_691768)

[drdanz]

This would boil down to

do_thing(args...)
validate_thing_args(args...)

i.e. we would have an optional extra step just for checking that the arguments to install_basic_package_files() were consistent.

But that would allow to do just the validation_thing_args if you create the package in some other way...
Also eventually, even if I don't think this is a good thing you can have a separate module and call that module inside install_basic_package_files()

---

You mean like this?

project(Foo ...)
add_library(Foo INTERFACE)
# no calls to `find_package(Bar)` or `target_link_libraries(Foo PRIVATE Bar::Bar)` here!
install_basic_package_files(Foo
    COMPATIBILITY SameMajorVersion
    DEPENDENCIES
        "Bar") # not actually available on this system

That would indeed break my check. But is this really a sensible scenario? It would also lead to inconsistency; consuming the targets of Foo through add_subdirectory() would cause Foo to have transitive dependencies different from what one would get through find_package().

Unfortunately it is, see YARP_eigen and YARP_cv for example.

---

I agree with @traversaro that add_test() should not silently be called by install_basic_package_files() itself. But add_test() is probably fine if we put the check in a separate function.

👍 to this, I'm in favour of having it in a separate module, using add_test()

---

Why do you dislike generating a custom target for the check? CMake already generates some custom targets by itself (e.g. list_install_components, edit_cache, cmake_check_build_system), so I don't think this is necessarily inappropriate.

Because almost nobody knows about them, and they are not used by anyone, and if you have lots of targets it's hard to notice them. Also if you have tests in your program it doesn't make sense to have some checks run by make test and some other run by make more_tests.

---

But install_basic_package_files() used to have a TARGETS argument which was deprecated in favor of EXPORTS (I think). It would feel odd to introduce a companion function add_package_test() which expects a TARGETS argument.

But I was actually thinking to split it in one command to check a single target, and eventually some other command that checks the export calling the first command for each target in the export. This could give you some more flexibility and allow to skip tests that require extra dependencies not available on the system at build time.

Adding also the header to include, it could become a good test to ensure that each target will work.

[mbeutel]

That would indeed break my check. But is this really a sensible scenario? It would also lead to inconsistency; consuming the targets of Foo through add_subdirectory() would cause Foo to have transitive dependencies different from what one would get through find_package().

Unfortunately it is, see YARP_eigen and YARP_cv for example.

I agree with @traversaro that add_test() should not silently be called by install_basic_package_files() itself. But add_test() is probably fine if we put the check in a separate function.

👍 to this, I'm in favour of having it in a separate module, using add_test()

So a separate module it is. I'll submit a new PR.

But install_basic_package_files() used to have a TARGETS argument which was deprecated in favor of EXPORTS (I think). It would feel odd to introduce a companion function add_package_test() which expects a TARGETS argument.

But I was actually thinking to split it in one command to check a single target, and eventually some other command that checks the export calling the first command for each target in the export. This could give you some more flexibility and allow to skip tests that require extra dependencies not available on the system at build time.

Adding also the header to include, it could become a good test to ensure that each target will work.

Perhaps like this?

add_package_test(<Name>
    [EXPORT <export> | TARGETS <target>...] # defaults to EXPORT <name>
    [INCLUDE <header>...])

Then I could say

add_package_test(Foo)

to generate a config-only test that just checks whether all CMake targets are available (i.e. what my current PR does, but under a dedicated test named, say, check_package_dependencies-Foo), or

add_package_test(Foo
    TARGETS Foo::Foo
    INCLUDE <foo/foo.hpp>)

to additionally generate code that #includes <foo/foo.hpp> and to build the target as well (which only makes sense if there is at least one header to include).

Would you prefer INCLUDE or HEADERS?

[drdanz]

Looks good, but I just had an idea, what do you think about using the PUBLIC_HEADERS property instead of the HEADER option to test additionally that all the headers are self contained?

[mbeutel]

Looks good, but I just had an idea, what do you think about using the PUBLIC_HEADERS property instead of the HEADER option to test additionally that all the headers are self contained?

What would that test entail? Would it just test that a header includes all its dependencies, i.e. that compiling a source file consisting of #include <the/header> succeeds? Because I don't think this is related to packaging; regular unit tests should cover this.

I've thought about add_package_test() a bit more, and I'm not sure I see any benefits in testing headers. Let's talk about some potential errors that might be caught by add_package_test():

• First, the original example: my exported target has a package and target dependency, but I forgot to add that dependency to install_basic_package_files(). This can be detected by configuring (not building) a test project, which has a one-time cost (CMake cache inialization for the subproject) and negligible running costs (re-running CMake configure is typically very fast).

• Another source of errors is if any of the install steps for headers or libraries is missing:

  target_include_directories(foo
      PUBLIC
          "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>"
          "$<INSTALL_INTERFACE:include>") # <-- (1)
  
  install(DIRECTORY "${PROJECT_SOURCE_DIR}/include/" TYPE INCLUDE) # <-- (2)
  
  install(TARGETS foo EXPORT foo INCLUDES ARCHIVE LIBRARY RUNTIME) # <-- (3)

  If I forgot any of (1), (2), or (3), I won't notice during development. My current idea of add_package_test() wouldn't notice either because its find_package() finds the build directory. To detect these errors we would have to make install the project to some location first and then run the test as post-install action.

• Some of our public headers might forget to include their dependencies:

  // foo/foo.hpp
  std::string frobnicate(std::string const& s);
  
  // bar.cpp
  #include <string>
  #include <foo/foo.hpp> // happens to work because <string> was included before
  
  // baz.cpp
  #include <foo/foo.hpp> // error

  While I agree that it would be nice to prevent this with automated tests, I'd argue that this is not related to packaging and probably should go in a dedicated function. I'll also note that this test is rather expensive because one source file would need to be built for every public header. And we cannot speed it up with a PCH because that would defeat the purpose of the test.

Which other errors would be prevented by having add_package_test() compile source files which #include some of the target's header files?