| Message ID | 1343739695-7757-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.)
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 {} +".
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
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
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.
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
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.
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. > >
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
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
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(-)