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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Apr 28, 2020
Merged

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

merged 1 commit into from
Apr 28, 2020

Conversation

skliper
Copy link
Contributor

@skliper skliper commented Apr 1, 2020

Describe the contribution
Fix #346, Adds library build, functional, and coverage test to CI

Testing performed
Steps taken to test the contribution:

  1. CI from fork - https://travis-ci.com/github/skliper/osal/builds/157641624

Expected behavior changes
Just adds tests to CI

System(s) tested on

  • Hardware: AMD
  • OS: Ubuntu 18.04
  • Versions: bundle + warning fix from previous pull request + this change

Additional context
None

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman - NASA/GSFC

@skliper skliper added this to the 5.1.0 milestone Apr 1, 2020
@skliper skliper mentioned this pull request Apr 2, 2020
Closed
@skliper
Copy link
Contributor Author

skliper commented Apr 9, 2020

@astrogeco we seem to be skipping a few pull requests that got submitted but aren't marked ready for CCB. Can we just assume if they are new they should be discussed (as long as they aren't tagged w/ WIP)?

@skliper skliper added the CCB:Ignore Incomplete Pull Request with open actions. label Apr 9, 2020
@skliper
Copy link
Contributor Author

skliper commented Apr 9, 2020

Or review everything that isn't marked NOT ready?

@astrogeco
Copy link
Contributor

Or review everything that isn't marked NOT ready?

Good point, though we've been keeping the agenda pretty full despite the fact we're missing those. Ill take a look.

@skliper skliper force-pushed the fix346-add-ci-test branch 2 times, most recently from d97afc0 to 2373364 Compare April 15, 2020 20:50
@skliper skliper added CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) and removed CCB:Ignore Incomplete Pull Request with open actions. labels Apr 15, 2020
@skliper
Copy link
Contributor Author

skliper commented Apr 15, 2020

Rebased and updated per #404...

@skliper skliper mentioned this pull request Apr 15, 2020
Closed
@skliper
Copy link
Contributor Author

skliper commented Apr 15, 2020

Now cppcheck is --quiet to clean up the CI log.

@stanislaw
Copy link
Contributor

stanislaw commented Apr 16, 2020 •
edited
Loading

It is great to see that this is being prepared. I have been waiting for it since I opened the macOS support branch.

I have a few comments on what is being implemented here. They are in no way intended to be blocking the current changeset which is a great enhancement.

From quite some experience of maintaining private and public CI setups, I would highly recommend you to not put your CI commands directly into the .travis.yml. The reason is that you cannot run them run them locally on your developer machine easily to reproduce 100% what is run on CI. The sequence of commands being introduced is already quite advanced and without being put into some executable scripts you can get into a very inconvenient situation when it will be easier run the pushes/PRs on CI instead of testing them with convenience locally.

I have seen many options but the following three are on my active list:

a) Good old Makefile.

Extremely typical example I have been working with recently: doorstop/Makefile.

All commands can be put into the tasks such as: build, test, gcov, etc. These can be called from .travis.yml and from a local developer machine.

The experience taught me and my colleagues that this approach does not scale well when you get a large number of tasks and when you have to parametrize your script in advanced way.

b) Ansible playbooks

I am sure you are aware of Ansible playbooks.

This is what @AlexDenisov and I are using in our Mull project with a great convenience as we have to scale our tool across many versions of OSes and LLVM. Please see examples: https://github.com/mull-project/mull/blob/master/infrastructure/macos-playbook.yaml.

c) Invoke

This is a Python tool which gives you a combination of Makefile tasks functionality and Python scripting. See the example here: https://github.com/stanislaw/FileCheck.py/blob/master/tasks.py. What is run by CI can run 1-1 locally on all supported platforms: Linux, macOS and Windows.

These days, If I had to start a new project equivalent in scale, importance and complexity to such of the nasa/osal, I would go for a combination of the approaches b) and c):

Keeping the infrastructure/dependencies logic in Ansible playbooks (the Mull example above) while having all tasks such as _functional_test in the modularized suite of Python files based on Invoke (the FileCheck example above).

I hope this advice could help you in one or another way.

I have some more comments about the changeset and I will put them in the form of specific comments.

stanislaw
stanislaw reviewed Apr 16, 2020
View reviewed changes
src/bsp/pc-linux/build_options.cmake Outdated
# Note - although GCC understands the same flags for compile and link here, this may
# not be true on all platforms so the compile and link flags are specified separately.
if (NOT CMAKE_CROSSCOMPILING)
set(UT_COVERAGE_COMPILE_FLAGS -pg --coverage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be non-portable:

clang: error: the clang compiler does not support -pg option on versions of OS X 10.9 and later

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another note, even if it might be beyond the scope of this changeset. Usually such functionality is created in separate files like cmake/coverage.cmake to not mix the logic of the targets with the logic of the coverage.

The same applies to things like clang-format, clang-tidy, other code linters, spell checking, etc.

One good example can be found here: https://github.com/StableCoder/cmake-scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #420

@skliper
Copy link
Contributor Author

skliper commented Apr 16, 2020

@stanislaw Great suggestions, thanks! Definitely a goal to improve.

This was referenced Apr 16, 2020
Closed
Open
@skliper
Copy link
Contributor Author

skliper commented Apr 20, 2020

Rebased on master

@stanislaw stanislaw mentioned this pull request Apr 22, 2020
Closed
14 tasks
@astrogeco
Copy link
Contributor

CCB 20200422 - APPROVED

@skliper skliper added CCB - 20200422 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Apr 22, 2020
@skliper skliper added the CCB:Approved Indicates code review and approval by community CCB label Apr 22, 2020
@astrogeco astrogeco changed the base branch from master to integration-candidate April 27, 2020 23:00
@astrogeco astrogeco merged commit a138fba into nasa:integration-candidate Apr 28, 2020
@astrogeco astrogeco mentioned this pull request Apr 30, 2020
Merged
@skliper skliper deleted the fix346-add-ci-test branch February 1, 2021 21:36
jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022
@skliper
Fix nasa#540, Scrub User's Guide and related comments
784fe09 
NOTE - Comments/documentation only, no logic updates
Also fixes nasa#403, resolves all warnings
Also fixes nasa#536, comment typo in CFE_SB_RcvMsg implementation header
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stanislaw stanislaw stanislaw left review comments

Assignees
No one assigned
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Milestone
Bootes
Development

Successfully merging this pull request may close these issues.

Add OSAL library build, coverage test, and functional tests to CI
3 participants
@skliper @astrogeco @stanislaw