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

Incorrect include directories when using add_cfe_tables outside app #2284

Closed
jphickey opened this issue Apr 6, 2023 · 8 comments · Fixed by #2299
Closed

Incorrect include directories when using add_cfe_tables outside app #2284

jphickey opened this issue Apr 6, 2023 · 8 comments · Fixed by #2299

Comments

@jphickey
Copy link
Contributor

jphickey commented Apr 6, 2023

Is your feature request related to a problem? Please describe.
If the TGTNAME variable is set when this function is called, the function does not examine the include directories of the app (via the APP_NAME parameter).

As a result, when this function is used in a custom setting to provide a mission-specific table definition, the include directories used will be different than when this function is used from the context of the app receipe itself.

Describe the solution you'd like
If APP_NAME is valid and is an app target name, then the target INCLUDE_DIRECTORIES for that app should always be included in the table build.

Currently when this function is used outside the app build context, it tries to get INCLUDE_DIRECTORIES for a "cpu1" target, which does not exist, and produces an error.

Additional context
This include directory problem only occurs when trying to use customized table files, rather than the default/example files included with apps.

Requester Info
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey self-assigned this Apr 6, 2023
@jphickey
Copy link
Contributor Author

jphickey commented Apr 6, 2023

Actually this may not be an issue in the mainline, I may need to check it further, will postpone this.

@jphickey
Copy link
Contributor Author

jphickey commented Apr 7, 2023

OK - Did a little more isolation here - there is in fact a problem with apps such as "hs" that have more than one table file.

For the framework examples like sch_lab and sample_app, the recommendation has been to name the table as same as the app, in which case the add_cfe_tables will detect this relationship and use the INCLUDE_DIRECTORIES from the app target, which works fine.

Problem occurs for apps like HS that have multiple tables, as these cannot all have the same name.

A simple solution would be to use the <APP_NAME>.<TABLE_NAME> convention in cmake - this is used elsewhere in CFE/CFS, so its not new, and I confirmed that a dot (.) is valid for CMake target names too, so it should work fine.

@skliper
Copy link
Contributor

skliper commented Apr 7, 2023

I don't follow the issue with not matching the table name. I add_cfe_tables for SC outside of the SC build, the RTS's specifically. Doesn't add_cfe_tables use the APP_NAME to figure out the relationship? I'm a little behind main, so maybe I missed a change?

@skliper
Copy link
Contributor

skliper commented Apr 7, 2023

Nevermind, read the issue closer... I get it.

@skliper
Copy link
Contributor

skliper commented Apr 7, 2023

Actually, I don't get it. Isn't this just using APP_NAME? add_cfe_table(sc rts001.c) works for me outside of SC. Why would you need the app name in both the APP_NAME field and the table name?

cFE/cmake/arch_build.cmake

Lines 208 to 217 in 7af467e

# NOTE: On newer CMake versions this should become an OBJECT library which makes this simpler.
# On older versions one may not reference the TARGET_OBJECTS property from the custom command.
# As a workaround this is built into a static library, and then the desired object is extracted
# before passing to elf2cfetbl. It is roundabout but it works.
add_library(${TABLE_LIBNAME} STATIC ${TBL_SRC})
target_link_libraries(${TABLE_LIBNAME} PRIVATE core_api)
if (TARGET ${APP_NAME})
target_include_directories(${TABLE_LIBNAME} PRIVATE $<TARGET_PROPERTY:${APP_NAME},INCLUDE_DIRECTORIES>)
target_compile_definitions(${TABLE_LIBNAME} PRIVATE $<TARGET_PROPERTY:${APP_NAME},COMPILE_DEFINITIONS>)
endif()

@jphickey
Copy link
Contributor Author

Yeah, my bad, I kinda went back and forth a bit - I was seeing something odd in some cases. I narrowed it down to the feature we added a while back to support "alternate" table files that the user can choose at deployment or target start-up time. That is, from a cpu-specific install_custom.cmake file, you can do something like:

add_cfe_tables(sample_app alt_sample_tbl.c alt2_sample_tbl.c)

In this case it will build all of these table files, and put them in the install dir, and the choice of which one to load is made later on either by choosing which one to copy to the target OR somehow telling the app which one to actually load.

It was this case where these alternate/extra tables files were not getting the correct include paths.

@jphickey
Copy link
Contributor Author

Part of (my own) confusion here is that I'm trying to isolate diffs between the EDS build and backport relevant stuff, the original issue I was seeing was in the EDS build, but some of the changes there were actually breaking stuff. I'm still working on isolating what (if anything) needs to change for the GSFC baseline build here.

@jphickey
Copy link
Contributor Author

Note that in the case of SC app, it's probably fine because SC uses the target_include_directories() as it should. But sample_app and to_lab framework apps were still relying on directory-scope include_directories here.

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.

4 participants