On 20 Dec 2023, at 15:04, Aaron Conole wrote:
> Eelco Chaudron <[email protected]> writes: > >> 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 :) > > 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 copied your reply to the v4, so we can track it there. //Eelco >> //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
