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.

Sign up for GitHub

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

Jump to bottom

Fix #581, Propagate the OSAL compile definitions to CFE build #585

Merged
merged 2 commits into from
Apr 14, 2020
Merged

Fix #581, Propagate the OSAL compile definitions to CFE build #585

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60a5f65
Merge pull request #586 from nasa/integration-candidate
astrogeco Apr 9, 2020
436bc4b
Fix #581, Propagate the OSAL compile definitions to CFE build
jphickey Apr 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions cmake/arch_build.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,20 @@ function(process_arch SYSVAR)
include_directories(${MISSION_SOURCE_DIR}/cfe/fsw/cfe-core/src/inc)
include_directories(${MISSION_SOURCE_DIR}/cfe/cmake/target/inc)

# propagate any OSAL interface compile definitions and include directories to this build
# 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

target_* with PUBLIC and INTERFACE specifiers are designed to propagate corresponding definitions to their user targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@stanislaw stanislaw Apr 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #626 to track

get_target_property(OSAL_INCLUDE_DIRECTORIES osal INTERFACE_INCLUDE_DIRECTORIES)

if (OSAL_COMPILE_DEFINITIONS)
set_directory_properties(PROPERTIES COMPILE_DEFINITIONS "${OSAL_COMPILE_DEFINITIONS}")
endif (OSAL_COMPILE_DEFINITIONS)
if (OSAL_INCLUDE_DIRECTORIES)
include_directories(${OSAL_INCLUDE_DIRECTORIES})
endif (OSAL_INCLUDE_DIRECTORIES)

# Append the PSP and OSAL selections to the Doxyfile so it will be included
# in the generated documentation automatically.
# Also extract the "-D" options within CFLAGS and inform Doxygen about these
Expand Down