On 1/2/24 17:14, Eelco Chaudron wrote:
>
>
> On 2 Jan 2024, at 16:55, Ilya Maximets wrote:
>
>> 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.
>
> ACK, this is true. But with this patch in place, the robot should have
> captured this for you ;)
> And your actual push to the main branch will also fail. Guess the previously
> suggested patch -1
> will have the same problem.
>
> Don’t think there will be a ‘full’ solution that will work in all cases 100%
> of the time.
But why don't you like "closest merge-base against upstream heads" approach?
It seems less error prone to me. At least I can't right away pinpoint a
simple scenario where it will do something unexpected, unless the branch is
completely diverged from anything upstream.
> This approach works for individual developers and the robot. For maintainers,
> we can push the
> new branch, and then the actual patches, although making changes in the
> middle might require
> deleting the remote branch, re-creating it, and re-apply. Tried this with
> stgit, and this seems easy.
>
> //Eelco
>
>>>>> 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
