Re: [PATCH] Makefile: add missing phony target

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Dec 10, 2015, 18:37 UTC
Message-ID: <vpqio46b162.fsf@anie.imag.fr>
In-Reply-To: <1449761094-37915-1-git-send-email-gitter.spiros@gmail.com>

Elia Pinto <gitter.spiros@gmail.com> writes:

> Also put the .PHONY
> declaration immediately before the target declaration, where necessary,
> for a better readability and a uniform style.

[...]

Show 15 quoted lines

> --- a/Makefile
> +++ b/Makefile
> @@ -522,11 +522,11 @@ SCRIPT_PYTHON_INS = $(filter-out $(NO_INSTALL),$(SCRIPT_PYTHON_GEN))
>  # "make -C ../.. SCRIPT_PERL=contrib/foo/bar.perl build-perl-script"
>  # from subdirectories like contrib/*/
>  .PHONY: build-perl-script build-sh-script build-python-script
> +.PHONY: install-perl-script install-sh-script install-python-script
>  build-perl-script: $(SCRIPT_PERL_GEN)
>  build-sh-script: $(SCRIPT_SH_GEN)
>  build-python-script: $(SCRIPT_PYTHON_GEN)
>  
> -.PHONY: install-perl-script install-sh-script install-python-script
>  install-sh-script: $(SCRIPT_SH_INS)
>  	$(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  install-perl-script: $(SCRIPT_PERL_INS)

Isn't this hunk doing exactly the opposite of what the commit message says? install-sh-script's .PHONY used to be right before the target, and it's now in the paragraph above.

Show 6 quoted lines

> @@ -534,7 +534,7 @@ install-perl-script: $(SCRIPT_PERL_INS)
>  install-python-script: $(SCRIPT_PYTHON_INS)
>  	$(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  
> -.PHONY: clean-perl-script clean-sh-script clean-python-script
> +.PHONY: clean-sh-script clean-perl-script  clean-python-script

double-space befroe clean-python-script.

Show 6 quoted lines

> @@ -546,7 +546,6 @@ SCRIPTS = $(SCRIPT_SH_INS) \
>  	  $(SCRIPT_PERL_INS) \
>  	  $(SCRIPT_PYTHON_INS) \
>  	  git-instaweb
> -
>  ETAGS_TARGET = TAGS

Is this needed/good?

Show 6 quoted lines

> @@ -1792,7 +1794,7 @@ GIT-PERL-DEFINES: FORCE
>  	    fi
>  
>  
> -.PHONY: gitweb
> +.PHONY: gitweb git-instaweb

No: git-instaweb is not a .PHONY target, it's a real file generated from
git-instaweb.sh.

This bug in your patch is hard to spot in its current form where you mix code movement and subtle changes. The patch cannot be reviewed in good condition as-is.

> +.PHONY: tags cscope FORCE
>  tags: FORCE
>  	$(RM) tags
>  	$(FIND_SOURCE_FILES) | xargs ctags -a

If tags depends on FORCE, it doesn't need to be .PHONY. I strongly prefer the old style, since tags is actually a file, and I prefer using .PHONY for targets that are not meant to generate files, and depend on FORCE where the rule actually generate a file named after its target, but needs to be re-ran every time it's called.

If you disagree with this, then you need to justify the change in the commit message.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/