* [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows
@ 2017-07-25 1:15 Philippe Mathieu-Daudé
2017-07-25 8:46 ` Daniel P. Berrange
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-07-25 1:15 UTC (permalink / raw)
To: Sameeh Jubran, Michael Roth, Daniel Rempel,
Tomáš Golembiovský
Cc: Philippe Mathieu-Daudé, qemu-devel, Laszlo Ersek,
Peter Maydell, Daniel P . Berrange
Reported-by: Sameeh Jubran <sjubran@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
original report from Sameeh Jubran:
http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg00906.html
Makefile | 2 +-
configure | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index ef721480eb..5f18243d05 100644
--- a/Makefile
+++ b/Makefile
@@ -490,7 +490,7 @@ clean:
rm -f qemu-options.def
rm -f *.msi
find . \( -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 $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) TAGS cscope.* *.pod *~ */*~
rm -f fsdev/*.pod
rm -f qemu-img-cmds.h
rm -f ui/shader/*-vert.h ui/shader/*-frag.h
diff --git a/configure b/configure
index 48d4d7a2a7..f8b1d014d7 100755
--- a/configure
+++ b/configure
@@ -5073,7 +5073,7 @@ fi
if [ "$guest_agent" != "no" ]; then
if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" -o "$mingw32" = "yes" ] ; then
- tools="qemu-ga $tools"
+ tools="qemu-ga\$(EXESUF) $tools"
guest_agent=yes
elif [ "$guest_agent" != yes ]; then
guest_agent=no
--
2.13.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows
2017-07-25 1:15 [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows Philippe Mathieu-Daudé
@ 2017-07-25 8:46 ` Daniel P. Berrange
2017-07-25 23:03 ` Michael Roth
2017-07-26 3:32 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 10+ messages in thread
From: Daniel P. Berrange @ 2017-07-25 8:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Sameeh Jubran, Michael Roth, Daniel Rempel,
Tomáš Golembiovský, qemu-devel, Laszlo Ersek,
Peter Maydell
On Mon, Jul 24, 2017 at 10:15:30PM -0300, Philippe Mathieu-Daudé wrote:
> Reported-by: Sameeh Jubran <sjubran@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> original report from Sameeh Jubran:
> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg00906.html
>
> Makefile | 2 +-
> configure | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
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 :|
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows
2017-07-25 1:15 [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows Philippe Mathieu-Daudé
2017-07-25 8:46 ` Daniel P. Berrange
@ 2017-07-25 23:03 ` Michael Roth
2017-07-26 1:45 ` Philippe Mathieu-Daudé
2017-07-26 3:32 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 10+ messages in thread
From: Michael Roth @ 2017-07-25 23:03 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Tomáš Golembiovský,
Daniel Rempel, Sameeh Jubran
Cc: Peter Maydell, Laszlo Ersek, qemu-devel
Quoting Philippe Mathieu-Daudé (2017-07-24 20:15:30)
> Reported-by: Sameeh Jubran <sjubran@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> original report from Sameeh Jubran:
> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg00906.html
AFAICT the issue discussed in the context of that patch is simply that
the qemu-ga.exe file isn't deleted as part of `make clean`, which does
seem to be an issue.
But qemu.ga.exe is created just fine as part of `make qemu-ga`, at least
as of:
fafcaf1d build: qemu-ga: add 'qemu-ga' build target for w32
The 'qemu-ga' target is actually an intermediate target which, on w32,
creates the MSI package (if configured) as well as qemu-ga.exe.
>
> Makefile | 2 +-
> configure | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index ef721480eb..5f18243d05 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -490,7 +490,7 @@ clean:
> rm -f qemu-options.def
> rm -f *.msi
> find . \( -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 $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) TAGS cscope.* *.pod *~ */*~
> rm -f fsdev/*.pod
> rm -f qemu-img-cmds.h
> rm -f ui/shader/*-vert.h ui/shader/*-frag.h
> diff --git a/configure b/configure
> index 48d4d7a2a7..f8b1d014d7 100755
> --- a/configure
> +++ b/configure
> @@ -5073,7 +5073,7 @@ fi
>
> if [ "$guest_agent" != "no" ]; then
> if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" -o "$mingw32" = "yes" ] ; then
> - tools="qemu-ga $tools"
> + tools="qemu-ga\$(EXESUF) $tools"
> guest_agent=yes
> elif [ "$guest_agent" != yes ]; then
> guest_agent=no
> --
> 2.13.3
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows
2017-07-25 23:03 ` Michael Roth
@ 2017-07-26 1:45 ` Philippe Mathieu-Daudé
2017-07-26 2:53 ` Michael Roth
0 siblings, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-07-26 1:45 UTC (permalink / raw)
To: Michael Roth, Tomáš Golembiovský,
Daniel P. Berrange, Sameeh Jubran
Cc: Peter Maydell, Laszlo Ersek, qemu-devel
Hi Michael,
On 07/25/2017 08:03 PM, Michael Roth wrote:
> Quoting Philippe Mathieu-Daudé (2017-07-24 20:15:30)
>> Reported-by: Sameeh Jubran <sjubran@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> original report from Sameeh Jubran:
>> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg00906.html
>
> AFAICT the issue discussed in the context of that patch is simply that
> the qemu-ga.exe file isn't deleted as part of `make clean`, which does
> seem to be an issue.
>
> But qemu.ga.exe is created just fine as part of `make qemu-ga`, at least
> as of:
>
> fafcaf1d build: qemu-ga: add 'qemu-ga' build target for w32
>
Yes, on w32 if "make ASDF" invokes the linker, it will create ASDF.exe.
> The 'qemu-ga' target is actually an intermediate target which, on w32,
> creates the MSI package (if configured) as well as qemu-ga.exe.
>
>>
>> Makefile | 2 +-
>> configure | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index ef721480eb..5f18243d05 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -490,7 +490,7 @@ clean:
>> rm -f qemu-options.def
>> rm -f *.msi
>> find . \( -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 $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) TAGS cscope.* *.pod *~ */*~
On win32, this patch add qemu-ga.exe instead of qemu-ga in $TOOLS, so
when the 'clean' target is executed it removes qemu-ga.exe (Sameeh
Jubran report). Before this patch the $TOOLS looks like:
"qemu-img.exe qemu-io.exe qemu-nbd.exe ivshmem-client.exe
ivshmem-server.exe qemu-ga fsdev/virtfs-proxy-helper.exe"
The only executables which doesn't match %.exe is qemu-ga, so the
'clean' target remove all .exe _but_ qemu-ga.exe.
Now if by "this is not an issue" you mean it is not a bug, I agree this
can wait 2.11.
Regards,
Phil.
>> rm -f fsdev/*.pod
>> rm -f qemu-img-cmds.h
>> rm -f ui/shader/*-vert.h ui/shader/*-frag.h
>> diff --git a/configure b/configure
>> index 48d4d7a2a7..f8b1d014d7 100755
>> --- a/configure
>> +++ b/configure
>> @@ -5073,7 +5073,7 @@ fi
>>
>> if [ "$guest_agent" != "no" ]; then
>> if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" -o "$mingw32" = "yes" ] ; then
>> - tools="qemu-ga $tools"
>> + tools="qemu-ga\$(EXESUF) $tools"
>> guest_agent=yes
>> elif [ "$guest_agent" != yes ]; then
>> guest_agent=no
>> --
>> 2.13.3
>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows
2017-07-26 1:45 ` Philippe Mathieu-Daudé
@ 2017-07-26 2:53 ` Michael Roth
2017-07-26 2:56 ` Michael Roth
0 siblings, 1 reply; 10+ messages in thread
From: Michael Roth @ 2017-07-26 2:53 UTC (permalink / raw)
To: Daniel P. Berrange, Tomáš Golembiovský,
Philippe Mathieu-Daudé, Sameeh Jubran
Cc: Peter Maydell, Laszlo Ersek, qemu-devel
Quoting Philippe Mathieu-Daudé (2017-07-25 20:45:31)
> Hi Michael,
>
> On 07/25/2017 08:03 PM, Michael Roth wrote:
> > Quoting Philippe Mathieu-Daudé (2017-07-24 20:15:30)
> >> Reported-by: Sameeh Jubran <sjubran@redhat.com>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >> original report from Sameeh Jubran:
> >> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg00906.html
> >
> > AFAICT the issue discussed in the context of that patch is simply that
> > the qemu-ga.exe file isn't deleted as part of `make clean`, which does
> > seem to be an issue.
> >
> > But qemu.ga.exe is created just fine as part of `make qemu-ga`, at least
> > as of:
> >
> > fafcaf1d build: qemu-ga: add 'qemu-ga' build target for w32
> >
>
> Yes, on w32 if "make ASDF" invokes the linker, it will create ASDF.exe.
>
> > The 'qemu-ga' target is actually an intermediate target which, on w32,
> > creates the MSI package (if configured) as well as qemu-ga.exe.
> >
> >>
> >> Makefile | 2 +-
> >> configure | 2 +-
> >> 2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Makefile b/Makefile
> >> index ef721480eb..5f18243d05 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -490,7 +490,7 @@ clean:
> >> rm -f qemu-options.def
> >> rm -f *.msi
> >> find . \( -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 $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) TAGS cscope.* *.pod *~ */*~
>
> On win32, this patch add qemu-ga.exe instead of qemu-ga in $TOOLS, so
> when the 'clean' target is executed it removes qemu-ga.exe (Sameeh
Ok, that makes more sense, but it's not what the commit msg implies.
> Jubran report). Before this patch the $TOOLS looks like:
> "qemu-img.exe qemu-io.exe qemu-nbd.exe ivshmem-client.exe
> ivshmem-server.exe qemu-ga fsdev/virtfs-proxy-helper.exe"
That change was done explicitly via fafcaf1d so that `make qemu-ga` and
`make` without --disable-tools specified to configure will generate the
MSI package when the user configures it. So, unlike the other $TOOLS
targets, the qemu-ga "distributables" encompass more than just the .exe
on w32, so we use the "qemu-ga" target instead of "qemu-ga.exe" directly.
Reverting that change to coax `make clean` into cleaning up qemu-ga.exe
means that `make` no longer builds the qemu-ga-*.msi when the user
configures it, which is a regression.
> The only executables which doesn't match %.exe is qemu-ga, so the
> 'clean' target remove all .exe _but_ qemu-ga.exe.
As with Sameeh's original patch, the qemu-ga target would already
require special handling to deal with qemu-ga-*.msi file. We should
similarly just cleanup qemu-ga.exe as a special case instead of
modifying $TOOLS, since that brings about unecessary side-effects
described above.
As a workaround to the issue you/Peter pointed out with Sameeh's patch
(nuking the entire source tree for posix builds where $EXESUF == ""), I
proposed we just do:
make clean:
...
ifneq($EXESUF,)
rm -f *$(EXESUF)
endif
But given your clarification here, I understand that $TOOLS already
takes care of everything except qemu-ga.exe, so I think you've already
mentioned the most straightforward fix in the other thread:
- rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
+ rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga${EXESUF} TAGS cscope.* *.pod *~ */*~
It's a bit ugly since `rm -f qemu-ga.exe` would silently fail on posix,
but `rm -f $TOOLS qemu-ga ...` already silently fails since it already gets
removed via $TOOLS.
Alternatively, you can explicitly check for qemu-ga.exe and remove it if
it exists. I would consider either acceptable, but not this patch as it
currently stands.
>
> Now if by "this is not an issue" you mean it is not a bug, I agree this
> can wait 2.11.
I just mean the issue as described in the commit msg.
For the `make clean` stuff, if it's simple enough it might be acceptable for
rc1 if you can get it out this week. Otherwise we can wait for 2.11.
>
> Regards,
>
> Phil.
>
> >> rm -f fsdev/*.pod
> >> rm -f qemu-img-cmds.h
> >> rm -f ui/shader/*-vert.h ui/shader/*-frag.h
> >> diff --git a/configure b/configure
> >> index 48d4d7a2a7..f8b1d014d7 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -5073,7 +5073,7 @@ fi
> >>
> >> if [ "$guest_agent" != "no" ]; then
> >> if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" -o "$mingw32" = "yes" ] ; then
> >> - tools="qemu-ga $tools"
> >> + tools="qemu-ga\$(EXESUF) $tools"
> >> guest_agent=yes
> >> elif [ "$guest_agent" != yes ]; then
> >> guest_agent=no
> >> --
> >> 2.13.3
> >>
> >>
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows
2017-07-26 2:53 ` Michael Roth
@ 2017-07-26 2:56 ` Michael Roth
2017-07-26 3:01 ` Michael Roth
2017-07-26 3:28 ` Philippe Mathieu-Daudé
0 siblings, 2 replies; 10+ messages in thread
From: Michael Roth @ 2017-07-26 2:56 UTC (permalink / raw)
To: Daniel P. Berrange, Tomáš Golembiovský,
Philippe Mathieu-Daudé, Sameeh Jubran
Cc: Peter Maydell, Laszlo Ersek, qemu-devel
Quoting Michael Roth (2017-07-25 21:53:41)
> Quoting Philippe Mathieu-Daudé (2017-07-25 20:45:31)
> > Hi Michael,
> >
> > On 07/25/2017 08:03 PM, Michael Roth wrote:
> > > Quoting Philippe Mathieu-Daudé (2017-07-24 20:15:30)
> > >> Reported-by: Sameeh Jubran <sjubran@redhat.com>
> > >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > >> ---
> > >> original report from Sameeh Jubran:
> > >> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg00906.html
> > >
> > > AFAICT the issue discussed in the context of that patch is simply that
> > > the qemu-ga.exe file isn't deleted as part of `make clean`, which does
> > > seem to be an issue.
> > >
> > > But qemu.ga.exe is created just fine as part of `make qemu-ga`, at least
> > > as of:
> > >
> > > fafcaf1d build: qemu-ga: add 'qemu-ga' build target for w32
> > >
> >
> > Yes, on w32 if "make ASDF" invokes the linker, it will create ASDF.exe.
> >
> > > The 'qemu-ga' target is actually an intermediate target which, on w32,
> > > creates the MSI package (if configured) as well as qemu-ga.exe.
> > >
> > >>
> > >> Makefile | 2 +-
> > >> configure | 2 +-
> > >> 2 files changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/Makefile b/Makefile
> > >> index ef721480eb..5f18243d05 100644
> > >> --- a/Makefile
> > >> +++ b/Makefile
> > >> @@ -490,7 +490,7 @@ clean:
> > >> rm -f qemu-options.def
> > >> rm -f *.msi
> > >> find . \( -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 $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) TAGS cscope.* *.pod *~ */*~
> >
> > On win32, this patch add qemu-ga.exe instead of qemu-ga in $TOOLS, so
> > when the 'clean' target is executed it removes qemu-ga.exe (Sameeh
>
> Ok, that makes more sense, but it's not what the commit msg implies.
>
> > Jubran report). Before this patch the $TOOLS looks like:
> > "qemu-img.exe qemu-io.exe qemu-nbd.exe ivshmem-client.exe
> > ivshmem-server.exe qemu-ga fsdev/virtfs-proxy-helper.exe"
>
> That change was done explicitly via fafcaf1d so that `make qemu-ga` and
> `make` without --disable-tools specified to configure will generate the
> MSI package when the user configures it. So, unlike the other $TOOLS
> targets, the qemu-ga "distributables" encompass more than just the .exe
> on w32, so we use the "qemu-ga" target instead of "qemu-ga.exe" directly.
>
> Reverting that change to coax `make clean` into cleaning up qemu-ga.exe
> means that `make` no longer builds the qemu-ga-*.msi when the user
> configures it, which is a regression.
>
> > The only executables which doesn't match %.exe is qemu-ga, so the
> > 'clean' target remove all .exe _but_ qemu-ga.exe.
>
> As with Sameeh's original patch, the qemu-ga target would already
> require special handling to deal with qemu-ga-*.msi file. We should
> similarly just cleanup qemu-ga.exe as a special case instead of
> modifying $TOOLS, since that brings about unecessary side-effects
> described above.
>
> As a workaround to the issue you/Peter pointed out with Sameeh's patch
> (nuking the entire source tree for posix builds where $EXESUF == ""), I
> proposed we just do:
>
> make clean:
> ...
> ifneq($EXESUF,)
> rm -f *$(EXESUF)
> endif
>
> But given your clarification here, I understand that $TOOLS already
> takes care of everything except qemu-ga.exe, so I think you've already
> mentioned the most straightforward fix in the other thread:
>
> - rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
> + rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga${EXESUF} TAGS cscope.* *.pod *~ */*~
>
> It's a bit ugly since `rm -f qemu-ga.exe` would silently fail on posix,
On w32 I mean, sorry.
> but `rm -f $TOOLS qemu-ga ...` already silently fails since it already gets
> removed via $TOOLS.
>
> Alternatively, you can explicitly check for qemu-ga.exe and remove it if
> it exists. I would consider either acceptable, but not this patch as it
> currently stands.
>
> >
> > Now if by "this is not an issue" you mean it is not a bug, I agree this
> > can wait 2.11.
>
> I just mean the issue as described in the commit msg.
>
> For the `make clean` stuff, if it's simple enough it might be acceptable for
> rc1 if you can get it out this week. Otherwise we can wait for 2.11.
>
> >
> > Regards,
> >
> > Phil.
> >
> > >> rm -f fsdev/*.pod
> > >> rm -f qemu-img-cmds.h
> > >> rm -f ui/shader/*-vert.h ui/shader/*-frag.h
> > >> diff --git a/configure b/configure
> > >> index 48d4d7a2a7..f8b1d014d7 100755
> > >> --- a/configure
> > >> +++ b/configure
> > >> @@ -5073,7 +5073,7 @@ fi
> > >>
> > >> if [ "$guest_agent" != "no" ]; then
> > >> if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" -o "$mingw32" = "yes" ] ; then
> > >> - tools="qemu-ga $tools"
> > >> + tools="qemu-ga\$(EXESUF) $tools"
> > >> guest_agent=yes
> > >> elif [ "$guest_agent" != yes ]; then
> > >> guest_agent=no
> > >> --
> > >> 2.13.3
> > >>
> > >>
> > >
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows
2017-07-26 2:56 ` Michael Roth
@ 2017-07-26 3:01 ` Michael Roth
2017-07-26 3:28 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 10+ messages in thread
From: Michael Roth @ 2017-07-26 3:01 UTC (permalink / raw)
To: Daniel P. Berrange, Tomáš Golembiovský,
Philippe Mathieu-Daudé, Sameeh Jubran
Cc: Peter Maydell, Laszlo Ersek, qemu-devel
Quoting Michael Roth (2017-07-25 21:56:14)
> Quoting Michael Roth (2017-07-25 21:53:41)
> > Quoting Philippe Mathieu-Daudé (2017-07-25 20:45:31)
> > > Hi Michael,
> > >
> > > On 07/25/2017 08:03 PM, Michael Roth wrote:
> > > > Quoting Philippe Mathieu-Daudé (2017-07-24 20:15:30)
> > > >> Reported-by: Sameeh Jubran <sjubran@redhat.com>
> > > >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > >> ---
> > > >> original report from Sameeh Jubran:
> > > >> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg00906.html
> > > >
> > > > AFAICT the issue discussed in the context of that patch is simply that
> > > > the qemu-ga.exe file isn't deleted as part of `make clean`, which does
> > > > seem to be an issue.
> > > >
> > > > But qemu.ga.exe is created just fine as part of `make qemu-ga`, at least
> > > > as of:
> > > >
> > > > fafcaf1d build: qemu-ga: add 'qemu-ga' build target for w32
> > > >
> > >
> > > Yes, on w32 if "make ASDF" invokes the linker, it will create ASDF.exe.
> > >
> > > > The 'qemu-ga' target is actually an intermediate target which, on w32,
> > > > creates the MSI package (if configured) as well as qemu-ga.exe.
> > > >
> > > >>
> > > >> Makefile | 2 +-
> > > >> configure | 2 +-
> > > >> 2 files changed, 2 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/Makefile b/Makefile
> > > >> index ef721480eb..5f18243d05 100644
> > > >> --- a/Makefile
> > > >> +++ b/Makefile
> > > >> @@ -490,7 +490,7 @@ clean:
> > > >> rm -f qemu-options.def
> > > >> rm -f *.msi
> > > >> find . \( -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 $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) TAGS cscope.* *.pod *~ */*~
> > >
> > > On win32, this patch add qemu-ga.exe instead of qemu-ga in $TOOLS, so
> > > when the 'clean' target is executed it removes qemu-ga.exe (Sameeh
> >
> > Ok, that makes more sense, but it's not what the commit msg implies.
> >
> > > Jubran report). Before this patch the $TOOLS looks like:
> > > "qemu-img.exe qemu-io.exe qemu-nbd.exe ivshmem-client.exe
> > > ivshmem-server.exe qemu-ga fsdev/virtfs-proxy-helper.exe"
> >
> > That change was done explicitly via fafcaf1d so that `make qemu-ga` and
> > `make` without --disable-tools specified to configure will generate the
> > MSI package when the user configures it. So, unlike the other $TOOLS
> > targets, the qemu-ga "distributables" encompass more than just the .exe
> > on w32, so we use the "qemu-ga" target instead of "qemu-ga.exe" directly.
> >
> > Reverting that change to coax `make clean` into cleaning up qemu-ga.exe
> > means that `make` no longer builds the qemu-ga-*.msi when the user
> > configures it, which is a regression.
> >
> > > The only executables which doesn't match %.exe is qemu-ga, so the
> > > 'clean' target remove all .exe _but_ qemu-ga.exe.
> >
> > As with Sameeh's original patch, the qemu-ga target would already
> > require special handling to deal with qemu-ga-*.msi file. We should
> > similarly just cleanup qemu-ga.exe as a special case instead of
> > modifying $TOOLS, since that brings about unecessary side-effects
> > described above.
> >
> > As a workaround to the issue you/Peter pointed out with Sameeh's patch
> > (nuking the entire source tree for posix builds where $EXESUF == ""), I
> > proposed we just do:
> >
> > make clean:
> > ...
> > ifneq($EXESUF,)
> > rm -f *$(EXESUF)
> > endif
> >
> > But given your clarification here, I understand that $TOOLS already
> > takes care of everything except qemu-ga.exe, so I think you've already
> > mentioned the most straightforward fix in the other thread:
> >
> > - rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
> > + rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga${EXESUF} TAGS cscope.* *.pod *~ */*~
> >
> > It's a bit ugly since `rm -f qemu-ga.exe` would silently fail on posix,
>
> On w32 I mean, sorry.
Ugh, no, on posix. Let's just try that again:
It's a bit ugly since `rm -f ... qemu-ga${EXESUF}` would silently fail on posix,
but `rm -f $TOOLS qemu-ga ...` already silently fails since qemu-ga already gets
removed via $TOOLS, so it's not any worse.
>
> > but `rm -f $TOOLS qemu-ga ...` already silently fails since it already gets
> > removed via $TOOLS.
> >
> > Alternatively, you can explicitly check for qemu-ga.exe and remove it if
> > it exists. I would consider either acceptable, but not this patch as it
> > currently stands.
> >
> > >
> > > Now if by "this is not an issue" you mean it is not a bug, I agree this
> > > can wait 2.11.
> >
> > I just mean the issue as described in the commit msg.
> >
> > For the `make clean` stuff, if it's simple enough it might be acceptable for
> > rc1 if you can get it out this week. Otherwise we can wait for 2.11.
> >
> > >
> > > Regards,
> > >
> > > Phil.
> > >
> > > >> rm -f fsdev/*.pod
> > > >> rm -f qemu-img-cmds.h
> > > >> rm -f ui/shader/*-vert.h ui/shader/*-frag.h
> > > >> diff --git a/configure b/configure
> > > >> index 48d4d7a2a7..f8b1d014d7 100755
> > > >> --- a/configure
> > > >> +++ b/configure
> > > >> @@ -5073,7 +5073,7 @@ fi
> > > >>
> > > >> if [ "$guest_agent" != "no" ]; then
> > > >> if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" -o "$mingw32" = "yes" ] ; then
> > > >> - tools="qemu-ga $tools"
> > > >> + tools="qemu-ga\$(EXESUF) $tools"
> > > >> guest_agent=yes
> > > >> elif [ "$guest_agent" != yes ]; then
> > > >> guest_agent=no
> > > >> --
> > > >> 2.13.3
> > > >>
> > > >>
> > > >
> > >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows
2017-07-26 2:56 ` Michael Roth
2017-07-26 3:01 ` Michael Roth
@ 2017-07-26 3:28 ` Philippe Mathieu-Daudé
2017-07-26 10:37 ` Eric Blake
1 sibling, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-07-26 3:28 UTC (permalink / raw)
To: Michael Roth, Daniel P. Berrange,
Tomáš Golembiovský, Sameeh Jubran
Cc: Peter Maydell, Laszlo Ersek, qemu-devel
On 07/25/2017 11:56 PM, Michael Roth wrote:
> Quoting Michael Roth (2017-07-25 21:53:41)
>> Quoting Philippe Mathieu-Daudé (2017-07-25 20:45:31)
[...]
>> That change was done explicitly via fafcaf1d so that `make qemu-ga` and
>> `make` without --disable-tools specified to configure will generate the
>> MSI package when the user configures it. So, unlike the other $TOOLS
>> targets, the qemu-ga "distributables" encompass more than just the .exe
>> on w32, so we use the "qemu-ga" target instead of "qemu-ga.exe" directly.
I see, I didn't think about running git blame on this line, sorry.
>>
>> Reverting that change to coax `make clean` into cleaning up qemu-ga.exe
>> means that `make` no longer builds the qemu-ga-*.msi when the user
>> configures it, which is a regression.
>>
>>> The only executables which doesn't match %.exe is qemu-ga, so the
>>> 'clean' target remove all .exe _but_ qemu-ga.exe.
>>
>> As with Sameeh's original patch, the qemu-ga target would already
>> require special handling to deal with qemu-ga-*.msi file. We should
>> similarly just cleanup qemu-ga.exe as a special case instead of
>> modifying $TOOLS, since that brings about unecessary side-effects
>> described above.
>>
>> As a workaround to the issue you/Peter pointed out with Sameeh's patch
>> (nuking the entire source tree for posix builds where $EXESUF == ""), I
>> proposed we just do:
>>
>> make clean:
>> ...
>> ifneq($EXESUF,)
>> rm -f *$(EXESUF)
>> endif
>>
Now I understand your fix. And looking at the Makefile structure it
seems to be written with only UNIX system in mind (well, Windows world
was not written with UNIX philosophy and GNU Make has some limitations
out of it).
I can't think of a non-ugly short way to resolve Sameeh's issue. So I'll
drop the patch I purposed for 2.10.
>> But given your clarification here, I understand that $TOOLS already
>> takes care of everything except qemu-ga.exe, so I think you've already
>> mentioned the most straightforward fix in the other thread:
>>
>> - rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
>> + rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga${EXESUF} TAGS cscope.* *.pod *~ */*~
>>
>> It's a bit ugly since `rm -f qemu-ga.exe` would silently fail on posix,
>
> On w32 I mean, sorry.
side question: should we use $(RM) with something like:
ifeq ($(OS),Windows_NT)
RM = cmd //C del //Q //F
else
RM = rf -f
endif
>
>> but `rm -f $TOOLS qemu-ga ...` already silently fails since it already gets
>> removed via $TOOLS.
>>
>> Alternatively, you can explicitly check for qemu-ga.exe and remove it if
>> it exists. I would consider either acceptable, but not this patch as it
>> currently stands.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows
2017-07-25 1:15 [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows Philippe Mathieu-Daudé
2017-07-25 8:46 ` Daniel P. Berrange
2017-07-25 23:03 ` Michael Roth
@ 2017-07-26 3:32 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-07-26 3:32 UTC (permalink / raw)
To: Sameeh Jubran, Michael Roth, Daniel Rempel,
Tomáš Golembiovský
Cc: qemu-devel, Laszlo Ersek, Peter Maydell, Daniel P . Berrange
As noticed by Michael Roth the ./configure entry for qemu-ga is missing
the $(EXESUF) on purpose (see fafcaf1d).
Patch dropped for 2.10
On 07/24/2017 10:15 PM, Philippe Mathieu-Daudé wrote:
> Reported-by: Sameeh Jubran <sjubran@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> original report from Sameeh Jubran:
> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg00906.html
>
> Makefile | 2 +-
> configure | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index ef721480eb..5f18243d05 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -490,7 +490,7 @@ clean:
> rm -f qemu-options.def
> rm -f *.msi
> find . \( -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 $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) TAGS cscope.* *.pod *~ */*~
> rm -f fsdev/*.pod
> rm -f qemu-img-cmds.h
> rm -f ui/shader/*-vert.h ui/shader/*-frag.h
> diff --git a/configure b/configure
> index 48d4d7a2a7..f8b1d014d7 100755
> --- a/configure
> +++ b/configure
> @@ -5073,7 +5073,7 @@ fi
>
> if [ "$guest_agent" != "no" ]; then
> if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" -o "$mingw32" = "yes" ] ; then
> - tools="qemu-ga $tools"
> + tools="qemu-ga\$(EXESUF) $tools"
> guest_agent=yes
> elif [ "$guest_agent" != yes ]; then
> guest_agent=no
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows
2017-07-26 3:28 ` Philippe Mathieu-Daudé
@ 2017-07-26 10:37 ` Eric Blake
0 siblings, 0 replies; 10+ messages in thread
From: Eric Blake @ 2017-07-26 10:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Michael Roth, Daniel P. Berrange,
Tomáš Golembiovský, Sameeh Jubran
Cc: Peter Maydell, Laszlo Ersek, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]
On 07/25/2017 10:28 PM, Philippe Mathieu-Daudé wrote:
>>> - rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS
>>> cscope.* *.pod *~ */*~
>>> + rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga${EXESUF}
>>> TAGS cscope.* *.pod *~ */*~
>>>
>>> It's a bit ugly since `rm -f qemu-ga.exe` would silently fail on posix,
But that's the whole point of 'rm -f' - to silently ignore files that
didn't exist.
>>
>> On w32 I mean, sorry.
>
> side question: should we use $(RM) with something like:
>
> ifeq ($(OS),Windows_NT)
> RM = cmd //C del //Q //F
> else
> RM = rf -f
> endif
>
Absolutely not. We can at least assume a mingw-like environment that
has all the usual tools like rm.
>>>
>>> Alternatively, you can explicitly check for qemu-ga.exe and remove it if
>>> it exists. I would consider either acceptable, but not this patch as it
>>> currently stands.
But that's what 'rm -f' already does - I don't see the point in adding a
racy check for existence prior to deletion, when deletion already
handles the race.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-07-26 10:37 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-25 1:15 [Qemu-devel] [PATCH] configure: fix qemu-ga missing '.exe' extension on windows Philippe Mathieu-Daudé
2017-07-25 8:46 ` Daniel P. Berrange
2017-07-25 23:03 ` Michael Roth
2017-07-26 1:45 ` Philippe Mathieu-Daudé
2017-07-26 2:53 ` Michael Roth
2017-07-26 2:56 ` Michael Roth
2017-07-26 3:01 ` Michael Roth
2017-07-26 3:28 ` Philippe Mathieu-Daudé
2017-07-26 10:37 ` Eric Blake
2017-07-26 3:32 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).