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. > //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
