[PATCH v2] Makefile: error out invoking strip target

[PATCH v2] Makefile: error out invoking strip target

[Bagas Sanjaya]

Now that $INSTALL_STRIP variable can be defined since 3231f41009 (make:
add INSTALL_STRIP option variable, 2021-09-05), it is redundant to have
'strip' target when $INSTALL_STRIP does the job. Error out when invoking
the target so that users are forced to define the variable instead.

Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
 Changes since v1 [1]:
   - use $(error) function (suggested by Ævar)
   - message rewording (suggested by Ævar)

 [1]:
https://lore.kernel.org/git/211123.86a6hvuikj.gmgdl@evledraar.gmail.com/T/#u

 Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Makefile b/Makefile
index 12be39ac49..d569b5cba8 100644
--- a/Makefile
+++ b/Makefile
@@ -2159,6 +2159,9 @@ please_set_SHELL_PATH_to_a_more_modern_shell:
 shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 
 strip: $(PROGRAMS) git$X
+	$(error You are about to install stripped Git binaries using 'strip' \
+target, but it is deprecated and will be removed in future version of Git, \
+define INSTALL_STRIP instead)
 	$(STRIP) $(STRIP_OPTS) $^
 
 ### Flags affecting all rules

base-commit: 35151cf0720460a897cde9b8039af364743240e7
-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v2] Makefile: error out invoking strip target

[Junio C Hamano]

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> Now that $INSTALL_STRIP variable can be defined since 3231f41009 (make:
> add INSTALL_STRIP option variable, 2021-09-05), it is redundant to have
> 'strip' target when $INSTALL_STRIP does the job. Error out when invoking
> the target so that users are forced to define the variable instead.

It is not exactly redundant for folks who like to build and use in
place without installing.

What is the reason why we might want to eventually remove the
"strip" target, making "make strip" an error?  I do not quite see
much downsides for having just a target with a simple one-liner
recipe.

Re: [PATCH v2] Makefile: error out invoking strip target

[Bagas Sanjaya]

On 26/11/21 14.29, Junio C Hamano wrote:
> Bagas Sanjaya <bagasdotme@gmail.com> writes:
> 
>> Now that $INSTALL_STRIP variable can be defined since 3231f41009 (make:
>> add INSTALL_STRIP option variable, 2021-09-05), it is redundant to have
>> 'strip' target when $INSTALL_STRIP does the job. Error out when invoking
>> the target so that users are forced to define the variable instead.
> 
> It is not exactly redundant for folks who like to build and use in
> place without installing.
> 
> What is the reason why we might want to eventually remove the
> "strip" target, making "make strip" an error?  I do not quite see
> much downsides for having just a target with a simple one-liner
> recipe.
> 

I think we have two ways to do the same thing (installing stripped) and I want 
to push users to go with $INSTALL_STRIP instead of strip target.

Regarding deprecation, making $(warning) message instead of $(error) is better 
option, because users can still use the target (albeit it is deprecated) and 
they can update their build recipe to use $INSTALL_STRIP before we flip to 
$(error) or remove the target.

-- 
An old man doll... just what I always wanted! - Clara