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

Fix #746, CMake mission dependency cleanup #751

Merged

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Jun 19, 2020

Describe the contribution

Put more dependencies into the "mission_defaults.cmake" file for more visibility and ease of configuration. This now includes all "implicit" modules such as cfe-core, osal, and psp.

Also push the calls to "generate_config_includefile" to a sub-script which can be distributed with each app and evaluated as part of the build. This reduces dependencies on special naming conventions like fsw/mission_inc and fsw/platform_inc, and app developers can explicitly manage the files that users are expected to override.

Fixes #746

Testing performed
Build and execute CFE and sanity test
Build and execute all unit tests

Expected behavior changes
No impact to behavior, affects build system only.
Should be fully backward compatible - in that the defaults are applied if a user has nothing specifically configured in their targets.cmake file. The defaults will select osal, cfe-core, and psp as they always have. However, the user now has the option to explicitly configure/control the inclusion of these modules and also provide mission-specific search paths to override them as desired.

System(s) tested on
Ubuntu 20.04

Additional context
Related to previous PR #720, #728, and #740. Not easy to separate this out from those so this is currently based on a merge of those previous changes. Hopefully the combined solution can be acceptable.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey
Copy link
Contributor Author

Note - only commit c80294d is relevant here to this PR.

If acceptable then all three of these can be rebased and merged together.

Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

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

I like it. Definitely moving in the right direction!

@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Jun 24, 2020
@astrogeco astrogeco added CCB-20200624 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Jun 24, 2020
@astrogeco
Copy link
Contributor

CCB-20200624: APPROVED. Will open an issue to document the cmake build system.

@skliper
Copy link
Contributor

skliper commented Jun 25, 2020

I'd like to start using this change ASAP, is it ready? Can it be merged to IC soon or should I pull it into my work at risk?

@skliper
Copy link
Contributor

skliper commented Jun 25, 2020

Basically I want to try out the generate_config_includefile to facilitate the message module header redefinition, to avoid multiple header files of the same name in the search paths since it's prone to error/confusion.

@jphickey
Copy link
Contributor Author

It's ready just need to decide how to handle the baseline/merge ... one option is to rebase (and close) the previous #728 and #740 and put everything into this PR. Should I do that?

@skliper
Copy link
Contributor

skliper commented Jun 25, 2020

I'm good with that approach...

Add more hooks for additional flexibility when adding
modular code blobs into the build.

Three new directives are added:

MISSION_CORE_MODULES, for modular components which are
direct dependencies of CFE core and/or extend its functionality.

MISSION_GLOBAL_APPLIST, for applications/libraries which
should be built for every target, as if they were listed
in every TGTx_APPLIST setting.

MISSION_GLOBAL_STATIC_APPLIST, same as above but for the
TGTx_STATIC_APPLIST setting.

This also simplifies/reworks the search path to remove
some logic that was never really utilized.
The existing build system built target executables grouped by toolchain
as a proxy for CPU architecture + machine options/flags.  The app binaries
would be built once and copied to any/all targets sharing that toolchain.

The side effect of doing this is that the application needs to be written
in an CPU-agnostic manner, performing its subscriptions and configurations
from runtime table data rather than hardcoded/fixed values.  Unfortunately
most apps are not coded that way, so workarounds were needed.

This changes the top level process to include the "platform" within this
target build logic, effectively treating different platform configs as
entirely different builds, even if they share the same toolchain file.

As a result, binaries will only be shared between targets that explicitly
set the "TGTx_PLATFORM" setting in targets.cmake to the same value.
Put more dependencies into the "mission_defaults.cmake" file for
more visibility and ease of configuration.  This now includes all
"implicit" modules such as cfe-core, osal, and psp.

Also push the calls to "generate_config_includefile" to a sub-script
which can be distributed with each app and evaluated as part of the
build.  This reduces dependencies on special naming conventions like
"fsw/mission_inc" and "fsw/platform_inc", and apps can explicitly manage
the files that users are expected to override.
@jphickey
Copy link
Contributor Author

Commit 09cbe08 rebases this change and all dependencies to latest master, should be just a simple merge, ready to go now. This supersedes the two previous PRs (will close).

@jphickey jphickey marked this pull request as ready for review June 26, 2020 02:45
@astrogeco astrogeco added the CCB:Approved Indicates code review and approval by community CCB label Jul 1, 2020
@astrogeco astrogeco changed the base branch from master to integration-candidate July 1, 2020 13:49
@astrogeco astrogeco merged commit b764b05 into nasa:integration-candidate Jul 1, 2020
@skliper
Copy link
Contributor

skliper commented Jul 2, 2020

Did this break how native_osconfig.cmake (in sample_defs) gets appended to osconfig.h after processing default_osconfig.cmake? I just saw a startup error... haven't dug into it much but the generated file doesn't seem to have the additional line from the native file.

@skliper
Copy link
Contributor

skliper commented Jul 2, 2020

Note I moved set(OSAL_CONFIG_DEBUG_PERMISSIVE_MODE TRUE) into default_osconfig.cmake as a quick fix and it worked.

@jphickey
Copy link
Contributor Author

jphickey commented Jul 2, 2020

Yep, I pushed a patch in commit ab99578 to fix. Needed to treat TARGETSYSTEM as a list, I hadn't noticed because - for other reasons - I had my default config set to permissive.

@skliper skliper linked an issue Jul 27, 2020 that may be closed by this pull request
@skliper skliper added this to the 6.8.0 milestone Aug 21, 2020
@jphickey jphickey deleted the fix-746-mission-deps-cleanup branch October 14, 2020 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
3 participants