search

nasa / osal

The Core Flight System (cFS) Operating System Abstraction Layer (OSAL)
Apache License 2.0
557 stars 218 forks source link

classic build broken #97

Closed skliper closed 5 years ago

skliper commented 5 years ago

Mike Scott (ASO project at Ames) reports that with the most recent update to development, his build process that uses the classic build of OSAL is no longer working. The initial failure is being unable to find osconfig.h

I started looking into this, and there are further issues that prevent the classic OSAL build instructions from working now, and will document each issue (and its repair) here.

skliper commented 5 years ago

Imported from trac issue 74. Created by glimes on 2015-07-15T10:48:07, last modified: 2019-03-27T14:45:15

skliper commented 5 years ago

Trac comment by glimes on 2015-07-15 11:06:32:

First issue: osconfig.h was missing.

The recent update to development removed build/inc/osconfig.h from the Git repositories, merging its updated content back into the per-BSP version in src/bsp/$bsp/config/osconfig.h allowing it to correctly have content that was BSP specific.

As a consequence, Git also discarded the now-empty build/inc directory. The make config step of the Classic OSAL build instructions then used this command: {{{ cp -f ../src/bsp/pc-linux/config/osconfig.h inc }}} This works when inc is a directory, but does unexpected things when it is not.

The fix (IMHO) is to, first, adjust the cp command so that it either writes the correct file or fails; and second, adjust the build rule to create the target directory if necessary.

Changes are being made in the trac-74-classic-build branch of the cfs_osal project on Babelfish. Commit [changeset:f80fef5] contains the initial pass at fixing this specific condition.

skliper commented 5 years ago

Trac comment by glimes on 2015-07-15 11:11:34:

Second issue: utassert.h not found during make depend.

The UtAssert facilities are located in a new header directory, which the new CMake system knows about, but which the classic build system does not. As a consequence, attempting to build various tests that now use UtAssert will fail under Classic builds.

Unfortunately, this error occurs in each /tests/ program, and there is no central place where we can add this -I flag that reaches them all, so the change needed is to edit each of the Makefiles in the functional tests and add the new directory.

Commit [changeset:9c06acd] addresses this issue.

skliper commented 5 years ago

Trac comment by glimes on 2015-07-15 11:43:58:

Third issue: link errors relating to UtAssert

The functional tests are failing to link as we have not yet arranged for them to build and link the UtAssert library, which is now required by the test programs.

Unfortunately, as before, the Makefiles in /tests/ each individually explicitly pick objects and provide no central mechanism that can be used to extend the lists. It is necessary to apply this change to each of the files individually.

Commit [changeset:8f683dd] addresses this issue.

skliper commented 5 years ago

Trac comment by glimes on 2015-07-15 11:56:57:

Fourth issue: warnings related to UtAssert

The header files for UtAssert make use of anonymous variadic macros which were added in C99, which is only fifteen years ago, so the compiler warns us, as it defaults to "C90 plus GCC extensions."

I have temporary compile flags set up in my test build, will seek a more robust solution before committing.

skliper commented 5 years ago

Trac comment by glimes on 2015-07-15 12:19:19:

Fifth issue: ut-modules.mak is missing.

When the new CMake mechanism for producing the loadable modules needed by the osloader test was set up, the old mechanism was removed, which causes classic OSAL unit test builds to terminate during "make clean" when osloader-test tries to use it.

I have resurrected the old version into my test build, will commit when the build gets me a clean image of the loadable modules.

skliper commented 5 years ago

Trac comment by glimes on 2015-07-15 13:00:25:

Sixth issue: main vs OS_Application_Startup

The programs in /unit-tests/ conditionally provide either a main function or an OS_Application_Startup, depending on whether _OSAL_UNITTEST is defined.

In this environment, they are being linked with the BSP files, which provide main and expect to call OS_Application_Startup.

Previously, _OSAL_UNITTEST was being provided as a compile-time option by the pc-linux-ut BSP in its compiler-opts.mak file, and literally the only uses of this macro were to switch the interface within the /unit-tests/ programs, where there was literally no use case for the code that appeared when it was not defined.

With that in mind, my fix is likely to be the removal of the never-to-be-used code from the /unit-tests/ sources.

However, we may need to use this symbol in future within some BSP implementations, where they really do need to have differences between production and unit-test, so no changes aside from stomping on unit test sources themselves.

A changeset implementing this is currently being tested.

skliper commented 5 years ago

Trac comment by jphickey on 2015-07-15 13:43:25:

Regarding the fourth issue - compiler warnings --- I find this odd because the CMake build does not throw warnings about this, and it certainly would use the same version of gcc.

-Wall is used in all cases. By default no special "-std" argument is given.

Is the classic make using different -W flags?

skliper commented 5 years ago

Trac comment by glimes on 2015-07-15 16:05:14:

I suspect the difference is that compiler-opts.mak includes -pedantic which is usually the first option stripped away when people get frustrated about getting heaps of warnings.

Personally, I like -pedantic but I know I'm weird.

[edit: I didn't push the -Wno-variadic-macros hack, so this warning will be visible until hacked away.]

skliper commented 5 years ago

Trac comment by glimes on 2015-07-15 16:10:56:

Currently testing support scripts that manage to get master working as well as I have ever got it to work (not parsing /tests/ outputs), with a change for this ticket that gets us most of the way there: we can now build with the classic method, stage and run tests, but I need to dig out what to do in the classic build to get the tests running right.

Pushed [changeset:bc04749] as ic-gll-classic and trac-74-classic-build to Babelfish, and set up Bamboo to allow Classic builds for the Master branch and my Classic branch. Went into Master and tossed everything into Quarantine that I expected to fail, and we now have a Green/Quarantine bar showing for Master.

Have told Bamboo it can notice test repo changes again, so we are in the middle of the resulting buildstorm.

skliper commented 5 years ago

Trac comment by jphickey on 2015-07-15 16:17:17:

Replying to [comment:10 glimes]:

I suspect the difference is that compiler-opts.mak includes -pedantic which is usually the first option stripped away when people get frustrated about getting heaps of warnings.

Personally, I like -pedantic but I know I'm weird.

I also often use "-pedantic" but I use it in conjunction with "-std=c99"

I see no good reason to limit ourselves to the pre-C99 stuff ... it is 16 years old, and C99 addressed some really really painful issues with the prior versions. I don't think it is too far fetched to assume that any reasonable compiler these days will support at least the parts of C99 that we need (e.g. variadic macros)

skliper commented 5 years ago

Trac comment by glimes on 2015-07-15 16:21:26:

There may be young greybeards out there who think that they still have to pander to broken embedded systems vendors whose tools are still incapable of compiling C99 sources.

I think even Wind River has caught up to C99 by now, and if they can do it, anyone can.

It's not like we're shoving C++14 code into the project ... as much as I would like to ... ''auto ftw'' ... ''lambda ftw'' ... ''RAII ftw'' ... ''move semantics ftl'' ;) ...

skliper commented 5 years ago

Trac comment by jphickey on 2015-07-15 16:23:03:

I would also suggest that we do the same thing as we do for the CMake build here: Include {{{-Wall}}} but leave out {{{-pedantic}}} or {{{-Werror}}} in the sample files.

If a project so chooses, they can add those options back in. Perhaps leave them in as a comment, that the user can uncomment if they so choose after they know the build works warning-free with their specific compiler version.

skliper commented 5 years ago

Trac comment by jphickey on 2015-07-15 16:31:42:

IIRC it is also C99 that specifies designated member initializers for arrays and structs i.e. {{{ struct my_struct = { .member = 10 }; }}}

This is an absolutely critical feature. Nobody should be writing code without this, it breaks far too easily and without warning if "my_struct" ever adds/removes members.

To me, this is reason enough to never consider using any toolchain that doesn't at least support basic C99 stuff.

skliper commented 5 years ago

Trac comment by glimes on 2015-07-16 11:14:53:

Breaking out the --std=c99 issue for further study. See OSAL ticket [#102 79: make --std=c99 build] See CFE ticket [cfs_cfe:82 82: make --std=c99 build]

skliper commented 5 years ago

Trac comment by glimes on 2015-07-16 15:26:08:

See also [cfs_cfe:54] for the CFE side of this.

skliper commented 5 years ago

Trac comment by glimes on 2015-07-22 12:26:27:

Many commits mentioned above have been squashed for brevity. See [changeset:bc04749] for the roll-up.

skliper commented 5 years ago

Trac comment by glimes on 2015-07-22 15:58:30:

Changes to /tests/*/Makefile need to be implemented in the /unit-tests/ and, unfortunately, /examples/ trees in order for "make depend" to complete properly.

Amended merge is [changeset:fd229d2]

skliper commented 5 years ago

Trac comment by glimes on 2015-07-23 19:29:05:

Doing more tuning based on the merge of trac-22 and development, changeset is going to continue to move. Watch babelfish/ic-gll-classic and https://babelfish.arc.nasa.gov/bamboo/browse/CFS-CFSOSAL53/latest for evolving details and results.

skliper commented 5 years ago

Trac comment by glimes on 2015-07-24 12:42:30:

Tracking the latest CCB candidate branch; added {{{ $(OSAL_SRC)/bsp/$(BSP)/ut-src }}} to VPATH for all test (and example) builds, so that the build can find bsp-ut in its new location.

This is happening in ic-gll-merge for now, which will become ic-gll-classic after we accept the current CCB proposal (and maybe some rebasing will be involved).

Build results for ic-gll-merge -- being used at the moment to track this -- are at:

https://babelfish.arc.nasa.gov/bamboo/browse/CFS-CFSOSAL54/latest

By the way ... this sort of "add a line to a dozen Makefiles" is exactly where CMake is, for me, vastly superior to the classic build tree. Just in case anyone out there still needs convincing.

''note: ic-gll-merge is ephemeral. If this comment is old, the branch may have changed scope and may be merging in a whole different pile of things.''

skliper commented 5 years ago

Trac comment by jphickey on 2015-11-05 15:05:49:

FYI - pushed a NEW, rebased branch trac-74-classic-build-take2 bringing this up-to-date again. This should be included in the next OSAL to continue supporting classic build methods.

Note that this does //not// include the path to $(OSAL_SRC)/bsp/$(BSP)/ut-src anymore - this does not exist in the current directory tree.

commit [changeset:5667e02] is the CCB review item.

skliper commented 5 years ago

Trac comment by jphickey on 2015-11-06 11:27:01:

== CORRECTION ==

The $(OSAL_SRC)/bsp/$(BSP)/ut-src paths are indeed required for compatibility with the change introduced in the previous ticket (#96).

I am retracting the commit mentioned in my prior comment.

I have put a new commit (again), this time as a child of the trac-73 commit. These would BOTH need to be taken together - not worth trying to separate this.

The corrected CCB review item is: [changeset:c6703fb]

skliper commented 5 years ago

Trac comment by sstrege on 2015-11-08 13:47:46:

Approve/accept

skliper commented 5 years ago

Trac comment by glimes on 2015-11-09 20:56:43:

Recommend ACCEPT (but I am biased).

Bamboo threw a hissy when this went in, but I've fed it and now it's happy, and some "sed" commands in the build script are older and wiser ;)

skliper commented 5 years ago

Trac comment by acudmore on 2015-11-10 10:39:18:

recommend accept

skliper commented 5 years ago

Trac comment by sduran on 2015-11-10 12:17:48:

There are a number of changes were #ifdefs have been removed. Some may need to be added back in. In particular

ifdef _OSAL_UNIT_TEST

void OS_Application_Startup(void)

else

int main(int argc, char* argv[])

endif

JSC is still reviewing closely to determine which of these deletions can be accepted or not.

skliper commented 5 years ago

Trac comment by jphickey on 2015-11-10 12:28:10:

Steve - the modified makefiles also link with the UT assert library already included in OSAL.

When linking with this UT assert framework, it defines the entry point function, in this case main() on Linux, which then calls OS_Application_Startup() to set up the tests. The problem with defining main() directly is that it is OS-dependent, so if we want to run these tests on e.g. RTEMS then it is a different entry point.

(BTW - these unit tests DO run on RTEMS - without modification - in the development branch).

In short - you should never have to define main() in your test routine anymore. This #ifdef should not be needed.

skliper commented 5 years ago

Trac comment by sduran on 2015-11-17 10:21:21:

I verified that the #ifdef _OSAL_UNIT_TEST '''do need to be added back in''' for the way that JSC currently builds the unit tests - '''they fail to compile otherwise'''. The OSAL BSP is not used as part of the build. In Linux, executables, requiring a main of their own are needed to compile and run the tests.

skliper commented 5 years ago

Trac comment by jphickey on 2015-11-17 11:31:20:

== UPDATED per CCB comments on 2015-10-11 ==

As noted earlier this is dependent upon the fix for #96, which was updated. This change set is now adjusted to work with the latest proposal for the previous fix.

New review commit [changeset:59e19c1], branch trac-74-classic-build-take3.

Note that the "-take2" branch has been removed.

skliper commented 5 years ago

Trac comment by jphickey on 2015-11-17 11:42:14:

Replying to [comment:30 sduran]:

I verified that the #ifdef _OSAL_UNIT_TEST '''do need to be added back in''' for the way that JSC currently builds the unit tests - '''they fail to compile otherwise'''. The OSAL BSP is not used as part of the build. In Linux, executables, requiring a main of their own are needed to compile and run the tests.

This is because you are only using half of this changeset -- while you are using the source code changes, you are //not// using the corresponding required makefile changes to make everything work. The (fairly minor) makefile changes are summarized:

These basic changes would need to be hand-ported over into your JSC-specific makefiles. I'm happy to help, but I do not have access to your makefiles.

Please note -- //this combined changeset has the benefit of allowing these tests to run in RTEMS or any other OS equally//. IMO this is a critical feature, to support the tests for all operating systems in addition to the runtime. With the way the tests had been implemented, they will not run on RTEMS, as that does not use main() as an entry point.

skliper commented 5 years ago

Trac comment by sduran on 2015-11-17 15:32:06:

You say I am only using part of the change set…we do not use the cmake or makefiles that you are using, as I have describe before, we build unit test in a gsfc_build project, very similar to performing an integrated CFS build. Attached are our makefiles for oscore-test for Linux and ut699-vxworks6. So how exactly would I change those to not require #ifdef _OSAL_UNIT_TEST?

Also running into issues with _wb tests. It appears that our xxx_testrunner.c files have been removed too. Our build requires these. Attached is the makefile.

gmake[1]: No rule to make target osapi_testrunner.o', needed byoscore_wb_UT.o'. Stop. gmake[1]: Leaving directory `/home/sduran/CFSTST_Workspace/CFS_TST/CFS_TST/build/ut699-vxworks6_UT/cfe/osal/unit-test2/oscore_wb-test' gmake: [osal_ut_build_oscore_wb] Error 2

skliper commented 5 years ago

Trac comment by jphickey on 2015-11-17 15:58:49:

Replying to [comment:33 sduran]:

You say I am only using part of the change set…we do not use the cmake or makefiles that you are using, as I have describe before, we build unit test in a gsfc_build project, very similar to performing an integrated CFS build.

Correct - to clarify, in commit [changeset:59e19c1], there is a change to the source code - to eliminate the local definition of main() - as well as a corresponding change to the makefile in "build/unit-test" to pull in the object that defines main() to replace this.

These two changes, when taken together, makes everything work the same but structured in such a way that tests can be executed on RTEMS (and others) without future changes or #ifdef statements.

You are using your local (unmodified) makefiles so it is understandable that this would not work, you are using the source code change without using the makefile change that goes along with it.

Is there a reason you can't just copy the makefile structure in "build/unit-test" into your CFS_TST and see if that resolves your issues? It looks like it should work, although you might have to set the BSP environment variable (if it is not already set) in your setvars.sh or equivalent.

Also running into issues with _wb tests. It appears that our xxx_testrunner.c files have been removed too. Our build requires these. Attached is the makefile.

The osapi_testrunner.o is also replaced by the common code - specifically bsp_ut.o. This "testrunner" is duplicated in every test however it is not portable to different operating systems that don't use main() as an entry point. This is why it had to be moved - the "mechanics" to start a test are not part of the test itself.

This should also be fixed if you update your CFS_TST makefiles to match the ones provided in this changeset - specifically the src/unit-test-coverage/vxworks6 directory.

skliper commented 5 years ago

Trac comment by glimes on 2015-11-20 14:06:32:

Just to record this -- once this goes in, I will commit to getting the unit tests building for Classic SPARC VxWorks builds (and running, as soon as we have target systems ;)

We also want to encapsulate the differences between our classic makefiles and the JSC makefiles attached to this ticket, maybe pull them out as environment variables and push those up into the environment script so that different sites can set them appropriately (and keep the makefiles common and similar ;)

skliper commented 5 years ago

Trac comment by glimes on 2015-11-20 15:34:05:

now part of ic-2015-11-20.

skliper commented 5 years ago

Trac comment by jphickey on 2015-11-20 15:56:05:

Here is a summary of what I see as differences between the JSC oscore-test Linux makefile (attached above) and what we currently have in the "build" subdirectory in OSAL (this changeset). This is what would have to be reconciled before the JSC makefile and the OSAL makefile could be called "compatible" again:

Variable/macro value settings:

Included sub-makefile component pathname differences:

Target differences:

I will open another ticket and we can discuss the best way to reconcile those diffs. I don't see anything in that list as a "showstopper" by any means.

skliper commented 5 years ago

Trac comment by glimes on 2015-12-02 10:29:56:

ic-2015-11-20 is now included in the development branch.

skliper commented 5 years ago

Trac comment by jhageman on 2019-03-27 14:45:15:

See changeset [changeset:59e19c1]