| Message ID | 20170725011530.17958-1-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
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
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 > >
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 >> >> >
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 > >> > >> > > >
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 > > >> > > >> > > > > >
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 > > > >> > > > >> > > > > > > >
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.
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 >
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.
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
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(-)