Re: [PATCH 1/6] ci: add a GitHub workflow to submit Coverity scans

From: Jeff King <peff@peff.net>
Date: Sep 23, 2023, 06:49 UTC
Message-ID: <20230923064948.GB1469941@coredump.intra.peff.net>
In-Reply-To: <8cb92968c5ebd38f328ed325ddf7f2e531dc9190.1695379323.git.gitgitgadget@gmail.com>

On Fri, Sep 22, 2023 at 10:41:58AM +0000, Johannes Schindelin via GitGitGadget wrote:

Show 8 quoted lines

> Note: The initial version of this patch used
> `vapier/coverity-scan-action` to benefit from that Action's caching of
> the Coverity tool, which is rather large. Sadly, that Action only
> supports Linux, and we want to have the option of building on Windows,
> too. Besides, in the meantime Coverity requires `cov-configure` to be
> runantime, and that Action was not adjusted accordingly, i.e. it seems
> not to be maintained actively. Therefore it would seem prudent to
> implement the steps manually instead of using that Action.

I'm still unsure of the cov-configure thing, as I have never needed it (and the "vapier" Action worked fine for me). But the lack of Windows support is obviously a deal-breaker. I wondered if it might be worth trying to submit a PR to that project to fix it for everybody, but I saw that you commented on their "Windows support" issue, which has been sitting unanswered since it was opened in May. It's possible they might be more responsive to an actual PR, but I agree that it may be simpler to just go our own way here.

Show 8 quoted lines

> +jobs:
> +  coverity:
> +    if: contains(fromJSON(vars.ENABLE_COVERITY_SCAN_FOR_BRANCHES || '[""]'), github.ref_name)
> +    runs-on: ubuntu-latest
> +    env:
> +      COVERITY_PROJECT: git
> +      COVERITY_LANGUAGE: cxx
> +      COVERITY_PLATFORM: linux64

Ah, now I see why you were bothered by using "git/git" at the project name earlier. That is what I assumed we would use (and certainly I use "peff/git" on the Coverity side), but I forgot that we already have the general "git" name on the Coverity side (which isn't to say we couldn't switch to using the "git/git" name, but I am happy for us to be just "git" there).

Show 12 quoted lines

> +    steps:
> +      - uses: actions/checkout@v3
> +      - run: ci/install-dependencies.sh
> +        env:
> +          runs_on_pool: ubuntu-latest
> +
> +      - name: download the Coverity Build Tool (${{ env.COVERITY_LANGUAGE }} / ${{ env.COVERITY_PLATFORM}})
> +        run: |
> +          curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
> +            --no-progress-meter \
> +            --output $RUNNER_TEMP/cov-analysis.tgz \
> +            --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT"

You might want "--fail" or "--fail-with-body" here. I think any server-side errors (like a missing or invalid token or project name) will result in a 401. Having curl reported that as a non-zero exit should stop the Action with a failure, rather than silently continuing.

This is mostly a style suggestion, but I think you can use:

  --form token="${{ secrets.COVERITY_SCAN_TOKEN }}" \
  --form project="$COVERITY_PROJECT"

here, which IMHO is a little more readable than "data". It probably doesn't matter in practice, but I think it would also would handle any encoding for us (though note that if we wanted to be really careful, the TOKEN secret would need shell quoting).

Using --form will use multipart/form-data instead of application/x-www-form-url-encoded, but coverity seems happy with either.

> +      - name: extract the Coverity Build Tool
> +        run: |
> +          mkdir $RUNNER_TEMP/cov-analysis &&
> +          tar -xzf $RUNNER_TEMP/cov-analysis.tgz --strip 1 -C $RUNNER_TEMP/cov-analysis

OK, we are starting without Windows support yet. :)

Show 7 quoted lines

> +      - name: build with cov-build
> +        run: |
> +          export PATH="$RUNNER_TEMP/cov-analysis/bin:$PATH" &&
> +          cov-configure --gcc &&
> +          cov-build --dir cov-int make -j$(nproc)
> +      - name: package the build
> +        run: tar -czvf cov-int.tgz cov-int

OK, this looks a lot like what my custom rule does (no surprise, since we are all adapting Coverity's instructions).

Show 8 quoted lines

> +      - name: submit the build to Coverity Scan
> +        run: |
> +          curl \
> +            --form token="${{ secrets.COVERITY_SCAN_TOKEN }}" \
> +            --form email="${{ secrets.COVERITY_SCAN_EMAIL }}" \
> +            --form file=@cov-int.tgz \
> +            --form version="${{ github.sha }}" \
> +            "https://scan.coverity.com/builds?project=$COVERITY_PROJECT"

Likewise. I add:

  --form description="$(./git version)"

to mine, but I am not even sure where that ends up (the "version" is probably the most interesting bit, as that is shown on the Coverity project page).

I notice you put the "project" variable in the query string. Can it be a --form, too, for symmetry? (In mine, I seem to have it as _both_, which is probably just a mistake). Not a huge deal either way, but just a small readability thing.

As with above, we'd probably want --fail or --fail-with-body here to detect errors (since otherwise a failed upload goes completely unreported).

-Peff