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 <ddstr...@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 :|
