Re: [PATCH] bootstrap: When a commit hash is specified, do a shallow fetch if possible

[Daniel Kiper]

On Fri, Oct 22, 2021 at 04:26:41PM -0500, Glenn Washburn wrote:
> On Fri, 22 Oct 2021 18:44:15 +0200
> Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > On Thu, Oct 21, 2021 at 12:49:19PM -0500, Glenn Washburn wrote:
> > > The gnulib sources are large but more importantly have lots of changes. So
> > > initial checkout of the repository can take a long time when network or
> > > cpu resources are limited. The later is especially acute in a non-KVM QEMU
> > > virtual machine (which can take 40+ minutes compared to <30 seconds with
> > > this change[1]). The problem is specific to how GRUB uses gnulib, which is
> > > by pegging the desired checkout to a specific git revision. In this case,
> > > git can not do a shallow clone by using the --depth option because git 
> > > does
> > > not know ahead of time how deep the revision is from the tip. So git must
> > > clone the whole repository.
> > >
> > > However, there is an alternate method that requires support from the git
> > > server[2], namely by asking for a specific commit on fetch. Refactor to 
> > > use
> > > fetch and fallback to fetching the entire repository if fetching by commit
> > > hash fails.
> > >
> > > Currently the git server hosting the official gnulib git repository does 
> > > not
> > > support fetch by commit hash[3]. However, there are mirrors which do 
> > > support
> > > this[4], and can be specified by setting the $GNULIB_URL.
>
> This paragraph is now out of date, the git server configuration change
> happened much more quickly than I expected. So before committing it
> should be removed along with the last two references.
>
> > >
> > > [1] https://savannah.nongnu.org/support/index.php?110553#comment1
> > > [2] https://stackoverflow.com/a/3489576/2108011
> > > [3] https://savannah.nongnu.org/support/index.php?110553
> > > [4] https://github.com/coreutils/gnulib
> > >
> > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > ---
> > >  bootstrap | 16 ++++++++++++++--
> > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/bootstrap b/bootstrap
> > > index 5b08e7e2d..914f911f8 100755
> > > --- a/bootstrap
> > > +++ b/bootstrap
> > > @@ -665,9 +665,21 @@ if $use_gnulib; then
> > >        shallow=
> > >        if test -z "$GNULIB_REVISION"; then
> > >          git clone -h 2>&1 | grep -- --depth > /dev/null && 
> > > shallow='--depth 2'
> > > +        git clone $shallow ${GNULIB_URL:-$default_gnulib_url} 
> > > "$gnulib_path" \
> > > +          || cleanup_gnulib
> > > +      else
> > > +        git fetch -h 2>&1 | grep -- --depth > /dev/null && 
> > > shallow='--depth 2'
> > > +        mkdir -p "$gnulib_path"
> > > +        git -C "$gnulib_path" init
> > > +        git -C "$gnulib_path" remote add origin 
> > > ${GNULIB_URL:-$default_gnulib_url}
> > > +        # Can not do a shallow fetch if fetch by commit hash fails 
> > > because we
> > > +        # do not know how the to go to get to $GNULIB_REVISION, so we 
> > > must get
> > > +        # all commits.
> > > +        git -C "$gnulib_path" fetch $shallow origin "$GNULIB_REVISION" \
> > > +          || git -C "$gnulib_path" fetch origin \
> > > +          || cleanup_gnulib
> > > +        git -C "$gnulib_path" reset --hard FETCH_HEAD
> >
> > Is this hunk from gnulib upstream? If not I would prefer if you upstream
> > it into gnulib. We should avoid custom patches for imported code as much
> > as possible.
>
> I agree in general. As mentioned in a follow up reply to this patch, I
> did submit the patch upstream[1]. The only difference is the commit

Oh, sorry, I missed this somehow.

> message. It has not been responded to yet. However, even once accepted
> upstream we'll have to cherry-pick it to the GRUB repo. Are you
> thinking we'd need to get all changes to bootstrap at that time? It
> doesn't look like there's anything that would be that useful (except
> maybe using the https repo url). I'm curious how you'd like to handle
> this.

I want to bump the gnulib version to the latest one. Though first of all
we have to upstream a few gnulib issues discovered by the Coverity.
Darren, CC-ed, is working on it.

> I don't think the issue of a custom patch in this instance is a problem
> though. Bootstrap very rarely changes, even in upstream (<10 changes in
> the last 2 years). So there I'm not concerned about having to maintain
> a lot of custom patches on top of it (like the situation of many
> distros vis-a-vis GRUB). So I think if its not accepted upstream (I
> don't see why it wouldn't be), I think we should use in GRUB because it
> provides significant reduction in bandwith and a drastic reduction in
> time to checkout on a qemu VM (but also significant not on a VM, 5min
> vs 1min) depending on the CPU resources available. This is especially
> useful for a CI/testing system that is frequently run.

If gnulib upstream folks reject it then I can consider taking this patch
into the GRUB.

> I'm fine with waiting a few weeks and see what happens upstream.

Sounds like a plan.

Daniel

> Glenn
>
> [1] https://lists.gnu.org/archive/html/bug-gnulib/2021-10/msg00052.html