On 20 Dec 2023, at 11:19, Eelco Chaudron wrote:
> This patch identifies new static analysis issues during a GitHub action > run and reports them. The process involves analyzing the changes introduced > in the current commit and comparing them to those in the preceding commit. > > However, there are two cases when the GitHub push action runner does not > provide enough details to determen the preceding commit. These cases are > a new branch or a forced push. The strategy for these exceptions is to > select the first commit not done by the current author as the base commit. Including Aarons comment on v3 into v4. +++ As an alternative we can look at the author (go back in time until +++ we find a different author), so if you’re lucky and it’s one of your +++ older patches you will be reminded again and again ;) Not sure if +++ this is easaly possible with a git command, but it might be worth +++ trying out. ++ I did some experiments with the above approach, and it seems to work ++ in all the use cases. The only problem could be that if you are also ++ the owner of a previous patch series, it might use a reference branch ++ too deep. This could potentially show issues not introduced by your ++ latest patches. But as you are still the author, you should be ++ “punished” for your earlier mistakes ;) ++ ++ I’ll send out a v4, and we can continue the discussion there if needed :) + I'll take a look after Jan 1, but I guess we might be able to check + against an upstream reference (for example, we can reference towards the + 'upstream' repository - such a reference does require some kind of + history being pulled) which we know should be stable and 'good'. IE: + the script could add the OVS github remote if it doesn't already exist. + I think we could use that to find the common ancestor. But I need to + dive into this more. I guess we can determine the origin branch going back, and then do something like `git merge-base upstream/master HEAD` assuming we fetched the upstream repository. Both solutions will work, and I have no real preference. //Eelco > An example error output might look like this: > > error level: +0 -0 no changes > warning level: +2 +0 > New issue "deadcode.DeadStores Value stored to 'remote' is never read" (1 > occurrence) > file:///home/runner/work/ovs/ovs/vswitchd/ovs-vswitchd.c:86 > New issue "unix.Malloc Potential leak of memory pointed to by 'remote'" > (1 occurrence) > file:///home/runner/work/ovs/ovs/vswitchd/ovs-vswitchd.c:95 > note level: +0 -0 no changes > all levels: +2 +0 > > Signed-off-by: Eelco Chaudron <[email protected]> > --- > > changes in v2: > - When it's a new branch, it compares it to the HEAD of the default branch. > > changes in v3: > - Include the clang version as part of the cache > - Change the way it looks for the 'default' branch so it will work > for patch branches. > - Also compare to the base branch for forced commits. > > changes in v4: > - No longer look for a default branch, but consume all patches > from the current author. > > .ci/linux-build.sh | 29 ++++++++++ > .github/workflows/build-and-test.yml | 103 > ++++++++++++++++++++++++++++++++++ > 2 files changed, 132 insertions(+) > > diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh > index aa2ecc505..fedf1398a 100755 > --- a/.ci/linux-build.sh > +++ b/.ci/linux-build.sh > @@ -49,6 +49,30 @@ function build_ovs() > make -j4 > } > > +function clang_analyze() > +{ > + [ -d "./base-clang-analyzer-results" ] && cache_build=false \ > + || cache_build=true > + if [ "$cache_build" = true ]; then > + # If this is a cache build, proceed to the base branch's directory. > + cd base_ovs_main > + fi; > + > + configure_ovs $OPTS > + make clean > + scan-build -o ./clang-analyzer-results -sarif --use-cc=clang make -j4 > + > + if [ "$cache_build" = true ]; then > + # Move results, so it will be picked up by the cache. > + mv ./clang-analyzer-results ../base-clang-analyzer-results > + cd .. > + else > + # Only do the compare on the none cache builds. > + sarif --check note diff ./base-clang-analyzer-results \ > + ./clang-analyzer-results > + fi; > +} > + > if [ "$DEB_PACKAGE" ]; then > ./boot.sh && ./configure --with-dpdk=$DPDK && make debian > mk-build-deps --install --root-cmd sudo --remove debian/control > @@ -116,6 +140,11 @@ fi > > OPTS="${EXTRA_OPTS} ${OPTS} $*" > > +if [ "$CLANG_ANALYZE" ]; then > + clang_analyze > + exit 0 > +fi > + > if [ "$TESTSUITE" = 'test' ]; then > # 'distcheck' will reconfigure with required options. > # Now we only need to prepare the Makefile without sparse-wrapped CC. > diff --git a/.github/workflows/build-and-test.yml > b/.github/workflows/build-and-test.yml > index 09654205e..cb277ff43 100644 > --- a/.github/workflows/build-and-test.yml > +++ b/.github/workflows/build-and-test.yml > @@ -223,6 +223,109 @@ jobs: > name: logs-linux-${{ join(matrix.*, '-') }} > path: logs.tgz > > + build-clang-analyze: > + needs: build-dpdk > + env: > + dependencies: | > + automake bc clang-tools libbpf-dev libnuma-dev libpcap-dev \ > + libunbound-dev libunwind-dev libssl-dev libtool llvm-dev \ > + python3-unbound > + CC: clang > + DPDK: dpdk > + CLANG_ANALYZE: true > + name: clang-analyze > + runs-on: ubuntu-22.04 > + timeout-minutes: 30 > + > + steps: > + - name: checkout > + uses: actions/checkout@v3 > + with: > + fetch-depth: 0 > + > + - name: get base branch sha > + id: base_branch > + env: > + BASE_SHA: ${{ github.event.pull_request.base.sha }} > + EVENT_BEFORE: ${{ github.event.before }} > + FORCED_PUSH: ${{ github.event.forced }} > + run: | > + if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then > + echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT > + else > + if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" > ] \ > + || [ "$FORCED_PUSH" = true ]; then > + echo "sha=$(git log --pretty=format:"%H %ae" | \ > + grep -m1 -v $(git log -1 --pretty=format:"%ae") | \ > + awk '{print $1}')" >> $GITHUB_OUTPUT > + else > + echo "sha=$EVENT_BEFORE" >> $GITHUB_OUTPUT > + fi > + fi > + > + - name: checkout base branch > + uses: actions/checkout@v3 > + with: > + ref: ${{ steps.base_branch.outputs.sha }} > + path: base_ovs_main > + > + - name: update PATH > + run: | > + echo "$HOME/bin" >> $GITHUB_PATH > + echo "$HOME/.local/bin" >> $GITHUB_PATH > + > + - name: generate cache key > + id: cache_key > + run: | > + ver=$(clang -v 2>&1 | grep version | \ > + sed 's/.*version \([0-9]*\.[0-9]*\.[0-9]*\).*/\1/g') > + echo "key=clang-${ver}-analyze-$(git -C base_ovs_main rev-parse > HEAD)" \ > + >> $GITHUB_OUTPUT > + > + - name: check for analyzer result cache > + id: clang_cache > + uses: actions/cache@v3 > + with: > + path: base-clang-analyzer-results > + key: ${{ steps.cache_key.outputs.key }} > + > + - name: set up python > + uses: actions/setup-python@v4 > + with: > + python-version: '3.9' > + > + - name: get cached dpdk-dir > + uses: actions/cache/restore@v3 > + with: > + path: dpdk-dir > + key: ${{ needs.build-dpdk.outputs.dpdk_key }} > + > + - name: update APT cache > + run: sudo apt update || true > + > + - name: install common dependencies > + run: sudo apt install -y ${{ env.dependencies }} > + > + - name: install sarif tools > + run: sudo pip3 install --disable-pip-version-check sarif-tools > + > + - name: prepare > + run: ./.ci/linux-prepare.sh > + > + - name: build base reference > + if: steps.clang_cache.outputs.cache-hit != 'true' > + run: ./.ci/linux-build.sh > + > + - name: build > + run: ./.ci/linux-build.sh > + > + - name: save cache > + uses: actions/cache/save@v3 > + if: always() && steps.clang_cache.outputs.cache-hit != 'true' > + with: > + path: base-clang-analyzer-results > + key: ${{ steps.cache_key.outputs.key }} > + > build-osx: > env: > CC: clang > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
