On 12/20/23 16:07, Eelco Chaudron wrote:
>
>
> 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.
The 'determine the origin branch' is still tricky and requires some
guesswork in case of a new branch or a force push. Maybe something
like listing all the heads in upstream repo and finding the one that
has the closest merge-base may be an option. Then use that merge base
as a base commit to compare against.
The merge-base with upstream/master will be a branching point for a
release branch, and that is way too far.
>
> 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 <echau...@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.
>>
>> .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
pushd/popd might be better here.
>> + fi;
>> +
>> + configure_ovs $OPTS
>> + make clean
>> + scan-build -o ./clang-analyzer-results -sarif --use-cc=clang make -j4
We have a make target for clang-analyze and it depends on the clean already.
Maybe we can extend it to accept custom arguments via env variable and pass
the '-sarif' this way? E.g. make clang-analyze SCAN_BUILD_OPTIONS="-sarif".
Also, -j4 should be changed to ${JOBS} since that patch got merged.
And we have CC defined in the job description, so should not use clang directly.
>> +
>> + 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
Do we need a 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 | \
s/clang/${CC}/ ?
And if we grep for ' version ', the check will work for gcc as well,
even if we do not really care about this case. :)
>> + 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
'--user' to avoid sudo. Might also be better to add this to the
existing command in the linux-prepare script instead to allow pip
to choose compatible versions of all the packages.
>> +
>> + - 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'
So, we will save the cache even if the build of the base reference failed?
Maybe move this before the 'build' stage and remove the 'always()' check?
>> + with:
>> + path: base-clang-analyzer-results
>> + key: ${{ steps.cache_key.outputs.key }}
>> +
>> build-osx:
>> env:
>> CC: clang
>>
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
