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 #1942, add missing inclusions in CFE API headers #1943

Merged

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Sep 9, 2021

Describe the contribution
Some CFE API headers were missing dependency inclusions, where the header was referencing a type or symbol but not directly including the header file that provides that type/symbol.

Adding the dependent include allows the headers to work more consistently.

Fixes #1942

Testing performed
Build and sanity check CFE

Expected behavior changes
None for framework config (these inclusions were already satisfied in existing use cases)
But fixes a broken build for external code that only included these headers directly.

System(s) tested on
Ubuntu

Additional context
This type of thing was intended to be caught by the "headercheck" concept in the module code, but that is currently not enabled/enforced in the build.

There theoretically could be other instances of stuff like this, but these are the only two noted so far.

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

Some CFE API headers were missing dependency inclusions, where the header
was referencing a type or symbol but not directly including the header
file that provides that type/symbol.

Adding the dependent include allows the headers to work more consistently.
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Sep 9, 2021
@jphickey
Copy link
Contributor Author

jphickey commented Sep 9, 2021

This is the type of stuff I'd suggest to queue up for a "caelum.1" release ... given that caelum is already tagged or soon to be. probably will continue to slowly find other gotchas as the code is used for more stuff by more users.

@astrogeco
Copy link
Contributor

Is there a way to enforce the "headercheck" concept in CI?

@skliper skliper added this to the cFS-Draco milestone Sep 10, 2021
@astrogeco astrogeco added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Sep 15, 2021
@astrogeco
Copy link
Contributor

CCB:2021-09-15 APPROVED

  • Explore using cfe_app_check_intf from modules/CMakeLists.txt in continuous-integration checks

@astrogeco astrogeco changed the base branch from main to integration-candidate September 16, 2021 20:43
@astrogeco
Copy link
Contributor

@skliper can we move this to Caelum?

@skliper skliper modified the milestones: cFS-Draco, 7.0.0 Sep 21, 2021
@skliper
Copy link
Contributor

skliper commented Sep 21, 2021

@astrogeco Absolutely! Moved to Caelum.

@astrogeco astrogeco merged commit b6928b2 into nasa:integration-candidate Sep 21, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Sep 21, 2021
astrogeco added a commit to astrogeco/cFS that referenced this pull request Sep 22, 2021
**Combines**

nasa/cFE#1967 v6.8.0-rc1+dev1024

nasa/osal#1158 v5.1.0-rc1+dev619

nasa/sch_lab#92 v2.4.0-rc1+dev53

nasa/sample_app#155 v1.2.0-rc1+dev73

nasa/to_lab#105 v2.4.0-rc1+dev58

nasa/ci_lab#93 v2.4.0-rc1+dev46

**Includes:**

*cFE*

nasa/cFE#1948, Update name of macros/functions added to "cfe_assert"

nasa/cFE#1950, Fix Mismatch between MSG API and test case in "TestMsgId" functional test

nasa/cFE#1962, Single time domain in functional time tests

nasa/cFE#1943, add missing inclusions in CFE API headers

nasa/cFE#1964, Use existing /ram for FS header test

nasa/cFE#1956, Add static local to function test so data section is nonzero

nasa/cFE#1960, Make invalid buffer length consistent in es task test

nasa/cFE#1953, Only check base filename in library info functional

nasa/cFE#1970, Confirm sb/time reset requirements in coverage test

nasa/cFE#1947, Fix broken link in App Developers Guide

nasa/cFE#1972, Fix #1971, avoid alias warning on some compilers

*osal*

nas/osal#1149, Enable symbol api test and MIR dump too large

nasa/osal#1152, Fix OSAL loader test hard fails if size exceeded

nasa/osal#1155, add bsp-specific configuration flag registry

nasa/osal#1157, Add os-specifc socket flag function

*Apps*

nasa/ci_lab#92, Apply CFE_SB_ValueToMsgId where required

nasa/sample_app#154, Apply CFE_SB_ValueToMsgId where required

nasa/sch_lab#91, Apply CFE_SB_ValueToMsgId where required

nasa/to_lab#104, Apply CFE_SB_ValueToMsgId where required

Co-authored-by: Jacob Hageman   <[email protected]>
Co-authored-by: Joseph Hickey   <[email protected]>
Co-authored-by: Adrien Chardon  <[email protected]>
astrogeco added a commit to nasa/cFS that referenced this pull request Sep 22, 2021
**Combines**

nasa/cFE#1967 v6.8.0-rc1+dev1024

nasa/osal#1158 v5.1.0-rc1+dev619

nasa/sch_lab#92 v2.4.0-rc1+dev53

nasa/sample_app#155 v1.2.0-rc1+dev73

nasa/to_lab#105 v2.4.0-rc1+dev58

nasa/ci_lab#93 v2.4.0-rc1+dev46

**Includes:**

*cFE*

nasa/cFE#1948, Update name of macros/functions added to "cfe_assert"

nasa/cFE#1950, Fix Mismatch between MSG API and test case in "TestMsgId" functional test

nasa/cFE#1962, Single time domain in functional time tests

nasa/cFE#1943, add missing inclusions in CFE API headers

nasa/cFE#1964, Use existing /ram for FS header test

nasa/cFE#1956, Add static local to function test so data section is nonzero

nasa/cFE#1960, Make invalid buffer length consistent in es task test

nasa/cFE#1953, Only check base filename in library info functional

nasa/cFE#1970, Confirm sb/time reset requirements in coverage test

nasa/cFE#1947, Fix broken link in App Developers Guide

nasa/cFE#1972, Fix #1971, avoid alias warning on some compilers

*osal*

nas/osal#1149, Enable symbol api test and MIR dump too large

nasa/osal#1152, Fix OSAL loader test hard fails if size exceeded

nasa/osal#1155, add bsp-specific configuration flag registry

nasa/osal#1157, Add os-specifc socket flag function

*Apps*

nasa/ci_lab#92, Apply CFE_SB_ValueToMsgId where required

nasa/sample_app#154, Apply CFE_SB_ValueToMsgId where required

nasa/sch_lab#91, Apply CFE_SB_ValueToMsgId where required

nasa/to_lab#104, Apply CFE_SB_ValueToMsgId where required

Co-authored-by: Jacob Hageman   <[email protected]>
Co-authored-by: Joseph Hickey   <[email protected]>
Co-authored-by: Adrien Chardon  <[email protected]>
@jphickey jphickey deleted the fix-1942-include-dependencies branch September 22, 2021 20:03
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
Development

Successfully merging this pull request may close these issues.

Missing some dependency include files in public API headers
4 participants