nasa / bplib

Apache License 2.0
27 stars 13 forks source link

DTNN-653 Rebaseline for v7 development #261

Open sara-gb opened 2 months ago

sara-gb commented 2 months ago

Describe the contribution Addresses DTNN-653. Rebaselines the repo with the necessary build structure and initial files needed for v7 development

Testing performed Unit and build tests

Expected behavior changes Library prototype is wiped and replaced with initial v7 files. All components have an empty init function and unit test framework.

System(s) tested on

Contributor Info - All information REQUIRED for consideration of pull request Sara Garcia-Beech

gskenned commented 1 month ago

Changes look good. Built with files from bplib Draft PR-260 Update README, tag PR-260.02.

gskenned@ip-10-1-23-64:~/repos/forks/bplib/doc/example-scripts/stand-alone
$ git log -1
commit 06749141411303be1c448503a56f1591ffbce49d (HEAD -> DTNN-132-clean-up-bplib-prototype-readme, tag: PR-260.02, origin/DTNN-132-clean-up-bplib-prototype-readme)
<truncated>

See bplib-dtnn-653.zip attached.

Build is in bplib-dtnn-653/build-bplib. Copied all files from PR-260.02 bplib/doc/example-scripts/stand-alone to build-bplib. Modified bplib-env-vars. The original is bplib-env-vars.0. Ran ./bplib-test-driver in build-bp because I already had the toolchain installed. (Otherwise, run ./bplib-install-toolchain.) This is a build test only. Successfully built bpcat for both Debug-POSIX and Release-POSIX.

gskenned@ip-10-1-23-64:~/repos/reviews
$ find -name bpcat
./bplib-dtnn-653/build-bplib/bplib-build-matrix-Debug-POSIX/app/bpcat
./bplib-dtnn-653/build-bplib/bplib-build-matrix-Release-POSIX/app/bpcat

See the logs in the tar file:

$ find -name "bplib-test-driver*.log"
./bplib-dtnn-653/build-bplib/bplib-test-driver-24211-1132-r-p.log
./bplib-dtnn-653/build-bplib/bplib-test-driver-24211-1132-d-p.log
gskenned commented 1 month ago

Great work! I built and ran this branch with bp-cfs/develop and bpnode/develop and COSMOS. It ran well with no issues.

  1. General question: Does the github runner run as root, or with sudo root, or as a regular user? I ask because I found that the common rbtree and ut_functional sanity tests required setuid root to work when run by a regular user. I don't know if it's an issue or not what privileges the github runner user has. I just think we should know.

  2. Commit 01d9a085 has too many lines of commit messages.

    commit 01d9a085d048b6d58b97d605c1bbd3bce32fc4e5
    Author: Sara Garcia-Beech <sara.garcia-beech@nasa.gov>
    Date:   Fri Aug 2 16:01:12 2024 +0000
    
    DTNN-653 Fixed functional test cmake
    
    DTNN-653 Added include dirs to functional test
    
    DTNN-653 Added link libs to functional test
    
    DTNN-653 Functional test compilation mods
    
    DTNN-653 Added bplib stubs to functional test
    and so on, for about 58 more lines.
  3. Where does the bplib build find utassert.h? In bp-cfs it is in osal/ut_assert/inc/utassert.h. Does the use of utassert.h depend on having OSAL?

  4. In a future version, I'd like to see the empty unit tests, like BPLib_Test in ut-functional/sanity-test.c, report that they are "unimplemented" or an "empty stub". They should still pass in the .github testing, but then we can see which tests are implemented and which are not.

sara-gb commented 1 month ago

@gskenned Replying to your comments:

  1. No idea, I've never set up github runners myself
  2. Ugh, I was trying to squash those commits, thanks for noticing that. I'll try to fix it.
  3. The BPLib unit test build depends on utassert/osal, but not the library build itself. If someone wanted to just build the library, they can do that as a standalone.
  4. Once people start adding at least stub functions to their components, that's a good idea, but right now the stubs are just temporary init functions, a lot of which will probably go away, I just wanted at least something in the source files to compile
sjonke commented 1 month ago

I built and ran with cosmos, running the test suite there and it worked without issue. Ran the unit tests and those were successful as well. I looked through the changes. I did not look at absolutely everything - after a point I started skimming through them noting that the general, repeating pattern was there. It looks good.