phillip.wood123@xxxxxxxxx writes: > 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 > Yeah, I think we can remove it. >> repository. This action defaults to using the GitHub REST API to obtain > > I'd maybe say "falls back" rather than "defaults" > Right! >> 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. > Yes indeed. >> 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. > Good point. >> 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 for the review. I'll add it locally to my tree. That way if I end up with a new version, It'll incorporate these changes. > Thanks > > Phillip > >> Helped-by: Phillip Wood <phillip.wood123@xxxxxxxxx> >> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx> >> --- >> .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 >>
Attachment:
signature.asc
Description: PGP signature
