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

Integration Candidate: Fast Track #78

Merged
merged 4 commits into from
May 5, 2020
Merged

Conversation

astrogeco
Copy link
Contributor

@astrogeco astrogeco commented May 4, 2020

Describe the contribution
Combines the following Fast Track Pull Requests
osal: nasa/osal#444
cfe: nasa/cFE#672

Testing performed
See PRs
Bundle CI - https://github.com/nasa/cFS/pull/78/checks?check_run_id=644721591

Expected behavior changes
See osal PR and cFE PR

System(s) tested on
Bundle CI - Ubuntu:Bionic

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

Gerardo E. Cruz-Ortiz and others added 2 commits May 4, 2020 15:42
@astrogeco
Copy link
Contributor Author

@jphickey see Travis unit test failures

 45 - bin-sem-flush-test (Child aborted)

	 46 - bin-sem-test (Child aborted)

	 47 - bin-sem-timeout-test (Child aborted)

	 48 - count-sem-test (Child aborted)

	 49 - file-api-test (Child aborted)

	 50 - mutex-test (Child aborted)

	 51 - osal-core-test (Child aborted)

	 52 - queue-timeout-test (Child aborted)

	 53 - sem-speed-test (Child aborted)

	 54 - symbol-api-test (Child aborted)

	 55 - timer-test (Child aborted)

	 56 - osal_core_UT (Failed)

	 57 - osal_loader_UT (Child aborted)

	 58 - osal_filesys_UT (Child aborted)

	 59 - osal_file_UT (Failed)

	 60 - osal_network_UT (Child aborted)

	 61 - osal_timer_UT (Child aborted)

@astrogeco astrogeco requested a review from skliper May 4, 2020 20:34
@astrogeco astrogeco marked this pull request as draft May 4, 2020 20:34
@jphickey
Copy link
Contributor

jphickey commented May 4, 2020

The issue is that it still wasn't picking up the PERMISSIVE mode setting. Two minor fixups solve it:

  • had to propagate the native_osconfig.cmake from my local work area to cfe/cmake/sample_defs directory so this build will also pick it up. Patched by nasa/cFE@4fba145.
  • OSAL also wasn't picking up the file because include() needed a foreach loop (a silent failure, it only read the first entry, ignoring the second, which is the one that had PERMISSIVE...). Patched by nasa/osal@20f8b18

@astrogeco
Copy link
Contributor Author

The issue is that it still wasn't picking up the PERMISSIVE mode setting. Two minor fixups solve it:

* had to propagate the `native_osconfig.cmake` from my local work area to `cfe/cmake/sample_defs` directory so this build will also pick it up.  Patched by [nasa/cFE@4fba145](https://github.com/nasa/cFE/commit/4fba145).

* OSAL also wasn't picking up the file because `include()` needed a foreach loop (a silent failure, it only read the first entry, ignoring the second, which is the one that had PERMISSIVE...).  Patched by [nasa/osal@20f8b18](https://github.com/nasa/osal/commit/20f8b18)

Can you add those to the respective IC branches?

Patch to resolve warnings about undefined references.
@skliper
Copy link
Contributor

skliper commented May 5, 2020

Could we call it osconfig-doxygen.h as a slightly more specific name?

@skliper
Copy link
Contributor

skliper commented May 5, 2020

Could we call it osconfig-doxygen.h as a slightly more specific name?

Or could the osconfig.h.in itself be passed in? I don't think it has to be a .h file.

@astrogeco astrogeco marked this pull request as ready for review May 5, 2020 14:27
@jphickey
Copy link
Contributor

jphickey commented May 5, 2020

Or could the osconfig.h.in itself be passed in? I don't think it has to be a .h file.

Tried this and it doesn't work ... doxygen appears to just ignore it.

I chose osconfig-example.h as it this is what it is (an example without real values) for documentation purposes. This only exists in the build dir, not in source tree obviously, so this name is never used except it will appear in the generated docs when you click on the links to stuff in this file.

Is this the only thing holding this up at this point? I have several other branches that need to be rebased after this goes to "master", so I'd like to move past this.

I recommend we just move forward with the way it is but if you don't like the name I chose, please just tell me specifically what name you will like, and I will patch it again.

@skliper
Copy link
Contributor

skliper commented May 5, 2020

Or could the osconfig.h.in itself be passed in? I don't think it has to be a .h file.

Tried this and it doesn't work ... doxygen appears to just ignore it.

It's possible (FILE_PATTERN, EXTENSION_MAPPING), but by the time I got it working it felt more complex than your example implementation. Lets just go with this.

@astrogeco astrogeco merged commit 8f8ec69 into master May 5, 2020
@skliper skliper added this to the 6.8.0 milestone Jun 1, 2020
chillfig pushed a commit to chillfig/cFS that referenced this pull request Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants