Re: [Qemu-devel] [PATCH v7 2/6] docker: don't rely on submodules existing in the main checkout

[Daniel P. Berrange]

On Thu, Sep 28, 2017 at 01:03:41PM -0500, Eric Blake wrote:
> On 09/28/2017 07:06 AM, Daniel P. Berrange wrote:
> > When building the tarball to pass into the docker/vm test image,
> > the code relies on the git submodules being checked out in the
> > main checkout.
> > 
> > ie if the developer has not run 'git submodule update --init dtc'
> > many of the docker tests will fail due to the libfdt package not
> 
> 'dtc' vs. 'libfdt' - is one of the acronyms wrong?

For reasons I don't understand we called our submodule 'dtc'
but it builds a libfdt library. So at least my wording here
reflects that mismatch.

> 
> > being present in the test images. Patchew manually checks out the
> > dtc submodule in the main git checkout, but this is a bad idea.
> > 
> > When running tests we want to have a predictable set of submodules
> > included in the source that's tested. The build environment is
> > completely independant of the developers host OS, so the submodules
> 
> s/independant/independent/
> 
> > the developer has checked out should not be considered relevant for
> > the tests.
> > 
> > This changes the archive-source.sh script so that it clones the
> > current git checkout into a temporary directory, checks out a
> > fixed set of submodules, builds the tarball and finally removes
> > the temporary git clone.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  scripts/archive-source.sh | 30 +++++++++++++++++++++++++++---
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> > 
> 
> > +++ b/scripts/archive-source.sh
> 
> > +# We want a predictable list of submodules for builds, that is
> > +# independant of what the developer currently has initialized
> 
> and again
> 
> > +# in their checkout, because the build environment is completely
> > +# different to the host OS.
> > +submodules="dtc"
> >  
> >  if test $? -ne 0; then
> >      error "git submodule command failed"
> > @@ -28,6 +34,21 @@ fi
> >  
> >  trap "status=$?; rm -f \"$list_file\"; exit \$status" 0 1 2 3 15
> >  
> > +if git diff-index --quiet HEAD -- &>/dev/null
> > +then
> > +    HEAD=HEAD
> > +else
> > +    HEAD=`git stash create`
> > +fi
> > +git clone --shared . "$vroot_dir"
> 
> set -e is not in effect; do you need error checking here?

Yes we shouldl

> 
> > +here=`pwd`
> 
> $PWD is cheaper than spawning a subprocess for `pwd`.

I always forget about $PWD :-)

> > +cd "$vroot_dir"
> 
> Definitely need error checking here.

Yep

> 
> > +git checkout $HEAD
> > +
> > +for sm in $submodules; do
> > +    git submodule update --init $sm

And here too

> > +done
> > +
> >  if test -n "$submodules"; then
> >      {
> >          git ls-files || error "git ls-files failed"
> > @@ -48,4 +69,7 @@ fi
> >  
> >  tar -cf "$tar_file" -T "$list_file" || error "failed to create tar file"
> >  
> > +cd "$here"
> 
> and again here
> 
> > +rm -rf "$vroot_dir"

I can move this into the earlier 'trap' cleanup and remove
this need to cd && rm.


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 :|