From: phillip.wood123@gmail.com
Date: Wed, 23 Apr 2025 10:05:02 GMT
Subject: Re: [PATCH v5 1/6] ci/github: install git before checking out the repository
Message-ID: <3607a937-9795-4348-981e-dce92203c6d7@gmail.com>
In-Reply-To: <20250423-505-wire-up-sparse-via-meson-v5-1-d1e2be4b2078@gmail.com>

Hi Karthik

This looks good, I've left a few comments about the wording of the 
commit message but I wouldn't worry too much unless you end up 
re-rolling for some other reason.

On 23/04/2025 09:15, Karthik Nayak wrote:
> The GitHub's CI workflow uses 'actions/checkout@v4' to checkout the

We don't need "The" here

> repository. This action defaults to using the GitHub REST API to obtain

I'd maybe say "falls back" rather than "defaults"

> the repository if the `git` executable isn't available.
> 
> The step to build Git in the GitHub workflow can be summarized as:
> 
>    ...
>    - uses: actions/checkout@v4 #1
>    - run: ci/install-dependencies.sh #2
>    ...
>    - run: sudo --preserve-env --set-home --user=builder ci/run-build-and-tests.sh #3
>    ...
> 
> Step #1, clones the repository, since the `git` executable isn't present

It would be more accurate to say that it tries to clone the repository - 
if we fall back to extracting a tarball then we're not cloning.

> at this step, it uses GitHub's REST API to obtain a tar of the
> repository.
> 
> Step #2, installs all dependencies, which includes the `git` executable.
> 
> Step #3, sets up the build, which includes setting up meson in the meson
> job. At this point the `git` executable is present.
> 
> This means while the `git` executable is present, the repository doesn't
> contain the '.git' folder.

I'd maybe say "source tree" instead of "repository" as it isn't a 
repository without a ".git" directory.

> To keep both the CI's (GitLab and GitHub)
> behavior consistent and to ensure that the build is performed on a
> real-world scenario, install `git` before the repository is checked out.
> This ensures that 'actions/checkout@v4' will clone the repository
> instead of using a tarball. We also update the package cache while
> installing `git`, this is because some distros will fail to locate the
> package without updating the cache.

Nice explanation, the code changes look good

Thanks

Phillip

> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>   .github/workflows/main.yml | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 37541f3d10..e9112b3a64 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -414,6 +414,20 @@ jobs:
>       - name: prepare libc6 for actions
>         if: matrix.vector.jobname == 'linux32'
>         run: apt -q update && apt -q -y install libc6-amd64 lib64stdc++6
> +    - name: install git in container
> +      run: |
> +        if command -v git
> +        then
> +          : # nothing to do
> +        elif command -v apk
> +        then
> +          apk add --update git
> +        elif command -v dnf
> +        then
> +          dnf -yq update && dnf -yq install git
> +        else
> +          apt-get -q update && apt-get -q -y install git
> +        fi
>       - uses: actions/checkout@v4
>       - run: ci/install-dependencies.sh
>       - run: useradd builder --create-home
>