| Message ID | b23951c569660e1891a7fb3ad2c2ea1952897bd7.1695332105.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] .github/workflows: add coverity action | expand |
Taylor Blau <me@ttaylorr.com> writes: > This fell to the bottom of my queue, but I got back to it today while > doing some ~~spring~~ fall inbox cleaning :-). Thanks Peff and Johannes > for helpful review in the first round. Range-diff is below: > > Range-diff against v1: > 1: f74ae75ddb < -: ---------- .github/workflows: add coverity action > -: ---------- > 1: b23951c569 .github/workflows: add coverity action That's a useful range-diff ;-). Even with --word-diff, range-diff does not notice that they correspond to each other, without an absurd setting like --creation-factor=999. > .github/workflows/coverity.yml | 22 ++++++++++++++++++++++ > ci/install-dependencies.sh | 2 +- > ci/lib.sh | 2 +- > 3 files changed, 24 insertions(+), 2 deletions(-) > create mode 100644 .github/workflows/coverity.yml > > diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml > new file mode 100644 > index 0000000000..3ba00b3929 > --- /dev/null > +++ b/.github/workflows/coverity.yml > @@ -0,0 +1,22 @@ > +name: Coverity > + > +on: [push, pull_request] This no longer hardcocdes the condition to master and tagged ones, as ... > +jobs: > + coverity: > + if: (vars.ENABLE_COVERITY == 'true') && > + (vars.COVERITY_BRANCHES == '' || > + contains(vars.COVERITY_BRANCHES, github.ref_name) || > + contains(vars.COVERITY_BRANCHES, '*')) ... this lets you control when to run it via the "vars". This round also can act on pull-requests in addition to pushes. > + runs-on: ubuntu-latest > + steps: > + - uses: actions/checkout@v3 > + - run: ci/install-dependencies.sh > + env: > + jobname: coverity > + - uses: vapier/coverity-scan-action@cae3c096a2eb21c431961a49375ac17aea2670ce > + with: > + email: ${{ secrets.COVERITY_SCAN_EMAIL }} > + token: ${{ secrets.COVERITY_SCAN_TOKEN }} > + command: make -j8 And the actual implementation is vastly different by just using a canned one, which requires less maintenance on our end, which is nice. > diff --git a/ci/lib.sh b/ci/lib.sh > index 6fbb5bade1..2ad0ae340e 100755 > --- a/ci/lib.sh > +++ b/ci/lib.sh > @@ -227,7 +227,7 @@ export SKIP_DASHED_BUILT_INS=YesPlease > > case "$runs_on_pool" in > ubuntu-*) > - if test "$jobname" = "linux-gcc-default" > + if test "$jobname" = "linux-gcc-default" || test "$jobname" = "coverity" > then > break > fi This part is new in this iteration, to avoid further customization that enables more exotic features, which makes sense.
Hi Taylor, thank you for continuing your work on this. The timing is a bit unfortunate, though: As I had not heard back from you in this thread, I had started work yesterday morning on converting Git for Windows' corresponding Azure Pipeline to a GitHub workflow as a consequence. I had to leave some polishing for this morning because the validation that the workflow actually runs correctly and is versatile enough to address both the needs of the Git project as well as of the Git for Windows project took longer than anticipated and revealed a couple of problems on which I will elaborate below. On Thu, 21 Sep 2023, Taylor Blau wrote: > [...] using the vapier/coverity-scan Action comes with some additional > niceties, such as caching the (rather large) Coverity tool download > between runs. While it is true that that Action caches the Coverity Tool, this GitHub Action comes with a few non-niceties, too, one of them being that it actually fails to do its job. Looking at https://github.com/vapier/coverity-scan-action/blob/main/action.yml it can be very easily verified that `cov-configure` is not called. But as https://github.com/git-for-windows/git/actions/runs/6267593979/job/17020975444#step:9:12 demonstrates, this command now _needs_ to be called: Error: Could not find file coverity_config.xml in directory D:/a/_temp/cov-analysis/config The cov-build.exe command needs a configuration file, generated by cov-configure, to know how to capture analysis inputs based on compiler invocations and/or scanning the filesystem. Examples of running cov-configure: cov-configure --cs Microsoft C# compiler (csc) cov-configure --gcc GNU C/C++ compiler (gcc/g++) cov-configure --java Oracle Java compiler (javac) cov-configure --javascript *.js JavaScript source code cov-configure --msvc Microsoft C/C++ compiler (cl) See cov-configure documentation for more information. I had encountered this issue already in April last year and had addressed it in https://github.com/git-for-windows/build-extra/commit/cec961d2536c, so it came as an unwelcome surprise to me that this GitHub Action leaves this problem unaddressed. Another issue I ran into: That Action defaults to using the URL-encoded name of the repository (in our case, `git%2Fgit`) as Coverity project name. That project does not exist, though, and the GitHub Action therefore silently assumes an empty MD5 and attempts to extract a tar file that is actually a text file instead, with the contents "Not found". Further, when I tried to specify `win64` as this GitHub Action's `platform` input variable (whose existence suggests that platforms other than `linux64` are allowed), it totally fell over, trying to untar a `.zip` file. For this and for various other reasons detailed in the cover letter of https://lore.kernel.org/git/pull.1588.git.1695379323.gitgitgadget@gmail.com, it would appear that unfortunately that GitHub Action won't be helpful here. Ciao, Johannes
On Thu, Sep 21, 2023 at 05:53:31PM -0400, Taylor Blau wrote: > diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml > new file mode 100644 > index 0000000000..3ba00b3929 > --- /dev/null > +++ b/.github/workflows/coverity.yml > @@ -0,0 +1,22 @@ > +name: Coverity > + > +on: [push, pull_request] > + > +jobs: > + coverity: > + if: (vars.ENABLE_COVERITY == 'true') && > + (vars.COVERITY_BRANCHES == '' || > + contains(vars.COVERITY_BRANCHES, github.ref_name) || > + contains(vars.COVERITY_BRANCHES, '*')) I wonder if we really need this ENABLE_COVERITY flag. It's not _too_ hard to set up, but it feels like we can eliminate a setup step and just infer it from other variables. Either: 1. Treat unset COVERITY_BRANCHES as "do not run". Unlike the CI job, there's not much useful signal in running this for every PR. A human has to go periodically look at the coverity output and make sense of it. So I think it's something that would get run periodically (say, on updates of "next") just for one or two branches. 2. Use COVERITY_SCAN_EMAIL as a clue that the feature it is enabled. I don't think we can do that as-is because it's a secret, not a var. But is there a reason for the EMAIL to be a secret? I don't think repository vars are fully public; they're just not hidden as deeply as secrets. It seems like the right level of privacy for an email. > + runs-on: ubuntu-latest > + steps: > + - uses: actions/checkout@v3 > + - run: ci/install-dependencies.sh > + env: > + jobname: coverity > + - uses: vapier/coverity-scan-action@cae3c096a2eb21c431961a49375ac17aea2670ce > + with: > + email: ${{ secrets.COVERITY_SCAN_EMAIL }} > + token: ${{ secrets.COVERITY_SCAN_TOKEN }} > + command: make -j8 I ran the action and it worked out of the box for me (I have everything set up on the coverity side already from my custom workflow), modulo one hiccup. I initially had COVERITY_SCAN_EMAIL as a var, so the upload failed, complaining of an empty email. But much to my surprise, the Action still succeeded. It didn't record the HTTP result code, so I'm not sure if we could detect this with "curl --fail-with-body" or if we'd have to scrape the output. It may not be that big a deal either way, though, since the coverity output really is useless until a human periodically scans over it (but it would be nice to know if it was routinely failing; I might not notice if it didn't). -Peff
On Fri, Sep 22, 2023 at 01:09:26PM +0200, Johannes Schindelin wrote: > While it is true that that Action caches the Coverity Tool, this GitHub > Action comes with a few non-niceties, too, one of them being that it > actually fails to do its job. Looking at > https://github.com/vapier/coverity-scan-action/blob/main/action.yml it can > be very easily verified that `cov-configure` is not called. But as > https://github.com/git-for-windows/git/actions/runs/6267593979/job/17020975444#step:9:12 > demonstrates, this command now _needs_ to be called: Hmm. I have been running Coverity for years without running cov-configure (using my own workflow), and I just tested Taylor's patch, which worked fine. I wonder what is different about our setups; maybe it is only an issue on the Windows version of Coverity? Or maybe it has to do with the project setup on Coverity's server side, since we have to provide the project name as part of the tool download (though I don't know if that is purely for logging/auditing on their side or if the delivered download is customized in any way). > Another issue I ran into: That Action defaults to using the URL-encoded > name of the repository (in our case, `git%2Fgit`) as Coverity project > name. That project does not exist, though, and the GitHub Action therefore > silently assumes an empty MD5 and attempts to extract a tar file that is > actually a text file instead, with the contents "Not found". It took me a minute to understand what you meant here, since the main use of the project name is for uploading our tarball. But we also must provide it (along with the token) just to download their scan software, so that step failed. And yes, I agree that the error checking in the Action could be better (I also mentioned elsewhere in the thread that it did not detect or report a failed upload). I'm not sure if it is easier to submit a patch there, or if we are better off just doing it all inline (it is not that much code either way). > Further, when I tried to specify `win64` as this GitHub Action's > `platform` input variable (whose existence suggests that platforms other > than `linux64` are allowed), it totally fell over, trying to untar a > `.zip` file. Hmm, yes, I hadn't really considered running Coverity with different build configurations. But I guess it needs to run code through the compiler to do its magic, so you'd want to be able to run it on a Windows build to hit any platform-specific code. For general git/git use, I'm not sure if we'd want to run it on multiple platforms, but I guess your plan would be to share the workflow code between git/ and git-for-windows/, and use a repo variable to select the platform for the latter. And for that case we can't rely on an Action which doesn't support Windows. I'll take a look at the patches you posted. -Peff
On Sat, Sep 23, 2023 at 02:21:37AM -0400, Jeff King wrote:
> I'll take a look at the patches you posted.
OK, I just read over them. I do think the result is not much more
complicated than using the vapier Action, and it fits our multi-platform
needs a bit better.
I had a few small comments about curl usage, but it mostly looks like a
good direction to me.
-Peff
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml new file mode 100644 index 0000000000..3ba00b3929 --- /dev/null +++ b/.github/workflows/coverity.yml @@ -0,0 +1,22 @@ +name: Coverity + +on: [push, pull_request] + +jobs: + coverity: + if: (vars.ENABLE_COVERITY == 'true') && + (vars.COVERITY_BRANCHES == '' || + contains(vars.COVERITY_BRANCHES, github.ref_name) || + contains(vars.COVERITY_BRANCHES, '*')) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - run: ci/install-dependencies.sh + env: + jobname: coverity + - uses: vapier/coverity-scan-action@cae3c096a2eb21c431961a49375ac17aea2670ce + with: + email: ${{ secrets.COVERITY_SCAN_EMAIL }} + token: ${{ secrets.COVERITY_SCAN_TOKEN }} + command: make -j8 + diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 4f407530d3..7e100ee63f 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -74,7 +74,7 @@ Documentation) test -n "$ALREADY_HAVE_ASCIIDOCTOR" || sudo gem install --version 1.5.8 asciidoctor ;; -linux-gcc-default) +linux-gcc-default|coverity) sudo apt-get -q update sudo apt-get -q -y install $UBUNTU_COMMON_PKGS ;; diff --git a/ci/lib.sh b/ci/lib.sh index 6fbb5bade1..2ad0ae340e 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -227,7 +227,7 @@ export SKIP_DASHED_BUILT_INS=YesPlease case "$runs_on_pool" in ubuntu-*) - if test "$jobname" = "linux-gcc-default" + if test "$jobname" = "linux-gcc-default" || test "$jobname" = "coverity" then break fi
Coverity is a static analysis tool that detects and generates reports on various security and code quality issues. It is particularly useful when diagnosing memory safety issues which may be used as part of exploiting a security vulnerability. Coverity's website provides a service accepts "builds" (which is the output of running `cov-build` on your project, which invokes `make` with additional magic) as input and generates reports as output. In order to generate a report, we have to first compile Git and then upload the build archive to Coverity. This Action generates and uploads a build archive to Coverity when a GitHub repository has done the following: - Configured the "COVERITY_SCAN_EMAIL" and "COVERITY_SCAN_TOKEN" repository secrets. Tokens are found on the "Project Settings" page at [1]. Tokens may be added as repository secrets on GitHub repositories by following the guide at [2]. - Enabled Coverity builds by (in addition to the above) creating a repository variable called `ENABLE_COVERITY`. Repository variables (which are different than secrets) can be added according to the guide at [3]. This enables Coverity to automatically report on new changes pushed to the configured branch set, which is specified via the `COVERITY_BRANCHES` repository variable. The implementation is mostly straightforward. Though note that we could upload the build archive to Coverity directly with a straightforward curl request. But using the vapier/coverity-scan Action comes with some additional niceties, such as caching the (rather large) Coverity tool download between runs. If the repository does not have the `ENABLE_COVERITY` variable set, or the list of branches specified by `COVERITY_BRANCHES` does not contain the branch being pushed to, this Action is a no-op. [1]: https://scan.coverity.com/projects/NAME?tab=project_settings [2]: https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions [3]: https://docs.github.com/en/actions/learn-github-actions/variables Helped-by: Jeff King <peff@peff.net> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- This fell to the bottom of my queue, but I got back to it today while doing some ~~spring~~ fall inbox cleaning :-). Thanks Peff and Johannes for helpful review in the first round. Range-diff is below: Range-diff against v1: 1: f74ae75ddb < -: ---------- .github/workflows: add coverity action -: ---------- > 1: b23951c569 .github/workflows: add coverity action .github/workflows/coverity.yml | 22 ++++++++++++++++++++++ ci/install-dependencies.sh | 2 +- ci/lib.sh | 2 +- 3 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/coverity.yml -- 2.42.0.242.gc844f407a1