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 #2

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-SomeIP require users to add the ${COMMONAPI_SOMEIP_INCLUDE_DIRS} variable to their project's include directories. This was done to support building against a build tree of CommonAPI-SomeIP by generating two different
CommonAPI-SomeIPConfig.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 CommonAPI-SomeIPConfig.cmake file.

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

target_link_libraries(${target} CommonAPI-SomeIP)

since the exported CommonAPI-SomeIP 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, the exported target required users to manually find a copy of vSomeIP and link against vSomeIP as well as against CommonAPI-SomeIP rather than automatically locating the vSomeIP dependency. Modify the target to also do this step automatically so that users of CommonAPI-SomeIP only need to care about the CommonAPI-SomeIP target.

This enables downstream projects that build CommonAPI interface libraries to no longer care about what the CommonAPI-SomeIP 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.

@gunnarx
Copy link

gunnarx commented Nov 1, 2018

Correct spelling is "publicly" FYI. You still have time to change it ;-)

@neverpanic
Copy link
Author

Will do, thanks for the hint.

Clemens Lang added 2 commits February 11, 2019 16:24
The headers for CommonAPI-SomeIP include vSomeIP headers. Consequently,
compiling against CommonAPI-SomeIP will also need the vSomeIP include
directories added to the include search path, which means
CommonAPI-SomeIP should not have vsomeip in its Requires.private field,
but in Requires.

Signed-off-by: Clemens Lang <[email protected]>
The CMake configuration files exported by CommonAPI-SomeIP require users
to add the ${COMMONAPI_SOMEIP_INCLUDE_DIRS} variable to their project's
include directories. This was done to support building against a build
tree of CommonAPI-SomeIP by generating two different
CommonAPI-SomeIPConfig.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
CommonAPI-SomeIPConfig.cmake file.

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

| target_link_libraries(${target} CommonAPI-SomeIP)

since the exported CommonAPI-SomeIP 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, the exported target required users to manually find a copy
of vSomeIP and link against vSomeIP as well as against CommonAPI-SomeIP
rather than automatically locating the vSomeIP dependency. Modify the
target to also do this step automatically so that users of
CommonAPI-SomeIP only need to care about the CommonAPI-SomeIP target.

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

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

Typo now fixed.

@derived-coder
Copy link

Why is this not merged?
@neverpanic
Can you also provide a fix for:
https://github.com/GENIVI/vsomeip ?

@neverpanic
Copy link
Author

@derived-coder I don't know. Internally, all ECU projects I'm aware of are shipping this patch for a couple of months now, so it's certainly well-tested by now.

There is a corresponding PR for vsomeip, btw: COVESA/vsomeip#41. I guess Lutz hasn't gotten around to publishing version 3.x yet.

@derived-coder
Copy link

@neverpanic
Thanks.
I guess all genivi projects don't use modern cmake atm.
Here I saw there is no PR open from you: https://github.com/GENIVI/dlt-daemon
Or do you have also one?

@neverpanic
Copy link
Author

@derived-coder I didn't make one for dlt, since it didn't have the problem of breaking dependent CMake configuration files similar to what the CAPI runtimes did (we have tooling to turn fidl and fdepl files into libraries rather than compiling them directly into your application, but in order to write CMake configurations for these libraries, the changes in the PRs I opened were required for proper cross-compiling; the same problem didn't apply for DLT).

Feel free to open a corresponding PR for DLT, though. Let me know if you do, I might be able to poke people to get that merged, or help with testing.

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