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

Clean up remaining deprecated/obsolete features used by the CMake scripts #1289

Closed
jphickey opened this issue Sep 14, 2022 · 0 comments · Fixed by #1291
Closed

Clean up remaining deprecated/obsolete features used by the CMake scripts #1289

jphickey opened this issue Sep 14, 2022 · 0 comments · Fixed by #1291

Comments

@jphickey
Copy link
Contributor

Is your feature request related to a problem? Please describe.
In recent years the CMake build has been adopting the current best-practices whenever feasible to do so, and moving away from features that have been marked as obsolete or deprecated.

  • Use interface properties to propagate compiler flags, definitions, etc to dependencies, rather than setting "magic variables" that need to be known by the dependency
  • Prefer target_include_directories as opposed to include_directories at directory scope
  • Prefer target_compile_definitions as opposed to add_definitions at directory scope
  • Use of target_link_libraries should include the scope keyword (PUBLIC/PRIVATE) rather than the "bare" signature (also noted by CFE user in add_cfe_app uses deprecated call to target_link_libraries cFE#2141)

Describe the solution you'd like
In particular - the following specific areas should be modernized:

  • Public API location should be an interface library:
    include_directories(${OSAL_API_INCLUDE_DIRECTORIES})
  • Management of impl-specific flags can also be done via interface library rather than manual propagation here:

    osal/CMakeLists.txt

    Lines 154 to 163 in 38559d4

    get_directory_property(OSAL_BASE_COMPILE_DEFINITIONS COMPILE_DEFINITIONS)
    get_target_property(OSAL_BSP_COMPILE_DEFINITIONS osal_${OSAL_SYSTEM_BSPTYPE}_impl INTERFACE_COMPILE_DEFINITIONS)
    set(OSAL_COMPILE_DEFINITIONS)
    if (OSAL_BASE_COMPILE_DEFINITIONS)
    list(APPEND OSAL_COMPILE_DEFINITIONS ${OSAL_BASE_COMPILE_DEFINITIONS})
    endif (OSAL_BASE_COMPILE_DEFINITIONS)
    if (OSAL_BSP_COMPILE_DEFINITIONS)
    list(APPEND OSAL_COMPILE_DEFINITIONS ${OSAL_BSP_COMPILE_DEFINITIONS})
    endif (OSAL_BSP_COMPILE_DEFINITIONS)
    set_directory_properties(PROPERTIES COMPILE_DEFINITIONS "${OSAL_COMPILE_DEFINITIONS}")
  • Use scope keyword on link libraries:
    target_link_libraries(osal osal_bsp)
  • Implement UT coverage interface library as alternative to "flag variables" exported here:

    osal/CMakeLists.txt

    Lines 354 to 355 in 38559d4

    set(UT_COVERAGE_COMPILE_FLAGS "${UT_COVERAGE_COMPILE_FLAGS}" PARENT_SCOPE)
    set(UT_COVERAGE_LINK_FLAGS "${UT_COVERAGE_LINK_FLAGS}" PARENT_SCOPE)

Additional context
It is not currently possible to propagate the UT coverage flags if OSAL and the application are in separate builds.

Using the target-scope features and interface libraries is of particular benefit to making a proper OSAL "package" that can be compiled and linked separately from the application (see #1284)

Requester Info
Joseph Hickey, Vantage Systems, Inc.

jphickey added a commit to jphickey/osal that referenced this issue Sep 23, 2022
Use target properties to define interfaces and compiler definitions
rather than referencing global variables or using "known" paths in
other modules.  This better aligns with current practices and creates
a more robust build environment that is less dependent on specific path
names existing in a given module.

This adds an "osal_public_api" interface target that contains the paths
to the public API headers as its INTERFACE_INCLUDE_DIRECTORIES and any
required compiler definitions as its INTERFACE_COMPILE_DEFINITIONS
property.  Applications should use this rather than referring to the
"${OSAL_SOURCE_DIR}/src/os/inc" include path directly.
jphickey added a commit to jphickey/osal that referenced this issue Sep 23, 2022
dzbaker added a commit that referenced this issue Oct 11, 2022
@dmknutsen dmknutsen added this to the Draco milestone Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants