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

[Eric Blake]

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 <address@hidden>
> ---
>  .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

signature.asc
Description: OpenPGP digital signature