[#60] integrate miri in ci with an allow list

xieyuschen commented 1 month ago

Allow CI failures for now

Notes for Reviewer

I have integrated miri in github action while supporting a shell-style .miri_allowlist file for all sub-folders to run cargo miri test.

Pre-Review Checklist for the PR Author

[X] Add sensible notes for the reviewer
[X] PR title is short, expressive and meaningful
[X] Relevant issues are linked in the References section
[X] Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
[X] Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
[X] Commits messages are according to this guideline
- [X] Commit messages have the issue ID ([#123] Add posix ipc example)
- [X] Commit author matches Eclipse Contributor Agreement (and ECA is signed)
[X] Tests follow the best practice for testing
[X] Changelog updated in the unreleased section including API breaking changes
[x] Assign PR to reviewer
[X] All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

[x] Commits are properly organized and messages are according to the guideline
[ ] ~~Unit tests have been written for new behavior~~
[ ] ~~Public API is documented~~
[x] PR title describes the changes

Post-review Checklist for the PR Author

[x] All open points are addressed and tracked via issues

References

Relates #60

xieyuschen commented 1 month ago

I don't have the permission to assign reviewers, @elfenpiff @elBoberido

elfenpiff commented 1 month ago

@xieyuschen Your CI run is failing because of a flaky test on our side, it is fixed with #422 When this PR is merged you can rebase and this should fix the problem.

xieyuschen commented 1 month ago

@xieyuschen Your CI run is failing because of a flaky test on our side, it is fixed with #422 When this PR is merged you can rebase and this should fix the problem.

sure, could I please ask if it’s possible to merge #423 today? Thank you!"

orecham commented 1 month ago

I just checked the "green" Miri job, it isn't running any tests:

Run ./internal/scripts/ci_run_miri.sh
Run cargo miri test under: /home/runner/work/iceoryx2/iceoryx2/iceoryx2-bb/testing
    Updating crates.io index
     Locking 151 packages to latest compatible versions
      Adding bindgen v0.69.[4](https://github.com/eclipse-iceoryx/iceoryx2/actions/runs/11124175590/job/30915883586?pr=421#step:4:5) (available: v0.70.1)
      Adding cbindgen v0.26.0 (available: v0.27.0)
      Adding human-panic v1.2.3 (available: v2.0.1)
      Adding windows-sys v0.48.0 (available: v0.[5](https://github.com/eclipse-iceoryx/iceoryx2/actions/runs/11124175590/job/30915883586?pr=421#step:4:6)9.0)
   Compiling iceoryx2-pal-configuration v0.4.1 (/home/runner/work/iceoryx2/iceoryx2/iceoryx2-pal/configuration)
   Compiling iceoryx2-bb-testing v0.4.1 (/home/runner/work/iceoryx2/iceoryx2/iceoryx2-bb/testing)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.[6](https://github.com/eclipse-iceoryx/iceoryx2/actions/runs/11124175590/job/30915883586?pr=421#step:4:7)5s
     Running unittests src/lib.rs (/home/runner/work/iceoryx2/iceoryx2/target/miri/x86_64-unknown-linux-gnu/debug/deps/iceoryx2_bb_testing-59d902fc[7](https://github.com/eclipse-iceoryx/iceoryx2/actions/runs/11124175590/job/30915883586?pr=421#step:4:8)96b2132)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

   Doc-tests iceoryx2_bb_testing

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

Is that intentional for this initial PR?

xieyuschen commented 1 month ago

I just checked the "green" Miri job, it isn't running any tests:

Run ./internal/scripts/ci_run_miri.sh
Run cargo miri test under: /home/runner/work/iceoryx2/iceoryx2/iceoryx2-bb/testing
    Updating crates.io index
     Locking 151 packages to latest compatible versions
      Adding bindgen v0.69.[4](https://github.com/eclipse-iceoryx/iceoryx2/actions/runs/11124175590/job/30915883586?pr=421#step:4:5) (available: v0.70.1)
      Adding cbindgen v0.26.0 (available: v0.27.0)
      Adding human-panic v1.2.3 (available: v2.0.1)
      Adding windows-sys v0.48.0 (available: v0.[5](https://github.com/eclipse-iceoryx/iceoryx2/actions/runs/11124175590/job/30915883586?pr=421#step:4:6)9.0)
   Compiling iceoryx2-pal-configuration v0.4.1 (/home/runner/work/iceoryx2/iceoryx2/iceoryx2-pal/configuration)
   Compiling iceoryx2-bb-testing v0.4.1 (/home/runner/work/iceoryx2/iceoryx2/iceoryx2-bb/testing)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.[6](https://github.com/eclipse-iceoryx/iceoryx2/actions/runs/11124175590/job/30915883586?pr=421#step:4:7)5s
     Running unittests src/lib.rs (/home/runner/work/iceoryx2/iceoryx2/target/miri/x86_64-unknown-linux-gnu/debug/deps/iceoryx2_bb_testing-59d902fc[7](https://github.com/eclipse-iceoryx/iceoryx2/actions/runs/11124175590/job/30915883586?pr=421#step:4:8)96b2132)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

   Doc-tests iceoryx2_bb_testing

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

Is that intentional for this initial PR?

I think so, the miri output looks confusing. Here I have some examples to show you:

mkdir demo && cd $_ && cargo init && cargo miri test: the same output(test result: ok. 0 passed; ...) Running miri under a cargo init project without any error should be the expected behavior.
cd iceoryx2-bb/testing && cargo miri test: the same output
cd iceoryx2-bb/memory && cargo miri test: you will see some errors.

orecham commented 1 month ago

I think so, the miri output looks confusing. Here I have some examples to show you:

mkdir demo && cd $_ && cargo init && cargo miri test: the same output(test result: ok. 0 passed; ...) Running miri under a cargo init project without any error should be the expected behavior.

cd iceoryx2-bb/testing && cargo miri test: the same output

cd iceoryx2-bb/memory && cargo miri test: you will see some errors.

@xieyuschen

From cargo miri --help:

Runs binary crates and tests in Miri

Usage:
    cargo miri [subcommand] [<cargo options>...] [--] [<program/test suite options>...]

Subcommands:
    run, r                   Run binaries
    test, t                  Run tests
    nextest                  Run tests with nextest (requires cargo-nextest installed)
    setup                    Only perform automatic setup, but without asking questions (for getting a proper libstd)
    clean                    Clean the Miri cache & target directory

So the command cargo miri test, as in your script, runs Miri for the tests defined in the crate. Perhaps misleading, but iceoryx2-bb/testing does not define any tests - this crate includes helpers for testing. The crate iceoryx2-bb/memory does define tests, hence the output showing errors (we haven't spent any time looking into Miri violations yet).

Fixing all Miri findings in the project will be an arduous task. I think it's OK to limit the scope of this PR to setting up the infrastructure for running Miri in CI.

However, it has occurred to me that running Miri on all crates defined in the 'allow list' might not be the best approach. It would make the check take longer than needed if unrelated crates are checked in every PR.

Limiting the check to the crates added or modified in the PR seems a better approach. So the check would run if crateOnAllowList && crateAddedOrModified. The allow list would still be necessary so that we are not blocked from adding new features while the work to address Miri violations for every crate is not yet done.

So how to move forward? My proposal is as follows, feel free to share your perspective:

Select one crate to fix Miri warnings for (see if there is a crate with an "easy" violation to fix, probably something in iceoryx2-bb)
Add it to the allow list, but verify the CI check does not yet run Miri on it (since you have not yet modified it)
Fix the violation(s) so that the Miri test passes locally
Push the fix and verify the check is run for the crate you have fixed, now that the crate is on the allow list AND you have modified it

This would give us a good foundation for:

Incrementally fixing Miri violations throughout the repository
Ensuring violations that are fixed are not broken when the corresponding crates are modified in the future

@elfenpiff @elBoberido Thoughts?

xieyuschen commented 1 month ago

Limiting the check to the crates added or modified in the PR seems a better approach

This approach sounds nice, but I think it has a shortage: The new PR should focus on its own goal, but involving miri fix requires the reviewers take more times to review the changes. If the miri error fixes are put in a separated PR, the PR is short and easy-to-win.

This workflow might be better:

have short time which is not enough for big change
run cargo miri test to find some errors
fix it and create an easy-to-win PR, add it in the allowlist

It's more fluent for developers to add features. The problem here is that if you never start the step1, it's hard to fix miri errors incrementally.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 79.00%. Comparing base (6d25b61) to head (b04b534). Report is 4 commits behind head on main.

Additional details and impacted files

[![Impacted file tree graph](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx2/pull/421/graphs/tree.svg?width=650&height=150&src=pr&token=FN3YFXTJCI&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx)](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx2/pull/421?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx) ```diff @@ Coverage Diff @@ ## main #421 +/- ## ========================================== + Coverage 78.93% 79.00% +0.07% ========================================== Files 195 195 Lines 22840 22836 -4 ========================================== + Hits 18029 18042 +13 + Misses 4811 4794 -17 ``` [see 10 files with indirect coverage changes](https://app.codecov.io/gh/eclipse-iceoryx/iceoryx2/pull/421/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse-iceoryx)

orecham commented 1 month ago

Limiting the check to the crates added or modified in the PR seems a better approach

This approach sounds nice, but I think it has a shortage: The new PR should focus on its own goal, but involving miri fix requires the reviewers take more times to review the changes. If the miri error fixes are put in a separated PR, the PR is short and easy-to-win.

This workflow might be better:

have short time which is not enough for big change

run cargo miri test to find some errors

fix it and create an easy-to-win PR, add it in the allowlist

It's more fluent for developers to add features. The problem here is that if you never start the step1, it's hard to fix miri errors incrementally.

@xieyuschen

Fine to skip the fixing a Miri violation in this PR - I had suggested it as a mechanism to verify the job behaviour before and changes to a crate we pushed. Could be done in a follow-up with the first fix, though.

I would still suggest to tweak the job so that it runs on crates changed in the given PR. You could do this by checking if the changed-files have a path prefix matching an entry in the .miri_allowlist (other ideas welcome). In the absence of an actual change in a crate, you can use logs to verify the logic is functioning as expected.

xieyuschen commented 1 month ago

@orecham done, can see https://github.com/xieyuschen/iceoryx2/pull/3 for ref.

orecham commented 1 month ago

@orecham done, can see his testPR](xieyuschen#3) for ref.

code change that hit the allow list

code change that doesn't hit the allow list

@xieyuschen Amazing ! Thanks for the contribution :)

eclipse-iceoryx / iceoryx2