On 19 Dec 2023, at 16:35, Eelco Chaudron wrote:
> 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. 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 :) //Eelco > 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
