[ovs-dev,v5] ci: Add clang-analyze to GitHub actions.

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 determine the preceding commit. These cases are
a new branch or a forced push. The strategy for these exceptions is to
find the first common commit on any upstream branch, and use that.

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 <echaudro@redhat.com>
---

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.

changes in v5:
  - Addressed Ilya's comments.
  - Checkout upstream branch and find common point to base delta on.

 .ci/linux-build.sh                   |  30 +++++++
 .ci/linux-prepare.sh                 |   2 +-
 .github/workflows/build-and-test.yml | 113 +++++++++++++++++++++++++++
 3 files changed, 144 insertions(+), 1 deletion(-)

On Thu, Jan 11, 2024 at 12:08:53AM +0100, 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 determine the preceding commit. These cases are
> a new branch or a forced push. The strategy for these exceptions is to
> find the first common commit on any upstream branch, and use that.
> 
> 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 <echaudro@redhat.com>
> ---
> 
> 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.
> 
> changes in v5:
>   - Addressed Ilya's comments.
>   - Checkout upstream branch and find common point to base delta on.

Thanks, this new logic seems to be working [1].
It seems to be the best approach so far.

Acked-by: Simon Horman <horms@ovn.org>

[1] https://github.com/ovsrobot/ovs/actions/runs/7481945508/job/20364544571

On 12 Jan 2024, at 11:19, Simon Horman wrote:

> On Thu, Jan 11, 2024 at 12:08:53AM +0100, 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 determine the preceding commit. These cases are
>> a new branch or a forced push. The strategy for these exceptions is to
>> find the first common commit on any upstream branch, and use that.
>>
>> 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 <echaudro@redhat.com>
>> ---
>>
>> 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.
>>
>> changes in v5:
>>   - Addressed Ilya's comments.
>>   - Checkout upstream branch and find common point to base delta on.
>
> Thanks, this new logic seems to be working [1].
> It seems to be the best approach so far.
>
> Acked-by: Simon Horman <horms@ovn.org>
>
> [1] https://github.com/ovsrobot/ovs/actions/runs/7481945508/job/20364544571

Thanks for the review, applied as:

* d662eee09 ci: Add clang-analyze to GitHub actions.

On 1/11/24 00:08, 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 determine the preceding commit. These cases are
> a new branch or a forced push. The strategy for these exceptions is to
> find the first common commit on any upstream branch, and use that.
> 
> 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 <echaudro@redhat.com>
> ---
> 
> 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.
> 
> changes in v5:
>   - Addressed Ilya's comments.
>   - Checkout upstream branch and find common point to base delta on.
> 
>  .ci/linux-build.sh                   |  30 +++++++
>  .ci/linux-prepare.sh                 |   2 +-
>  .github/workflows/build-and-test.yml | 113 +++++++++++++++++++++++++++
>  3 files changed, 144 insertions(+), 1 deletion(-)
> 
> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> index 90581c10b..4589a8ba2 100755
> --- a/.ci/linux-build.sh
> +++ b/.ci/linux-build.sh
> @@ -50,6 +50,31 @@ function build_ovs()
>      make ${JOBS}
>  }
>  
> +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.
> +        pushd base_ovs_main
> +    fi;
> +
> +    configure_ovs $OPTS
> +
> +    make clean
> +    scan-build -o ./clang-analyzer-results -sarif --use-cc=${CC} make ${JOBS}
> +
> +    if [ "$cache_build" = true ]; then
> +        # Move results, so it will be picked up by the cache.
> +        mv ./clang-analyzer-results ../base-clang-analyzer-results
> +        popd
> +    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
> @@ -117,6 +142,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/.ci/linux-prepare.sh b/.ci/linux-prepare.sh
> index c28b6819a..5028bdc44 100755
> --- a/.ci/linux-prepare.sh
> +++ b/.ci/linux-prepare.sh
> @@ -23,7 +23,7 @@ cd ..
>  #     https://github.com/pypa/pip/issues/10655
>  pip3 install --disable-pip-version-check --user wheel
>  pip3 install --disable-pip-version-check --user \
> -    flake8 'hacking>=3.0' netaddr pyparsing sphinx setuptools
> +    flake8 'hacking>=3.0' netaddr pyparsing sarif-tools sphinx setuptools
>  
>  # Install python test dependencies
>  pip3 install -r python/test_requirements.txt
> diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml
> index 710757693..f5858fdbe 100644
> --- a/.github/workflows/build-and-test.yml
> +++ b/.github/workflows/build-and-test.yml
> @@ -254,6 +254,119 @@ 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
> +      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
> +            BASE_SHA=HEAD~1
> +            MIN_DISTANCE=1000
> +            git remote add upstream https://github.com/openvswitch/ovs.git
> +            git fetch upstream
> +            for upstream_head in $(git ls-remote --heads upstream main master dpdk-latest branch-2.17 branch-[3456789]* | cut -f 1); do
> +              CURR_BASE=$(git merge-base ${upstream_head} HEAD 2>/dev/null)
> +              if [ ${CURR_BASE} ]; then
> +                DISTANCE=$(git log --oneline ${CURR_BASE}..HEAD | wc -l);
> +                if test ${MIN_DISTANCE} -gt ${DISTANCE}; then
> +                  BASE_SHA=${CURR_BASE}
> +                  MIN_DISTANCE=${DISTANCE}
> +                fi
> +              fi
> +            done
> +            echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT
> +          else
> +            echo "sha=$EVENT_BEFORE" >> $GITHUB_OUTPUT
> +          fi
> +        fi
> +
> +    - name: checkout base branch
> +      env:
> +        BASE_SHA: ${{ steps.base_branch.outputs.sha }}
> +      run: |
> +        cp -r $(pwd)/. /tmp/base_ovs_main && mv /tmp/base_ovs_main ./
> +        cd $(pwd)/base_ovs_main
> +        git checkout ${BASE_SHA}
> +
> +    - name: update PATH
> +      run: |
> +        echo "$HOME/bin"        >> $GITHUB_PATH
> +        echo "$HOME/.local/bin" >> $GITHUB_PATH
> +
> +    - name: generate cache key
> +      id: cache_key
> +      run: |
> +        ver=$(${CC} -v 2>&1 | grep ' version ' | \
> +              sed 's/.*version \([0-9]*\.[0-9]*\.[0-9]*\).*/\1/g')
> +        echo "key=${CC}-${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: prepare
> +      run:  ./.ci/linux-prepare.sh
> +
> +    - name: build base reference
> +      if: steps.clang_cache.outputs.cache-hit != 'true'
> +      run:  ./.ci/linux-build.sh

FWIW, this is not working if the base requires different version of DPDK.
Not sure what the solution would be though, except for the obvious disabling
DPDK for this job.

> +
> +    - name: save cache
> +      uses: actions/cache/save@v3
> +      if: steps.clang_cache.outputs.cache-hit != 'true'
> +      with:
> +        path: base-clang-analyzer-results
> +        key:  ${{ steps.cache_key.outputs.key }}
> +
> +    - name: build
> +      run:  ./.ci/linux-build.sh
> +
>    build-osx:
>      env:
>        CC:    clang