Fix #261, #312, and #362, OSAL build cleanup (multiple issues) #404

jphickey · 2020-04-02T20:16:48Z

Describe the contribution

General cleanup and rework of the OSAL build scripts. Fixes several issues:

Fixes #312
Fixes #261
Fixes #362
Fixes #363

These are combined into a single pull request as they each depend on the previous fix - they cannot be easily isolated into separate pull requests. However, each specific issue fix is still an individual commit for review.

Summary of changes:

Do not clobber (or generally even rely upon) the CMAKE_C_FLAGS variable, instead use the preferred approach of using the target_include_directories and target_compile_definitions functions from CMake to manage the build flags per target.
Build the implementation components using a separate CMakeLists.txt file per-directory rather than aux_source_directory. This allow allow BSP-specifc or OS-specific compiler flags to be set, where required.
Provide sufficient framework for combining the OSAL BSP, UT BSP, and the CFE PSP and eliminating the duplication/overlap between these items.
Change the OSAL Coverage Test to be built with the standard build whenever ENABLE_UNIT_TESTS is set.

Testing performed

Built CFE for all three supported platforms (native/x86-64 Linux, i686-rtems4.11, ppc-vxworks6.9) and confirm successful build.
Confirm UT passing on RTEMS 4.11 and native/x86-64 Linux
Sanity check on CFE (execute, send command) in RTEMS 4.11 and native/x86-64 Linux
Confirm standalone OSAL builds (without CFE) on basic Posix/PC-Linux combo and passes unit tests.

Expected behavior changes

OSAL coverage testing results shown in standard CFE test build (make test, etc).

System(s) tested on

Ubuntu 18.04 LTS 64-bit (native)
QEMU for testing RTEMS 4.11 builds

Additional context
VxWorks testing limited to building only at this point. Runtime tests pending on MCP750 hardware availability.

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

skliper · 2020-04-06T21:14:02Z

src/os/inc/osapi-os-core.h

@@ -1505,4 +1505,59 @@ void OS_printf_disable(void);
 void OS_printf_enable(void);
 /**@}*/

+


Curious on thoughts about these in osapi-os-core.h. I'd think they'd be candidates for a separate or different header.

Yes, these could be split into a separate header file.

Note that OSAL is "caching" the value from the BSP here, so the API really is still an OSAL API, not a BSP API. The BSP sets the cache when it boots and the API just reads it and returns it to the app. I was hoping to avoid having an actual BSP API - the goal should be to hide the BSP details as much as we can, not expose it.

FWIW, we should also consider making the inc directory structure more like other modules where we have the public API in fsw/inc and the source in fsw/src, rather than src/os/inc where OSAL has it right now. That would be a follow-on change but this may actually make it easier by removing at least some build system references to osal/src/os/inc.

Thanks, that background helps... historically I've broken includes into smaller "topics" to limit conceptual impact of changes, but I realize OSAL has an established pattern. It has paid off for me in relation to review panels and operational software support processes.

I think cFS 7.0 is due a major directory restructure... very interested in an in-depth design/review CCB discussion on the topic.

astrogeco · 2020-04-08T16:38:42Z

CCB 20200408 - Concept approved. @acudmore will review by April 13 2020

ghost · 2020-04-09T17:33:24Z

This is a nice clean up. I like the layering of the OSAL BSP with PSP. Something I have been thinking about for a while but did not quite know how to tackle.
A few Cmake novice questions:

In the top level CMakeLists.txt file line you have the add_library( osal_bsp ... ) - Could that be in a CMakeFiles.txt in the directory where the files are? ( just curious of listing files in here vs in the source directory like they are in the src/os/posix) Same applies for the add_library (osal ..) at line 163.
is the add_library ( OBJECT .. ) just used to create a collection of object files like the old make system did? In other words, not an .so or .a , but just a collection of .o files?
Will the files in osal/src/portable get a CMakeLists.txt file when they are no longer included?

Do not clobber the CMAKE_C_FLAGS value as part of the OSAL build. Instead, use target_compile_options and target_include_directories as needed to set the compile options for specific targets. This also creates a separate CMakeLists.txt file for each OS/BSP implementation library rather than using aux_source_directory. Each implementation-specific build can then set any additional options as required for that platform. Note that any entity needing to compile/link with OSAL should now obtain the requisite compile flags and directories by querying the INTERFACE_COMPILE_DEFINITIONS and INTERFACE_INCLUDE_DIRECTORIES properties on the osal library target.

Refactor the OSAL BSP code so that a single BSP implementation can work for both normal applications as well as unit tests. This intoduces a new bsp implementation abstraction layer akin to the low level OS implementation layer. This handles dealing with bootloader/command line arguments, and debug console manipluation.

Build the full suite of OSAL coverage tests as part of the normal build when ENABLE_UNIT_TESTS is true. This will use the same target OSAL BSP to execute the tests as is used for the FSW.

jphickey · 2020-04-10T02:28:53Z

Force-push: rebased to current master branch

jphickey · 2020-04-10T02:45:28Z

1. In the top level CMakeLists.txt file line you have the add_library( osal_bsp ... ) - Could that be in a CMakeFiles.txt in the directory where the files are? ( just curious of listing files in here vs in the source directory like they are in the src/os/posix) Same applies for the add_library (osal ..) at line 163.

Yes, I suppose it could be... I guess I didn't see the point in splitting those off to a separate file, because they are somewhat fixed and always used. Where the "impl" stuff has one subdirectory selected out of several choices, the "shared" stuff is always built in the same way from the same set, so splitting it into a separate subdirectory just meant yet another file to look at.

2. is the add_library ( OBJECT .. ) just used to create a collection of object files like the old make system did? In other words, not an .so or .a , but just a collection of .o files?

Yes, pretty much. an OBJECT library is just a collection of .o files, which are then brought into into the static lib (.a) using TARGET_OBJECTS expression. I had at one point done everything as .a files but ended up with some order-of-library dependency issues for the final link. This "object" approach yields fewer different .a files and less issues.

The main thing to note with OBJECT is that it was added to CMake around 2.8.8, which was an issue for RHEL5 which only shipped 2.6.4. But now that's finally EOL, so RHEL6 is probably the most-ancient tool version used in production, which has 2.8.12, so it should be fine.

3. Will the files in osal/src/portable get a CMakeLists.txt file when they are no longer included?

Not exactly, they'll be referenced from the other three implementations in the spots where they are relevant, similar to the way they #include them now. Because one wouldn't necessarily be able to even compile some files on a platform that didn't supply the API needed.

astrogeco · 2020-04-13T14:14:59Z

@acudmore how do you feel about @jphickey's reply?

skliper · 2020-04-13T19:57:41Z

Pushed a fix to resolve coverage test failures introduced by #387, recommending fasttrack

skliper · 2020-04-13T20:43:37Z

Still failing bundle CI, specifically for BUILDTYPE=release, https://travis-ci.com/github/skliper/cFS/builds/160109318

/home/travis/build/skliper/cFS/osal/src/bsp/pc-linux/src/bsp_console.c: In function ‘OS_BSP_ConsoleOutput_Impl’:
/home/travis/build/skliper/cFS/osal/src/bsp/pc-linux/src/bsp_console.c:68:5: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
write(STDOUT_FILENO, Str, DataLen);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

src/bsp/pc-linux/src/bsp_console.c

This ensures that the return value of the write() call is checked, and avoids a potential compiler warning. There is still no recourse if the write call fails, but it can retry if it was short.

skliper · 2020-04-14T15:55:44Z

Passes now, with https://travis-ci.com/github/skliper/cFS/builds/160232404 which includes nasa/cFE#585, and nasa/PSP#149

skliper · 2020-04-14T17:59:43Z

Jumped the gun...

Coverage report no longer includes cFE coverage analysis, and report is misleading for osal since it includes the unit-test-coverage code statistics. Also actual coverage looks questionable, since not all files included by vxworks look like they are being analyzed.

skliper

Need to fix coverage reporting

src/bsp/pc-linux/build_options.cmake

No change required here, just requires changes in cFE to maintain functionality

jphickey · 2020-04-14T19:02:24Z

See nasa/cFE#613.....

As the apps now also share this pattern, it might be worth providing a cmake function to build and link a UT. The alternative might be to just continue setting UT_C_FLAGS as equivalent to UT_COVERAGE_COMPILE_FLAGS.

Avoids calling write() if length is zero.

jphickey · 2020-04-15T14:15:18Z

FYI - additional commit added to fixup the previous fixup, I realized it should have a check for DataLen>0 in the loop. (with a conforming "write()" it should work either way, but this is better).

ghost · 2020-04-16T13:27:52Z

These changes are OK with me.. I did not mean to hold anything up, my questions were more for my knowledge.

This was linked to issues Apr 3, 2020

OSAL CMake script should not force/overwrite CMAKE_C_FLAGS #312

Closed

Improve consistency and reduce duplication in various BSP/PSP implementations #261

Closed

OSAL coverage test should be part of normal build #362

Closed

jphickey mentioned this pull request Apr 3, 2020

Fix #121, Unify CFE PSP with OSAL BSP nasa/PSP#149

Merged

jphickey added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Apr 6, 2020

skliper reviewed Apr 6, 2020

View reviewed changes

skliper mentioned this pull request Apr 8, 2020

Break up osapi-os-core.h into more focused includes #410

Closed

astrogeco added CCB - 20200408 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Apr 8, 2020

jphickey added 3 commits April 9, 2020 22:26

Fix nasa#362, integrate coverage with UT build

befa665

Build the full suite of OSAL coverage tests as part of the normal build when ENABLE_UNIT_TESTS is true. This will use the same target OSAL BSP to execute the tests as is used for the FSW.

jphickey force-pushed the fix-312-261-362-build-cleanup branch from 0c0c06c to befa665 Compare April 10, 2020 02:28

skliper approved these changes Apr 10, 2020

View reviewed changes

This was referenced Apr 10, 2020

OSAL fs defines scrub - deprecate OS_VolumeTable and related symbols #367

Closed

"duplicate symbol '_main'" on macOS when building tests #363

Closed

Update OSAL Configuration Guide, build instructions, directory references, etc #384

Closed

FASTTRACK: Fix coverage test failures

b05e906

skliper reviewed Apr 13, 2020

View reviewed changes

src/bsp/pc-linux/src/bsp_console.c Outdated Show resolved Hide resolved

Update nasa#404, Use loop in Console Write

f59b16b

This ensures that the return value of the write() call is checked, and avoids a potential compiler warning. There is still no recourse if the write call fails, but it can retry if it was short.

skliper approved these changes Apr 14, 2020

View reviewed changes

skliper previously requested changes Apr 14, 2020

View reviewed changes

skliper reviewed Apr 14, 2020

View reviewed changes

src/bsp/pc-linux/build_options.cmake Show resolved Hide resolved

jphickey mentioned this pull request Apr 14, 2020

Unit tests need to link with UT_COVERAGE_COMPILE_FLAGS/UT_COVERAGE_LINK_FLAGS nasa/cFE#612

Closed

This was referenced Apr 14, 2020

Unit tests need to link with UT_COVERAGE_COMPILE_FLAGS/UT_COVERAGE_LINK_FLAGS nasa/sample_app#56

Closed

Unit tests need to link with UT_COVERAGE_COMPILE_FLAGS/UT_COVERAGE_LINK_FLAGS nasa/sample_lib#19

Closed

astrogeco changed the title ~~OSAL build cleanup (multiple issues)~~ Fix #261, #312, and #362, OSAL build cleanup (multiple issues) Apr 14, 2020

This was referenced Apr 14, 2020

OSAL coverage report includes unit tests nasa/cFE#614

Closed

Fix #354, Shell related API separated for optional implementation #418

Merged

Update nasa#404, Add DataLen>0 check in write loop

eeab727

Avoids calling write() if length is zero.

skliper approved these changes Apr 15, 2020

View reviewed changes

This was referenced Apr 15, 2020

Fix #346, Adds library build, functional, and coverage test to CI #403

Merged

Separate cmake coverage logic and resolve clang support issue #420

Open

skliper linked an issue Apr 16, 2020 that may be closed by this pull request

"duplicate symbol '_main'" on macOS when building tests #363

Closed

astrogeco changed the base branch from master to integration-candidate April 16, 2020 14:56

astrogeco merged commit 38bb192 into nasa:integration-candidate Apr 16, 2020

astrogeco added IC - 20200408 CCB:Approved Indicates code review and approval by community CCB labels Apr 16, 2020

astrogeco mentioned this pull request Apr 16, 2020

Integration Candidate: 2020-04-15 #417

Merged

jphickey deleted the fix-312-261-362-build-cleanup branch April 20, 2020 15:00

jphickey mentioned this pull request Apr 20, 2020

Remove references to OSAL_OPAQUE_OBJECT_IDS and OSAL_ABSTRACT_FILESYS_TYPES #265

Closed

skliper linked an issue May 13, 2020 that may be closed by this pull request

Remove references to OSAL_OPAQUE_OBJECT_IDS and OSAL_ABSTRACT_FILESYS_TYPES #265

Closed

skliper added this to the 5.1.0 milestone Jun 1, 2020

jphickey added a commit to jphickey/osal that referenced this pull request Aug 10, 2022

Merge pull request nasa#404 for integration test

6c4b748

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #261, #312, and #362, OSAL build cleanup (multiple issues) #404

Fix #261, #312, and #362, OSAL build cleanup (multiple issues) #404

jphickey commented Apr 2, 2020 •

edited by skliper

Loading

skliper Apr 6, 2020

jphickey Apr 6, 2020

skliper Apr 7, 2020

astrogeco commented Apr 8, 2020

ghost commented Apr 9, 2020

jphickey commented Apr 10, 2020

jphickey commented Apr 10, 2020

astrogeco commented Apr 13, 2020

skliper commented Apr 13, 2020

skliper commented Apr 13, 2020

skliper commented Apr 14, 2020

skliper commented Apr 14, 2020

skliper left a comment

jphickey commented Apr 14, 2020

jphickey commented Apr 15, 2020

ghost commented Apr 16, 2020

		@@ -1505,4 +1505,59 @@ void OS_printf_disable(void);
		void OS_printf_enable(void);
		/*@}/

Fix #261, #312, and #362, OSAL build cleanup (multiple issues) #404

Fix #261, #312, and #362, OSAL build cleanup (multiple issues) #404

Conversation

jphickey commented Apr 2, 2020 • edited by skliper Loading

skliper Apr 6, 2020

Choose a reason for hiding this comment

jphickey Apr 6, 2020

Choose a reason for hiding this comment

skliper Apr 7, 2020

Choose a reason for hiding this comment

astrogeco commented Apr 8, 2020

ghost commented Apr 9, 2020

jphickey commented Apr 10, 2020

jphickey commented Apr 10, 2020

astrogeco commented Apr 13, 2020

skliper commented Apr 13, 2020

skliper commented Apr 13, 2020

skliper commented Apr 14, 2020

skliper commented Apr 14, 2020

skliper left a comment

Choose a reason for hiding this comment

jphickey commented Apr 14, 2020

jphickey commented Apr 15, 2020

ghost commented Apr 16, 2020

jphickey commented Apr 2, 2020 •

edited by skliper

Loading