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

[Yossi Hindin <yhindin@redhat.com>]

I am submitting second version of patches. 



----- Original Message -----
> From: "Paolo Bonzini" <pbonzini@redhat.com>
> To: "Yossi Hindin" <yhindin@redhat.com>, qemu-devel@nongnu.org
> Cc: yvugenfi@redhat.com, dfleytma@redhat.com, mdroth@linux.vnet.ibm.com
> Sent: Monday, May 4, 2015 12:03:02 PM
> Subject: Re: [PATCH 4/4] qemu-ga: Building Windows MSI installation with configure/Makefile
> 
> 
> 
> 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?

  Yes, it should. Fixed in the new patches.

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

  Fixed. BTW. there are other places  in the 'configure' script where ${} is used.

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

    Renamed

> 
> Also, please keep the guest agent options together.

   Done.

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

  The logic changed according to your comment. Also, Makefile doesn't try to build MSI if MSI was not enabled by 
configure script. 
> 
> > +
> > +
> > +  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.

  Done.

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


   Assignment of QEMU_GA_MSI_MINGW_DLL_PATH grouped wihh other QEMU GA-related variables.

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