threads / patch / 56978

v2Makefile: error out invoking strip target

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

## tl;dr

3 messages between Nov 25, 2021 and Nov 26, 2021. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Bagas Sanjaya· Nov 25, 2021, 12:26 UTC · lore

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

Show changes to Makefile +3 −0

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

Junio C Hamano· Nov 26, 2021, 07:29 UTC · re: Bagas Sanjaya · lore

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

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.

Bagas Sanjaya· Nov 26, 2021, 07:38 UTC · re: Junio C Hamano · lore

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

On 26/11/21 14.29, Junio C Hamano wrote:

Show 15 quoted lines

> 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