uptane / aktualizr

C++ Uptane Client
Mozilla Public License 2.0
15 stars 15 forks source link

Exporting cmake configuration script #104

Open EddyGharib opened 7 months ago

EddyGharib commented 7 months ago

This is a discussion that covers improving the user experience of external projects depending on aktualizr, including aktualizr-lite

Description

The proposal is to export an API from the aktualizr project so that it can be included from dependant projects, see the step 11 of the cmake tutorial for more information.

In short this would be enabling the dependent projects of using find_package(aktualizr REQUIRED) instead of including it as a git submodule, which would have the following advantages:

Proposed implementation

  1. Updating target references to solve transitive dependencies

In order to be able to export the cmake targets to an external project, these targets have to be setup correctly with the appropriate TAGET_PROPERTIES attached to them. Appropriate in the world of modern cmake means replacing the usage of cmake variables attached to a target (e.g. ${OPENSSL_INCLUDE_DIR} and ${OPENSSL_LIBRARIES}) with the corresponding target exported from the find_package'd package (like OpenSSL::SSL). This is important as it solves build time problems where the building environment of aktualizr and it's dependent project (e.g. aktualizr-lite) are different.

For example, let's consider both projects (aktualizr-lite depending on aktualizr) are built with yocto, each with a different recipe. yocto generates a sysroot per recipe when building, and openssl on the aktualizr sysroot would point to ${aktualizr_sysroot}/usr/lib/openssl.so. If ${OPENSSL_LIBRARIES} is attached as a cmake property to the aktualizr_lib target, exporting this target from the project would propagate the ${OPENSSL_LIBRARIES} entry within it's properties to the parent project (aktualizr-lite). Now the upstream project would be trying to find ${aktualizr_sysroot}/usr/lib/openssl.so when being built, however this induces a race condition since the ${aktualizr_sysroot} is safe to be deleted in the yocto context whenever the build process of aktualize finished.

An alternative to this behavior would be using OpenSSL::SSL as a publicly linked library to the aktualizr_lib, this way the target will be re-resolved in the parent project when stumbled upon. To help with the resolution of such a target, a find_dependency(OpenSSL) call should be added to the end of the aktualizrConfig.cmake file exported by this project. This line would be executed when find_package(aktualizr REQUIRED) is called from the parent project, and in turns define the OpenSSL::SSL target

  1. Adaptating the FindModules.cmake scripts defined in the project, and exporting them for transitive dependencies

Ideally, each project should export it's own Config.cmake scripts, but for projects not built with cmake, or old styled cmake projects, these scripts are not exported. This requires parent projects that are in need for such libraries to define their own FindModules.cmake scripts like in ./cmake-modules. However, if these find scripts do not define targets, the same problem mentioned in point 1 would appear, and therefore updating the modules is required to be able to export the project. That can be easily applied with the following declarations in these scripts

set_target_properties(libp11 PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${LIBP11_INCLUDE_DIR}")
set_target_properties(libp11 PROPERTIES IMPORTED_LOCATION "${LIBP11_LIBRARY}")
set_target_properties(libp11 PROPERTIES INTERFACE_LINK_LIBRARIES "${LIBP11_LIBRARIES}")

Also, these scripts are helpful for parent projects, so they can be also exported with the config.cmake file, and referenced from it, so that each project depending on aktualizr would have them available.

  1. Distinction between public and private headers

headers are used to reduce duplicate declarations in source files, but these headers may or may not be exported by the active project so that parent projects can include and use them. When deciding to install aktualizr (whether on the system or in a local test directory), a decision should be made about which headers are, and which are not exported by the project. A common pattern would be placing the private headers in the src/ directory of the project, and public headers in the include directory of the project. Then on installation time the include folder can be directly installed as is to the target location. The proposal here is exposing additional headers to the public api, like logging/logging.h and offline/client.h to the public interface, so that external projects can use it. Here we can reference the aktualizr-lite project that requires the aforementioned headers (see here for reference)

We compiled a list of headers that serve great with the current state of aktualizr-lite project. Ideally not each one of these should be exposed, but there were too many transitive dependencies within the project, which induced the following list:

./types.h
./packagemanagerinterface.h
./campaign.h
./secondaryinterface.h
./http/httpinterface.h
./packagemanagerfactory.h
./events.h
./secondary_provider.h
./json/json.h
./config.h
./uptane/tuf.h
./uptane/uptanerepository.h
./uptane/fetcher.h
./uptane/imagerepository.h
./logging/logging.h
./crypto/keymanager.h
./results.h
./aktualizr.h
./utilities/apiqueue.h
./utilities/utils.h
./package_manager/ostreemanager.h

In addition to that, these headers should be pushed to a directory matching the name of project, not to conflict with other headers from different installations or different projects. Therefore changes to the references of these headers should be made in the source code to push this namespace

All these changes are already performed in the following patch

0000-updates.patch

This can serve as an example that shows the amount of changes needed to perform such feature, the patch attached does not cover all the tests yet, but we are willing to compile it into a PR with all the fixes included if the proposed changes make sense from a project perspective

pattivacek commented 6 months ago

This looks promising to me! I think the API and cmake usage have significant room for improvement. My current employer also uses a custom patch to install additional headers for reuse, although it's amusingly a quite different list than yours. I'm not sure how best to handle this. I suppose some of these headers should be refactored (and split up) to keep the public parts to a minimum.