On 1/2/24 16:19, Eelco Chaudron wrote:
>
>
> 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.
It will not work in case you're incorporating someone else's patches in
your patch set, unless you're pushing them one by one. Robot will catch
such problems, but people may miss. Same problem if you are accumulating
multiple different patches before batch-applying them.
>
>>>
>>> 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
