| Message ID | 20240503172110.181326-6-jltobler@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8f19e82c5b313a1518119cc4b31c32ec3001b967 |
| Headers | show |
| Series | Add GitLab CI to check for whitespace errors | expand |
On Fri, May 03, 2024 at 12:21:07PM -0500, Justin Tobler wrote: > GitLab CI does not have a job to check for whitespace errors introduced > by a set of changes. Reuse the existing generic `whitespace-check.sh` to > create the job for GitLab pipelines. > > 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 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index c0fa2fe90b..619bf729fa 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-dependencies.sh Do we actually need to install dependencies? I imagine all that's needed would be Git. Other than this question the patch series looks good to me, thanks! Patrick > + script: > + - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" > + rules: > + - if: $CI_PIPELINE_SOURCE == 'merge_request_event' > -- > 2.45.0 > >
On 24/05/06 08:55AM, Patrick Steinhardt wrote: > On Fri, May 03, 2024 at 12:21:07PM -0500, Justin Tobler wrote: > > GitLab CI does not have a job to check for whitespace errors introduced > > by a set of changes. Reuse the existing generic `whitespace-check.sh` to > > create the job for GitLab pipelines. > > > > 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 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > > index c0fa2fe90b..619bf729fa 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-dependencies.sh > > Do we actually need to install dependencies? I imagine all that's needed > would be Git. You are correct. Git is really the only dependency we need. If we want to remove the need for the install dependencies script, we could fetch Git ourselves during the pre-script. Another option could be to use a different container image that has Git baked in. -Justin
Patrick Steinhardt <ps@pks.im> writes: >> +check-whitespace: >> + image: ubuntu:latest >> + before_script: >> + - ./ci/install-dependencies.sh > > Do we actually need to install dependencies? I imagine all that's needed > would be Git. > > Other than this question the patch series looks good to me, thanks! I am a bit puzzled. Is the proposal to check our sources with a pre-built Git (which by definition would be a bit older than what is being tested)? Not that I have serious trouble in that direction---I am just trying to make sure what is being proposed. Thanks.
On 24/05/06 10:17AM, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > >> +check-whitespace: > >> + image: ubuntu:latest > >> + before_script: > >> + - ./ci/install-dependencies.sh > > > > Do we actually need to install dependencies? I imagine all that's needed > > would be Git. > > > > Other than this question the patch series looks good to me, thanks! > > I am a bit puzzled. Is the proposal to check our sources with a > pre-built Git (which by definition would be a bit older than what is > being tested)? The GitLab `check-whitespace` CI job only needs Git to run and uses `ci/install-dependencies.sh` to download a pre-built Git package via `apt-get` since `ubuntu:latest` is the container image used. The `ci/install-dependencies.sh` script also fetches a bunch of other dependencies which are not needed. I think Patrick is proposing, to further simplify, we avoid using `ci/install-dependencies.sh` and only fetch Git. Patrick please correct me if I misunderstand :) -Justin
On Mon, May 06, 2024 at 02:21:52PM -0500, Justin Tobler wrote: > On 24/05/06 10:17AM, Junio C Hamano wrote: > > Patrick Steinhardt <ps@pks.im> writes: > > > > >> +check-whitespace: > > >> + image: ubuntu:latest > > >> + before_script: > > >> + - ./ci/install-dependencies.sh > > > > > > Do we actually need to install dependencies? I imagine all that's needed > > > would be Git. > > > > > > Other than this question the patch series looks good to me, thanks! > > > > I am a bit puzzled. Is the proposal to check our sources with a > > pre-built Git (which by definition would be a bit older than what is > > being tested)? > > The GitLab `check-whitespace` CI job only needs Git to run and uses > `ci/install-dependencies.sh` to download a pre-built Git package via > `apt-get` since `ubuntu:latest` is the container image used. The > `ci/install-dependencies.sh` script also fetches a bunch of other > dependencies which are not needed. > > I think Patrick is proposing, to further simplify, we avoid using > `ci/install-dependencies.sh` and only fetch Git. Patrick please correct > me if I misunderstand :) I just wondered how GitHub Workflows manages without installing any dependencies at all. Is Git already part of the default images? If so, there is no need to install anything and we can just execute the script directly, which saves some time. If there is a need to install Git we could either just manually install it in the `before_script` or leave it as-is. I don't mind it much either way. Patrick
On 24/05/07 06:12AM, Patrick Steinhardt wrote: > I just wondered how GitHub Workflows manages without installing any > dependencies at all. Is Git already part of the default images? If so, > there is no need to install anything and we can just execute the script > directly, which saves some time. Git is not bundled by default in the "ubuntu:latest" container image. We would have to install it ourselves. As for why GitHub Workflows do not have to install Git, it looks like each runner has a defined set of included software which happens to include Git. https://github.com/actions/runner-images/blob/ubuntu22/20240422.1/images/ubuntu/Ubuntu2204-Readme.md > If there is a need to install Git we could either just manually install > it in the `before_script` or leave it as-is. I don't mind it much either > way. I don't have strong opinions, but I think I would prefer to leave it as-is and reuse `ci/install-dependencies.sh`. I'll forgo sending another version unless there is addional feedback. Thanks -Justin
On Tue, May 07, 2024 at 01:06:04PM -0500, Justin Tobler wrote: > On 24/05/07 06:12AM, Patrick Steinhardt wrote: > > > I just wondered how GitHub Workflows manages without installing any > > dependencies at all. Is Git already part of the default images? If so, > > there is no need to install anything and we can just execute the script > > directly, which saves some time. > > Git is not bundled by default in the "ubuntu:latest" container image. We > would have to install it ourselves. As for why GitHub Workflows do not > have to install Git, it looks like each runner has a defined set of > included software which happens to include Git. Okay. > https://github.com/actions/runner-images/blob/ubuntu22/20240422.1/images/ubuntu/Ubuntu2204-Readme.md > > > If there is a need to install Git we could either just manually install > > it in the `before_script` or leave it as-is. I don't mind it much either > > way. > > I don't have strong opinions, but I think I would prefer to leave it > as-is and reuse `ci/install-dependencies.sh`. I'll forgo sending another > version unless there is addional feedback. Thanks Fair enough. Let's not overthink this then and get this merged. Patrick
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index c0fa2fe90b..619bf729fa 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-dependencies.sh + script: + - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" + rules: + - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
GitLab CI does not have a job to check for whitespace errors introduced by a set of changes. Reuse the existing generic `whitespace-check.sh` to create the job for GitLab pipelines. 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 +++++++++ 1 file changed, 9 insertions(+)