On 12/7/23 10:32, Eelco Chaudron wrote:
> This patch detects new static analyze issues, and report them.
> It does this by reporting on the delta for this branch, compared
> to the base branch.
>
> For example the error 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.
>
> .ci/linux-build.sh | 29 +++++++++
> .github/workflows/build-and-test.yml | 106
> ++++++++++++++++++++++++++++++++++
> 2 files changed, 135 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
> + fi;
> +
> + configure_ovs $OPTS
> + make clean
> + scan-build -o ./clang-analyzer-results -sarif --use-cc=clang make -j4
> +
> + 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..f8a000490 100644
> --- a/.github/workflows/build-and-test.yml
> +++ b/.github/workflows/build-and-test.yml
> @@ -223,6 +223,112 @@ 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
> + 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 }}
> + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }}
> + FORCED_PUSH: ${{ github.event.forced }}
> + run: |
> + if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then
> + echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT
> + else
> + if [ "$FORCED_PUSH" = true ] || [ "$EVENT_BEFORE" -eq 0 ]; then
> + set sha=$(git describe --all --no-abbrev --always \
> + --match master --match main \
> + --match branch-[0-9].[0-9]* | \
> + sed 's/.*\///')
> + [ -z "$sha" ] && echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT \
> + || echo "sha=$sha" >> $GITHUB_OUTPUT
Hi, Eelco. I was trying to test this patch and it breaks my workflow.
The issue is that commands above is looking for base branches in my
fork. It finds the 'master' branch, but this branch in my fork is
3-4 years old and fails to build. Even if it worked, comparing against
a version that old doesn't make sense. Also, it might happen that
the feature branch is based on an older master, which will cause
problems.
In even simpler example, if someone is working on a new feature in the
'master' branch of their own fork and force-pushing it, there is no
base branch to compare with.
This may also affect the robot, though to the lesser extent, because
IIRC robot updates its 'master' branch periodically. But it may still
add unrelated commits that went into upstream repo between updates
into the check, which may be confusing.
Looking for a base branch in a different repository (upstream one) also
doesn't seem right, because the fork may diverge from upstream and
comparing with a newer branch may case issues.
It appears to me that there is no actual way to find a base branch
on force-push or branch creation, simply because that base branch
may just not exist in the repository.
I suggested before to just test against HEAD~1, but that may give a
false sense of everything being fine while the issue is in HEAD~2.
So, that would work only for a robot that pushes patches one by one.
The only solution I see here is to skip the test under this conditions,
at least it will be visible that it didn't run. Robot, I guess,
can be modified to first create a branch and then start pushing changes,
so the tests will not be skipped for it.
But humans will just have to pay attention to tests being or not being
run, unfortunately. If we used GitLab pipelines we could have worked
around the problem by providing git push options (git push -o my_base=SHA),
but GHA doesn't seem to support that, i.e. push options in GHA are not
accessible. I don't know if those are available in CirrusCI.
Thoughts?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
