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

cmake: Modernize exported CMake config files #10

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

Conversation

neverpanic
Copy link

The CMake configuration files exported by CommonAPI require users to add the ${COMMONAPI_INCLUDE_DIRS} variable to their project's include directories. This was done to support building against a build tree of CommonAPI by generating two different CommonAPIConfig.cmake files that defined this variable with different values.

This is now no longer necessary since CMake supports the $<BUILD_INTERFACE> and $<INSTALL_INTERFACE> generator expressions which can be used to make the same distinction with only a single CommonAPIConfig.cmake file.

This also allows users of CommonAPI to get the required include directories into their build by using

target_link_libraries(${target} CommonAPI)

since the exported CommonAPI CMake target now has the required include directories set as a property. This is much more idiomatic for users of CMake and is thus preferred.

Additionally, this enables downstream projects that build CommonAPI interface libraries to no longer care about what the CommonAPI include directories are, which simplifies writing CMake config files for them.

I've locally compiled vSomeIP, CommonAPI, CommonAPI-SomeIP and a test project using CommonAPI-SomeIP to test this.

Clemens Lang added 2 commits February 11, 2019 16:17
Signed-off-by: Clemens Lang <[email protected]>
The CMake configuration files exported by CommonAPI require users to add
the ${COMMONAPI_INCLUDE_DIRS} variable to their project's include
directories. This was done to support building against a build tree of
CommonAPI by generating two different CommonAPIConfig.cmake files that
defined this variable with different values.

This is now no longer necessary since CMake supports the
$<BUILD_INTERFACE> and $<INSTALL_INTERFACE> generator expressions which
can be used to make the same distinction with only a single
CommonAPIConfig.cmake file.

This also allows users of CommonAPI to get the required include
directories into their build by using

| target_link_libraries(${target} CommonAPI)

since the exported CommonAPI CMake target now has the required include
directories set as a property. This is much more idiomatic for users of
CMake and is thus preferred.

Additionally, this enables downstream projects that build CommonAPI
interface libraries to no longer care about what the CommonAPI include
directories are, which simplifies writing CMake config files for them.

Signed-off-by: Clemens Lang <[email protected]>
@derived-coder
Copy link

derived-coder commented Apr 14, 2020

@neverpanic The dlt-daemon should use modern cmake too, so we got the transitive dependency here. Only linking against CommonAPI and automatically linking against dlt-daemon.
Do you have a patch for this too?

One addition:
I would be good, to link against like this, via target alias,
target_link_libraries(${target} CommonAPI::CommonAPI)
so we would get a cmake config error, when it is not available.

@kheaactua
Copy link

I like this PR. I'm also working on a similar PR to modernize the vsomeip CMakeList files.

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.

3 participants