Re: [Qemu-devel] [PATCH 3/4] build: qemu-ga: fix VSS dependencies

[Michael Roth]

Quoting Michael Roth (2015-09-07 14:55:47)
> Quoting Paolo Bonzini (2015-09-07 05:48:27)
> > 
> > 
> > On 28/08/2015 01:55, Michael Roth wrote:
> > > Now we can build qemu-ga MSI package with:
> > >   ./configure ...
> > >   make qemu-ga.exe
> > >   make msi
> > > 
> > > or simply:
> > >   ./configure ...
> > >   make msi
> > 
> > Shouldn't the latter have always worked?
> 
> Hmm, at the time of the patch I'm not sure, since out-of-tree builds
> were broken with `make msi` until the recent:
> 
>   decdfbd qemu-ga: Fixed paths issue with MSI build
> 
> With that patch in place I noticed out-of-tree builds were still
> broken:
> 
>   address@hidden qemu-build-w64]$ ../w/qemu4.git/configure 
> --enable-guest-agent --target-list=x86_64-softmmu --extra-cflags=-Wall 
> --enable-guest-agent-msi --cross-prefix=x86_64-w64-mingw32- 
> --with-vss-sdk=/home/mdroth/w/vss-win32/ && make msi
>   ...
>     AR    libqemustub.a
>     LINK  qemu-ga.exe
>     CXX   qga/vss-win32/requester.o
>   ...
>     CXX   qga/vss-win32/provider.o
>     CXX   qga/vss-win32/install.o
>     LINK  qga/vss-win32/qga-vss.dll
>     WIXL  qemu-ga-x86_64.msi
>   Couldn't find file /home/mdroth/qemu-build-w64/qga/vss-win32/qga-vss.tlb
>   make: *** [qemu-ga-x86_64.msi] Error 1
> 
> For out-of-tree, qga-vss.tlb dependency gets met by QEMU tools target, which
> is what motivated this patch.
> 
> But for in-tree builds, qga-vss.tlb is already present in working directory,
> so it might have actually worked for that case.
> 
> So I may have misdiagnosed the root issue here: that *.tlb (not just
> *.dll) needed to be added to MSI dependency list instead of being
> assumed (via in-tree build or full qemu build with tools).
> 
> > 
> > I think that if someone does "make qemu-ga.exe" they should *not* get
> > the VSS files.  Perhaps we can add a Win32-specific phony qemu-ga target
> > to build both qemu-ga.exe and the VSS files, but this patch's use of
> > filter-out is a bit ugly.
> 
> I think it might make sense to re-de-couple VSS from qemu-ga.exe, but
> I'm not sure I like the idea of making MSI target responsible for
> VSS files. MSI is relatively new, but VSS support has been around for a
> while when documentated install procedure for qemu-ga.exe was manually
> copying files. MSI isn't the source of the issue, presumably everybody
> distributing qemu-ga.exe in this manner was relying on the full build
> to get the VSS files. But moving VSS completely to MSI means that use
> case would break (whereas moving them to qemu-ga.exe would still work,
> since qemu-ga.exe is build as part of default/full build)
> 
> I think the ideal solution is too keep things as they are with this
> patch (previous working methods supported), but move to a new 'qemu-ga'
> build target that just does the right thing on each platform:
> 
>   on posix:
>     1) build 'qemu-ga' executable
>   
>   on mingw:
>     1) build 'qemu-ga.exe' executable (no need to build .exe directly)
>     2) build VSS if VSS supported/requested
>     3) build MSI package if supported/requested (and include VSS files
>        if VSS supported)
> 
> That would let us drop the wierd filtering, and bring the w32 build
> process more inline with posix.
> 
> I'm not even sure if that's possible atm, but if that's reasonable I can
> look into it as a follow-up (this series is merged already).

diff --git a/Makefile b/Makefile
index 9ce3972..d0ee41e 100644
--- a/Makefile
+++ b/Makefile
@@ -293,15 +293,15 @@ $(qga-obj-y) qemu-ga.o: $(QGALIB_GEN)
 # we require QGA_VSS_PROVIDER files to be built alongside qemu-ga
 # executable since they are shipped together, but we don't want to actually
 # link against them
-qemu-ga$(EXESUF): $(qga-obj-y) libqemuutil.a libqemustub.a $(QGA_VSS_PROVIDER)
-   $(call LINK, $(filter-out $(QGA_VSS_PROVIDER), $^))
+qemu-ga$(EXESUF): $(qga-obj-y) libqemuutil.a libqemustub.a
+   $(call LINK, $^)
 
 ifdef QEMU_GA_MSI_ENABLED
 QEMU_GA_MSI=qemu-ga-$(ARCH).msi
 
 msi: $(QEMU_GA_MSI)
 
-$(QEMU_GA_MSI): qemu-ga.exe
+$(QEMU_GA_MSI): qemu-ga.exe $(QGA_VSS_PROVIDER)
 
 $(QEMU_GA_MSI): config-host.mak
 
@@ -313,6 +313,8 @@ msi:
    @echo "MSI build not configured or dependency resolution failed 
(reconfigure with --enable-guest-agent-msi option)"
 endif
 
+qemu-ga: qemu-ga$(EXESUF) $(QGA_VSS_PROVIDER)                                  
                                                                                
           
 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

This kinda does it, although it introduces a circular dependency warning
on posix (since qemu-ga$(EXESUF) == qemu-ga there, yet qemu-ga has
a qemu-ga$(EXESUF) dependency.

Could be fixed by making a new 'qemu-guest-agent' do-the-right-thing target,
but that means new build process for posix and w32 instead of just w32. The
alternative is to only define the 'qemu-ga' do-the-right-thing target for w32
(since posix already has such a target). That requires a Makefile ifdef MINGW
or somesuch though, which isn't ideal. I think the latter is worth the
simplified build process (same as we currently do on posix, but with some
extra config params)

> 
> > 
> > Paolo
> > 
> 
>