| Message ID | 20240430003323.6210-3-jltobler@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add GitLab CI to check for whitespace errors | expand |
Thanks for putting this together, Justin! > +check-whitespace: > + image: ubuntu:latest I wonder if we should pin to `ubuntu:22.04` and only update this for each LTS release. It seems like we've done this for the `static-analysis` job above. > + before_script: > + - ./ci/install-docker-dependencies.sh > + script: > + - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA > + rules: > + - if: $CI_PIPELINE_SOURCE == 'merge_request_event' > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh > new file mode 100755 > index 0000000000..1cad2d7374 > --- /dev/null > +++ b/ci/check-whitespace.sh > @@ -0,0 +1,16 @@ > +#! /bin/sh nit: there seems to be extra whitespace after the shebang :D
On 24/04/30 11:59AM, James Liu wrote: > Thanks for putting this together, Justin! > > > +check-whitespace: > > + image: ubuntu:latest > > I wonder if we should pin to `ubuntu:22.04` and only update this for > each LTS release. It seems like we've done this for the > `static-analysis` job above. I'm not sure if it particually matters. I know Patrick recently submitted the following also using `ubuntu:latest`: https://lore.kernel.org/git/01fb94999f8e2014ba4d09ce7451a4f5d315ee72.1714371146.git.ps@pks.im/#r I can change it though if there is strong opinion one way or the other. > > > + before_script: > > + - ./ci/install-docker-dependencies.sh > > + script: > > + - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA > > + rules: > > + - if: $CI_PIPELINE_SOURCE == 'merge_request_event' > > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh > > new file mode 100755 > > index 0000000000..1cad2d7374 > > --- /dev/null > > +++ b/ci/check-whitespace.sh > > @@ -0,0 +1,16 @@ > > +#! /bin/sh > > nit: there seems to be extra whitespace after the shebang :D > Whoops... I'll address this in a subsequent version. -Justin
On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote: > To check for whitespace errors introduced by a set of changes, there is > the `.github/workflows/check-whitespace.yml` GitHub action. This script > executes `git log --check` over a range containing the new commits and > parses the output to generate a markdown formatted artifact that > summarizes detected errors with GitHub links to the affected commits and > blobs. > > Since this script is rather specific to GitHub actions, a more general > and simple `ci/check-whitespace.sh` is added instead that functions the > same, but does not generate the markdown file for the action summary. > From this, a new GitLab CI job is added to support the whitespace error > check. I still wonder whether we can unify these. Yes, the GitHub thing is quite specific. But ultimately, what it does is to generate a proper summary of where exactly the whitespaces issues are, which is something that your version doesn't do. It's useful though for consumers of a failed CI job to know exactly which commit has the issue. So can't we pull out the logic into a script, refactor it such that it knows to print both GitHub- and GitLab-style URLs, and then also print the summary in GitLab CI? > Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only > available in GitLab merge request pipelines and therefore the CI job is > configured to only run as part of those pipelines. > > Signed-off-by: Justin Tobler <jltobler@gmail.com> > --- > .gitlab-ci.yml | 9 +++++++++ > ci/check-whitespace.sh | 16 ++++++++++++++++ > 2 files changed, 25 insertions(+) > create mode 100755 ci/check-whitespace.sh > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index c0fa2fe90b..31cf420a11 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -102,3 +102,12 @@ static-analysis: > script: > - ./ci/run-static-analysis.sh > - ./ci/check-directional-formatting.bash > + > +check-whitespace: > + image: ubuntu:latest > + before_script: > + - ./ci/install-docker-dependencies.sh > + script: > + - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA Let's quote this variable. > + rules: > + - if: $CI_PIPELINE_SOURCE == 'merge_request_event' > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh > new file mode 100755 > index 0000000000..1cad2d7374 > --- /dev/null > +++ b/ci/check-whitespace.sh > @@ -0,0 +1,16 @@ > +#! /bin/sh > +# > +# Check that commits after a specified point do not contain new or modified > +# lines with whitespace errors. > +# > + > +baseSha=${1} Two nits: first, I wouldn't call it "sha" because it really is a commit ID that may or may not be SHA if we were ever to grow a hash function that is not SHA. So "baseCommit" would be more descriptive. Second, our shell variables are typically written in all-uppercase. So that'd make it "BASE_COMMIT". > +git log --check --pretty=format:"---% h% s" ${baseSha}.. Nit: there's no need for the curly braces here. Also, let's quote the value. Patrick > +if test $? -ne 0 > +then > + echo "A whitespace issue was found in one or more of the commits." > + echo "Run the following command to resolve whitespace issues:" > + echo "\tgit rebase --whitespace=fix ${baseSha}" > + exit 2 > +fi > -- > 2.44.0 > >
On Mon, Apr 29, 2024 at 10:01:15PM -0500, Justin Tobler wrote: > On 24/04/30 11:59AM, James Liu wrote: > > Thanks for putting this together, Justin! > > > > > +check-whitespace: > > > + image: ubuntu:latest > > > > I wonder if we should pin to `ubuntu:22.04` and only update this for > > each LTS release. It seems like we've done this for the > > `static-analysis` job above. > > I'm not sure if it particually matters. I know Patrick recently > submitted the following also using `ubuntu:latest`: > https://lore.kernel.org/git/01fb94999f8e2014ba4d09ce7451a4f5d315ee72.1714371146.git.ps@pks.im/#r > > I can change it though if there is strong opinion one way or the other. I'd say it probably doesn't matter much for this case. But using `ubuntu:latest` means that there's less maintenance going forward because we don't ever have to update it. Patrick
On Tue, Apr 30, 2024 at 07:04:46AM +0200, Patrick Steinhardt wrote: > On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote: > > To check for whitespace errors introduced by a set of changes, there is > > the `.github/workflows/check-whitespace.yml` GitHub action. This script > > executes `git log --check` over a range containing the new commits and > > parses the output to generate a markdown formatted artifact that > > summarizes detected errors with GitHub links to the affected commits and > > blobs. > > > > Since this script is rather specific to GitHub actions, a more general > > and simple `ci/check-whitespace.sh` is added instead that functions the > > same, but does not generate the markdown file for the action summary. > > From this, a new GitLab CI job is added to support the whitespace error > > check. > > I still wonder whether we can unify these. Yes, the GitHub thing is > quite specific. But ultimately, what it does is to generate a proper > summary of where exactly the whitespaces issues are, which is something > that your version doesn't do. It's useful though for consumers of a > failed CI job to know exactly which commit has the issue. > > So can't we pull out the logic into a script, refactor it such that it > knows to print both GitHub- and GitLab-style URLs, and then also print > the summary in GitLab CI? This was my feeling as well when I read the cover letter. I don't think we should either (a) have to remove functionality from the GitHub specific version of this job, or (b) have to maintain two different but highly similar implementations of the same job. It seems more reasonable that we'd do some third option, which is what Patrick suggests. Thanks, Taylor
On 24/04/30 07:04AM, Patrick Steinhardt wrote: > On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote: > > To check for whitespace errors introduced by a set of changes, there is > > the `.github/workflows/check-whitespace.yml` GitHub action. This script > > executes `git log --check` over a range containing the new commits and > > parses the output to generate a markdown formatted artifact that > > summarizes detected errors with GitHub links to the affected commits and > > blobs. > > > > Since this script is rather specific to GitHub actions, a more general > > and simple `ci/check-whitespace.sh` is added instead that functions the > > same, but does not generate the markdown file for the action summary. > > From this, a new GitLab CI job is added to support the whitespace error > > check. > > I still wonder whether we can unify these. Yes, the GitHub thing is > quite specific. But ultimately, what it does is to generate a proper > summary of where exactly the whitespaces issues are, which is something > that your version doesn't do. It's useful though for consumers of a > failed CI job to know exactly which commit has the issue. Just to clarify, this new CI job still prints the output of `git log --check` which details the exact commit and file with line number of the whitespace error. The difference is that it does not write an additional markdown file with links to the commit and blob. Here is a failed execution of the GitLab whitespace check job: https://gitlab.com/gitlab-org/git/-/jobs/6749580210#L1289 > > So can't we pull out the logic into a script, refactor it such that it > knows to print both GitHub- and GitLab-style URLs, and then also print > the summary in GitLab CI? We can do this, but for GitLab CI there probably isn't a point to generating a summary file since there is nothing that would pick it up and display it. Having links though directly in the job output would be nice. I'll give it another go. > > > Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only > > available in GitLab merge request pipelines and therefore the CI job is > > configured to only run as part of those pipelines. > > > > Signed-off-by: Justin Tobler <jltobler@gmail.com> > > --- > > .gitlab-ci.yml | 9 +++++++++ > > ci/check-whitespace.sh | 16 ++++++++++++++++ > > 2 files changed, 25 insertions(+) > > create mode 100755 ci/check-whitespace.sh > > > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > > index c0fa2fe90b..31cf420a11 100644 > > --- a/.gitlab-ci.yml > > +++ b/.gitlab-ci.yml > > @@ -102,3 +102,12 @@ static-analysis: > > script: > > - ./ci/run-static-analysis.sh > > - ./ci/check-directional-formatting.bash > > + > > +check-whitespace: > > + image: ubuntu:latest > > + before_script: > > + - ./ci/install-docker-dependencies.sh > > + script: > > + - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA > > Let's quote this variable. > > > + rules: > > + - if: $CI_PIPELINE_SOURCE == 'merge_request_event' > > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh > > new file mode 100755 > > index 0000000000..1cad2d7374 > > --- /dev/null > > +++ b/ci/check-whitespace.sh > > @@ -0,0 +1,16 @@ > > +#! /bin/sh > > +# > > +# Check that commits after a specified point do not contain new or modified > > +# lines with whitespace errors. > > +# > > + > > +baseSha=${1} > > Two nits: first, I wouldn't call it "sha" because it really is a commit > ID that may or may not be SHA if we were ever to grow a hash function > that is not SHA. So "baseCommit" would be more descriptive. > > Second, our shell variables are typically written in all-uppercase. So > that'd make it "BASE_COMMIT". > > > +git log --check --pretty=format:"---% h% s" ${baseSha}.. > > Nit: there's no need for the curly braces here. Also, let's quote the > value. Thanks for the review Patrick! I'll address these in the next version :) -Justin
On Tue, Apr 30, 2024 at 09:00:59AM -0500, Justin Tobler wrote: > On 24/04/30 07:04AM, Patrick Steinhardt wrote: > > On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote: > > > To check for whitespace errors introduced by a set of changes, there is > > > the `.github/workflows/check-whitespace.yml` GitHub action. This script > > > executes `git log --check` over a range containing the new commits and > > > parses the output to generate a markdown formatted artifact that > > > summarizes detected errors with GitHub links to the affected commits and > > > blobs. > > > > > > Since this script is rather specific to GitHub actions, a more general > > > and simple `ci/check-whitespace.sh` is added instead that functions the > > > same, but does not generate the markdown file for the action summary. > > > From this, a new GitLab CI job is added to support the whitespace error > > > check. > > > > I still wonder whether we can unify these. Yes, the GitHub thing is > > quite specific. But ultimately, what it does is to generate a proper > > summary of where exactly the whitespaces issues are, which is something > > that your version doesn't do. It's useful though for consumers of a > > failed CI job to know exactly which commit has the issue. > > Just to clarify, this new CI job still prints the output of > `git log --check` which details the exact commit and file with line > number of the whitespace error. The difference is that it does not write > an additional markdown file with links to the commit and blob. > > Here is a failed execution of the GitLab whitespace check job: > https://gitlab.com/gitlab-org/git/-/jobs/6749580210#L1289 Okay, fair enough. I'm still of the opinion that the infra here should be shared. > > So can't we pull out the logic into a script, refactor it such that it > > knows to print both GitHub- and GitLab-style URLs, and then also print > > the summary in GitLab CI? > > We can do this, but for GitLab CI there probably isn't a point to > generating a summary file since there is nothing that would pick it up > and display it. Having links though directly in the job output would be > nice. I'll give it another go. Well, we could print the output to the console so that a user can see it when they open the failed job. The nice formatting may be kind of moot, but on the other hand it doesn't hurt, either. I guess most people are used to reading plain markdown-style docs anyway. Patrick
On 24/04/30 04:05PM, Patrick Steinhardt wrote: > On Tue, Apr 30, 2024 at 09:00:59AM -0500, Justin Tobler wrote: > > On 24/04/30 07:04AM, Patrick Steinhardt wrote: > > > On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote: > > > > To check for whitespace errors introduced by a set of changes, there is > > > > the `.github/workflows/check-whitespace.yml` GitHub action. This script > > > > executes `git log --check` over a range containing the new commits and > > > > parses the output to generate a markdown formatted artifact that > > > > summarizes detected errors with GitHub links to the affected commits and > > > > blobs. > > > > > > > > Since this script is rather specific to GitHub actions, a more general > > > > and simple `ci/check-whitespace.sh` is added instead that functions the > > > > same, but does not generate the markdown file for the action summary. > > > > From this, a new GitLab CI job is added to support the whitespace error > > > > check. > > > > > > I still wonder whether we can unify these. Yes, the GitHub thing is > > > quite specific. But ultimately, what it does is to generate a proper > > > summary of where exactly the whitespaces issues are, which is something > > > that your version doesn't do. It's useful though for consumers of a > > > failed CI job to know exactly which commit has the issue. > > > > Just to clarify, this new CI job still prints the output of > > `git log --check` which details the exact commit and file with line > > number of the whitespace error. The difference is that it does not write > > an additional markdown file with links to the commit and blob. > > > > Here is a failed execution of the GitLab whitespace check job: > > https://gitlab.com/gitlab-org/git/-/jobs/6749580210#L1289 > > Okay, fair enough. I'm still of the opinion that the infra here should > be shared. > > > > So can't we pull out the logic into a script, refactor it such that it > > > knows to print both GitHub- and GitLab-style URLs, and then also print > > > the summary in GitLab CI? > > > > We can do this, but for GitLab CI there probably isn't a point to > > generating a summary file since there is nothing that would pick it up > > and display it. Having links though directly in the job output would be > > nice. I'll give it another go. > > Well, we could print the output to the console so that a user can see it > when they open the failed job. The nice formatting may be kind of moot, > but on the other hand it doesn't hurt, either. I guess most people are > used to reading plain markdown-style docs anyway. I'm thinking we can generalize the summary writing in some manner. When run as a GitHub action, the summary can be markdown formatted and written to `$GITHUB_STEP_SUMMARY` with no output to the console as done today. When run as GitLab CI, the summary can be written directly to console with links. All other runs just output normally to console. -Justin
On Tue, Apr 30, 2024 at 09:41:47AM -0500, Justin Tobler wrote: > On 24/04/30 04:05PM, Patrick Steinhardt wrote: > > On Tue, Apr 30, 2024 at 09:00:59AM -0500, Justin Tobler wrote: > > > On 24/04/30 07:04AM, Patrick Steinhardt wrote: > > > > On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote: > > > > > To check for whitespace errors introduced by a set of changes, there is > > > > > the `.github/workflows/check-whitespace.yml` GitHub action. This script > > > > > executes `git log --check` over a range containing the new commits and > > > > > parses the output to generate a markdown formatted artifact that > > > > > summarizes detected errors with GitHub links to the affected commits and > > > > > blobs. > > > > > > > > > > Since this script is rather specific to GitHub actions, a more general > > > > > and simple `ci/check-whitespace.sh` is added instead that functions the > > > > > same, but does not generate the markdown file for the action summary. > > > > > From this, a new GitLab CI job is added to support the whitespace error > > > > > check. > > > > > > > > I still wonder whether we can unify these. Yes, the GitHub thing is > > > > quite specific. But ultimately, what it does is to generate a proper > > > > summary of where exactly the whitespaces issues are, which is something > > > > that your version doesn't do. It's useful though for consumers of a > > > > failed CI job to know exactly which commit has the issue. > > > > > > Just to clarify, this new CI job still prints the output of > > > `git log --check` which details the exact commit and file with line > > > number of the whitespace error. The difference is that it does not write > > > an additional markdown file with links to the commit and blob. > > > > > > Here is a failed execution of the GitLab whitespace check job: > > > https://gitlab.com/gitlab-org/git/-/jobs/6749580210#L1289 > > > > Okay, fair enough. I'm still of the opinion that the infra here should > > be shared. > > > > > > So can't we pull out the logic into a script, refactor it such that it > > > > knows to print both GitHub- and GitLab-style URLs, and then also print > > > > the summary in GitLab CI? > > > > > > We can do this, but for GitLab CI there probably isn't a point to > > > generating a summary file since there is nothing that would pick it up > > > and display it. Having links though directly in the job output would be > > > nice. I'll give it another go. > > > > Well, we could print the output to the console so that a user can see it > > when they open the failed job. The nice formatting may be kind of moot, > > but on the other hand it doesn't hurt, either. I guess most people are > > used to reading plain markdown-style docs anyway. > > I'm thinking we can generalize the summary writing in some manner. When > run as a GitHub action, the summary can be markdown formatted and > written to `$GITHUB_STEP_SUMMARY` with no output to the console as done > today. When run as GitLab CI, the summary can be written directly to > console with links. All other runs just output normally to console. The script can probably be generalized to take a file name as argument. For GitHub we'd then pass `$GITHUB_STEP_SUMMARY`, whereas for GitLab we pass in a temporary filename that we than simply cat(1) to the console. That'd allow us to move the CI-specific bits into the respective CIs whereas the script itself remains generic. Patrick
On 24/04/30 04:45PM, Patrick Steinhardt wrote: > On Tue, Apr 30, 2024 at 09:41:47AM -0500, Justin Tobler wrote: > > On 24/04/30 04:05PM, Patrick Steinhardt wrote: > > > On Tue, Apr 30, 2024 at 09:00:59AM -0500, Justin Tobler wrote: > > > > On 24/04/30 07:04AM, Patrick Steinhardt wrote: > > > > > On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote: > > > > > > To check for whitespace errors introduced by a set of changes, there is > > > > > > the `.github/workflows/check-whitespace.yml` GitHub action. This script > > > > > > executes `git log --check` over a range containing the new commits and > > > > > > parses the output to generate a markdown formatted artifact that > > > > > > summarizes detected errors with GitHub links to the affected commits and > > > > > > blobs. > > > > > > > > > > > > Since this script is rather specific to GitHub actions, a more general > > > > > > and simple `ci/check-whitespace.sh` is added instead that functions the > > > > > > same, but does not generate the markdown file for the action summary. > > > > > > From this, a new GitLab CI job is added to support the whitespace error > > > > > > check. > > > > > > > > > > I still wonder whether we can unify these. Yes, the GitHub thing is > > > > > quite specific. But ultimately, what it does is to generate a proper > > > > > summary of where exactly the whitespaces issues are, which is something > > > > > that your version doesn't do. It's useful though for consumers of a > > > > > failed CI job to know exactly which commit has the issue. > > > > > > > > Just to clarify, this new CI job still prints the output of > > > > `git log --check` which details the exact commit and file with line > > > > number of the whitespace error. The difference is that it does not write > > > > an additional markdown file with links to the commit and blob. > > > > > > > > Here is a failed execution of the GitLab whitespace check job: > > > > https://gitlab.com/gitlab-org/git/-/jobs/6749580210#L1289 > > > > > > Okay, fair enough. I'm still of the opinion that the infra here should > > > be shared. > > > > > > > > So can't we pull out the logic into a script, refactor it such that it > > > > > knows to print both GitHub- and GitLab-style URLs, and then also print > > > > > the summary in GitLab CI? > > > > > > > > We can do this, but for GitLab CI there probably isn't a point to > > > > generating a summary file since there is nothing that would pick it up > > > > and display it. Having links though directly in the job output would be > > > > nice. I'll give it another go. > > > > > > Well, we could print the output to the console so that a user can see it > > > when they open the failed job. The nice formatting may be kind of moot, > > > but on the other hand it doesn't hurt, either. I guess most people are > > > used to reading plain markdown-style docs anyway. > > > > I'm thinking we can generalize the summary writing in some manner. When > > run as a GitHub action, the summary can be markdown formatted and > > written to `$GITHUB_STEP_SUMMARY` with no output to the console as done > > today. When run as GitLab CI, the summary can be written directly to > > console with links. All other runs just output normally to console. > > The script can probably be generalized to take a file name as argument. > For GitHub we'd then pass `$GITHUB_STEP_SUMMARY`, whereas for GitLab we > pass in a temporary filename that we than simply cat(1) to the console. > That'd allow us to move the CI-specific bits into the respective CIs > whereas the script itself remains generic. This sounds good, I was also considering this. We will still need CI-specific bits in the script in order to generate correctly formatted links. That is, unless we also want to pass these as arguments, but that might start to get a little messy. -Justin
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index c0fa2fe90b..31cf420a11 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -102,3 +102,12 @@ static-analysis: script: - ./ci/run-static-analysis.sh - ./ci/check-directional-formatting.bash + +check-whitespace: + image: ubuntu:latest + before_script: + - ./ci/install-docker-dependencies.sh + script: + - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA + rules: + - if: $CI_PIPELINE_SOURCE == 'merge_request_event' diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh new file mode 100755 index 0000000000..1cad2d7374 --- /dev/null +++ b/ci/check-whitespace.sh @@ -0,0 +1,16 @@ +#! /bin/sh +# +# Check that commits after a specified point do not contain new or modified +# lines with whitespace errors. +# + +baseSha=${1} + +git log --check --pretty=format:"---% h% s" ${baseSha}.. +if test $? -ne 0 +then + echo "A whitespace issue was found in one or more of the commits." + echo "Run the following command to resolve whitespace issues:" + echo "\tgit rebase --whitespace=fix ${baseSha}" + exit 2 +fi
To check for whitespace errors introduced by a set of changes, there is the `.github/workflows/check-whitespace.yml` GitHub action. This script executes `git log --check` over a range containing the new commits and parses the output to generate a markdown formatted artifact that summarizes detected errors with GitHub links to the affected commits and blobs. Since this script is rather specific to GitHub actions, a more general and simple `ci/check-whitespace.sh` is added instead that functions the same, but does not generate the markdown file for the action summary. From this, a new GitLab CI job is added to support the whitespace error check. Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only available in GitLab merge request pipelines and therefore the CI job is configured to only run as part of those pipelines. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- .gitlab-ci.yml | 9 +++++++++ ci/check-whitespace.sh | 16 ++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100755 ci/check-whitespace.sh