Re: [Qemu-devel] [PATCH v1 04/14] archive-source.sh: Clone the submodules locally

[Daniel P . Berrangé]

On Fri, Jan 25, 2019 at 02:00:07PM +0000, Alex Bennée wrote:
> From: Philippe Mathieu-Daudé <address@hidden>
> 
> We cloned the QEMU repository from the local storage. Since the
> submodules are also available there, clone them too. This is
> quicker and reduce network use.

FYI there's another attempt at solving it here:

  https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg07710.html

though that has some problems, so I prefer what you've done
here.  There's still scope for replacing the git ls-tree + tar
command with a git archive command, but that's tangential to
the problem of reducing network usage.

> 
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> Signed-off-by: Alex Bennée <address@hidden>
> ---
>  scripts/archive-source.sh | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh
> index 6eed2a29bd..ce31be67b0 100755
> --- a/scripts/archive-source.sh
> +++ b/scripts/archive-source.sh
> @@ -38,6 +38,10 @@ else
>  fi
>  git clone --shared . "$vroot_dir"
>  test $? -ne 0 && error "failed to clone into '$vroot_dir'"
> +for sm in $submodules; do
> +    git clone --shared "$sm" "$vroot_dir/$sm"
> +    test $? -ne 0 && error "failed to clone submodule $sm"
> +done

I don't think this is reliable. Notice the message further up
where we define $submodules

  # We want a predictable list of submodules for builds, that is
  # independent of what the developer currently has initialized
  # in their checkout, because the build environment is completely
  # different to the host OS.


IOW, if the developers host build does not require 'dtc', then
the 'dtc' directory will be an empty directory, not a git
submodule checkout. At which point "git clone" has nothing
available.

Second, even if 'dtc' is checked out, we can't assume that
it is checked out at the right commit hash - 'git clone' will
match whatever is currently checked out.

So this needs to be wrapped

   if test -d "$sm/.git"
   then
       git clone --shared "$sm" "$vroot_dir/$sm"
       test $? -ne 0 && error "failed to clone submodule $sm"
   fi


>  
>  cd "$vroot_dir"
>  test $? -ne 0 && error "failed to change into '$vroot_dir'"
> @@ -45,11 +49,6 @@ test $? -ne 0 && error "failed to change into '$vroot_dir'"
>  git checkout $HEAD
>  test $? -ne 0 && error "failed to checkout $HEAD revision"
>  
> -for sm in $submodules; do
> -    git submodule update --init $sm
> -    test $? -ne 0 && error "failed to init submodule $sm"
> -done

This should not be deleted. It ensures that the checkout we
just cloned gets moved to the correct HEAD. It might still
use the network, but that is genuinely neccessary in some
cases. At least the network usage will be minimized if the
developers host already had an updated submodule set.

> -
>  if test -n "$submodules"; then
>      {
>          git ls-files || error "git ls-files failed"


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|