[Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Peter Maydell]

Avoid having an explicit list of directories in the 'clean'
target by using 'find' to remove all .o and .d files instead.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I figured that (unlike Makefile.target) we should probably take
the xargs route here since otherwise the rm command line is huge...

There's also an argument that there's not much point having a clean
target in Makefile.target when this one blows away most of it anyway.

 Makefile |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 621cb86..9f7eaa8 100644
--- a/Makefile
+++ b/Makefile
@@ -212,13 +212,10 @@ 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 *.o *.d *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
+	find . -name '*.[od]' | xargs rm -f
+	rm -f *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
 	rm -Rf .libs
-	rm -f slirp/*.o slirp/*.d audio/*.o audio/*.d block/*.o block/*.d net/*.o net/*.d fsdev/*.o fsdev/*.d ui/*.o ui/*.d qapi/*.o qapi/*.d qga/*.o qga/*.d
-	rm -f qom/*.o qom/*.d
-	rm -f usb/*.o usb/*.d hw/*.o hw/*.d
 	rm -f qemu-img-cmds.h
-	rm -f trace/*.o trace/*.d
 	rm -f trace-dtrace.dtrace trace-dtrace.dtrace-timestamp
 	@# May not be present in GENERATED_HEADERS
 	rm -f trace-dtrace.h trace-dtrace.h-timestamp
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Stefan Hajnoczi]

On Tue, Jul 31, 2012 at 02:01:35PM +0100, Peter Maydell wrote:
> Avoid having an explicit list of directories in the 'clean'
> target by using 'find' to remove all .o and .d files instead.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I figured that (unlike Makefile.target) we should probably take
> the xargs route here since otherwise the rm command line is huge...
> 
> There's also an argument that there's not much point having a clean
> target in Makefile.target when this one blows away most of it anyway.
> 
>  Makefile |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)

Looks fine but waiting for others to review in case there is a subtlety
that affects some build environments.

Stefan

Re: [Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Avoid having an explicit list of directories in the 'clean'
> target by using 'find' to remove all .o and .d files instead.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I figured that (unlike Makefile.target) we should probably take
> the xargs route here since otherwise the rm command line is huge...
>
> There's also an argument that there's not much point having a clean
> target in Makefile.target when this one blows away most of it anyway.
>
>  Makefile |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 621cb86..9f7eaa8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -212,13 +212,10 @@ 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 *.o *.d *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
> +	find . -name '*.[od]' | xargs rm -f
> +	rm -f *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~

Shit happens if you somehow manage to create a "mean" file name in the
build tree.  Sure you don't want to -print0 | xargs -0?

>  	rm -Rf .libs
> -	rm -f slirp/*.o slirp/*.d audio/*.o audio/*.d block/*.o block/*.d net/*.o net/*.d fsdev/*.o fsdev/*.d ui/*.o ui/*.d qapi/*.o qapi/*.d qga/*.o qga/*.d
> -	rm -f qom/*.o qom/*.d
> -	rm -f usb/*.o usb/*.d hw/*.o hw/*.d
>  	rm -f qemu-img-cmds.h
> -	rm -f trace/*.o trace/*.d
>  	rm -f trace-dtrace.dtrace trace-dtrace.dtrace-timestamp
>  	@# May not be present in GENERATED_HEADERS
>  	rm -f trace-dtrace.h trace-dtrace.h-timestamp

Re: [Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Peter Maydell]

On 31 July 2012 15:19, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> +     find . -name '*.[od]' | xargs rm -f
>> +     rm -f *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
>
> Shit happens if you somehow manage to create a "mean" file name in the
> build tree.  Sure you don't want to -print0 | xargs -0?

-print0 isn't POSIX, so I wasn't sure it would be present on all
our platforms. (It's probably fairly safe, though...)

-- PMM

Re: [Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 31 July 2012 15:19, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> +     find . -name '*.[od]' | xargs rm -f
>>> + rm -f *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod
>>> *~ */*~
>>
>> Shit happens if you somehow manage to create a "mean" file name in the
>> build tree.  Sure you don't want to -print0 | xargs -0?
>
> -print0 isn't POSIX, so I wasn't sure it would be present on all
> our platforms. (It's probably fairly safe, though...)

Another option is "-exec rm {} +".

Re: [Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Daniel P. Berrange]

On Tue, Jul 31, 2012 at 04:35:42PM +0200, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On 31 July 2012 15:19, Markus Armbruster <armbru@redhat.com> wrote:
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >>> +     find . -name '*.[od]' | xargs rm -f
> >>> + rm -f *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod
> >>> *~ */*~
> >>
> >> Shit happens if you somehow manage to create a "mean" file name in the
> >> build tree.  Sure you don't want to -print0 | xargs -0?
> >
> > -print0 isn't POSIX, so I wasn't sure it would be present on all
> > our platforms. (It's probably fairly safe, though...)
> 
> Another option is "-exec rm {} +".

Isn't using 'find' somewhat overkill here really. QEMU only creates
.o and .d files in 2 levels of directory, so sure we can just avoid
find entirely

  rm -f *.[od] */*.[od]

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Peter Maydell]

On 31 July 2012 15:38, Daniel P. Berrange <berrange@redhat.com> wrote:
> Isn't using 'find' somewhat overkill here really. QEMU only creates
> .o and .d files in 2 levels of directory, so sure we can just avoid
> find entirely
>
>   rm -f *.[od] */*.[od]

That's exactly the bug this change is addressing (in a more
general way than a couple of the proposed point fixes). There
are subdirectories of hw/, so for instance we have a hw/usb/bus.o
which your rm would not delete.

-- PMM

Re: [Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Stefan Weil]

Am 31.07.2012 17:00, schrieb Peter Maydell:
> On 31 July 2012 15:38, Daniel P. Berrange <berrange@redhat.com> wrote:
>> Isn't using 'find' somewhat overkill here really. QEMU only creates
>> .o and .d files in 2 levels of directory, so sure we can just avoid
>> find entirely
>>
>>    rm -f *.[od] */*.[od]
>
> That's exactly the bug this change is addressing (in a more
> general way than a couple of the proposed point fixes). There
> are subdirectories of hw/, so for instance we have a hw/usb/bus.o
> which your rm would not delete.
>
> -- PMM

Yes, QEMU creates files in 3 levels. We could use

     rm -f *.[od] */*.[od] */*/*.[od]

I suggest using the wrapper $(call quiet-command,...)
to suppress printing of all removed file names.

For me, your patch using find would also be sufficient.

Only developers use "make clean", and they should know
how to name files. Even if there are files with blanks
in their name, in most cases nothing bad will happen
(as long as they are not named "-rf .. .o" which would
be the worst scenario I can imagine).

Regards,
Stefan W.

Re: [Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Peter Maydell]

On 31 July 2012 16:49, Stefan Weil <sw@weilnetz.de> wrote:
> Yes, QEMU creates files in 3 levels. We could use
>
>     rm -f *.[od] */*.[od] */*/*.[od]
>
> I suggest using the wrapper $(call quiet-command,...)
> to suppress printing of all removed file names.

My worry was not so much what we print as that we
might be exceeding the total command line length
limits on some systems (whether you did it this way
or via "rm -f $(find ...)").

-- PMM

Re: [Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Stefan Weil]

Am 31.07.2012 17:51, schrieb Peter Maydell:
> On 31 July 2012 16:49, Stefan Weil <sw@weilnetz.de> wrote:
>> Yes, QEMU creates files in 3 levels. We could use
>>
>>      rm -f *.[od] */*.[od] */*/*.[od]
>>
>> I suggest using the wrapper $(call quiet-command,...)
>> to suppress printing of all removed file names.
>
> My worry was not so much what we print as that we
> might be exceeding the total command line length
> limits on some systems (whether you did it this way
> or via "rm -f $(find ...)").
>
> -- PMM

That's quite possible:

$ find -name "*.[od]"|wc
    4862    4862  151329
$ ls *.[od] */*.[od] */*/*.[od] */*/*/*.[od] */*/*/*/*.[od]|wc
    4862    4862  141605

(I was somewhat surprised to see that we use 5 levels of directories).

The command line would be more than 140000 characters which is indeed 
very long.

Separating .o and .d files and the levels reduces that number:

$ ls *.o|wc
      80      80     952
$ ls */*.o|wc
     819     819   20048
$ ls */*/*.o|wc
    1406    1406   45573
$ ls */*/*/*.o|wc
      87      87    2579
$ ls */*/*/*/*.o|wc
      16      16     796

The 2nd and the 3rd level are potentially critical, but they could be
split up further if needed.

What about removing support for in-tree builds?
For out-of-tree builds 'make distclean' is nearly trivial.

-- Stefan W.

Re: [Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Blue Swirl]

On Tue, Jul 31, 2012 at 4:20 PM, Stefan Weil <sw@weilnetz.de> wrote:
> Am 31.07.2012 17:51, schrieb Peter Maydell:
>>
>> On 31 July 2012 16:49, Stefan Weil <sw@weilnetz.de> wrote:
>>>
>>> Yes, QEMU creates files in 3 levels. We could use
>>>
>>>      rm -f *.[od] */*.[od] */*/*.[od]
>>>
>>> I suggest using the wrapper $(call quiet-command,...)
>>> to suppress printing of all removed file names.
>>
>>
>> My worry was not so much what we print as that we
>> might be exceeding the total command line length
>> limits on some systems (whether you did it this way
>> or via "rm -f $(find ...)").
>>
>> -- PMM
>
>
> That's quite possible:
>
> $ find -name "*.[od]"|wc
>    4862    4862  151329
> $ ls *.[od] */*.[od] */*/*.[od] */*/*/*.[od] */*/*/*/*.[od]|wc
>    4862    4862  141605
>
> (I was somewhat surprised to see that we use 5 levels of directories).
>
> The command line would be more than 140000 characters which is indeed very
> long.
>
> Separating .o and .d files and the levels reduces that number:
>
> $ ls *.o|wc
>      80      80     952
> $ ls */*.o|wc
>     819     819   20048
> $ ls */*/*.o|wc
>    1406    1406   45573
> $ ls */*/*/*.o|wc
>      87      87    2579
> $ ls */*/*/*/*.o|wc
>      16      16     796
>
> The 2nd and the 3rd level are potentially critical, but they could be
> split up further if needed.
>
> What about removing support for in-tree builds?
> For out-of-tree builds 'make distclean' is nearly trivial.

We could also recommend out of tree build for those hosts which have
command line restrictions (can they be detected?). Even better, always
warn if in tree build is detected.

>
> -- Stefan W.
>
>

Re: [Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

On 07/31/2012 08:19 AM, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> Avoid having an explicit list of directories in the 'clean'
>> target by using 'find' to remove all .o and .d files instead.
>>

>>  	rm -f qemu-options.def
>> -	rm -f *.o *.d *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
>> +	find . -name '*.[od]' | xargs rm -f
>> +	rm -f *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
> 
> Shit happens if you somehow manage to create a "mean" file name in the
> build tree.  Sure you don't want to -print0 | xargs -0?

Except that 'find -print0' and 'xargs -0' are both GNU extensions, not
available everywhere.  We may be requiring gmake and gcc, but are we
also requiring GNU find?

The POSIX way to write this, without relying on extensions, is:

find . -name '*.[od]' -exec rm -f {} +

(although then you get into the arguments of whether 'find -exec {} +'
is portable yet, even though it has now been required by POSIX for more
than 4 years.)

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH] Makefile: Avoid explicit list of directories in clean target

[Stefan Hajnoczi]

On Tue, Jul 31, 2012 at 08:24:58AM -0600, Eric Blake wrote:
> On 07/31/2012 08:19 AM, Markus Armbruster wrote:
> > Peter Maydell <peter.maydell@linaro.org> writes:
> > 
> >> Avoid having an explicit list of directories in the 'clean'
> >> target by using 'find' to remove all .o and .d files instead.
> >>
> 
> >>  	rm -f qemu-options.def
> >> -	rm -f *.o *.d *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
> >> +	find . -name '*.[od]' | xargs rm -f
> >> +	rm -f *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
> > 
> > Shit happens if you somehow manage to create a "mean" file name in the
> > build tree.  Sure you don't want to -print0 | xargs -0?
> 
> Except that 'find -print0' and 'xargs -0' are both GNU extensions, not
> available everywhere.  We may be requiring gmake and gcc, but are we
> also requiring GNU find?
> 
> The POSIX way to write this, without relying on extensions, is:
> 
> find . -name '*.[od]' -exec rm -f {} +
> 
> (although then you get into the arguments of whether 'find -exec {} +'
> is portable yet, even though it has now been required by POSIX for more
> than 4 years.)

This portable approach seems reasonable.  I have checked the find(1) man
page on: FreeBSD, OpenBSD, Solaris 11, Mac OS X.

Let's give find -exec + a shot.  If it breaks something then the folks
who care can provide a buildslave to ensure their host platform
continues to be supported in the future.

Stefan