ci: run `make sparse` as a GitHub workflow
PATCH 1/1 ci: run `make sparse` as a GitHub workflow
From: Johannes Schindelin <[email protected]>
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
---
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
.github/workflows/run-sparse.yml new file +22 -0
@@ -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
base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb
Signature · 1 line
gitgitgadget
Quoted 6 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
Occasionally we receive reviews after patches were integrated, where
sparseidentified problems such as file-local variables or functions being declared as global.
Quoted 8 lines
By running
sparseas 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 <[email protected]>
Thanks for doing this change.
Some minor nits below.
.github/workflows/run-sparse.yml line 1
Quoted 7 lines
.github/workflows/run-sparse.yml new file +1
-0@@ -0,0 +1,22 @@ +name: Run `sparse`
Markdown doesn't work with Workflow's name.
Please remove those backticks.
Quoted 11 lines
.github/workflows/run-sparse.yml new file +11
-0+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
.github/workflows/run-sparse.yml line 22
Quoted 10 lines
.github/workflows/run-sparse.yml new file +9
-0+ 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
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.
Signature · 1 line
Danh
Hi Danh,
Quoted 14 lines · Đoàn Trần Công Danh
.github/workflows/run-sparse.yml line 1
Quoted 10 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
[...]
.github/workflows/run-sparse.yml new file +1
-0@@ -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.
Quoted 16 lines
Quoted 16 lines
.github/workflows/run-sparse.yml new file +16
-0+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: |
Quoted 8 lines
.github/workflows/run-sparse.yml line 22
Quoted 5 lines
.github/workflows/run-sparse.yml new file +4
-0+ 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 sparseThe last step's name and run is the same. We can just drop name, it'll use run as name.
Good point.
Quoted 1 line
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
Hi Dscho,
Le 2021-07-13 à 07:51, Johannes Schindelin via GitGitGadget a écrit :
Quoted 7 lines
From: Johannes Schindelin <[email protected]>
Occasionally we receive reviews after patches were integrated, where
sparseidentified 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 ?
Quoted 9 lines
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 <[email protected]>
.github/workflows/run-sparse.yml line 16
Quoted 8 lines
.github/workflows/run-sparse.yml new file +8
-0+ - 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.
Hi Philippe,
Quoted 7 lines · Philippe Blain
Le 2021-07-13 à 07:51, Johannes Schindelin via GitGitGadget a écrit :
Quoted 6 lines
From: Johannes Schindelin <[email protected]>
Occasionally we receive reviews after patches were integrated, where
sparseidentified problems such as file-local variables or functions being declared as global.
Quoted 5 lines
Quoted 1 line
By running
sparsemaybe 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.
Quoted 18 lines
Quoted 9 lines
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 <[email protected]>.github/workflows/run-sparse.yml line 16
Quoted 8 lines
.github/workflows/run-sparse.yml new file +8
-0+ - 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
Quoted 2 lines
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
Hi again,
Quoted 31 lines · Johannes Schindelin
Quoted 13 lines · Philippe Blain
Le 2021-07-13 à 07:51, Johannes Schindelin via GitGitGadget a écrit :
.github/workflows/run-sparse.yml line 16
Quoted 11 lines
From: Johannes Schindelin <[email protected]>
[...]
.github/workflows/run-sparse.yml new file +8
-0+ - 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_*.debQuoted 2 lines
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
sparseDebian 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 currentsparsepackage available for Ubuntu, and Ramsay mentioned that a newer version would be required to runmake 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 thevcpkgpart of our CI builds).
Quoted 6 lines
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
sparsemight 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
Quoted 40 lines · Johannes Schindelin
Hi again,
Quoted 36 lines · Johannes Schindelin
Quoted 13 lines · Philippe Blain
Le 2021-07-13 à 07:51, Johannes Schindelin via GitGitGadget a écrit :
.github/workflows/run-sparse.yml line 16
Quoted 11 lines
From: Johannes Schindelin <[email protected]>
[...]
.github/workflows/run-sparse.yml new file +8
-0+ - 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_*.debQuoted 2 lines
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
sparseDebian 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 currentsparsepackage available for Ubuntu, and Ramsay mentioned that a newer version would be required to runmake 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 thevcpkgpart 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
sparsemight be available. And lo and behold, this is the case!
Quoted 6 lines
Quoted 1 line
Therefore, v2 will no longer jump through that hoop.
*Never mind*
I made a mistake in testing, and Ubuntu-20.04's
sparsepackage_istooold_. 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 via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Johannes Schindelin <[email protected]>
Occasionally we receive reviews after patches were integrated, where
sparseidentified 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.
Quoted 20 lines
By running
sparseas 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 <[email protected]>---
ci: run make sparse as a GitHub workflowOne 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
Quoted 7 lines
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.
Quoted 31 lines
.github/workflows/run-sparse.yml new file +22
-0@@ -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 sparsebase-commit:
ebf3c04b262aa27fbb97f8a0156c2347fecafafb
Hi Junio,
Quoted 5 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 2 lines · writes:
From: Johannes Schindelin <[email protected]>
Quoted 8 lines
Quoted 3 lines
.github/workflows/run-sparse.yml | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 .github/workflows/run-sparse.ymlWe 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
Quoted 8 lines · Johannes Schindelin
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.
Hi Junio,
Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Johannes Schindelin
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.
Quoted 8 lines
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
Quoted 24 lines · Johannes Schindelin
Hi Junio,
Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Johannes Schindelin
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.
Quoted 8 lines
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.
Quoted 4 lines
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.
Hi Junio,
Quoted 24 lines · Junio C Hamano
Quoted 22 lines · Johannes Schindelin
Quoted 19 lines · Junio C Hamano
Quoted 8 lines · Johannes Schindelin
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.
Quoted 7 lines
Quoted 4 lines
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/[email protected]/
(I don't think this is fixed as of now)
Quoted 10 lines · Johannes Schindelin
Quoted 2 lines
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-gccorosx-clangjobs (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.
Quoted 6 lines
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.
Hi Junio,
Quoted 19 lines · Junio C Hamano
Quoted 10 lines · Johannes Schindelin
Quoted 2 lines
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-gccorosx-clangjobs (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.Quoted 6 lines
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.
Quoted 5 lines
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
Quoted 3 lines · Johannes Schindelin
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.
PATCH v2 1/1 ci: run `make sparse` as part of the GitHub workflow
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Occasionally we receive reviews after patches were integrated, where sparse (https://sparse.docs.kernel.org/en/latest/ has more information on that project) 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
Note: Not even Ubuntu 20.04 ships with a new enough version of sparse to accommodate Git's needs. The symptom looks like this:
add-interactive.c:537:51: error: Using plain integer as NULL pointer
To counter that, we download and install the custom-built sparse package from the Azure Pipeline that we specifically created to address this issue.
---
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/pull/994/checks?check_run_id=3065255116
Changes since v1:
* The job was folded into main.yml
* The commit message and a code comment now explain why we have to download & install a custom sparse package instead of using Ubuntu's default one
* The commit message now contains a link to the documentation of the sparse tool
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-994%2Fdscho%2Fci-enable-sparse-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-994/dscho/ci-enable-sparse-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/994
Range-diff vs v1:
1: d1af618dd73 ! 1: 8b631acfaa2 ci: run make sparse as a GitHub workflow
@@ Metadata
Author: Johannes Schindelin <[email protected]>
## Commit message ##
- ci: run make sparse as a GitHub workflow
+ ci: run make sparse as part of the GitHub workflow
Occasionally we receive reviews after patches were integrated, where
- sparse identified problems such as file-local variables or functions
- being declared as global.
+ sparse (https://sparse.docs.kernel.org/en/latest/ has more information
+ on that project) 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
@@ Commit message
This addresses https://github.com/gitgitgadget/git/issues/345
+ Note: Not even Ubuntu 20.04 ships with a new enough version of sparse
+ to accommodate Git's needs. The symptom looks like this:
+
+ add-interactive.c:537:51: error: Using plain integer as NULL pointer
+
+ To counter that, we download and install the custom-built sparse
+ package from the Azure Pipeline that we specifically created to address
+ this issue.
+
- ## .github/workflows/run-sparse.yml (new) ## -@@
-+name: Run sparse
-+
-+on: [push, pull_request]
-+
-+jobs:
+ ## .github/workflows/main.yml ##
+@@ .github/workflows/main.yml: jobs:
+ - uses: actions/checkout@v1
+ - run: ci/install-dependencies.sh
+ - run: ci/run-static-analysis.sh
+ sparse:
++ needs: ci-config
++ if: needs.ci-config.outputs.enabled == 'yes'
++ env:
++ jobname: sparse
+ runs-on: ubuntu-20.04
+ steps:
-+ - name: Download the sparse package
++ - name: Download a current sparse package
++ # Ubuntu's sparse version is too old for us
+ uses: git-for-windows/get-azure-pipelines-artifact@v0
+ with:
+ repository: git/git
+ definitionId: 10
+ artifact: sparse-20.04
-+ - name: Install the sparse package
++ - name: Install the current sparse package
+ run: sudo dpkg -i sparse-20.04/sparse_*.deb
-+ - name: Install a couple of dependencies
++ - name: Install other 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
++ - run: make sparse
+ documentation:
+ needs: ci-config
+ if: needs.ci-config.outputs.enabled == 'yes'
.github/workflows/main.yml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
.github/workflows/main.yml +21 -0
@@ -350,6 +350,27 @@ jobs: - uses: actions/checkout@v1 - run: ci/install-dependencies.sh - run: ci/run-static-analysis.sh + sparse: + needs: ci-config + if: needs.ci-config.outputs.enabled == 'yes' + env: + jobname: sparse + runs-on: ubuntu-20.04 + steps: + - name: Download a current `sparse` package + # Ubuntu's `sparse` version is too old for us + uses: git-for-windows/get-azure-pipelines-artifact@v0 + with: + repository: git/git + definitionId: 10 + artifact: sparse-20.04 + - name: Install the current `sparse` package + run: sudo dpkg -i sparse-20.04/sparse_*.deb + - name: Install other dependencies + run: | + sudo apt-get install -q -y libssl-dev libcurl4-openssl-dev libexpat-dev gettext zlib1g-dev + - uses: actions/checkout@v2 + - run: make sparse documentation: needs: ci-config if: needs.ci-config.outputs.enabled == 'yes'
base-commit: 75ae10bc75336db031ee58d13c5037b929235912
Signature · 1 line
gitgitgadget
PATCH 1/1 ci: run "apt-get update" before "apt-get install"
Review
Quoted 3 lines · Johannes Schindelin via GitGitGadget
(quoted diff) +3
-0+ - name: Install other dependencies + run: | + sudo apt-get install -q -y libssl-dev libcurl4-openssl-dev libexpat-dev gettext zlib1g-dev
My CI runs based on 'next' started failing today. I think we'd want this on top (of js/ci-make-sparse):
-- >8 --
Subject: [PATCH] ci: run "apt-get update" before "apt-get install"
The "sparse" workflow runs "apt-get install" to pick up a few necessary packages. But it needs to run "apt-get update" first, or it risks trying to download an old package version that no longer exists. And in fact this happens now, with output like:
2021-07-26T17:40:51.2551880Z E: Failed to fetch http://security.ubuntu.com/ubuntu/pool/main/c/curl/libcurl4-openssl-dev_7.68.0-1ubuntu2.5_amd64.deb 404 Not Found [IP: 52.147.219.192 80] 2021-07-26T17:40:51.2554304Z E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?
Our other ci jobs don't suffer from this; they rely on scripts in ci/, and ci/install-dependencies does the appropriate "apt-get update".
---
I guess this package setup could also be moved into ci/install-dependencies.sh, but I don't think it really buys anything (the "apt-get update" line would not even be shared, because the outermost layer is a big switch statement on the jobname). OTOH, it looks like other one-off jobs are in there (e.g., StaticAnalysis).
Anyway, this is the minimal fixup.
.github/workflows/main.yml | 1 + 1 file changed, 1 insertion(+)
.github/workflows/main.yml +1 -0
@@ -368,6 +368,7 @@ jobs: run: sudo dpkg -i sparse-20.04/sparse_*.deb - name: Install other dependencies run: | + sudo apt-get update -q && sudo apt-get install -q -y libssl-dev libcurl4-openssl-dev libexpat-dev gettext zlib1g-dev - uses: actions/checkout@v2 - run: make sparse
Signature · 1 line
2.32.0.805.g1fa0022869
Re: [PATCH] ci: run "apt-get update" before "apt-get install"
Quoted 7 lines · Jeff King
I guess this package setup could also be moved into
ci/install-dependencies.sh, but I don't think it really buys anything (the "apt-get update" line would not even be shared, because the outermost layer is a big switch statement on the jobname). OTOH, it looks like other one-off jobs are in there (e.g., StaticAnalysis).Anyway, this is the minimal fixup.
That would look like this on top. I don't have a strong feeling either way.
-- >8 --
Subject: [PATCH] ci/install-dependencies: handle "sparse" job package installs
This just matches the style/location of the package installation for other jobs. There should be no functional change.
I did flip the order of the options and command-name ("-y update" instead of "update -y") for consistency with other lines in the same file.
Note also that we have to reorder the dependency install with the "checkout" action, so that we actually have the "ci" scripts available.
--- .github/workflows/main.yml | 6 ++---- ci/install-dependencies.sh | 5 +++++ 2 files changed, 7 insertions(+), 4 deletions(-)
.github/workflows/main.yml +2 -4
@@ -366,11 +366,9 @@ jobs: artifact: sparse-20.04 - name: Install the current `sparse` package run: sudo dpkg -i sparse-20.04/sparse_*.deb - - name: Install other dependencies - run: | - sudo apt-get update -q && - sudo apt-get install -q -y libssl-dev libcurl4-openssl-dev libexpat-dev gettext zlib1g-dev - uses: actions/checkout@v2 + - name: Install other dependencies + run: ci/install-dependencies.sh - run: make sparse documentation: needs: ci-config
ci/install-dependencies.sh +5 -0
@@ -65,6 +65,11 @@ StaticAnalysis) sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \ libexpat-dev gettext make ;; +sparse) + sudo apt-get -q update -q + sudo apt-get -q -y install libssl-dev libcurl4-openssl-dev \ + libexpat-dev gettext zlib1g-dev + ;; Documentation) sudo apt-get -q update sudo apt-get -q -y install asciidoc xmlto docbook-xsl-ns make
Signature · 1 line
2.32.0.806.g45f317a2a5
Re: [PATCH] ci: run "apt-get update" before "apt-get install"
In reply to Jeff KingQuoted 8 lines · Jeff King
Review
Quoted 3 lines · Johannes Schindelin via GitGitGadget
(quoted diff) +3
-0+ - name: Install other dependencies + run: | + sudo apt-get install -q -y libssl-dev libcurl4-openssl-dev libexpat-dev gettext zlib1g-devMy CI runs based on 'next' started failing today. I think we'd want this on top (of js/ci-make-sparse):
Makes sense.
Quoted 39 lines
-- >8 --
Subject: [PATCH] ci: run "apt-get update" before "apt-get install"The "sparse" workflow runs "apt-get install" to pick up a few necessary packages. But it needs to run "apt-get update" first, or it risks trying to download an old package version that no longer exists. And in fact this happens now, with output like:
2021-07-26T17:40:51.2551880Z E: Failed to fetch http://security.ubuntu.com/ubuntu/pool/main/c/curl/libcurl4-openssl-dev_7.68.0-1ubuntu2.5_amd64.deb 404 Not Found [IP: 52.147.219.192 80] 2021-07-26T17:40:51.2554304Z E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?Our other ci jobs don't suffer from this; they rely on scripts in ci/, and ci/install-dependencies does the appropriate "apt-get update".
Signed-off-by Jeff King <[email protected]>---
I guess this package setup could also be moved intoci/install-dependencies.sh, but I don't think it really buys anything (the "apt-get update" line would not even be shared, because the outermost layer is a big switch statement on the jobname). OTOH, it looks like other one-off jobs are in there (e.g., StaticAnalysis).Anyway, this is the minimal fixup.
.github/workflows/main.yml | 1 + 1 file changed, 1 insertion(+).github/workflows/main.yml +1
-0@@ -368,6 +368,7 @@ jobs: run: sudo dpkg -i sparse-20.04/sparse_*.deb - name: Install other dependencies run: | + sudo apt-get update -q && sudo apt-get install -q -y libssl-dev libcurl4-openssl-dev libexpat-dev gettext zlib1g-dev - uses: actions/checkout@v2 - run: make sparse
PATCH v3 1/1 ci: run `make sparse` as part of the GitHub workflow
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Occasionally we receive reviews after patches were integrated, where sparse (https://sparse.docs.kernel.org/en/latest/ has more information on that project) 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
Note: Not even Ubuntu 20.04 ships with a new enough version of sparse to accommodate Git's needs. The symptom looks like this:
add-interactive.c:537:51: error: Using plain integer as NULL pointer
To counter that, we download and install the custom-built sparse package from the Azure Pipeline that we specifically created to address this issue.
---
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/pull/994/checks?check_run_id=3065255116
Changes since v2:
* We're now reusing the ci/install-dependencies.sh script even in the sparse job.
Changes since v1:
* The job was folded into main.yml
* The commit message and a code comment now explain why we have to download & install a custom sparse package instead of using Ubuntu's default one
* The commit message now contains a link to the documentation of the sparse tool
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-994%2Fdscho%2Fci-enable-sparse-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-994/dscho/ci-enable-sparse-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/994
Range-diff vs v2:
1: 8b631acfaa2 ! 1: 762c4cbb6e2 ci: run make sparse as part of the GitHub workflow
@@ Commit message
package from the Azure Pipeline that we specifically created to address this issue.
+ Helped-by: Jeff King <[email protected]>
## .github/workflows/main.yml ##
@@ .github/workflows/main.yml: jobs:
+ artifact: sparse-20.04
+ - name: Install the current sparse package
+ run: sudo dpkg -i sparse-20.04/sparse_*.deb
-+ - name: Install other dependencies
-+ run: |
-+ sudo apt-get install -q -y libssl-dev libcurl4-openssl-dev libexpat-dev gettext zlib1g-dev
+ - uses: actions/checkout@v2
++ - name: Install other dependencies
++ run: ci/install-dependencies.sh
+ - run: make sparse
documentation:
needs: ci-config
if: needs.ci-config.outputs.enabled == 'yes'
+
+ ## ci/install-dependencies.sh ##
+@@ ci/install-dependencies.sh: StaticAnalysis)
+ sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
+ libexpat-dev gettext make
+ ;;
++sparse)
++ sudo apt-get -q update -q
++ sudo apt-get -q -y install libssl-dev libcurl4-openssl-dev \
++ libexpat-dev gettext zlib1g-dev
++ ;;
+ Documentation)
+ sudo apt-get -q update
+ sudo apt-get -q -y install asciidoc xmlto docbook-xsl-ns make
.github/workflows/main.yml | 20 ++++++++++++++++++++ ci/install-dependencies.sh | 5 +++++ 2 files changed, 25 insertions(+)
.github/workflows/main.yml +20 -0
@@ -350,6 +350,26 @@ jobs: - uses: actions/checkout@v1 - run: ci/install-dependencies.sh - run: ci/run-static-analysis.sh + sparse: + needs: ci-config + if: needs.ci-config.outputs.enabled == 'yes' + env: + jobname: sparse + runs-on: ubuntu-20.04 + steps: + - name: Download a current `sparse` package + # Ubuntu's `sparse` version is too old for us + uses: git-for-windows/get-azure-pipelines-artifact@v0 + with: + repository: git/git + definitionId: 10 + artifact: sparse-20.04 + - name: Install the current `sparse` package + run: sudo dpkg -i sparse-20.04/sparse_*.deb + - uses: actions/checkout@v2 + - name: Install other dependencies + run: ci/install-dependencies.sh + - run: make sparse documentation: needs: ci-config if: needs.ci-config.outputs.enabled == 'yes'
ci/install-dependencies.sh +5 -0
@@ -65,6 +65,11 @@ StaticAnalysis) sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \ libexpat-dev gettext make ;; +sparse) + sudo apt-get -q update -q + sudo apt-get -q -y install libssl-dev libcurl4-openssl-dev \ + libexpat-dev gettext zlib1g-dev + ;; Documentation) sudo apt-get -q update sudo apt-get -q -y install asciidoc xmlto docbook-xsl-ns make
base-commit: 75ae10bc75336db031ee58d13c5037b929235912
Signature · 1 line
gitgitgadget
Re: [PATCH v3] ci: run `make sparse` as part of the GitHub workflow
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 19 lines · writes:
From: Johannes Schindelin <[email protected]>
Occasionally we receive reviews after patches were integrated, where
sparse(https://sparse.docs.kernel.org/en/latest/ has more information on that project) identified problems such as file-local variables or functions being declared as global.By running
sparseas 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
Note: Not even Ubuntu 20.04 ships with a new enough version of
sparseto accommodate Git's needs. The symptom looks like this:add-interactive.c:537:51: error: Using plain integer as NULL pointer
Quoted 7 lines
To counter that, we download and install the custom-built
sparsepackage from the Azure Pipeline that we specifically created to address this issue.Helped-by Jeff King <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>---
OK, this is with Peff's "oh, we need 'apt-get update' to avoid stale package list" fix already rolled in? It seems you took both of them, and they look good to me, too.
Unfortunately, ci-make-sparse has already been in 'next' for the past week or so, so incrementals are vastly preferred.
Let me squish in your "Acked-by" to both of Peff's patches and queue them on top of js/ci-make-sparse topic.
Thanks.