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

diff --git a/.gitignore b/.gitignore
index 40acfcb9e2..06bf972fc3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -112,6 +112,7 @@
 /docs/version.texi
 *.tps
 .stgit-*
+.git-submodule-status
 cscope.*
 tags
 TAGS
diff --git a/Makefile b/Makefile
index 2be61fcf1c..e9d9e1840f 100644
--- a/Makefile
+++ 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)'; \
+    ./scripts/git-submodule.sh status $(GIT_SUBMODULES); \
+    echo $$?; \
+  )
+
+ifeq (1,$(git_module_status))
+git-submodule-update:
+	$(call quiet-command, \
+          (cd $(SRC_PATH); ./scripts/git-submodule.sh update $(GIT_SUBMODULES)), \
+          "GIT","$(GIT_SUBMODULES)")
+endif
+endif
+
+.git-submodule-status: git-submodule-update
+
 # Check that we're not trying to do an out-of-tree build from
 # a tree that's been used for an in-tree build.
 ifneq ($(realpath $(SRC_PATH)),$(realpath .))
@@ -329,7 +350,7 @@ DTC_MAKE_ARGS=-I$(SRC_PATH)/dtc VPATH=$(SRC_PATH)/dtc -C dtc V="$(V)" LIBFDT_src
 DTC_CFLAGS=$(CFLAGS) $(QEMU_CFLAGS)
 DTC_CPPFLAGS=-I$(BUILD_DIR)/dtc -I$(SRC_PATH)/dtc -I$(SRC_PATH)/dtc/libfdt
 
-subdir-dtc:dtc/libfdt dtc/tests
+subdir-dtc: .git-submodule-status dtc/libfdt dtc/tests
 	$(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" LDFLAGS="$(LDFLAGS)" ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) libfdt/libfdt.a,)
 
 dtc/%:
diff --git a/configure b/configure
index 7727f6ba5b..ed4048f8b2 100755
--- a/configure
+++ 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
-    # have submodule DTC - use it
-    fdt=yes
-    dtc_internal="yes"
-    mkdir -p dtc
-    if [ "$pwd_is_source_path" != "y" ] ; then
-       symlink "$source_path/dtc/Makefile" "dtc/Makefile"
-       symlink "$source_path/dtc/scripts" "dtc/scripts"
-    fi
-    fdt_cflags="-I\$(SRC_PATH)/dtc/libfdt"
-    fdt_libs="-L\$(BUILD_DIR)/dtc/libfdt $fdt_libs"
-  elif test "$fdt" = "yes" ; then
-    # have neither and want - prompt for system/submodule install
-    error_exit "DTC (libfdt) version >= 1.4.2 not present. Your options:" \
-        "  (1) Preferred: Install the DTC (libfdt) devel package" \
-        "  (2) Fetch the DTC submodule, using:" \
-        "      git submodule update --init dtc"
   else
-    # don't have and don't want
-    fdt_libs=
-    fdt=no
+      # have GIT checkout, so activate dtc submodule
+      if test -d ${source_path}/.git ; then
+	  git_submodules="${git_submodules} dtc"
+      fi
+      if test -d ${source_path}/dtc/libfdt || test -d ${source_path}/.git ; then
+	  fdt=yes
+	  dtc_internal="yes"
+	  mkdir -p dtc
+	  if [ "$pwd_is_source_path" != "y" ] ; then
+	      symlink "$source_path/dtc/Makefile" "dtc/Makefile"
+	      symlink "$source_path/dtc/scripts" "dtc/scripts"
+	  fi
+	  fdt_cflags="-I\$(SRC_PATH)/dtc/libfdt"
+	  fdt_libs="-L\$(BUILD_DIR)/dtc/libfdt $fdt_libs"
+      elif test "$fdt" = "yes" ; then
+	  # 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"
+      else
+	  # don't have and don't want
+	  fdt_libs=
+	  fdt=no
+      fi
   fi
 fi
 
@@ -5290,6 +5294,7 @@ echo "local state directory   queried at runtime"
 echo "Windows SDK       $win_sdk"
 fi
 echo "Source path       $source_path"
+echo "GIT submodules    $git_submodules"
 echo "C compiler        $cc"
 echo "Host C compiler   $host_cc"
 echo "C++ compiler      $cxx"
@@ -5477,6 +5482,7 @@ echo "extra_cxxflags=$EXTRA_CXXFLAGS" >> $config_host_mak
 echo "extra_ldflags=$EXTRA_LDFLAGS" >> $config_host_mak
 echo "qemu_localedir=$qemu_localedir" >> $config_host_mak
 echo "libs_softmmu=$libs_softmmu" >> $config_host_mak
+echo "GIT_SUBMODULES=$git_submodules" >> $config_host_mak
 
 echo "ARCH=$ARCH" >> $config_host_mak
 
diff --git a/scripts/git-submodule.sh b/scripts/git-submodule.sh
new file mode 100755
index 0000000000..f8e7d44ce6
--- /dev/null
+++ b/scripts/git-submodule.sh
@@ -0,0 +1,31 @@
+#!/bin/bash
+
+set -e
+
+command=$1
+shift
+modules="$@"
+
+test -z "$modules" && exit 0
+
+if ! test -d ".git"
+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"
+    trap "rm -f ${substat}.tmp" EXIT
+    diff "${substat}" "${substat}.tmp" >/dev/null
+    exit $?
+    ;;
+update)
+    git submodule update --init $modules 1>/dev/null 2>&1
+    git submodule status $modules > "${substat}"
+    ;;
+esac
-- 
2.13.5

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?

> +    ./scripts/git-submodule.sh status $(GIT_SUBMODULES); \
> +    echo $$?; \
> +  )
> +
> +ifeq (1,$(git_module_status))
> +git-submodule-update:
> +	$(call quiet-command, \
> +          (cd $(SRC_PATH); ./scripts/git-submodule.sh update $(GIT_SUBMODULES)), \

and again

> +          "GIT","$(GIT_SUBMODULES)")
> +endif
> +endif
> +
> +.git-submodule-status: git-submodule-update
> +

> +++ 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.

> +	  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.

> +	  fdt=yes
> +	  dtc_internal="yes"
> +	  mkdir -p dtc
> +	  if [ "$pwd_is_source_path" != "y" ] ; then
> +	      symlink "$source_path/dtc/Makefile" "dtc/Makefile"
> +	      symlink "$source_path/dtc/scripts" "dtc/scripts"
> +	  fi
> +	  fdt_cflags="-I\$(SRC_PATH)/dtc/libfdt"
> +	  fdt_libs="-L\$(BUILD_DIR)/dtc/libfdt $fdt_libs"
> +      elif test "$fdt" = "yes" ; then
> +	  # 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?

> +      else
> +	  # don't have and don't want
> +	  fdt_libs=
> +	  fdt=no
> +      fi
>    fi
>  fi
>  

> +++ 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?

> +
> +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.

> +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?

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

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
> 

> +++ b/scripts/git-submodule.sh
> @@ -0,0 +1,31 @@
> +#!/bin/bash
> +
> +set -e

New file, but appears not to be covered by MAINTAINERS; that should be
fixed.  Also, an explicit copyright/license might be needed.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Thu, Sep 28, 2017 at 12:59:56PM -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
> > 
> 
> > +++ b/scripts/git-submodule.sh
> > @@ -0,0 +1,31 @@
> > +#!/bin/bash
> > +
> > +set -e
> 
> New file, but appears not to be covered by MAINTAINERS; that should be
> fixed.  Also, an explicit copyright/license might be needed.

Yes to both.

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