[Qemu-devel] [PATCH] Makefile: Don't find and delete when $(DSOSUF) is empty in "make clean"

[Qemu-devel] [PATCH] Makefile: Don't find and delete when $(DSOSUF) is empty in "make clean"

[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!

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(-)

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
-- 
1.9.0

Re: [Qemu-devel] [PATCH] Makefile: Don't find and delete when $(DSOSUF) is empty in "make clean"

[Stefan Hajnoczi]

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>

Re: [Qemu-devel] [PATCH] Makefile: Don't find and delete when $(DSOSUF) is empty in "make clean"

[Stefan Weil]

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

Re: [Qemu-devel] [PATCH] Makefile: Don't find and delete when $(DSOSUF) is empty in "make clean"

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH] Makefile: Don't find and delete when $(DSOSUF) is empty in "make clean"

[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.

[...]

Re: [Qemu-devel] [PATCH] Makefile: Don't find and delete when $(DSOSUF) is empty in "make clean"

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] Makefile: Don't find and delete when $(DSOSUF) is empty in "make clean"

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] Makefile: Don't find and delete when $(DSOSUF) is empty in "make clean"

[Markus Armbruster]

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: ;

Re: [Qemu-devel] [PATCH] Makefile: Don't find and delete when $(DSOSUF) is empty in "make clean"

[Markus Armbruster]

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!