On 15 Dec 2023, at 10:34, Ilya Maximets wrote:
> 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 <[email protected]> >> --- >> 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. As you pointed out, using HEAD~1 doesn't address the issue in scenarios where multiple patches exist. The original behavior of forced commits involved comparing them with the commit they were overwriting. This approach revealed only new issues, neglecting to display pre-existing ones, those that remained unresolved. This led me to modify the process, making both ‘new branches’ and ‘forced pushes’ attempt to identify the most recent branch. > 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. There is no way to mark the test as skipped once you start it. I could try to split up the tests, but it might require an additional checkout. > 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. I guess pushing the empty branch will already trigger a GitHub action run, don’t think this is what we want either. > But humans will just have to pay attention to tests being or not being > run, unfortunately. Not sure how to solve this either… The robot will have the “old branch” problem, but a HEAD~1 will work here. For local commits with new branches, this will ignore potential other patches, but the last one. This might cause them to be unnoticed during development, but the robot will catch this ;) I’m leaning towards doing the patch - 1 for force and new branch commits. For a short moment, I thought about looking at the commit time stamps, but this did not work either. 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. Ilya/Aaron/Simon thoughts? > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
