Add a GitHub workflow to submit builds to Coverity Scan

[dscho]

Coverity is a powerful static analysis tool that helps prevent vulnerabilities. It is free to use by open source projects, and Git benefits from this, as well as Git for Windows. As is the case with many powerful tools, using Coverity comes with its own set of challenges, one of which being that submitting a build is quite laborious.

The help with this, the Git for Windows project created an Azure Pipeline to automate submitting builds to Coverity Scan which is ported to a GitHub workflow in this here patch series, keeping an eye specifically on allowing both the Git and the Git for Windows project to use this workflow.

Since Coverity build submissions require authentication, this workflow is skipped by default. To enable it, the repository variable ENABLE_COVERITY_SCAN_FOR_BRANCHES needs to be added. Its value needs to be a JSON string array containing the branch names, e.g. ["master", "next"]. Further, two repository secrets need to be set: COVERITY_SCAN_EMAIL and COVERITY_SCAN_TOKEN.

An example run in the Git for Windows project can be admired here.

Note: While inspired by vapier/coverity-scan-action, I found too many limitations in that Action to be used here. However, I would be willing to fork that Action into the git organization on GitHub, improve the code to accommodate Git's needs, and maintain that Action, if there is enough support for taking that route instead of simply hard-coding the steps in Git's .github/workflows/coverity.yml.

This patch series is based on v2.42.0, but would apply literally everywhere because it adds a new file and modifies no existing one.

Changes since v1:

• After verifying that cURL's --fail option does what we need in Coverity's context, I switched to using that in every curl invocation.
• Adjusted quoting (the ${{ ... }} constructs do not require double quotes because they are interpolated before the script is run).
• Touched up a few commit messages, based on the feedback I received.
• Addressed an actionlint warning.

cc: Jeff King peff@peff.net

[dscho]

/submit

[gitgitgadget]

Submitted as pull.1588.git.1695379323.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1588/dscho/coverity-workflow-v1

To fetch this version to local tag pr-1588/dscho/coverity-workflow-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1588/dscho/coverity-workflow-v1

[gitgitgadget]

This branch is now known as js/ci-coverity.

[gitgitgadget]

This patch series was integrated into seen via git@7fede36.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch js/ci-coverity on the Git mailing list:

GitHub CI workflow has learned to trigger Coverity check.

source: <pull.1588.git.1695379323.gitgitgadget@gmail.com>

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

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

[gitgitgadget]

User Jeff King <peff@peff.net> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> It would add a 1GB+ download for every run, better cache it.
> 
> This is inspired by the GitHub Action `vapier/coverity-scan-action`,
> however, it uses the finer-grained `restore`/`save` method to be able to
> cache the Coverity Build Tool even if an unrelated step in the GitHub
> workflow fails later on.

Nice. This is the big thing that I think the vapier action was providing
us, and it does not look too bad. I have never used actions/cache
before, but it all looks plausibly correct to me (and I assume you did a
few test-runs to check it).

One note:

> +      # The Coverity site says the tool is usually updated twice yearly, so the
> +      # MD5 of download can be used to determine whether there's been an update.
> +      - name: get the Coverity Build Tool hash
> +        id: lookup
> +        run: |
> +          MD5=$(curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
> +                   --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT&md5=1")
> +          echo "hash=$MD5" >>$GITHUB_OUTPUT

We probably want --fail here, too (and presumably &&-chaining) so that
we don't accidentally write a bogus cache entry. Possibly even check
that $MD5 isn't blank if we want to be double-paranoid.

That made me wonder: if we do end up with a bogus cache entry, how does
one clear it? And it looks like it can be managed directly via
https://github.com/$user/$project/actions/caches. Nice.

-Peff

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Peff,

On Sat, 23 Sep 2023, Jeff King wrote:

> On Fri, Sep 22, 2023 at 10:41:59AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > It would add a 1GB+ download for every run, better cache it.
> >
> > This is inspired by the GitHub Action `vapier/coverity-scan-action`,
> > however, it uses the finer-grained `restore`/`save` method to be able to
> > cache the Coverity Build Tool even if an unrelated step in the GitHub
> > workflow fails later on.
>
> Nice. This is the big thing that I think the vapier action was providing
> us, and it does not look too bad.
>
> I have never used actions/cache before, but it all looks plausibly
> correct to me (and I assume you did a few test-runs to check it).

I use `actions/cache` extensively, both in GitHub workflows via the Action
as well as in custom Actions like `setup-git-for-windows-sdk`, so I am
confident that I am using this tool correctly here, too.

>
> One note:
>
> > +      # The Coverity site says the tool is usually updated twice yearly, so the
> > +      # MD5 of download can be used to determine whether there's been an update.
> > +      - name: get the Coverity Build Tool hash
> > +        id: lookup
> > +        run: |
> > +          MD5=$(curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
> > +                   --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT&md5=1")
> > +          echo "hash=$MD5" >>$GITHUB_OUTPUT
>
> We probably want --fail here, too.

I concur, after verifying that the scary manual page note about
authentication issues often not being handled correctly by `curl --fail`
does not affect this particular scenario.

Ciao,
Johannes

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

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

> +# By default, the builds are submitted to the Coverity project `git`. To override this,
> +# set the repository variable `COVERITY_PROJECT`.

This may not matter all that much, because I don't expect most people to
set this up for their forks (and if we have git/git results that I have
access to, I will probably even stop building my peff/git one). But I
just wondered if a better default would be the GitHub project name
(i.e., $user/git).

It has been long enough that I do not remember all of the setup on the
Coverity side, but I assumed there was some "set it up for this GitHub
project" button. But maybe I just picked that name myself.

-Peff

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Peff,

On Sat, 23 Sep 2023, Jeff King wrote:

> On Fri, Sep 22, 2023 at 10:42:00AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > +# By default, the builds are submitted to the Coverity project `git`. To override this,
> > +# set the repository variable `COVERITY_PROJECT`.
>
> This may not matter all that much, because I don't expect most people to
> set this up for their forks

Except, of course, Git for Windows. And that has been the entire
motivation for me to work on this here patch series in the first place, so
it would be rather pointless if I could not override this in the
git-for-windows/git fork.

Of course, I could address this differently. I could add a commit on top
and rebase that all the time. I'd just as well avoid that though. There is
already too much stuff in the Git for Windows fork that I have to rebase
so often.

Based on your response, I was on my way to enhance the commit message
accordingly, but then I saw this already being there:

	The Git for Windows project would like to use this workflow, too,
	though, and needs the builds to be submitted to the
	`git-for-windows` Coverity project.

Would you have any suggestion how that could make the motivation and
intention of this patch clearer?

Ciao,
Johannes

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Mon, Sep 25, 2023 at 01:52:47PM +0200, Johannes Schindelin wrote:

> Hi Peff,
> 
> On Sat, 23 Sep 2023, Jeff King wrote:
> 
> > On Fri, Sep 22, 2023 at 10:42:00AM +0000, Johannes Schindelin via GitGitGadget wrote:
> >
> > > +# By default, the builds are submitted to the Coverity project `git`. To override this,
> > > +# set the repository variable `COVERITY_PROJECT`.
> >
> > This may not matter all that much, because I don't expect most people to
> > set this up for their forks
> 
> Except, of course, Git for Windows. And that has been the entire
> motivation for me to work on this here patch series in the first place, so
> it would be rather pointless if I could not override this in the
> git-for-windows/git fork.

I didn't at all mean that it should not be possible to override it. It
was obvious to me you would want to do so for git-for-windows. I meant
that the default should be $user/$fork from the Actions environment,
rather than just "git". That would be a more appropriate default for
people who wanted to run this out of their forks (but again, the part
you quoted above is basically "I'm not sure anybody even wants to do
that").

> Of course, I could address this differently. I could add a commit on top
> and rebase that all the time. I'd just as well avoid that though. There is
> already too much stuff in the Git for Windows fork that I have to rebase
> so often.
> 
> Based on your response, I was on my way to enhance the commit message
> accordingly, but then I saw this already being there:
> 
> 	The Git for Windows project would like to use this workflow, too,
> 	though, and needs the builds to be submitted to the
> 	`git-for-windows` Coverity project.
> 
> Would you have any suggestion how that could make the motivation and
> intention of this patch clearer?

I understood your intention. I think you misunderstood mine. :) The
commit message is fine as-is, I think.

-Peff

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Jeff,

On Mon, 25 Sep 2023, Jeff King wrote:

> On Mon, Sep 25, 2023 at 01:52:47PM +0200, Johannes Schindelin wrote:
>
> > On Sat, 23 Sep 2023, Jeff King wrote:
> >
> > > On Fri, Sep 22, 2023 at 10:42:00AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > >
> > > > +# By default, the builds are submitted to the Coverity project
> > > > +# `git`. To override this, set the repository variable
> > > > +# `COVERITY_PROJECT`.
> > >
> > > This may not matter all that much, because I don't expect most
> > > people to set this up for their forks
> >
> > Except, of course, Git for Windows. And that has been the entire
> > motivation for me to work on this here patch series in the first
> > place, so it would be rather pointless if I could not override this in
> > the git-for-windows/git fork.
>
> I didn't at all mean that it should not be possible to override it.

Aha! The "This" in "This may not matter all that much" was referring to
the `git` instead of the `COVERITY_PROJECT` part. That had not been clear
to me.

> I meant that the default should be $user/$fork from the Actions
> environment,

I'd much rather default to the value needed by the Git project than a
value that may be needed by any other user. I do aim to support the Git
project with this patch series, first and foremost.

Ciao,
Johannes

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I meant that the default should be $user/$fork from the Actions
>> environment,
>
> I'd much rather default to the value needed by the Git project than a
> value that may be needed by any other user. I do aim to support the Git
> project with this patch series, first and foremost.

I appreciate the sentiment.

At the same time, it would be one less thing they need to tweak
before starting to use it, and if there are two or more users to do
so, it would already have paid off.  Developers typically outnumber
projects they work on.

I also have to wonder if it might make it more obvious what is going
on if you made the default to $user/$fork and have the project
override it, which hopefully may make it easier to find out what
they need to do for those who want to override it to a different
value to suit their need?

Thanks.

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Tue, Sep 26, 2023 at 07:19:46AM -0700, Junio C Hamano wrote:

> At the same time, it would be one less thing they need to tweak
> before starting to use it, and if there are two or more users to do
> so, it would already have paid off.  Developers typically outnumber
> projects they work on.
> 
> I also have to wonder if it might make it more obvious what is going
> on if you made the default to $user/$fork and have the project
> override it, which hopefully may make it easier to find out what
> they need to do for those who want to override it to a different
> value to suit their need?

Yeah, that was my thinking (and what I had been proposing).

But I really think it probably doesn't matter that much either way. I
would not be surprised if there are zero developers who use this,
because of the setup on the coverity side, and the fact that the results
are not always immediately actionable.

Even I, who has been running coverity on my local fork for a few years,
will probably just switch to using the git.git run and occasionally
looking at the results (that creates an extra headache because somebody
has to grant acess to the git.git run results to interested parties, but
it's also a one-time setup thing).

-Peff

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Tue, Sep 26, 2023 at 04:02:27PM +0200, Johannes Schindelin wrote:

> > > > This may not matter all that much, because I don't expect most
> > > > people to set this up for their forks
> > >
> > > Except, of course, Git for Windows. And that has been the entire
> > > motivation for me to work on this here patch series in the first
> > > place, so it would be rather pointless if I could not override this in
> > > the git-for-windows/git fork.
> >
> > I didn't at all mean that it should not be possible to override it.
> 
> Aha! The "This" in "This may not matter all that much" was referring to
> the `git` instead of the `COVERITY_PROJECT` part. That had not been clear
> to me.

I think we are on the same page now, but I wanted to elaborate here
because this miscommunication interested me. The "this" in what I wrote
was actually the "X" in:

  This may not matter because of $FOO, but X.

The X got snipped in what you quoted, but was "But I just wondered if a
better default would would be...". I certainly did not help with
readability by inserting a huge parenthetical phrase and putting in a
full-stop and starting the new sentence on "But" (my ninth grade English
teacher would be horrified).

I mention it mostly as a note to myself about trying to make sure I keep
my writing clear and readable (and a cautionary tale for others!).

-Peff

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff King <peff@peff.net> writes:

> On Tue, Sep 26, 2023 at 07:19:46AM -0700, Junio C Hamano wrote:
>
>> At the same time, it would be one less thing they need to tweak
>> before starting to use it, and if there are two or more users to do
>> so, it would already have paid off.  Developers typically outnumber
>> projects they work on.
>> 
>> I also have to wonder if it might make it more obvious what is going
>> on if you made the default to $user/$fork and have the project
>> override it, which hopefully may make it easier to find out what
>> they need to do for those who want to override it to a different
>> value to suit their need?
>
> Yeah, that was my thinking (and what I had been proposing).
>
> But I really think it probably doesn't matter that much either way. I
> would not be surprised if there are zero developers who use this,
> because of the setup on the coverity side, and the fact that the results
> are not always immediately actionable.
>
> Even I, who has been running coverity on my local fork for a few years,
> will probably just switch to using the git.git run and occasionally
> looking at the results (that creates an extra headache because somebody
> has to grant acess to the git.git run results to interested parties, but
> it's also a one-time setup thing).

Sure.  

I do not care too much either way, and in a situation like this
where the design decision does not have a crucial longer-term impact
either way exactly because it is a one-time thing for any user,
whoever has invested their work on should have the final say.

Thanks.

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> By adding the repository variable `ENABLE_COVERITY_SCAN_ON_OS` with a
> value, say, `["windows-latest"]`, this GitHub workflow now runs on
> Windows, allowing to analyze Windows-specific issues.

Makes sense. I figured we'd key this on COVERITY_PLATFORM itself, but I
guess we need to map Actions environments to Coverity platform names,
so starting with the Actions names makes sense.

> +# The workflow runs on `ubuntu-latest` by default. This can be overridden by setting
> +# the repository variable `ENABLE_COVERITY_SCAN_ON_OS` to a JSON string array specifying
> +# the operating systems, e.g. `["ubuntu-latest", "windows-latest"]`.

OK. I was envisioning that we'd just run on one platform, and maybe
git-for-windows would run on another. But it does not hurt to be able to
do both from one repo. I'm not sure how Coverity presents that, but it
should be able to figure out based on "version" and "platform" fields
that they are two builds of the same version (and not, say, one
overriding the other as the "latest").

-Peff

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> For completeness' sake, let's add support for submitting macOS builds to
> Coverity Scan.

I don't have any real problem with this, and it will check a few extra
bits of platform-specific code not covered elsewhere. My big question
would be: how much extra does this cost to run each time?

I guess it is not too much (compared to regular CI); my coverity build
job ran in 7 minutes, and that is including the download of the tool.

-Peff

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Peff,

On Sat, 23 Sep 2023, Jeff King wrote:

> On Fri, Sep 22, 2023 at 10:42:02AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > For completeness' sake, let's add support for submitting macOS builds to
> > Coverity Scan.
>
> I don't have any real problem with this, and it will check a few extra
> bits of platform-specific code not covered elsewhere.

Just to make sure: The patch series, as presented here, will only build on
`ubuntu-latest` by default, unless that is specifically overridden in the
repository variables of `git/git`. It makes most sense to me that way.

> My big question would be: how much extra does this cost to run each
> time?

I linked the example runs in the cover letter, which record the runtimes
of the `build with cov-build` step as:

	ubuntu-latest	5m20s
	windows-latest	17m40s
	macos-latest	1m59s

But again, I don't see much sense building `git/git` for anything else
than `ubuntu-latest`, at least for now.

Ciao,
Johannes

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Mon, Sep 25, 2023 at 01:52:52PM +0200, Johannes Schindelin wrote:

> > > For completeness' sake, let's add support for submitting macOS builds to
> > > Coverity Scan.
> >
> > I don't have any real problem with this, and it will check a few extra
> > bits of platform-specific code not covered elsewhere.
> 
> Just to make sure: The patch series, as presented here, will only build on
> `ubuntu-latest` by default, unless that is specifically overridden in the
> repository variables of `git/git`. It makes most sense to me that way.

Yeah, that makes sense to me, too. But then I wondered why we have this
macOS code if nobody is going to run it. On the other hand, maybe it
eventually will come in handy, and you already did the work, and it is
not hurting anybody in the meantime.

-Peff

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> When trying to obtain the MD5 of the Coverity Scan Tool (in order to
> decide whether a cached version can be used or a new version has to be
> downloaded), it is possible to get a 401 (Authorization required) due to
> either an incorrect token, or even more likely due to an incorrect
> Coverity project name.
> 
> Let's detect that scenario and provide a helpful error message instead
> of trying to go forward with an empty string instead of the correct MD5.

Ah. :) I think using "curl --fail" is probably a simpler solution here.

-Peff

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Jeff,

On Sat, 23 Sep 2023, Jeff King wrote:

> On Fri, Sep 22, 2023 at 10:42:03AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When trying to obtain the MD5 of the Coverity Scan Tool (in order to
> > decide whether a cached version can be used or a new version has to be
> > downloaded), it is possible to get a 401 (Authorization required) due to
> > either an incorrect token, or even more likely due to an incorrect
> > Coverity project name.
> >
> > Let's detect that scenario and provide a helpful error message instead
> > of trying to go forward with an empty string instead of the correct MD5.
>
> Ah. :) I think using "curl --fail" is probably a simpler solution here.

Apart from the unintuitive error message. I myself was puzzled and
struggled quite a few times until I realized that it was not the token
that was incorrect, but the project name.

To help people with a similar mental capacity to my own, I would therefore
really want to keep this here patch.

As a compromise, I will switch to using `--fail` and then looking at the
exit code (with 22 indicating authentication issues).

Ciao,
Johannes

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Mon, Sep 25, 2023 at 01:52:56PM +0200, Johannes Schindelin wrote:

> > > Let's detect that scenario and provide a helpful error message instead
> > > of trying to go forward with an empty string instead of the correct MD5.
> >
> > Ah. :) I think using "curl --fail" is probably a simpler solution here.
> 
> Apart from the unintuitive error message. I myself was puzzled and
> struggled quite a few times until I realized that it was not the token
> that was incorrect, but the project name.
> 
> To help people with a similar mental capacity to my own, I would therefore
> really want to keep this here patch.
> 
> As a compromise, I will switch to using `--fail` and then looking at the
> exit code (with 22 indicating authentication issues).

Oh, I do not at all mind de-mystifying the error message for the user. I
mostly meant that using --fail would be better than scraping the http
code from the header file. Though as you note, I guess we have to
interpret the exit code in this case anyway, so it is not that much of a
simplification.

-Peff

[dscho]

/submit

[gitgitgadget]

Submitted as pull.1588.v2.git.1695642662.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1588/dscho/coverity-workflow-v2

To fetch this version to local tag pr-1588/dscho/coverity-workflow-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1588/dscho/coverity-workflow-v2

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Mon, Sep 25, 2023 at 11:50:56AM +0000, Johannes Schindelin via GitGitGadget wrote:

> Changes since v1:
> 
>  * After verifying that cURL's --fail option does what we need in Coverity's
>    context, I switched to using that in every curl invocation.
>  * Adjusted quoting (the ${{ ... }} constructs do not require double quotes
>    because they are interpolated before the script is run).
>  * Touched up a few commit messages, based on the feedback I received.
>  * Addressed an actionlint [https://rhysd.github.io/actionlint/] warning.

Thanks, this looks fine to me.

The only other comment is the same one I made for Taylor's version: that
COVERITY_SCAN_EMAIL could probably be a "var" and not a "secret". Though
the main advantage there (besides it being a little easier for the user
to edit in the web UI) is that it could be used in the jobs.*.if context
(to skip the job in unconfigured repos). But since your "if" uses a
default-disallow when ENABLE_COVERITY_SCAN_FOR_BRANCHES is not set, it
is not that important to check COVERITY_SCAN_EMAIL.

-Peff

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> Coverity [https://scan.coverity.com/] is a powerful static analysis tool
> that helps prevent vulnerabilities. It is free to use by open source
> projects, and Git benefits from this, as well as Git for Windows. As is the
> case with many powerful tools, using Coverity comes with its own set of
> challenges, one of which being that submitting a build is quite laborious.
> ...

One thing that caught my eye was the asterisks around "22" that look
as if they were designed to confuse readers and cause them wonder if
there are other codes like 122 and 224 that we would also want to
catch there.  Unless they know what the case statement replaced,
that is---the old code to match http_code that was scraped from a
text file may not have the code alone and may contain other cruft,
so it is entirely understandable, but the new one checks $? and
there is no reason other than to catch 122 and 224 to use *22*.



>      -+          http_code="$(sed -n 1p <"$RUNNER_TEMP"/headers.txt)"
>      -+          case "$http_code" in
>      -+          *200*) ;; # okay
>      -+          *401*) # access denied
>      -+            echo "::error::incorrect token or project? ($http_code)" >&2
>      +                    --fail \
>      +                    --form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
>      +                    --form project="$COVERITY_PROJECT" \
>      +-                   --form md5=1) &&
>      ++                   --form md5=1)
>      ++          case $? in
>      ++          0) ;; # okay
>      ++          *22*) # 40x, i.e. access denied
>      ++            echo "::error::incorrect token or project?" >&2
>       +            exit 1
>       +            ;;
>       +          *) # other error
>      -+            echo "::error::HTTP error $http_code" >&2
>      ++            echo "::error::Failed to retrieve MD5" >&2
>       +            exit 1
>       +            ;;
>       +          esac

Other than that, while I was watching from the sideline, I am very
happy to see that you, with Peff's constructive input, came up with
a new iteration that looks simpler and more consistent in its use of
curl.

Will replace but I may be tempted to edit those asterisks out myself
while queueing.

Thanks.

[gitgitgadget]

This patch series was integrated into seen via git@987eadf.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch js/ci-coverity on the Git mailing list:

GitHub CI workflow has learned to trigger Coverity check.

Looking good.
source: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

On Mon, 25 Sep 2023, Junio C Hamano wrote:

> One thing that caught my eye was the asterisks around "22" that look
> as if they were designed to confuse readers [...]
>
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > [...]
> >      ++          case $? in
> >      ++          0) ;; # okay
> >      ++          *22*) # 40x, i.e. access denied
> >      ++            echo "::error::incorrect token or project?" >&2
> >       +            exit 1
> >       +            ;;
>
> [...]
>
> Will replace but I may be tempted to edit those asterisks out myself
> while queueing.

D'oh, of course. Thank you,
Johannes

[gitgitgadget]

This patch series was integrated into seen via git@5c7c79d.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch js/ci-coverity on the Git mailing list:

GitHub CI workflow has learned to trigger Coverity check.

Looking good.
source: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@718095a.

[gitgitgadget]

This patch series was integrated into seen via git@241c6fd.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch js/ci-coverity on the Git mailing list:

GitHub CI workflow has learned to trigger Coverity check.

Looking good.
source: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@2982eef.

[gitgitgadget]

This patch series was integrated into seen via git@56e3878.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch js/ci-coverity on the Git mailing list:

GitHub CI workflow has learned to trigger Coverity check.

Looking good.
source: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@2b719ab.

[gitgitgadget]

This patch series was integrated into next via git@253788f.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch js/ci-coverity on the Git mailing list:

GitHub CI workflow has learned to trigger Coverity check.

Will merge to 'master'.
source: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@977005f.

[gitgitgadget]

This patch series was integrated into seen via git@0ce8c64.

[gitgitgadget]

This patch series was integrated into seen via git@a8cd85e.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch js/ci-coverity on the Git mailing list:

GitHub CI workflow has learned to trigger Coverity check.

Will merge to 'master'.
source: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@727ee06.

[gitgitgadget]

This patch series was integrated into seen via git@4ae4c70.

[gitgitgadget]

This patch series was integrated into master via git@4ae4c70.

[gitgitgadget]

This patch series was integrated into next via git@4ae4c70.

[gitgitgadget]

Closed via 4ae4c70.