threads / patch / 32899

patchMakefile: don't run rm without any files

Subject: [PATCH] Makefile: don't run rm without any files

## tl;dr

5 messages between Feb 13, 2013 and Feb 13, 2013. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Matt Kraai· Feb 13, 2013, 15:57 UTC · lore
From: Matt Kraai <matt.kraai@amo.abbott.com>

"rm -f -r" fails on QNX when not passed any files to remove. This breaks the clean target, since dep_dirs is empty. Avoid this by merging two rm command lines.

Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
---
 Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Show changes to Makefile +1 −2
diff --git a/Makefile b/Makefile
index 5a2e02d..c2e3666 100644
--- a/Makefile
+++ b/Makefile
@@ -2414,8 +2414,7 @@ clean: profile-clean
 		builtin/*.o $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS)
-	$(RM) -r bin-wrappers
-	$(RM) -r $(dep_dirs)
+	$(RM) -r bin-wrappers $(dep_dirs)
 	$(RM) -r po/build/
 	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags cscope*
 	$(RM) -r $(GIT_TARNAME) .doc-tmp-dir

-- 
1.8.1.3.570.g3074c9d
Junio C Hamano· Feb 13, 2013, 16:51 UTC · re: Matt Kraai · lore

Re: [PATCH] Makefile: don't run rm without any files

Matt Kraai <kraai@ftbfs.org> writes:
> From: Matt Kraai <matt.kraai@amo.abbott.com>
>
> "rm -f -r" fails on QNX when not passed any files to remove.
I do not think it is limited to QNX.
> the clean target, since dep_dirs is empty.

And dep_dirs being empty under some circumstance shouldn't be limited to QNX, either.

I think your change does no harm, may be a good change if dep_dirs goes empty, but the justification is lacking. What caused your dep_dirs to become empty in the first place?

I am scratching my head because I see
    OBJECTS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
	$(XDIFF_OBJS) \
	$(VCSSVN_OBJS) \
	git.o
    dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))
Show 22 quoted lines
> Avoid this by merging two rm
> command lines.
>
> Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
> ---
>  Makefile | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 5a2e02d..c2e3666 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2414,8 +2414,7 @@ clean: profile-clean
>  		builtin/*.o $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
>  	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
>  	$(RM) $(TEST_PROGRAMS)
> -	$(RM) -r bin-wrappers
> -	$(RM) -r $(dep_dirs)
> +	$(RM) -r bin-wrappers $(dep_dirs)
>  	$(RM) -r po/build/
>  	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags cscope*
>  	$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
Matt Kraai· Feb 13, 2013, 17:00 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: don't run rm without any files

On Wed, Feb 13, 2013 at 08:51:45AM -0800, Junio C Hamano wrote:
Show 24 quoted lines
> Matt Kraai <kraai@ftbfs.org> writes:
> 
> > From: Matt Kraai <matt.kraai@amo.abbott.com>
> >
> > "rm -f -r" fails on QNX when not passed any files to remove.
> 
> I do not think it is limited to QNX.
> 
> > the clean target, since dep_dirs is empty.
> 
> And dep_dirs being empty under some circumstance shouldn't be
> limited to QNX, either.
> 
> I think your change does no harm, may be a good change if dep_dirs
> goes empty, but the justification is lacking.  What caused your
> dep_dirs to become empty in the first place?
> 
> I am scratching my head because I see
> 
>     OBJECTS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
> 	$(XDIFF_OBJS) \
> 	$(VCSSVN_OBJS) \
> 	git.o
>     dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))

I don't set COMPUTE_HEADER_DEPENDENCIES, so it defaults to "auto". The automatic detection determines that the compiler doesn't support it, so it's then set to "no". CHECK_HEADER_DEPENDENCIES isn't set either, so about 20 lines below the dep_dirs assignment you quoted, dep_dirs is cleared:

 ifneq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
 ifndef CHECK_HEADER_DEPENDENCIES
 dep_dirs =
 ...
Should I submit an updated patch with a different commit message?
Junio C Hamano· Feb 13, 2013, 20:01 UTC · re: Matt Kraai · lore

Re: [PATCH] Makefile: don't run rm without any files

Matt Kraai <kraai@ftbfs.org> writes:
Show 12 quoted lines
> I don't set COMPUTE_HEADER_DEPENDENCIES, so it defaults to "auto".
> The automatic detection determines that the compiler doesn't support
> it, so it's then set to "no".  CHECK_HEADER_DEPENDENCIES isn't set
> either, so about 20 lines below the dep_dirs assignment you quoted,
> dep_dirs is cleared:
>
>  ifneq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
>  ifndef CHECK_HEADER_DEPENDENCIES
>  dep_dirs =
>  ...
>
> Should I submit an updated patch with a different commit message?
I amended the log message like so:
commit bd9df384b16077337fffe9836c9255976b0e7b91
Author: Matt Kraai <matt.kraai@amo.abbott.com>
Date:   Wed Feb 13 07:57:48 2013 -0800
    Makefile: don't run rm without any files
    
    When COMPUTE_HEADER_DEPENDENCIES is set to "auto" and the compiler
    does not support it, $(dep_dirs) becomes empty.  "make clean" runs
    "rm -rf $(dep_dirs)", which fails in such a case.
    
    Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jonathan Nieder· Feb 13, 2013, 20:12 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: don't run rm without any files

Junio C Hamano wrote:
Show 11 quoted lines
> I amended the log message like so:
>
> commit bd9df384b16077337fffe9836c9255976b0e7b91
> Author: Matt Kraai <matt.kraai@amo.abbott.com>
> Date:   Wed Feb 13 07:57:48 2013 -0800
>
>     Makefile: don't run rm without any files
>
>     When COMPUTE_HEADER_DEPENDENCIES is set to "auto" and the compiler
>     does not support it, $(dep_dirs) becomes empty.  "make clean" runs
>     "rm -rf $(dep_dirs)", which fails in such a case.

To pedantic, that only fails on some platforms. The autoconf manual explains:

	It is not portable to invoke rm without options or operands. On the
	other hand, Posix now requires rm -f to silently succeed when there are
	no operands (useful for constructs like rm -rf $filelist without first
	checking if ‘$filelist’ was empty). But this was not always portable; at
	least NetBSD rm built before 2008 would fail with a diagnostic.
Anyway, looks like a good fix.  Thanks.

← back to recent threads