| Message ID | 1394786300-18017-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 14, 2014 at 04:38:20PM +0800, Fam Zheng wrote: > DANGEROUS: don't try it before you read to the end. > > A first "make distclean" will unset $(DSOSUF), a following "make > distclean" or "make clean" will find all the files and delete it. > > Including all the files in the .git directory! > > Fix it by only do it when $(DSOSUF) is not empty. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Am 14.03.2014 09:38, schrieb Fam Zheng: > DANGEROUS: don't try it before you read to the end. > > A first "make distclean" will unset $(DSOSUF), a following "make > distclean" or "make clean" will find all the files and delete it. > > Including all the files in the .git directory! If you only use out-of-tree build, you are safe here. Maybe we should no longer support in-tree builds. Personally, I nearly never use them. > Fix it by only do it when $(DSOSUF) is not empty. s/do/doing/ > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index bd9cd4f..0666d6e 100644 > --- a/Makefile > +++ b/Makefile > @@ -267,7 +267,7 @@ clean: > rm -f qemu-options.def > find . -name '*.[oda]' -type f -exec rm -f {} + > find . -name '*.l[oa]' -type f -exec rm -f {} + > - find . -name '*$(DSOSUF)' -type f -exec rm -f {} + > + if test -n "$(DSOSUF)"; then find . -name '*$(DSOSUF)' -type f -exec rm -f {} +; fi > find . -name '*.mo' -type f -exec rm -f {} + > rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~ > rm -f fsdev/*.pod No, I think it is still too dangerous to use a macro here. There are only two valid possibilities, so it's easy to name them explicitly: find -name "*.dll" -o -name "*.so" Is there a good reason why rm is called with option -f? Normally, -f should not be needed when cleaning generated files because those files are not write protected. The only other reason for -f would be suppressing an error message if rm tries to remove a non existing files, but that does not apply here. I'd also combine all find statements in a single statement. It is not necessary to parse the directory tree several times. That can be done in a separate patch. Regards, Stefan
On Fri, 03/14 18:49, Stefan Weil wrote: > Am 14.03.2014 09:38, schrieb Fam Zheng: > > DANGEROUS: don't try it before you read to the end. > > > > A first "make distclean" will unset $(DSOSUF), a following "make > > distclean" or "make clean" will find all the files and delete it. > > > > Including all the files in the .git directory! > > If you only use out-of-tree build, you are safe here. Maybe we should no > longer support in-tree builds. Personally, I nearly never use them. > > > Fix it by only do it when $(DSOSUF) is not empty. > > s/do/doing/ > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Makefile b/Makefile > > index bd9cd4f..0666d6e 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -267,7 +267,7 @@ clean: > > rm -f qemu-options.def > > find . -name '*.[oda]' -type f -exec rm -f {} + > > find . -name '*.l[oa]' -type f -exec rm -f {} + > > - find . -name '*$(DSOSUF)' -type f -exec rm -f {} + > > + if test -n "$(DSOSUF)"; then find . -name '*$(DSOSUF)' -type f -exec rm -f {} +; fi > > find . -name '*.mo' -type f -exec rm -f {} + > > rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~ > > rm -f fsdev/*.pod > > No, I think it is still too dangerous to use a macro here. There are > only two valid possibilities, so it's easy to name them explicitly: > > find -name "*.dll" -o -name "*.so" > > Is there a good reason why rm is called with option -f? Normally, -f > should not be needed when cleaning generated files because those files > are not write protected. The only other reason for -f would be > suppressing an error message if rm tries to remove a non existing files, > but that does not apply here. > > I'd also combine all find statements in a single statement. It is not > necessary to parse the directory tree several times. That can be done in > a separate patch. > Sounds good to me. I'll respin. Thanks, Fam
Stefan Weil <sw@weilnetz.de> writes: > Am 14.03.2014 09:38, schrieb Fam Zheng: >> DANGEROUS: don't try it before you read to the end. >> >> A first "make distclean" will unset $(DSOSUF), a following "make >> distclean" or "make clean" will find all the files and delete it. >> >> Including all the files in the .git directory! > > If you only use out-of-tree build, you are safe here. Maybe we should no > longer support in-tree builds. Personally, I nearly never use them. Same here. Building in-tree is calling for trouble. I'd support a patch that prevents it. [...]
Am 17.03.2014 09:30, schrieb Markus Armbruster: > Stefan Weil <sw@weilnetz.de> writes: > >> Am 14.03.2014 09:38, schrieb Fam Zheng: >>> DANGEROUS: don't try it before you read to the end. >>> >>> A first "make distclean" will unset $(DSOSUF), a following "make >>> distclean" or "make clean" will find all the files and delete it. >>> >>> Including all the files in the .git directory! >> >> If you only use out-of-tree build, you are safe here. Maybe we should no >> longer support in-tree builds. Personally, I nearly never use them. > > Same here. Building in-tree is calling for trouble. I'd support a > patch that prevents it. What about disabling it only when .git/ is available? There seems nothing wrong with doing one-time builds inside an extracted tarball on central build servers; most of the problems arise when building inside the developer's git checkout. If we do drop support for building in-tree (always or with Git), we could also drop most of the .gitignore clutter. :) Cheers, Andreas
On 17 March 2014 18:51, Andreas Färber <afaerber@suse.de> wrote: > Am 17.03.2014 09:30, schrieb Markus Armbruster: >> Stefan Weil <sw@weilnetz.de> writes: >> >>> Am 14.03.2014 09:38, schrieb Fam Zheng: >>>> DANGEROUS: don't try it before you read to the end. >>>> >>>> A first "make distclean" will unset $(DSOSUF), a following "make >>>> distclean" or "make clean" will find all the files and delete it. >>>> >>>> Including all the files in the .git directory! >>> >>> If you only use out-of-tree build, you are safe here. Maybe we should no >>> longer support in-tree builds. Personally, I nearly never use them. >> >> Same here. Building in-tree is calling for trouble. I'd support a >> patch that prevents it. > > What about disabling it only when .git/ is available? The other idea that's been suggested in the past is to have the makefile/configure simply map attempts to build in-tree into 'create builddir and build in it'. This is probably easier to suggest than to get right in all situations :-) thanks -- PMM
Andreas Färber <afaerber@suse.de> writes: > Am 17.03.2014 09:30, schrieb Markus Armbruster: >> Stefan Weil <sw@weilnetz.de> writes: >> >>> Am 14.03.2014 09:38, schrieb Fam Zheng: >>>> DANGEROUS: don't try it before you read to the end. >>>> >>>> A first "make distclean" will unset $(DSOSUF), a following "make >>>> distclean" or "make clean" will find all the files and delete it. >>>> >>>> Including all the files in the .git directory! >>> >>> If you only use out-of-tree build, you are safe here. Maybe we should no >>> longer support in-tree builds. Personally, I nearly never use them. >> >> Same here. Building in-tree is calling for trouble. I'd support a >> patch that prevents it. > > What about disabling it only when .git/ is available? There seems > nothing wrong with doing one-time builds inside an extracted tarball on > central build servers; Except for this one: in-tree build will rot even faster. > most of the problems arise when building inside > the developer's git checkout. > > If we do drop support for building in-tree (always or with Git), we > could also drop most of the .gitignore clutter. :) Bonus!
Peter Maydell <peter.maydell@linaro.org> writes: > On 17 March 2014 18:51, Andreas Färber <afaerber@suse.de> wrote: >> Am 17.03.2014 09:30, schrieb Markus Armbruster: >>> Stefan Weil <sw@weilnetz.de> writes: >>> >>>> Am 14.03.2014 09:38, schrieb Fam Zheng: >>>>> DANGEROUS: don't try it before you read to the end. >>>>> >>>>> A first "make distclean" will unset $(DSOSUF), a following "make >>>>> distclean" or "make clean" will find all the files and delete it. >>>>> >>>>> Including all the files in the .git directory! >>>> >>>> If you only use out-of-tree build, you are safe here. Maybe we should no >>>> longer support in-tree builds. Personally, I nearly never use them. >>> >>> Same here. Building in-tree is calling for trouble. I'd support a >>> patch that prevents it. >> >> What about disabling it only when .git/ is available? > > The other idea that's been suggested in the past is to > have the makefile/configure simply map attempts to build > in-tree into 'create builddir and build in it'. This > is probably easier to suggest than to get right in all > situations :-) Perfect is the enemy of good enough. all: %: force @$(MAKE) -C bld $@ force: ;
diff --git a/Makefile b/Makefile index bd9cd4f..0666d6e 100644 --- a/Makefile +++ b/Makefile @@ -267,7 +267,7 @@ clean: rm -f qemu-options.def find . -name '*.[oda]' -type f -exec rm -f {} + find . -name '*.l[oa]' -type f -exec rm -f {} + - find . -name '*$(DSOSUF)' -type f -exec rm -f {} + + if test -n "$(DSOSUF)"; then find . -name '*$(DSOSUF)' -type f -exec rm -f {} +; fi find . -name '*.mo' -type f -exec rm -f {} + rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~ rm -f fsdev/*.pod
DANGEROUS: don't try it before you read to the end. A first "make distclean" will unset $(DSOSUF), a following "make distclean" or "make clean" will find all the files and delete it. Including all the files in the .git directory! Fix it by only do it when $(DSOSUF) is not empty. Signed-off-by: Fam Zheng <famz@redhat.com> --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)