On 2 Jan 2024, at 13:43, Ilya Maximets wrote:
> 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.
ACK, I think going back in time till a different author will solve all this for
now. And this will even work in Ilya’s and my force commit workflow. So for now
I’ll keep it as it.
>>
>> 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.
ACK will do this next revision.
>
>>> + 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".
Let me look into this, as I like the idea.
> Also, -j4 should be changed to ${JOBS} since that patch got merged.
ACK
> And we have CC defined in the job description, so should not use clang
> directly.
ACK
>>> +
>>> + 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?
Added it, as I removed the 32-bit conditional install, but can verify if it
fails without.
>>> + 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. :)
ACK, will make the change below as well.
>>> + 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.
Let me move it to see if this will work.
>>> +
>>> + - 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?
Good catch, will move it up.
>>> + with:
>>> + path: base-clang-analyzer-results
>>> + key: ${{ steps.cache_key.outputs.key }}
>>> +
>>> build-osx:
>>> env:
>>> CC: clang
>>>
>
> Best regards, Ilya Maximets.
Thanks for the comments, will prep a v5.
//Eelco
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
