Re: [PATCH] configure: actually disable 'git_update' mode with --disable-git-update

[Daniel P . Berrangé]

On Wed, Jul 29, 2020 at 03:58:29PM -0400, Dan Streetman wrote:
> The --disable-git-update configure param sets git_update=no, but
> some later checks only look for the .git dir. This changes the
> --enable-git-update to set git_update=yes but also fail if it
> does not find a .git dir. Then all the later checks for the .git
> dir can just be changed to a check for $git_update = "yes".
> 
> Also update the Makefile to skip the 'git_update' checks if it has
> been disabled.
> 
> This is needed because downstream packagers, e.g. Debian, Ubuntu, etc,
> also keep the source code in git, but do not want to enable the
> 'git_update' mode; with the current code, that's not possible even
> if the downstream package specifies --disable-git-update.

Lets recap the original intended behaviour

 1. User building from master qemu.git or a fork
     a) git_update=yes (default)
         - Always sync submodules to required commit

     b) git_update=no  (--disable-git-update)
         - Never sync submodules, user is responsible for sync
         - Validate submodules are at correct commit and fail if not.

 2. User building from tarball
     - Never do anything git related at all


Your change is removing the validation from  1.b). This is not desirable
in general, because if a user has done a git pull and failed to sync the
submodules, they are liable to get obscure, hard to diagnose errors later
in the build process. This puts a burden on the user and maintainers who
have to waste time diagnosing such problems.



> Signed-off-by: Dan Streetman <ddstreet@canonical.com>
> ---
> Note this is a rebased resend of a previous email to qemu-trivial:
> https://lists.nongnu.org/archive/html/qemu-trivial/2020-07/msg00180.html

NB, I'm removing qemu-trivial, because I don't think this patch
qualifies as trivial.


> 
>  Makefile  | 15 +++++++++------
>  configure | 21 +++++++++++++--------
>  2 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index c2120d8d48..42550ae086 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -25,6 +25,8 @@ git-submodule-update:
>  
>  .PHONY: git-submodule-update
>  
> +# If --disable-git-update specified, skip these git checks
> +ifneq (no,$(GIT_UPDATE))
>  git_module_status := $(shell \
>    cd '$(SRC_PATH)' && \
>    GIT="$(GIT)" ./scripts/git-submodule.sh status $(GIT_SUBMODULES); \
> @@ -32,7 +34,12 @@ git_module_status := $(shell \
>  )
>  
>  ifeq (1,$(git_module_status))
> -ifeq (no,$(GIT_UPDATE))
> +ifeq (yes,$(GIT_UPDATE))
> +git-submodule-update:
> +     $(call quiet-command, \
> +          (cd $(SRC_PATH) && GIT="$(GIT)" ./scripts/git-submodule.sh update 
> $(GIT_SUBMODULES)), \
> +          "GIT","$(GIT_SUBMODULES)")
> +else
>  git-submodule-update:
>       $(call quiet-command, \
>              echo && \
> @@ -41,11 +48,7 @@ git-submodule-update:
>              echo "from the source directory checkout $(SRC_PATH)" && \
>              echo && \
>              exit 1)
> -else
> -git-submodule-update:
> -     $(call quiet-command, \
> -          (cd $(SRC_PATH) && GIT="$(GIT)" ./scripts/git-submodule.sh update 
> $(GIT_SUBMODULES)), \
> -          "GIT","$(GIT_SUBMODULES)")
> +endif
>  endif
>  endif
>  
> diff --git a/configure b/configure
> index 2acc4d1465..e7a241e971 100755
> --- a/configure
> +++ b/configure
> @@ -318,7 +318,7 @@ then
>      git_submodules="$git_submodules tests/fp/berkeley-testfloat-3"
>      git_submodules="$git_submodules tests/fp/berkeley-softfloat-3"
>  else
> -    git_update=no
> +    git_update=""
>      git_submodules=""
>  
>      if ! test -f "$source_path/ui/keycodemapdb/README"
> @@ -1598,7 +1598,12 @@ for opt do
>    ;;
>    --with-git=*) git="$optarg"
>    ;;
> -  --enable-git-update) git_update=yes
> +  --enable-git-update)
> +      git_update=yes
> +      if test ! -e "$source_path/.git"; then
> +          echo "ERROR: cannot --enable-git-update without .git"
> +          exit 1
> +      fi
>    ;;
>    --disable-git-update) git_update=no
>    ;;
> @@ -2011,7 +2016,7 @@ fi
>  # Consult white-list to determine whether to enable werror
>  # by default.  Only enable by default for git builds
>  if test -z "$werror" ; then
> -    if test -e "$source_path/.git" && \
> +    if test "$git_update" = "yes" && \
>          { test "$linux" = "yes" || test "$mingw32" = "yes"; }; then
>          werror="yes"
>      else
> @@ -4412,10 +4417,10 @@ EOF
>      fdt=system
>    else
>        # have GIT checkout, so activate dtc submodule
> -      if test -e "${source_path}/.git" ; then
> +      if test "$git_update" = "yes" ; then
>            git_submodules="${git_submodules} dtc"
>        fi
> -      if test -d "${source_path}/dtc/libfdt" || test -e 
> "${source_path}/.git" ; then
> +      if test -d "${source_path}/dtc/libfdt" || test "$git_update" = "yes" ; 
> then
>            fdt=git
>            mkdir -p dtc
>            if [ "$pwd_is_source_path" != "y" ] ; then
> @@ -5385,7 +5390,7 @@ case "$capstone" in
>    "" | yes)
>      if $pkg_config capstone; then
>        capstone=system
> -    elif test -e "${source_path}/.git" && test $git_update = 'yes' ; then
> +    elif test "$git_update" = "yes" ; then
>        capstone=git
>      elif test -e "${source_path}/capstone/Makefile" ; then
>        capstone=internal
> @@ -6414,7 +6419,7 @@ case "$slirp" in
>    "" | yes)
>      if $pkg_config slirp; then
>        slirp=system
> -    elif test -e "${source_path}/.git" && test $git_update = 'yes' ; then
> +    elif test "$git_update" = "yes" ; then
>        slirp=git
>      elif test -e "${source_path}/slirp/Makefile" ; then
>        slirp=internal
> @@ -6776,7 +6781,7 @@ if test "$cpu" = "s390x" ; then
>      roms="$roms s390-ccw"
>      # SLOF is required for building the s390-ccw firmware on s390x,
>      # since it is using the libnet code from SLOF for network booting.
> -    if test -e "${source_path}/.git" ; then
> +    if test "$git_update" = "yes" ; then
>        git_submodules="${git_submodules} roms/SLOF"
>      fi
>    fi
> -- 
> 2.25.1
> 
> 

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