Re: [Qemu-devel] [PATCH 4/4] qemu-ga: Building Windows MSI installation with configure/Makefile

[Paolo Bonzini]

On 26/04/2015 09:04, Yossi Hindin wrote:
> +ifdef QEMU_GA_MSI_WITH_VSS
> +${QEMU_GA_MSI}: qga/vss-win32/qga-vss.dll qemu-ga.exe

Shouldn't the qemu-ga.exe dependency be unconditional?

> +endif
> +
> +${QEMU_GA_MSI}: config-host.mak
> +
> +${QEMU_GA_MSI}:  qga/installer/qemu-ga.wxs
> +     $(call quiet-command,QEMU_GA_VERSION="$(QEMU_GA_VERSION)" 
> QEMU_GA_MANUFACTURER="$(QEMU_GA_MANUFACTURER)" 
> QEMU_GA_DISTRO="$(QEMU_GA_DISTRO)" \
> +     wixl -o $@ ${QEMU_GA_MSI_ARCH} ${QEMU_GA_MSI_WITH_VSS} 
> ${QEMU_GA_MSI_MINGW_DLL_PATH} $<, "  WIXL  $@")

Please use $(...) instead of ${...} for consistency.

> +
>  clean:
>  # avoid old build problems by removing potentially incorrect old files
>       rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h 
> gen-op-arm.h
>       rm -f qemu-options.def
> +     rm -f *.msi
>       find . \( -name '*.l[oa]' -o -name '*.so' -o -name '*.dll' -o -name 
> '*.mo' -o -name '*.[oda]' \) -type f -exec rm {} +
>       rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* 
> *.pod *~ */*~
>       rm -f fsdev/*.pod
> diff --git a/configure b/configure
> index 6969f6f..0aa79bb 100755
> --- a/configure
> +++ b/configure
> @@ -316,6 +316,7 @@ snappy=""
>  bzip2=""
>  guest_agent=""
>  guest_agent_with_vss="no"
> +guest_msi=""
>  vss_win32_sdk=""
>  win_sdk="no"
>  want_tools="yes"
> @@ -1069,6 +1070,10 @@ for opt do
>    ;;
>    --disable-guest-agent) guest_agent="no"
>    ;;
> +  --enable-guest-msi) guest_msi="yes"
> +  ;;
> +  --disable-guest-msi) guest_msi="no"
> +  ;;
>    --with-vss-sdk) vss_win32_sdk=""
>    ;;
>    --with-vss-sdk=*) vss_win32_sdk="$optarg"
> @@ -1407,6 +1412,8 @@ Advanced options (experts only):
>    --enable-quorum          enable quorum block filter support
>    --disable-numa           disable libnuma support
>    --enable-numa            enable libnuma support
> +  --enable-guest-msi       enable building guest agent Windows MSI 
> installation package
> +  --disable-guest-msi      disable building guest agent Windows MSI 
> installation package

--enable-guest-agent-msi?

Also, please keep the guest agent options together.

>  
>  NOTE: The object files are built at the place where configure is launched
>  EOF
> @@ -3832,6 +3839,54 @@ if test "$mingw32" = "yes" -a "$guest_agent" != "no" 
> -a "$guest_agent_with_vss"
>  fi
>  
>  ##########################################
> +# Guest agent Window MSI  package
> +
> +if test "$guest_msi" = "yes"; then

Since building the MSI is on-demand anyway ("make msi"), we might as well
treat it similar to other configure options:

- --enable-foo causes an error if the feature is not available
- --disable-foo forcibly disable the feature
- the default is to configure the feature if available, otherwise disable it

This would be something like this:

if test "$guest_agent" != yes; then
  if test "$guest_msi" = yes; then
    error_exit "MSI guest agent package requires guest agent enabled"
  fi
  guest_msi=no
elif test "$mingw32" != "yes"; then
  if test "$guest_msi" = "yes"; then
    error_exit "MSI guest agent package is available only for MinGW Windows 
cross-compilation"
  fi
  guest_msi=no
elif test ! has_wixl; then
  if test "$guest_msi" = "yes"; then
    error_exit "wixl not found, required for building MSI guest agent package"
  fi
  guest_msi=no
fi

if test "$guest_msi" != no; then
  ...
fi

> +
> +
> +  if test "$guest_agent_with_vss" = "yes"; then
> +    QEMU_GA_MSI_WITH_VSS="-D InstallVss"
> +  fi
> +
> +  if test "$QEMU_GA_MANUFACTURER" = ""; then
> +    QEMU_GA_MANUFACTURER=QEMU
> +  fi
> +
> +  if test "$QEMU_GA_DISTRO" = ""; then
> +    QEMU_GA_DISTRO=Linux
> +  fi
> +
> +  if test "$QEMU_GA_VERSION" = ""; then
> +      QEMU_GA_VERSION=`cat $source_path/VERSION`
> +  fi
> +
> +  case "$cpu" in
> +  x86_64)
> +    QEMU_GA_MSI_ARCH="-a x64 -D Arch=64"
> +    ;;
> +  i386)
> +    QEMU_GA_MSI_ARCH="-D Arch=32"
> +    ;;
> +  *)
> +    error_exit "CPU $cpu not supported for building installation package"
> +    ;;
> +  esac
> +
> +fi
> +
> +##########################################
>  
>  ##########################################
>  # check if we have fdatasync
> @@ -4500,6 +4555,14 @@ if test "$mingw32" = "yes" ; then
>      echo "CONFIG_QGA_VSS=y" >> $config_host_mak
>      echo "WIN_SDK=\"$win_sdk\"" >> $config_host_mak
>    fi
> +  if test "$guest_msi" = "yes"; then

Similarly, "!= no" here.

> +    echo "QEMU_GA_MSI_MINGW_DLL_PATH=-D Mingw_dlls=`$pkg_config 
> --variable=prefix glib-2.0`/bin" >> $config_host_mak

Why not set up the variable above?

Paolo

> +    echo "QEMU_GA_MSI_WITH_VSS=${QEMU_GA_MSI_WITH_VSS}" >> $config_host_mak
> +    echo "QEMU_GA_MSI_ARCH=${QEMU_GA_MSI_ARCH}" >> $config_host_mak
> +    echo "QEMU_GA_MANUFACTURER=${QEMU_GA_MANUFACTURER}" >> $config_host_mak
> +    echo "QEMU_GA_DISTRO=${QEMU_GA_DISTRO}" >> $config_host_mak
> +    echo "QEMU_GA_VERSION=${QEMU_GA_VERSION}" >> $config_host_mak
> +  fi
>  else
>    echo "CONFIG_POSIX=y" >> $config_host_mak
>  fi
>