| Message ID | 8cb92968c5ebd38f328ed325ddf7f2e531dc9190.1695379323.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add a GitHub workflow to submit builds to Coverity Scan | expand |
On Fri, Sep 22, 2023 at 10:41:58AM +0000, Johannes Schindelin via GitGitGadget wrote: > 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. > +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). > + 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. :) > + - 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). > + - 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
Hi Peff, On Sat, 23 Sep 2023, Jeff King wrote: > On Fri, Sep 22, 2023 at 10:41:58AM +0000, Johannes Schindelin via GitGitGadget wrote: > > > 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. It is quite possible that I only verified that `cov-configure --gcc` needs to be called when running on Windows, and not on Linux, as there were many more deal breakers to convince me that we should _not_ use `vapier/coverity-scan-action`. Unless we fork it into the `git` org and start maintaining it ourselves, which is an option to consider. > > + - 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. Sadly, https://curl.se/docs/manpage.html#-f says this: This method is not fail-safe and there are occasions where non-successful response codes slip through, especially when authentication is involved (response codes 401 and 407). 401 is the precise case we're hitting when the token or the project name are incorrect. Having said that, I just tested with this particular host, and `curl -f` does fail with [exit code 22](https://curl.se/docs/manpage.html#22) as one would desire. So I will make that change. As to `--fail-with-body`: it is too new to use (it was [introduced in cURL v7.76.0](https://curl.se/docs/manpage.html#--fail-with-body) and Ubuntu v20.04 [comes with v7.68.0](https://packages.ubuntu.com/search?suite=focal&searchon=names&keywords=curl), i.e. is missing that option). In any case, in my tests, `--fail-with-body` did not show anything more than `--fail` in this instance. Maybe for you it is different? > This is mostly a style suggestion, but I think you can use: > > --form token="${{ secrets.COVERITY_SCAN_TOKEN }}" \ > --form project="$COVERITY_PROJECT" That is how I did things in Git for Windows, but at some stage I copied over code from `vapier/coverity-scan-action`. It is yet another slight code smell about that Action that it sometimes uses `--data` and sometimes `--form`: https://github.com/vapier/coverity-scan-action/blob/cae3c096a2eb21c431961a49375ac17aea2670ce/action.yml#L89 https://github.com/vapier/coverity-scan-action/blob/cae3c096a2eb21c431961a49375ac17aea2670ce/action.yml#L118 > I notice you put the "project" variable in the query string. Can it be > a --form, too, for symmetry? The instructions at https://scan.coverity.com/projects/git/builds/new (in the "Automation" section) are very clear that `project` should be passed as a GET variable. Even if using a POST variable would work, I'd rather stay with the officially-documented way. Ciao, Johannes
On Mon, Sep 25, 2023 at 01:52:34PM +0200, Johannes Schindelin wrote: > > 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. > > Sadly, https://curl.se/docs/manpage.html#-f says this: > > This method is not fail-safe and there are occasions where > non-successful response codes slip through, especially when > authentication is involved (response codes 401 and 407). > > 401 is the precise case we're hitting when the token or the project name > are incorrect. > > Having said that, I just tested with this particular host, and `curl -f` > does fail with [exit code 22](https://curl.se/docs/manpage.html#22) as one > would desire. So I will make that change. The manpage is rather vague about what those "occasions" are, but I think we are probably OK since we are not sending HTTP-level credentials, according to: https://github.com/curl/curl/commit/cde5e35d9b046b224c64936c432d67c9de8bcc9e At any rate, even if this did not catch every failure, I think we are better off catching more rather than fewer. > As to `--fail-with-body`: it is too new to use (it was [introduced in cURL > v7.76.0](https://curl.se/docs/manpage.html#--fail-with-body) and Ubuntu > v20.04 [comes with > v7.68.0](https://packages.ubuntu.com/search?suite=focal&searchon=names&keywords=curl), > i.e. is missing that option). > > In any case, in my tests, `--fail-with-body` did not show anything more > than `--fail` in this instance. Maybe for you it is different? No, I don't think it's worth worrying about here. It could help with debugging if their server returns something generic like a 500 code along with a more detailed reason (we do something similar in git-remote-curl to show failed bodies). But if it's at all more hassle than typing "--with-body" it is not worth the effort. > > I notice you put the "project" variable in the query string. Can it be > > a --form, too, for symmetry? > > The instructions at https://scan.coverity.com/projects/git/builds/new (in > the "Automation" section) are very clear that `project` should be passed > as a GET variable. > > Even if using a POST variable would work, I'd rather stay with the > officially-documented way. That seems like good reasoning to me. -Peff
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml new file mode 100644 index 00000000000..24408f6282c --- /dev/null +++ b/.github/workflows/coverity.yml @@ -0,0 +1,56 @@ +name: Coverity + +# This GitHub workflow automates submitting builds to Coverity Scan. To enable it, +# set the repository variable `ENABLE_COVERITY_SCAN_FOR_BRANCHES` (for details, see +# https://docs.github.com/en/actions/learn-github-actions/variables) to a JSON +# string array containing the names of the branches for which the workflow should be +# run, e.g. `["main", "next"]`. +# +# In addition, two repository secrets must be set (for details how to add secrets, see +# https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions): +# `COVERITY_SCAN_EMAIL` and `COVERITY_SCAN_TOKEN`. The former specifies the +# email to which the Coverity reports should be sent and the latter can be +# obtained from the Project Settings tab of the Coverity project). + +on: + push: + +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 + 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" + - 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 + - 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 + - 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"