Fix #581, Propagate the OSAL compile definitions to CFE build #585
Conversation
jphickey
added
the
CCB:Ready
|
CCB 20200408 - APPROVED |
astrogeco
added
CCB - 20200408
CCB:Approved
Integration Candidate - 2020-04-01
Use the INTERFACE_COMPILE_DEFINITIONS and INTERFACE_INCLUDE_DIRECTORIES properties from the osal target and apply them to the entire CFE build. At this time, the OSAL library build does not use/export these properties so this is effectively a no-op for the CFE build and can be merged with no effect. However, in a future version, the OSAL library will export these interface properties and this will become important.
skliper
force-pushed
the
fix-581-osal-cflag-propagation
branch
from
88997eb
to
436bc4b
Compare
|
Rebased on master so it can be tested with the bundle |
| # This is set as a directory property here at the top level so it will apply to all code. | ||
| # This includes MODULE libraries that do not directly/statically link with OSAL but still | ||
| # should be compiled with these flags. | ||
| get_target_property(OSAL_COMPILE_DEFINITIONS osal INTERFACE_COMPILE_DEFINITIONS) |
There was a problem hiding this comment.
I am not sure if I understand the intention of this right but it looks like you could simply control this by specifying in the osal target's CMake file:
target_include_directories(osal
PUBLIC
...
INTERFACE
...
PRIVATE
...
)
and the same with target_compile_definitions.
There was a problem hiding this comment.
target_* with PUBLIC and INTERFACE specifiers are designed to propagate corresponding definitions to their user targets.
There was a problem hiding this comment.
That's exactly what the OSAL CMakeLists.txt file is doing in the upcoming update. However, it only works "implicitly" for targets which directly link with OSAL. For shared library/module targets that do not directly link with OSAL but yet still need to use the headers and a compatible set of definitions, this is why it needs to be done explicitly.
Unless there is another way of doing this for MODULE libraries?
There was a problem hiding this comment.
After your explanation I understand it better now.
There are also interface libraries: you could collect the needed flags and definitions of osal and attach them to an interface library, for example osal-interface. The library/module targets could then link to the interface library instead of linking to osal directly.
But I am not sure if this will simplify existing setup.
https://cmake.org/cmake/help/v3.0/command/add_library.html (INTERFACE)
There was a problem hiding this comment.
Interface libraries are an interesting option... this might actually be a possibility for simplifying the way OSAL manages the flags internally, then we can just put this inside add_cfe_app(). Will not have time to revisit this in the near term however, so hopefully this approach is good enough for now.
Describe the contribution
Use the
INTERFACE_COMPILE_DEFINITIONSandINTERFACE_INCLUDE_DIRECTORIESproperties from the osal target and apply them to the entire CFE build as a directory-scope property.At this time, the OSAL library build does not use/export these properties so this is effectively a no-op for the CFE build and can be merged with no effect. However, in a future version (specifically after nasa/osal#312 gets fixed), the OSAL library will export these interface properties and this will become important.
Fixes #581
Testing performed
Built for all three test platforms (ppc-vxworks6.9, i686-rtems4.11, native/x86-64 Linux). Sanity Check CFE build boots and runs, unit tests run.
Expected behavior changes
No impact to behavior - build is identical because these properties are currently empty/not set in the current OSAL build.
System(s) tested on
Ubuntu 18.04 LTS 64 bit.
Additional context
Also locally tested against a version of OSAL+PSP that includes all the latest proposed changes and confirm that this still works.
Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.