diff mbox series

ci: run `make sparse` as a GitHub workflow

Message ID pull.994.git.1626177086682.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series ci: run `make sparse` as a GitHub workflow | expand

Commit Message

Johannes Schindelin July 13, 2021, 11:51 a.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

Occasionally we receive reviews after patches were integrated, where
`sparse` identified problems such as file-local variables or functions
being declared as global.

By running `sparse` as part of our Continuous Integration, we can catch
such things much earlier. Even better: developers who activated GitHub
Actions on their forks can catch such issues before even sending their
patches to the Git mailing list.

This addresses https://github.com/gitgitgadget/git/issues/345

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    ci: run make sparse as a GitHub workflow
    
    One of the earliest open source static analyzers is called "sparse", and
    occasionally Ramsay Jones sends out mails on the Git mailing list that
    some function or other should be declared static because sparse found
    out that it is only used within the same file.
    
    Let's add a GitHub workflow running "make sparse".
    
    Example run: https://github.com/gitgitgadget/git/actions/runs/1026303823

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-994%2Fdscho%2Fci-enable-sparse-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-994/dscho/ci-enable-sparse-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/994

 .github/workflows/run-sparse.yml | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 .github/workflows/run-sparse.yml


base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb

Comments

Đoàn Trần Công Danh July 13, 2021, 4:55 p.m. UTC | #1
On 2021-07-13 11:51:26+0000, Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Occasionally we receive reviews after patches were integrated, where
> `sparse` identified problems such as file-local variables or functions
> being declared as global.
> 
> By running `sparse` as part of our Continuous Integration, we can catch
> such things much earlier. Even better: developers who activated GitHub
> Actions on their forks can catch such issues before even sending their
> patches to the Git mailing list.
> 
> This addresses https://github.com/gitgitgadget/git/issues/345
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thanks for doing this change.

Some minor nits below.

> diff --git a/.github/workflows/run-sparse.yml b/.github/workflows/run-sparse.yml
> new file mode 100644
> index 00000000000..25f6a6efb40
> --- /dev/null
> +++ b/.github/workflows/run-sparse.yml
> @@ -0,0 +1,22 @@
> +name: Run `sparse`

Markdown doesn't work with Workflow's name.
Please remove those backticks.

> +on: [push, pull_request]
> +
> +jobs:
> +  sparse:
> +    runs-on: ubuntu-20.04
> +    steps:
> +    - name: Download the `sparse` package
> +      uses: git-for-windows/get-azure-pipelines-artifact@v0
> +      with:
> +        repository: git/git
> +        definitionId: 10
> +        artifact: sparse-20.04
> +    - name: Install the `sparse` package
> +      run: sudo dpkg -i sparse-20.04/sparse_*.deb
> +    - name: Install a couple of dependencies
> +      run: |
> +        sudo apt-get install -q -y libssl-dev libcurl4-openssl-dev libexpat-dev gettext zlib1g-dev
> +    - uses: actions/checkout@v2
> +    - name: make sparse
> +      run: make sparse
> \ No newline at end of file

The last step's name and run is the same. We can just drop name, it'll
use run as name. Anyway, remember the newline

Otherwise, look good to me.
Philippe Blain July 13, 2021, 5:34 p.m. UTC | #2
Hi Dscho,

Le 2021-07-13 à 07:51, Johannes Schindelin via GitGitGadget a écrit :
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Occasionally we receive reviews after patches were integrated, where
> `sparse` identified problems such as file-local variables or functions
> being declared as global.
> 
> By running `sparse` 

maybe here, we could add a link to https://sparse.docs.kernel.org/en/latest/,
so interested readers who do not know about "sparse" can go and learn
about it ?

> as part of our Continuous Integration, we can catch
> such things much earlier. Even better: developers who activated GitHub
> Actions on their forks can catch such issues before even sending their
> patches to the Git mailing list.
> 
> This addresses https://github.com/gitgitgadget/git/issues/345
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

> +    - name: Download the `sparse` package
> +      uses: git-for-windows/get-azure-pipelines-artifact@v0
> +      with:
> +        repository: git/git
> +        definitionId: 10
> +        artifact: sparse-20.04
> +    - name: Install the `sparse` package
> +      run: sudo dpkg -i sparse-20.04/sparse_*.deb

Out of curiosity, why is this necessary (as opposed to using
the Ubuntu package, i.e. 'sudo apt-get -q -y sparse') ?

Thanks,
Philippe.
Junio C Hamano July 13, 2021, 10:41 p.m. UTC | #3
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Occasionally we receive reviews after patches were integrated, where
> `sparse` identified problems such as file-local variables or functions
> being declared as global.

I really appreciate addition of this task, as I often notice sparse
errors _after_ queuing and refreshing the incoming patches for the
day and soon after starting to run the day's test cycle.

> By running `sparse` as part of our Continuous Integration, we can catch
> such things much earlier. Even better: developers who activated GitHub
> Actions on their forks can catch such issues before even sending their
> patches to the Git mailing list.
>
> This addresses https://github.com/gitgitgadget/git/issues/345
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     ci: run make sparse as a GitHub workflow
>     
>     One of the earliest open source static analyzers is called "sparse", and
>     occasionally Ramsay Jones sends out mails on the Git mailing list that
>     some function or other should be declared static because sparse found
>     out that it is only used within the same file.
>     
>     Let's add a GitHub workflow running "make sparse".
>     
>     Example run: https://github.com/gitgitgadget/git/actions/runs/1026303823
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-994%2Fdscho%2Fci-enable-sparse-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-994/dscho/ci-enable-sparse-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/994
>
>  .github/workflows/run-sparse.yml | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 .github/workflows/run-sparse.yml

We choose to do this as a separate new workflow not as part of the
main one because this is more like check-whitespace where there is
no room for tests over the matrix of compilers and platforms play
any useful role?  Unlike check-whitespace one that has

    on:
      pull_request:
        types: [opened, synchronize]

but just like the primary one, this is triggered

    on: [push, pull_request]

and before the patches even hit the list via GGG, which would be
ideal.

As has already been pointed out downthread, use of separate and
different ways to install the sparse itself and its dependencies
looks strange, so it would be appropriate to either explain why they
have to be that way in the proposed log message or to install them
in the same way in the YAML file.

Also, "\ No newline at end of file" would be a good thing to fix
while we are at it.

> diff --git a/.github/workflows/run-sparse.yml b/.github/workflows/run-sparse.yml
> new file mode 100644
> index 00000000000..25f6a6efb40
> --- /dev/null
> +++ b/.github/workflows/run-sparse.yml
> @@ -0,0 +1,22 @@
> +name: Run `sparse`
> +
> +on: [push, pull_request]
> +
> +jobs:
> +  sparse:
> +    runs-on: ubuntu-20.04
> +    steps:
> +    - name: Download the `sparse` package
> +      uses: git-for-windows/get-azure-pipelines-artifact@v0
> +      with:
> +        repository: git/git
> +        definitionId: 10
> +        artifact: sparse-20.04
> +    - name: Install the `sparse` package
> +      run: sudo dpkg -i sparse-20.04/sparse_*.deb
> +    - name: Install a couple of dependencies
> +      run: |
> +        sudo apt-get install -q -y libssl-dev libcurl4-openssl-dev libexpat-dev gettext zlib1g-dev
> +    - uses: actions/checkout@v2
> +    - name: make sparse
> +      run: make sparse
> \ No newline at end of file
>
> base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb
Johannes Schindelin July 14, 2021, 9:09 a.m. UTC | #4
Hi Philippe,

On Tue, 13 Jul 2021, Philippe Blain wrote:

> Le 2021-07-13 à 07:51, Johannes Schindelin via GitGitGadget a écrit :
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Occasionally we receive reviews after patches were integrated, where
> > `sparse` identified problems such as file-local variables or functions
> > being declared as global.
> >
> > By running `sparse`
>
> maybe here, we could add a link to https://sparse.docs.kernel.org/en/latest/,
> so interested readers who do not know about "sparse" can go and learn
> about it ?

Good point.

> > as part of our Continuous Integration, we can catch
> > such things much earlier. Even better: developers who activated GitHub
> > Actions on their forks can catch such issues before even sending their
> > patches to the Git mailing list.
> >
> > This addresses https://github.com/gitgitgadget/git/issues/345
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> > +    - name: Download the `sparse` package
> > +      uses: git-for-windows/get-azure-pipelines-artifact@v0
> > +      with:
> > +        repository: git/git
> > +        definitionId: 10
> > +        artifact: sparse-20.04
> > +    - name: Install the `sparse` package
> > +      run: sudo dpkg -i sparse-20.04/sparse_*.deb
>
> Out of curiosity, why is this necessary (as opposed to using
> the Ubuntu package, i.e. 'sudo apt-get -q -y sparse') ?

This is actually a historical curiosity: years ago, I created an Azure
Pipeline that builds the `sparse` Debian package for the specific purpose
of using it in our CI builds (if you care to look at the issue 345 I
linked above, you will see how long ago that idea was in the making). Now,
the historical curiosity is that back then, there was no current `sparse`
package available for Ubuntu, and Ramsay mentioned that a newer version
would be required to run `make sparse`.

And when I implemented this patch yesterday, I did not even question this,
I was just happy that I had come up with the GitHub Action
`get-azure-pipelines-artifact` (to help with the `vcpkg` part of our CI
builds).

I was already writing a detailed paragraph in the commit message to
explain all that when it occurred to me that two years might make a big
difference and an up to date `sparse` might be available. And lo and
behold, this is the case!

Therefore, v2 will no longer jump through that hoop.

Ciao,
Dscho
Johannes Schindelin July 14, 2021, 9:12 a.m. UTC | #5
Hi Danh,

On Tue, 13 Jul 2021, Đoàn Trần Công Danh wrote:

> On 2021-07-13 11:51:26+0000, Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > [...]
> > diff --git a/.github/workflows/run-sparse.yml b/.github/workflows/run-sparse.yml
> > new file mode 100644
> > index 00000000000..25f6a6efb40
> > --- /dev/null
> > +++ b/.github/workflows/run-sparse.yml
> > @@ -0,0 +1,22 @@
> > +name: Run `sparse`
>
> Markdown doesn't work with Workflow's name.
> Please remove those backticks.

The backticks are here not to render this as Markdown, but to make it
easier to parse what is said. "Run sparse" is technically an English
sentence, and a grammatically incorrect and confusing one. By enclosing
the word "sparse" in backticks, I make sure that the reader will
understand that this refers to a programming term.

FWIW I use backticks on this here mailing list all the time. And I am
fairly certain that no reader renders my mails as Markdown before reading
them.

> > +on: [push, pull_request]
> > +
> > +jobs:
> > +  sparse:
> > +    runs-on: ubuntu-20.04
> > +    steps:
> > +    - name: Download the `sparse` package
> > +      uses: git-for-windows/get-azure-pipelines-artifact@v0
> > +      with:
> > +        repository: git/git
> > +        definitionId: 10
> > +        artifact: sparse-20.04
> > +    - name: Install the `sparse` package
> > +      run: sudo dpkg -i sparse-20.04/sparse_*.deb
> > +    - name: Install a couple of dependencies
> > +      run: |
> > +        sudo apt-get install -q -y libssl-dev libcurl4-openssl-dev libexpat-dev gettext zlib1g-dev
> > +    - uses: actions/checkout@v2
> > +    - name: make sparse
> > +      run: make sparse
> > \ No newline at end of file
>
> The last step's name and run is the same. We can just drop name, it'll
> use run as name.

Good point.

> Anyway, remember the newline

Right. I edited this in VS Code, which does not care for that trailing
newline, and ran it through GitHub Actions, which also does not care for
that trailing newline, and the `check-whitespace` job also did not point
out any issue, therefore I missed this.

Thanks,
Dscho
Johannes Schindelin July 14, 2021, 10:09 a.m. UTC | #6
Hi Junio,

On Tue, 13 Jul 2021, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> >  .github/workflows/run-sparse.yml | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >  create mode 100644 .github/workflows/run-sparse.yml
>
> We choose to do this as a separate new workflow not as part of the
> main one because this is more like check-whitespace where there is
> no room for tests over the matrix of compilers and platforms play
> any useful role?

Ubuntu's `sparse` package was historically not up to date, not enough at
least to support Git's `make sparse`. Hence I created an Azure Pipeline to
build an up to date package, and since v1 used the GitHub Action
`get-azure-pipelines-artifact`. As a consequence, I thought, that this was
inappropriate for `main.yml` because we still try to _somewhat_ keep that
in sync with `.travis.yml`.

However, I realized that there are already too many differences (all the
Windows builds for example, which our Travis CI definition did not follow
suite, even after Travis CI got support for Windows agents).

So I folded it into the regular GitHub workflow.

There is one really big downside to that, though: currently, there is no
way to re-run only failed jobs in GitHub workflows (this is in contrast to
Azure Pipelines). You can only re-run _all_ jobs.

Which means that the likelihood of a run to fail increases with the number
of jobs in said run (even innocuous problems such as transient failures to
download an Ubuntu package), and it also makes it much more painful to
re-run the entire thing because you may well end up wasting a grand total
of ~370 minutes even if only a 30-second-job would need to be re-run.

Having said that, I think you're right and the upside of keeping things
together may outweigh that downside.

Ciao,
Dscho
Johannes Schindelin July 14, 2021, 10:13 a.m. UTC | #7
Hi again,

On Wed, 14 Jul 2021, Johannes Schindelin wrote:

> On Tue, 13 Jul 2021, Philippe Blain wrote:
>
> > Le 2021-07-13 à 07:51, Johannes Schindelin via GitGitGadget a écrit :
> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > >
> > > [...]
> > > +    - name: Download the `sparse` package
> > > +      uses: git-for-windows/get-azure-pipelines-artifact@v0
> > > +      with:
> > > +        repository: git/git
> > > +        definitionId: 10
> > > +        artifact: sparse-20.04
> > > +    - name: Install the `sparse` package
> > > +      run: sudo dpkg -i sparse-20.04/sparse_*.deb
> >
> > Out of curiosity, why is this necessary (as opposed to using
> > the Ubuntu package, i.e. 'sudo apt-get -q -y sparse') ?
>
> This is actually a historical curiosity: years ago, I created an Azure
> Pipeline that builds the `sparse` Debian package for the specific purpose
> of using it in our CI builds (if you care to look at the issue 345 I
> linked above, you will see how long ago that idea was in the making). Now,
> the historical curiosity is that back then, there was no current `sparse`
> package available for Ubuntu, and Ramsay mentioned that a newer version
> would be required to run `make sparse`.
>
> And when I implemented this patch yesterday, I did not even question this,
> I was just happy that I had come up with the GitHub Action
> `get-azure-pipelines-artifact` (to help with the `vcpkg` part of our CI
> builds).
>
> I was already writing a detailed paragraph in the commit message to
> explain all that when it occurred to me that two years might make a big
> difference and an up to date `sparse` might be available. And lo and
> behold, this is the case!
>
> Therefore, v2 will no longer jump through that hoop.

*Never mind*

I made a mistake in testing, and Ubuntu-20.04's `sparse` package _is too
old_. So I will reintroduce that hoop even before sending v2.

Ciao,
Dscho
Junio C Hamano July 14, 2021, 4 p.m. UTC | #8
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Which means that the likelihood of a run to fail increases with the number
> of jobs in said run (even innocuous problems such as transient failures to
> download an Ubuntu package), and it also makes it much more painful to
> re-run the entire thing because you may well end up wasting a grand total
> of ~370 minutes even if only a 30-second-job would need to be re-run.
>
> Having said that, I think you're right and the upside of keeping things
> together may outweigh that downside.

I wasn't make a request or a demand to change or not to change
anything, so in this particular exchange there was no point where I
was right (or wrong, for that matter ;-).  I was asking if there was
a solid reasoning behind the split, and if there is, I am perfectly
happy to see it done as a separate workflow with the log message
that explains why it is separate.  I am also perfectly fine with
this rolled into the primary one, with clear reasoning behind the
choice recorded in the log message.

Thanks.
Johannes Schindelin July 14, 2021, 8:54 p.m. UTC | #9
Hi Junio,

On Wed, 14 Jul 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Which means that the likelihood of a run to fail increases with the number
> > of jobs in said run (even innocuous problems such as transient failures to
> > download an Ubuntu package), and it also makes it much more painful to
> > re-run the entire thing because you may well end up wasting a grand total
> > of ~370 minutes even if only a 30-second-job would need to be re-run.
> >
> > Having said that, I think you're right and the upside of keeping things
> > together may outweigh that downside.
>
> I wasn't make a request or a demand to change or not to change
> anything, so in this particular exchange there was no point where I
> was right (or wrong, for that matter ;-).  I was asking if there was
> a solid reasoning behind the split, and if there is, I am perfectly
> happy to see it done as a separate workflow with the log message
> that explains why it is separate.  I am also perfectly fine with
> this rolled into the primary one, with clear reasoning behind the
> choice recorded in the log message.

I do not think that it would be an improvement to defend the default
choice (i.e. to add this new job to `.github/workflows/main.yml`) in the
commit message. It is the default for new CI stuff to go, after all, and
we do not need to clutter the message by stating the obvious.

Ciao,
Dscho
Junio C Hamano July 14, 2021, 8:56 p.m. UTC | #10
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Wed, 14 Jul 2021, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > Which means that the likelihood of a run to fail increases with the number
>> > of jobs in said run (even innocuous problems such as transient failures to
>> > download an Ubuntu package), and it also makes it much more painful to
>> > re-run the entire thing because you may well end up wasting a grand total
>> > of ~370 minutes even if only a 30-second-job would need to be re-run.
>> >
>> > Having said that, I think you're right and the upside of keeping things
>> > together may outweigh that downside.
>>
>> I wasn't make a request or a demand to change or not to change
>> anything, so in this particular exchange there was no point where I
>> was right (or wrong, for that matter ;-).  I was asking if there was
>> a solid reasoning behind the split, and if there is, I am perfectly
>> happy to see it done as a separate workflow with the log message
>> that explains why it is separate.  I am also perfectly fine with
>> this rolled into the primary one, with clear reasoning behind the
>> choice recorded in the log message.
>
> I do not think that it would be an improvement to defend the default
> choice (i.e. to add this new job to `.github/workflows/main.yml`) in the
> commit message. It is the default for new CI stuff to go, after all, and
> we do not need to clutter the message by stating the obvious.

It wasn't quite obvious why we justify spending 370 minutes one more
time only to rerun 30-second job, though.
Johannes Schindelin July 14, 2021, 10:03 p.m. UTC | #11
Hi Junio,

On Wed, 14 Jul 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Wed, 14 Jul 2021, Junio C Hamano wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >>
> >> > Which means that the likelihood of a run to fail increases with the number
> >> > of jobs in said run (even innocuous problems such as transient failures to
> >> > download an Ubuntu package), and it also makes it much more painful to
> >> > re-run the entire thing because you may well end up wasting a grand total
> >> > of ~370 minutes even if only a 30-second-job would need to be re-run.
> >> >
> >> > Having said that, I think you're right and the upside of keeping things
> >> > together may outweigh that downside.
> >>
> >> I wasn't make a request or a demand to change or not to change
> >> anything, so in this particular exchange there was no point where I
> >> was right (or wrong, for that matter ;-).  I was asking if there was
> >> a solid reasoning behind the split, and if there is, I am perfectly
> >> happy to see it done as a separate workflow with the log message
> >> that explains why it is separate.  I am also perfectly fine with
> >> this rolled into the primary one, with clear reasoning behind the
> >> choice recorded in the log message.
> >
> > I do not think that it would be an improvement to defend the default
> > choice (i.e. to add this new job to `.github/workflows/main.yml`) in the
> > commit message. It is the default for new CI stuff to go, after all, and
> > we do not need to clutter the message by stating the obvious.
>
> It wasn't quite obvious why we justify spending 370 minutes one more
> time only to rerun 30-second job, though.

True.

And this is not a new problem. Every time anything happens in those
`osx-gcc` or `osx-clang` jobs (e.g. that transient problem with the broken
pipes in t5516 [*1*], that's a fun one), a full re-run is necessary, or
else the commit and/or PR will remain marked as broken.

In other words, while it is totally appropriate for me to explain this to
you in this here thread because it came up as a tangent, it would be
inappropriate to stick that explanation into this patch's commit message.
We do not make a habit of adding tangents that came up during patch
reviews into commit messages, and I do not intend to start such a habit
here, either.

Ciao,
Dscho

Footnote *1*: See
https://lore.kernel.org/git/20190829143805.GB1746@sigill.intra.peff.net/
(I don't think this is fixed as of now)
Junio C Hamano July 14, 2021, 10:27 p.m. UTC | #12
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> It wasn't quite obvious why we justify spending 370 minutes one more
>> time only to rerun 30-second job, though.
>
> True.
>
> And this is not a new problem. Every time anything happens in those
> `osx-gcc` or `osx-clang` jobs (e.g. that transient problem with the broken
> pipes in t5516 [*1*], that's a fun one), a full re-run is necessary, or
> else the commit and/or PR will remain marked as broken.
>
> In other words, while it is totally appropriate for me to explain this to
> you in this here thread because it came up as a tangent, it would be
> inappropriate to stick that explanation into this patch's commit message.
> We do not make a habit of adding tangents that came up during patch
> reviews into commit messages, and I do not intend to start such a habit
> here, either.

I do not agree; a brief mention "even though piling more and more on
the primary workflow would make it even less convenient to re-run,
it is already so bad that another one would not hurt too much more"
would be a clue good enough to motivate others to do something about
it if they feel like it.
Ramsay Jones July 16, 2021, 1:37 a.m. UTC | #13
On 14/07/2021 11:13, Johannes Schindelin wrote:
> Hi again,
> 
> On Wed, 14 Jul 2021, Johannes Schindelin wrote:
> 
>> On Tue, 13 Jul 2021, Philippe Blain wrote:
>>
>>> Le 2021-07-13 à 07:51, Johannes Schindelin via GitGitGadget a écrit :
>>>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>>>
>>>> [...]
>>>> +    - name: Download the `sparse` package
>>>> +      uses: git-for-windows/get-azure-pipelines-artifact@v0
>>>> +      with:
>>>> +        repository: git/git
>>>> +        definitionId: 10
>>>> +        artifact: sparse-20.04
>>>> +    - name: Install the `sparse` package
>>>> +      run: sudo dpkg -i sparse-20.04/sparse_*.deb
>>>
>>> Out of curiosity, why is this necessary (as opposed to using
>>> the Ubuntu package, i.e. 'sudo apt-get -q -y sparse') ?
>>
>> This is actually a historical curiosity: years ago, I created an Azure
>> Pipeline that builds the `sparse` Debian package for the specific purpose
>> of using it in our CI builds (if you care to look at the issue 345 I
>> linked above, you will see how long ago that idea was in the making). Now,
>> the historical curiosity is that back then, there was no current `sparse`
>> package available for Ubuntu, and Ramsay mentioned that a newer version
>> would be required to run `make sparse`.
>>
>> And when I implemented this patch yesterday, I did not even question this,
>> I was just happy that I had come up with the GitHub Action
>> `get-azure-pipelines-artifact` (to help with the `vcpkg` part of our CI
>> builds).
>>
>> I was already writing a detailed paragraph in the commit message to
>> explain all that when it occurred to me that two years might make a big
>> difference and an up to date `sparse` might be available. And lo and
>> behold, this is the case!
>>
>> Therefore, v2 will no longer jump through that hoop.
> 
> *Never mind*
> 
> I made a mistake in testing, and Ubuntu-20.04's `sparse` package _is too
> old_. So I will reintroduce that hoop even before sending v2.

Yes, last time I looked, 20.04's version of sparse was 0.6.1-2build1,
20.10 had 0.6.2-2 (both of which are too old), but 21.04, along with
Debian Testing, had 0.6.3-1 which would work fine.

[I am currently running v0.6.3-341-g8af24329]

So, maybe the next Ubuntu LTS, ... :-D

ATB,
Ramsay Jones
Johannes Schindelin July 16, 2021, 3:25 p.m. UTC | #14
Hi Junio,

On Wed, 14 Jul 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> It wasn't quite obvious why we justify spending 370 minutes one more
> >> time only to rerun 30-second job, though.
> >
> > True.
> >
> > And this is not a new problem. Every time anything happens in those
> > `osx-gcc` or `osx-clang` jobs (e.g. that transient problem with the broken
> > pipes in t5516 [*1*], that's a fun one), a full re-run is necessary, or
> > else the commit and/or PR will remain marked as broken.
> >
> > In other words, while it is totally appropriate for me to explain this to
> > you in this here thread because it came up as a tangent, it would be
> > inappropriate to stick that explanation into this patch's commit message.
> > We do not make a habit of adding tangents that came up during patch
> > reviews into commit messages, and I do not intend to start such a habit
> > here, either.
>
> I do not agree; a brief mention "even though piling more and more on
> the primary workflow would make it even less convenient to re-run,
> it is already so bad that another one would not hurt too much more"
> would be a clue good enough to motivate others to do something about
> it if they feel like it.

By that rationale recent additions to `.github/workflows/`, which have a
much much much much bigger impact on the runtime (such as the change that
lets `linux-clang` run the test suite with SHA-1 and then again with
SHA-256, almost doubling its runtime), should have added that apology to
the commit message.

But that did not happen.

Besides, for all we know the problem might go away at any stage because
pretty much all other main CI systems have a way to re-run only failed
jobs. GitHub Actions will probably get it at some stage, too, I vaguely
remember seeing it on some public roadmap somewhere.

So I really do not appreciate this pushing for including an explanation in
the commit message for something that is only relevant (if at all)
in the context of an utter tangent (don't we have many of those over
here!) during the review of the patch. It's just some curiosity that will
eventually be an exclusively historical curiosity, of no interest except
to a few CI nerds. The issue has everything to do with a currently
missing feature in GitHub Actions, and it has nothing to do at all with
what the patch is about, which is: to add a `sparse` job to our CI runs.

Ciao,
Dscho
Junio C Hamano July 16, 2021, 4:42 p.m. UTC | #15
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Besides, for all we know the problem might go away at any stage because
> pretty much all other main CI systems have a way to re-run only failed
> jobs.

Yes, that is something I find plausible.  That's one more reason why
we currently feel it is OK to roll it into the primary job.

Now we've left a handful of messages on the list, I think we are
safe against anybody who will soon complain that we are piling more
and more on top of the primary job---instead of pointing at the log
message of the change that did so, we can point at this discussion
thread to make them understand why we decided that it is OK.
diff mbox series

Patch

diff --git a/.github/workflows/run-sparse.yml b/.github/workflows/run-sparse.yml
new file mode 100644
index 00000000000..25f6a6efb40
--- /dev/null
+++ b/.github/workflows/run-sparse.yml
@@ -0,0 +1,22 @@ 
+name: Run `sparse`
+
+on: [push, pull_request]
+
+jobs:
+  sparse:
+    runs-on: ubuntu-20.04
+    steps:
+    - name: Download the `sparse` package
+      uses: git-for-windows/get-azure-pipelines-artifact@v0
+      with:
+        repository: git/git
+        definitionId: 10
+        artifact: sparse-20.04
+    - name: Install the `sparse` package
+      run: sudo dpkg -i sparse-20.04/sparse_*.deb
+    - name: Install a couple of dependencies
+      run: |
+        sudo apt-get install -q -y libssl-dev libcurl4-openssl-dev libexpat-dev gettext zlib1g-dev
+    - uses: actions/checkout@v2
+    - name: make sparse
+      run: make sparse
\ No newline at end of file