Re: [Qemu-devel] [PATCH v7 1/6] build: automatically handle GIT submodule checkout for dtc

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Sep 28, 2017 at 12:58:32PM -0500, Eric Blake wrote:
> On 09/28/2017 07:06 AM, Daniel P. Berrange wrote:
> > Currently if DTC is required by configure and not available in the host
> > OS install, we exit with an error message telling the user to checkout a
> > git submodule or install the library.
> > 
> > This introduces automatic handling of the git submodule checkout process
> > and enables it for dtc. This only runs if building from GIT, so users of
> > release tarballs still need the system library install. The current state
> > of the git checkout is stashed in .git-submodule-status, and a helper
> > program is used to determine if this state matches the desired submodule
> > state. A dependency against 'Makefile' ensures that the submodule state
> > is refreshed at the start of the build process
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  .gitignore               |  1 +
> >  Makefile                 | 23 ++++++++++++++++++++++-
> >  configure                | 46 ++++++++++++++++++++++++++--------------------
> >  scripts/git-submodule.sh | 31 +++++++++++++++++++++++++++++++
> >  4 files changed, 80 insertions(+), 21 deletions(-)
> >  create mode 100755 scripts/git-submodule.sh
> > 
> 
> > +++ b/Makefile
> > @@ -14,6 +14,27 @@ ifneq ($(wildcard config-host.mak),)
> >  all:
> >  include config-host.mak
> >  
> > +git-submodule-update:
> > +
> > +.PHONY: git-submodule-update
> > +
> > +ifeq (0,$(MAKELEVEL))
> > +  git_module_status := $(shell \
> > +    cd '$(SRC_PATH)'; \
> 
> Should this be && instead of ;, since I don't know if $(shell) implies
> 'set -e' semantics?

I don't know either, so lets just use &&


> > +++ b/configure
> > @@ -264,6 +264,7 @@ cc_i386=i386-pc-linux-gnu-gcc
> >  libs_qga=""
> >  debug_info="yes"
> >  stack_protector=""
> > +git_submodules=""
> >  
> >  # Don't accept a target_list environment variable.
> >  unset target_list
> > @@ -3580,27 +3581,30 @@ EOF
> >    if compile_prog "" "$fdt_libs" ; then
> >      # system DTC is good - use it
> >      fdt=yes
> > -  elif test -d ${source_path}/dtc/libfdt ; then
> ...
> > -       symlink "$source_path/dtc/Makefile" "dtc/Makefile"
> 
> Old code is inconsistent on whether it quotes the expansion of
> $source_path.  Given the unquoted usage, we don't support a $source_path
> that contains spaces; however...
> 
> > +      # have GIT checkout, so activate dtc submodule
> > +      if test -d ${source_path}/.git ; then
> 
> ...we might as well consistently use quotes in all new code, especially,
> since it is already a red flag any time someone does unquoted test -d $foo.

Ok, I'll quote everything

> 
> > +	  git_submodules="${git_submodules} dtc"
> > +      fi
> > +      if test -d ${source_path}/dtc/libfdt || test -d ${source_path}/.git ; then
> 
> and again
> 
> .git is not always a directory; it can be a symlink or text file
> containing a directory name.  It is probably sufficient to just use test
> -e instead of test -d.

Yep, I'll use -e

> > +	  # have neither and want - prompt for system/submodule install
> > +	  error_exit "DTC (libfdt) version >= 1.4.2 not present." \
> > +		     "Please install the DTC (libfdt) devel package"
> 
> Is the comment accurate, given that the error message doesn't mention a
> submodule?

I'll fix the comment

> > +++ b/scripts/git-submodule.sh
> > @@ -0,0 +1,31 @@
> > +#!/bin/bash
> > +
> > +set -e
> 
> 'set -e' is notoriously awkward to work with, especially if your shell
> script includes functions.  Do we really need it, or can we do proper
> error checking in place?

IMHO using -e leads to simpler code and we've no functions that would
trip it up, so its OK.

> > +
> > +command=$1
> > +shift
> > +modules="$@"
> > +
> > +test -z "$modules" && exit 0
> > +
> > +if ! test -d ".git"
> 
> Again, .git doesn't necessarily have to be a directory; test -e may be
> better.

Yep

> > +then
> > +    echo "$0: unexpectedly called with submodules but no git checkout exists"
> > +    exit 1
> > +fi
> > +
> > +substat=".git-submodule-status"
> > +
> > +case "$command" in
> > +status)
> > +    test -f "$substat" || exit 1
> > +    git submodule status $modules > "${substat}.tmp"
> 
> This is one place where if 'set -e' is not in effect, you need to decide
> if failure to run the command should cause early exit.
> 
> > +    trap "rm -f ${substat}.tmp" EXIT
> 
> Don't you want the trap installed one line earlier, before you create
> the file?

Opps, yes

> > +    diff "${substat}" "${substat}.tmp" >/dev/null
> > +    exit $?
> > +    ;;
> > +update)
> > +    git submodule update --init $modules 1>/dev/null 2>&1
> > +    git submodule status $modules > "${substat}"
> > +    ;;
> > +esac


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