| Message ID | 170110654511.400322.6668472531731038475.stgit@ebuild |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] ci: Add clang-analyze to GitHub actions. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 11/27/23 18:36, 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 previous 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 <echaudro@redhat.com> > --- > > changes in v2: > - When it's a new branch, it compares it to the HEAD of the default branch. > > .ci/linux-build.sh | 29 ++++++++++ > .github/workflows/build-and-test.yml | 96 ++++++++++++++++++++++++++++++++++ > 2 files changed, 125 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..d15105e7d 100644 > --- a/.github/workflows/build-and-test.yml > +++ b/.github/workflows/build-and-test.yml > @@ -223,6 +223,102 @@ 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 > + > + - name: get base branch sha > + id: base_branch > + run: | > + if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then > + echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT > + else > + if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then > + echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT How this is going ot work on patches for older branches? > + else > + echo "sha=$EVENT_BEFORE" >> $GITHUB_OUTPUT > + fi > + fi > + env: > + BASE_SHA: ${{ github.event.pull_request.base.sha }} > + EVENT_BEFORE: ${{ github.event.before }} > + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} Can we swap env and run sections, so everything that is used is already defined? > + > + - name: checkout base branch > + uses: actions/checkout@v3 > + with: > + ref: ${{ steps.base_branch.outputs.sha }} > + path: base_ovs_main > + > + - name: update PATH > + run: | > + echo "$HOME/bin" >> $GITHUB_PATH > + echo "$HOME/.local/bin" >> $GITHUB_PATH > + > + - name: generate cache key > + id: cache_key > + run: | > + echo "key=clang-analyze-$(git -C base_ovs_main rev-parse HEAD)" \ > + >> $GITHUB_OUTPUT > + > + - name: check for analyzer result cache > + id: clang_cache > + uses: actions/cache@v3 > + with: > + path: base-clang-analyzer-results > + key: ${{ steps.cache_key.outputs.key }} > + > + - name: set up python > + uses: actions/setup-python@v4 > + with: > + python-version: '3.9' > + > + - name: get cached dpdk-dir > + uses: actions/cache/restore@v3 > + with: > + path: dpdk-dir > + key: ${{ needs.build-dpdk.outputs.dpdk_key }} > + > + - name: update APT cache > + run: sudo apt update || true > + > + - name: install common dependencies > + run: sudo apt install -y ${{ env.dependencies }} > + > + - name: install sarif tools > + run: sudo pip3 install --disable-pip-version-check sarif-tools > + > + - name: prepare > + run: ./.ci/linux-prepare.sh > + > + - name: build base reference > + if: steps.clang_cache.outputs.cache-hit != 'true' > + run: ./.ci/linux-build.sh > + > + - name: build > + run: ./.ci/linux-build.sh > + > + - name: save cache > + uses: actions/cache/save@v3 > + if: always() && steps.clang_cache.outputs.cache-hit != 'true' > + with: > + path: base-clang-analyzer-results > + key: ${{ steps.cache_key.outputs.key }} > + > build-osx: > env: > CC: clang > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 27 Nov 2023, at 19:18, Ilya Maximets wrote: > On 11/27/23 18:36, 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 previous 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 <echaudro@redhat.com> >> --- >> >> changes in v2: >> - When it's a new branch, it compares it to the HEAD of the default branch. >> >> .ci/linux-build.sh | 29 ++++++++++ >> .github/workflows/build-and-test.yml | 96 ++++++++++++++++++++++++++++++++++ >> 2 files changed, 125 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..d15105e7d 100644 >> --- a/.github/workflows/build-and-test.yml >> +++ b/.github/workflows/build-and-test.yml >> @@ -223,6 +223,102 @@ 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 >> + >> + - name: get base branch sha >> + id: base_branch >> + run: | >> + if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then >> + echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT >> + else >> + if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then >> + echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT > > How this is going ot work on patches for older branches? Good question, it’s not :( Took a little bit to figure out how to do this, as we have no reference branch. The solution I came up with was to figure out all parent branches, and use the most recent main/master or branch-x.x one. So it looks like this: if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT else if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then set sha=$(git log --simplify-by-decoration --decorate=full \ --pretty=format:'%d' | \ grep -oP 'refs/remotes/origin/\K[^, )]+' | \ grep -m1 -E '^master$|^main$|^branch-[0-9]+\.[0-9]+$') [ -z "$sha" ] && echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT \ || echo "sha=$sha" >> $GITHUB_OUTPUT else echo "sha=$EVENT_BEFORE" >> $GITHUB_OUTPUT fi fi >> + else >> + echo "sha=$EVENT_BEFORE" >> $GITHUB_OUTPUT >> + fi >> + fi >> + env: >> + BASE_SHA: ${{ github.event.pull_request.base.sha }} >> + EVENT_BEFORE: ${{ github.event.before }} >> + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} > > Can we swap env and run sections, so everything that is used > is already defined? Actually, the order does not matter as the env is set before run in the same step, but I will swap as it’s more easy to understand/read what is going on. I’ll try to send out a v2 later this week. >> + >> + - name: checkout base branch >> + uses: actions/checkout@v3 >> + with: >> + ref: ${{ steps.base_branch.outputs.sha }} >> + path: base_ovs_main >> + >> + - name: update PATH >> + run: | >> + echo "$HOME/bin" >> $GITHUB_PATH >> + echo "$HOME/.local/bin" >> $GITHUB_PATH >> + >> + - name: generate cache key >> + id: cache_key >> + run: | >> + echo "key=clang-analyze-$(git -C base_ovs_main rev-parse HEAD)" \ >> + >> $GITHUB_OUTPUT >> + >> + - name: check for analyzer result cache >> + id: clang_cache >> + uses: actions/cache@v3 >> + with: >> + path: base-clang-analyzer-results >> + key: ${{ steps.cache_key.outputs.key }} >> + >> + - name: set up python >> + uses: actions/setup-python@v4 >> + with: >> + python-version: '3.9' >> + >> + - name: get cached dpdk-dir >> + uses: actions/cache/restore@v3 >> + with: >> + path: dpdk-dir >> + key: ${{ needs.build-dpdk.outputs.dpdk_key }} >> + >> + - name: update APT cache >> + run: sudo apt update || true >> + >> + - name: install common dependencies >> + run: sudo apt install -y ${{ env.dependencies }} >> + >> + - name: install sarif tools >> + run: sudo pip3 install --disable-pip-version-check sarif-tools >> + >> + - name: prepare >> + run: ./.ci/linux-prepare.sh >> + >> + - name: build base reference >> + if: steps.clang_cache.outputs.cache-hit != 'true' >> + run: ./.ci/linux-build.sh >> + >> + - name: build >> + run: ./.ci/linux-build.sh >> + >> + - name: save cache >> + uses: actions/cache/save@v3 >> + if: always() && steps.clang_cache.outputs.cache-hit != 'true' >> + with: >> + path: base-clang-analyzer-results >> + key: ${{ steps.cache_key.outputs.key }} >> + >> build-osx: >> env: >> CC: clang >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On Tue, Nov 28, 2023 at 06:50:59PM +0100, Eelco Chaudron wrote: > > > On 27 Nov 2023, at 19:18, Ilya Maximets wrote: > > > On 11/27/23 18:36, Eelco Chaudron wrote: ... > >> diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml > >> index 09654205e..d15105e7d 100644 > >> --- a/.github/workflows/build-and-test.yml > >> +++ b/.github/workflows/build-and-test.yml > >> @@ -223,6 +223,102 @@ 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 > >> + > >> + - name: get base branch sha > >> + id: base_branch > >> + run: | > >> + if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then > >> + echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT > >> + else > >> + if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then > >> + echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT > > > > How this is going ot work on patches for older branches? > > Good question, it’s not :( Took a little bit to figure out how to do this, as we have no reference branch. The solution I came up with was to figure out all parent branches, and use the most recent main/master or branch-x.x one. So it looks like this: > > if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then > echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT > else > if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then > set sha=$(git log --simplify-by-decoration --decorate=full \ > --pretty=format:'%d' | \ > grep -oP 'refs/remotes/origin/\K[^, )]+' | \ > grep -m1 -E '^master$|^main$|^branch-[0-9]+\.[0-9]+$') > [ -z "$sha" ] && echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT \ > || echo "sha=$sha" >> $GITHUB_OUTPUT > else > echo "sha=$EVENT_BEFORE" >> $GITHUB_OUTPUT > fi > fi Hi Eelco, is this useful here? git describe --all --no-abbrev --always \ --match master --match main --match branch-[0-9].[0-9]* ...
On 30 Nov 2023, at 13:29, Simon Horman wrote: > On Tue, Nov 28, 2023 at 06:50:59PM +0100, Eelco Chaudron wrote: >> >> >> On 27 Nov 2023, at 19:18, Ilya Maximets wrote: >> >>> On 11/27/23 18:36, Eelco Chaudron wrote: > > ... > >>>> diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml >>>> index 09654205e..d15105e7d 100644 >>>> --- a/.github/workflows/build-and-test.yml >>>> +++ b/.github/workflows/build-and-test.yml >>>> @@ -223,6 +223,102 @@ 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 >>>> + >>>> + - name: get base branch sha >>>> + id: base_branch >>>> + run: | >>>> + if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then >>>> + echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT >>>> + else >>>> + if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then >>>> + echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT >>> >>> How this is going ot work on patches for older branches? >> >> Good question, it’s not :( Took a little bit to figure out how to do this, as we have no reference branch. The solution I came up with was to figure out all parent branches, and use the most recent main/master or branch-x.x one. So it looks like this: >> >> if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then >> echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT >> else >> if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then >> set sha=$(git log --simplify-by-decoration --decorate=full \ >> --pretty=format:'%d' | \ >> grep -oP 'refs/remotes/origin/\K[^, )]+' | \ >> grep -m1 -E '^master$|^main$|^branch-[0-9]+\.[0-9]+$') >> [ -z "$sha" ] && echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT \ >> || echo "sha=$sha" >> $GITHUB_OUTPUT >> else >> echo "sha=$EVENT_BEFORE" >> $GITHUB_OUTPUT >> fi >> fi > > Hi Eelco, > > is this useful here? > > git describe --all --no-abbrev --always \ > --match master --match main --match branch-[0-9].[0-9]* Thanks Simon, I was looking for something like this, but I was not able to get it with ‘git describe’, but now I see the match option is the key ;) I guess it can now be changed to: git describe --all --no-abbrev --always \ --match master --match main --match branch-[0-9].[0-9]* | sed 's/.*\///' Will send a v3 next week, waiting for some more potential feedback… //Eelco > > ...
On 11/30/23 14:08, Eelco Chaudron wrote: > > > On 30 Nov 2023, at 13:29, Simon Horman wrote: > >> On Tue, Nov 28, 2023 at 06:50:59PM +0100, Eelco Chaudron wrote: >>> >>> >>> On 27 Nov 2023, at 19:18, Ilya Maximets wrote: >>> >>>> On 11/27/23 18:36, Eelco Chaudron wrote: >> >> ... >> >>>>> diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml >>>>> index 09654205e..d15105e7d 100644 >>>>> --- a/.github/workflows/build-and-test.yml >>>>> +++ b/.github/workflows/build-and-test.yml >>>>> @@ -223,6 +223,102 @@ 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 >>>>> + >>>>> + - name: get base branch sha >>>>> + id: base_branch >>>>> + run: | >>>>> + if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then >>>>> + echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT >>>>> + else >>>>> + if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then >>>>> + echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT >>>> >>>> How this is going ot work on patches for older branches? >>> >>> Good question, it’s not :( Took a little bit to figure out how to do this, as we have no reference branch. The solution I came up with was to figure out all parent branches, and use the most recent main/master or branch-x.x one. So it looks like this: >>> >>> if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then >>> echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT >>> else >>> if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then >>> set sha=$(git log --simplify-by-decoration --decorate=full \ >>> --pretty=format:'%d' | \ >>> grep -oP 'refs/remotes/origin/\K[^, )]+' | \ >>> grep -m1 -E '^master$|^main$|^branch-[0-9]+\.[0-9]+$') >>> [ -z "$sha" ] && echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT \ >>> || echo "sha=$sha" >> $GITHUB_OUTPUT >>> else >>> echo "sha=$EVENT_BEFORE" >> $GITHUB_OUTPUT >>> fi >>> fi >> >> Hi Eelco, >> >> is this useful here? >> >> git describe --all --no-abbrev --always \ >> --match master --match main --match branch-[0-9].[0-9]* > > Thanks Simon, I was looking for something like this, but I was not able to get it with ‘git describe’, but now I see the match option is the key ;) > > I guess it can now be changed to: > > git describe --all --no-abbrev --always \ > --match master --match main --match branch-[0-9].[0-9]* | sed 's/.*\///' > > Will send a v3 next week, waiting for some more potential feedback… Maybe we could just make it simple and test only the last commit (HEAD~1)? Unless the base is explicitly known. It should be enough for a 0-day bot, because it is testing patch sets incrementally, i.e. applying patches one by one. Also, the EVENT_BEFORE seems to not be documented anywhere, I wonder how safe it is to rely on this variable. Or did I miss the docs somewhere? Would also be nice to know how this variable behaves on force-push with a branch history rolling back as it might end up with spurious failures during local feature development. I also frequently use the same branches in my fork during code review, force-pushing them frequently, so this process may be affected. Best regards, Ilya Maximets.
On 11/30/23 14:25, Ilya Maximets wrote: > On 11/30/23 14:08, Eelco Chaudron wrote: >> >> >> On 30 Nov 2023, at 13:29, Simon Horman wrote: >> >>> On Tue, Nov 28, 2023 at 06:50:59PM +0100, Eelco Chaudron wrote: >>>> >>>> >>>> On 27 Nov 2023, at 19:18, Ilya Maximets wrote: >>>> >>>>> On 11/27/23 18:36, Eelco Chaudron wrote: >>> >>> ... >>> >>>>>> diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml >>>>>> index 09654205e..d15105e7d 100644 >>>>>> --- a/.github/workflows/build-and-test.yml >>>>>> +++ b/.github/workflows/build-and-test.yml >>>>>> @@ -223,6 +223,102 @@ 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 >>>>>> + >>>>>> + - name: get base branch sha >>>>>> + id: base_branch >>>>>> + run: | >>>>>> + if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then >>>>>> + echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT >>>>>> + else >>>>>> + if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then >>>>>> + echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT >>>>> >>>>> How this is going ot work on patches for older branches? >>>> >>>> Good question, it’s not :( Took a little bit to figure out how to do this, as we have no reference branch. The solution I came up with was to figure out all parent branches, and use the most recent main/master or branch-x.x one. So it looks like this: >>>> >>>> if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then >>>> echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT >>>> else >>>> if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then >>>> set sha=$(git log --simplify-by-decoration --decorate=full \ >>>> --pretty=format:'%d' | \ >>>> grep -oP 'refs/remotes/origin/\K[^, )]+' | \ >>>> grep -m1 -E '^master$|^main$|^branch-[0-9]+\.[0-9]+$') >>>> [ -z "$sha" ] && echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT \ >>>> || echo "sha=$sha" >> $GITHUB_OUTPUT >>>> else >>>> echo "sha=$EVENT_BEFORE" >> $GITHUB_OUTPUT >>>> fi >>>> fi >>> >>> Hi Eelco, >>> >>> is this useful here? >>> >>> git describe --all --no-abbrev --always \ >>> --match master --match main --match branch-[0-9].[0-9]* >> >> Thanks Simon, I was looking for something like this, but I was not able to get it with ‘git describe’, but now I see the match option is the key ;) >> >> I guess it can now be changed to: >> >> git describe --all --no-abbrev --always \ >> --match master --match main --match branch-[0-9].[0-9]* | sed 's/.*\///' >> >> Will send a v3 next week, waiting for some more potential feedback… > > Maybe we could just make it simple and test only the last commit (HEAD~1)? > Unless the base is explicitly known. It should be enough for a 0-day bot, > because it is testing patch sets incrementally, i.e. applying patches one > by one. > > Also, the EVENT_BEFORE seems to not be documented anywhere, I wonder how > safe it is to rely on this variable. Or did I miss the docs somewhere? > Would also be nice to know how this variable behaves on force-push with a > branch history rolling back as it might end up with spurious failures during > local feature development. I also frequently use the same branches in my > fork during code review, force-pushing them frequently, so this process > may be affected. > One more thing about clang-analyze. Should the clang version be part of the cache key? I'm guessing that if the previous cache is constructed with an older version of the checker, we may have extra warnings reported, unrelated to the current patch. > Best regards, Ilya Maximets.
On 30 Nov 2023, at 14:28, Ilya Maximets wrote: > On 11/30/23 14:25, Ilya Maximets wrote: >> On 11/30/23 14:08, Eelco Chaudron wrote: >>> >>> >>> On 30 Nov 2023, at 13:29, Simon Horman wrote: >>> >>>> On Tue, Nov 28, 2023 at 06:50:59PM +0100, Eelco Chaudron wrote: >>>>> >>>>> >>>>> On 27 Nov 2023, at 19:18, Ilya Maximets wrote: >>>>> >>>>>> On 11/27/23 18:36, Eelco Chaudron wrote: >>>> >>>> ... >>>> >>>>>>> diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml >>>>>>> index 09654205e..d15105e7d 100644 >>>>>>> --- a/.github/workflows/build-and-test.yml >>>>>>> +++ b/.github/workflows/build-and-test.yml >>>>>>> @@ -223,6 +223,102 @@ 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 >>>>>>> + >>>>>>> + - name: get base branch sha >>>>>>> + id: base_branch >>>>>>> + run: | >>>>>>> + if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then >>>>>>> + echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT >>>>>>> + else >>>>>>> + if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then >>>>>>> + echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT >>>>>> >>>>>> How this is going ot work on patches for older branches? >>>>> >>>>> Good question, it’s not :( Took a little bit to figure out how to do this, as we have no reference branch. The solution I came up with was to figure out all parent branches, and use the most recent main/master or branch-x.x one. So it looks like this: >>>>> >>>>> if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then >>>>> echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT >>>>> else >>>>> if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then >>>>> set sha=$(git log --simplify-by-decoration --decorate=full \ >>>>> --pretty=format:'%d' | \ >>>>> grep -oP 'refs/remotes/origin/\K[^, )]+' | \ >>>>> grep -m1 -E '^master$|^main$|^branch-[0-9]+\.[0-9]+$') >>>>> [ -z "$sha" ] && echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT \ >>>>> || echo "sha=$sha" >> $GITHUB_OUTPUT >>>>> else >>>>> echo "sha=$EVENT_BEFORE" >> $GITHUB_OUTPUT >>>>> fi >>>>> fi >>>> >>>> Hi Eelco, >>>> >>>> is this useful here? >>>> >>>> git describe --all --no-abbrev --always \ >>>> --match master --match main --match branch-[0-9].[0-9]* >>> >>> Thanks Simon, I was looking for something like this, but I was not able to get it with ‘git describe’, but now I see the match option is the key ;) >>> >>> I guess it can now be changed to: >>> >>> git describe --all --no-abbrev --always \ >>> --match master --match main --match branch-[0-9].[0-9]* | sed 's/.*\///' >>> >>> Will send a v3 next week, waiting for some more potential feedback… >> >> Maybe we could just make it simple and test only the last commit (HEAD~1)? >> Unless the base is explicitly known. It should be enough for a 0-day bot, >> because it is testing patch sets incrementally, i.e. applying patches one >> by one. This will work for the robot, but not on our main branch? But I guess you only mean for this case where we do not have a valid EVENT_BEFORE? >> Also, the EVENT_BEFORE seems to not be documented anywhere, I wonder how >> safe it is to rely on this variable. Or did I miss the docs somewhere? It’s the API variable ${{ github.event.before }} --> https://docs.github.com/en/rest/overview/github-event-types?apiVersion=2022-11-28#event-payload-object-for-pushevent >> Would also be nice to know how this variable behaves on force-push with a >> branch history rolling back as it might end up with spurious failures during >> local feature development. I also frequently use the same branches in my >> fork during code review, force-pushing them frequently, so this process >> may be affected. Thought I looked at the forced push as I use this all the time with stg, and I think it was the ref of the overwritten pushed branch (but I’ll take a look again before I send the v3). > One more thing about clang-analyze. Should the clang version be part of > the cache key? I'm guessing that if the previous cache is constructed > with an older version of the checker, we may have extra warnings reported, > unrelated to the current patch. Good catch!!
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..d15105e7d 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -223,6 +223,102 @@ 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 + + - name: get base branch sha + id: base_branch + run: | + if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then + echo "sha=$BASE_SHA" >> $GITHUB_OUTPUT + else + if [ "$EVENT_BEFORE" = "0000000000000000000000000000000000000000" ]; then + echo "sha=$DEFAULT_BRANCH" >> $GITHUB_OUTPUT + else + echo "sha=$EVENT_BEFORE" >> $GITHUB_OUTPUT + fi + fi + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + EVENT_BEFORE: ${{ github.event.before }} + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + + - name: checkout base branch + uses: actions/checkout@v3 + with: + ref: ${{ steps.base_branch.outputs.sha }} + path: base_ovs_main + + - name: update PATH + run: | + echo "$HOME/bin" >> $GITHUB_PATH + echo "$HOME/.local/bin" >> $GITHUB_PATH + + - name: generate cache key + id: cache_key + run: | + echo "key=clang-analyze-$(git -C base_ovs_main rev-parse HEAD)" \ + >> $GITHUB_OUTPUT + + - name: check for analyzer result cache + id: clang_cache + uses: actions/cache@v3 + with: + path: base-clang-analyzer-results + key: ${{ steps.cache_key.outputs.key }} + + - name: set up python + uses: actions/setup-python@v4 + with: + python-version: '3.9' + + - name: get cached dpdk-dir + uses: actions/cache/restore@v3 + with: + path: dpdk-dir + key: ${{ needs.build-dpdk.outputs.dpdk_key }} + + - name: update APT cache + run: sudo apt update || true + + - name: install common dependencies + run: sudo apt install -y ${{ env.dependencies }} + + - name: install sarif tools + run: sudo pip3 install --disable-pip-version-check sarif-tools + + - name: prepare + run: ./.ci/linux-prepare.sh + + - name: build base reference + if: steps.clang_cache.outputs.cache-hit != 'true' + run: ./.ci/linux-build.sh + + - name: build + run: ./.ci/linux-build.sh + + - name: save cache + uses: actions/cache/save@v3 + if: always() && steps.clang_cache.outputs.cache-hit != 'true' + with: + path: base-clang-analyzer-results + key: ${{ steps.cache_key.outputs.key }} + build-osx: env: CC: clang
This patch detects new static analyze issues, and report them. It does this by reporting on the delta for this branch, compared to the previous 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 <echaudro@redhat.com> --- changes in v2: - When it's a new branch, it compares it to the HEAD of the default branch. .ci/linux-build.sh | 29 ++++++++++ .github/workflows/build-and-test.yml | 96 ++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+)