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

Testcase code should not directly include "cfe_test_msgids.h" (yet) #2395

Closed
jphickey opened this issue Jul 14, 2023 · 6 comments · Fixed by #2399 or #2404
Closed

Testcase code should not directly include "cfe_test_msgids.h" (yet) #2395

jphickey opened this issue Jul 14, 2023 · 6 comments · Fixed by #2399 or #2404
Assignees
Milestone

Comments

@jphickey
Copy link
Contributor

Describe the bug
The module-specific header file is a relatively new addition. But for a user that has overridden the previous all-inclusive cfe_msgids.h file, including the test_msgids.h directly will cause conflict with the overrides.

To Reproduce
Override (only) cfe_msgids.h in a build, and change the msgids in here. Do not override cfe_test_msgids.h.
Attempt to build cfe_testcase -- the cfe_test_msgids.h inclusion will bypass the modified msgids, and get defaults, which will conflict / mismatch.

Expected behavior
Should be backward compatible with an existing override of cfe_msgids.h

System observed on:
Debian

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey self-assigned this Jul 14, 2023
jphickey added a commit that referenced this issue Jul 14, 2023
For a user that has customized cfe_msgids.h, this will get conflicting
values.  If/when users have migrated to module-specific msgid files,
this will be OK, but for now this can break things.
jphickey added a commit to jphickey/cFE that referenced this issue Jul 14, 2023
For a user that has customized cfe_msgids.h, this will get conflicting
values.  If/when users have migrated to module-specific msgid files,
this will be OK, but for now this can break things.
dzbaker added a commit that referenced this issue Jul 17, 2023
Fix #2395, do not directly use cfe_test_msgids.h
@dzbaker dzbaker reopened this Jul 17, 2023
@dzbaker dzbaker linked a pull request Jul 17, 2023 that will close this issue
dzbaker added a commit that referenced this issue Jul 17, 2023
Revert "Fix #2395, do not directly use cfe_test_msgids.h"
dzbaker added a commit to nasa/cFS that referenced this issue Jul 17, 2023
@skliper skliper reopened this Jul 28, 2023
@skliper skliper added this to the Equuleus milestone Jul 28, 2023
@skliper
Copy link
Contributor

skliper commented Jul 28, 2023

Reopening w/ Dan's concurrence, this really should be fixed. Another option is for the modules override search to accept a top level *_msgids.h... then it wouldn't matter since all the generated wrappers would pull in the right file.

Basically it currently searches for:

*_defs/cfe_test_msgids.h
*_defs/default_cfe_test_msgids.h
*_defs/cpu1_cfe_test_msgids.h
*_defs/cpu1/cfe_test_msgids.h
*_defs/cpu1/default_cfe_test_msgids.h
*_defs/default/cfe_test_msgids.h
*_defs/default/cpu1_cfe_test_msgids.h
*_defs/config/cfe_test_msgids.h
*_defs/config/default_cfe_test_msgids.h
*_defs/config/cpu1_cfe_test_msgids.h
*_defs/cpu1/config/cfe_test_msgids.h
*_defs/cpu1/config/default_cfe_test_msgids.h
*_defs/default/config/cfe_test_msgids.h
*_defs/default/config/cpu1_cfe_test_msgids.h*_defs/cfe_test_msgids.h

but the first check should be for a global msgid override (*_defs/cpu1_msgids.h is what we use). Similar pattern for performance ids. Related to

@jphickey
Copy link
Contributor Author

jphickey commented Aug 1, 2023

@skliper -- Is there a specific problem with your current msgid file? It should still find (and use) a file called cpu1_msgids.h and use that for cfe_msgids.h on cpu1. Can you confirm the content of "cfe_msgids.h" in your build is still as you expected it to be?

And yes - cfe_test_msgids.h (along with all the others) will still be generated. For now, they should be ignored -- nothing includes them, if not using the default msgids.

I will look again into some method of (possibly) avoiding the confusion of generating a file that is not used. But other than that potentially-confusing aspect - is something broken here?

@skliper
Copy link
Contributor

skliper commented Aug 1, 2023

@jphickey - the test code directly references cfe_test_msgids.h, this is what's broken for us (if it was ignored we'd be fine). As this issue states, the test code should not directly include cfe_test_msgids.h if you want to maintain backwards compatibility.

We are currently maintaining a work around by managing an override cfe_test_msgids.h in our *_defs (as just a link to cpu1_msgids.h).

@skliper
Copy link
Contributor

skliper commented Sep 1, 2023

@dzbaker Recommend closing this in favor of #2405 due to the issues this causes per comment on #2404 about breaking builds that don't include cfe_testcase.

#2404 (comment)

@skliper
Copy link
Contributor

skliper commented Sep 1, 2023

Note that I was able to work around my issue by overriding the cfe_test_msgids.h source from target_defs/arch_build_custom.cmake:
set(TEST_CFGFILE_SRC_cfe_test_msgids ${MISSION_DEFS}/cpu1_msgids.h)

The another option would be to create a symbolic link in target_defs from cfe_test_msgids.h to cpu1_msgids.h but that was less desireable.

@dzbaker
Copy link
Collaborator

dzbaker commented Sep 5, 2023

Superseded by #2405.

@dzbaker dzbaker closed this as completed Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants