[PATCH] make strip: include `scalar`

[PATCH] make strip: include `scalar`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When Scalar was made a canonical part of Git in 7b5c93c6c68 (scalar:
include in standard Git build & installation, 2022-09-02), it was added
to all relevant Makefile targets except for the `strip` target.

Let's correct that.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    make strip: include scalar
    
    This is something I noticed while working on aligning Git for Windows
    better with MSYS2.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2004%2Fdscho%2Finclude-scalar-in-the-strip-Makefile-target-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2004/dscho/include-scalar-in-the-strip-Makefile-target-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2004

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 7e0f77e298..62f7f7bf56 100644
--- a/Makefile
+++ b/Makefile
@@ -2565,7 +2565,7 @@ 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
+strip: $(PROGRAMS) git$X scalar$X
 	$(STRIP) $(STRIP_OPTS) $^
 
 ### Target-specific flags and dependencies

base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed
-- 
gitgitgadget

Re: [PATCH] make strip: include `scalar`

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When Scalar was made a canonical part of Git in 7b5c93c6c68 (scalar:
> include in standard Git build & installation, 2022-09-02), it was added
> to all relevant Makefile targets except for the `strip` target.
>
> Let's correct that.

The motivation makes perfect sense.

> diff --git a/Makefile b/Makefile
> index 7e0f77e298..62f7f7bf56 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2565,7 +2565,7 @@ 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
> +strip: $(PROGRAMS) git$X scalar$X
>  	$(STRIP) $(STRIP_OPTS) $^

I wonder why the original names git$X here explicitly, instead of
using say $(OTHER_PROGRAMS) that covers both of these.  I know that
the undocumented INCLUDE_DLLS_IN_ARTIFACTS knob uses OTHER_PROGRAMS
by throwing in non-programs like DLLs to it, so that artifacts-tar
target would include them, but perhaps instead of working around the
misdesign of that target, wouldn't it be better to correct its use
of OTHER_PROGRAMS and use it here instead?

The change (including the "strip scalar, too!" part) should look
like this, I think.

Also do we need a matching change to CMake and meson?

 Makefile | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git c/Makefile w/Makefile
index 70d1543b6b..a63a4adbc7 100644
--- c/Makefile
+++ w/Makefile
@@ -682,6 +682,7 @@ LIB_OBJS =
 LIBGIT_PUB_OBJS =
 SCALAR_OBJS =
 OBJECTS =
+OTHER_ARTIFACTS =
 OTHER_PROGRAMS =
 PROGRAM_OBJS =
 PROGRAMS =
@@ -2499,7 +2500,7 @@ 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
+strip: $(PROGRAMS) $(OTHER_PROGRAMS)
 	$(STRIP) $(STRIP_OPTS) $^
 
 ### Target-specific flags and dependencies
@@ -3697,10 +3698,11 @@ rpm::
 .PHONY: rpm
 
 ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
-OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll t/unit-tests/bin/*.dll)
+OTHER_ARTIFACTS += $(shell echo *.dll t/helper/*.dll t/unit-tests/bin/*.dll)
 endif
 
 artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
+		$(OTHER_ARTIFACTS) \
 		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
 		$(UNIT_TEST_PROGS) $(CLAR_TEST_PROG) $(MOFILES)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \

Re: [PATCH] make strip: include `scalar`

[Johannes Schindelin]

Hi Junio,

On Mon, 17 Nov 2025, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When Scalar was made a canonical part of Git in 7b5c93c6c68 (scalar:
> > include in standard Git build & installation, 2022-09-02), it was added
> > to all relevant Makefile targets except for the `strip` target.
> >
> > Let's correct that.
> 
> The motivation makes perfect sense.
> 
> > diff --git a/Makefile b/Makefile
> > index 7e0f77e298..62f7f7bf56 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2565,7 +2565,7 @@ 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
> > +strip: $(PROGRAMS) git$X scalar$X
> >  	$(STRIP) $(STRIP_OPTS) $^
> 
> I wonder why the original names git$X here explicitly, instead of
> using say $(OTHER_PROGRAMS) that covers both of these.  I know that
> the undocumented INCLUDE_DLLS_IN_ARTIFACTS knob uses OTHER_PROGRAMS
> by throwing in non-programs like DLLs to it, so that artifacts-tar
> target would include them, but perhaps instead of working around the
> misdesign of that target, wouldn't it be better to correct its use
> of OTHER_PROGRAMS and use it here instead?
> 
> The change (including the "strip scalar, too!" part) should look
> like this, I think.

Sure.

> Also do we need a matching change to CMake and meson?

I am unfamiliar with Meson, and do not see anything about stripping in
`meson.build` apart from a `--strip` option that is mentioned in a comment
(and which I would assume already handles all executables, otherwise the
move to Meson really is not worth all the hassle).

About CMake: It was always meant as a tool to help Visual Studio users to
build and debug Git for Windows conveniently (something that Meson
distinctly fails to accomplish). As such, there is no support for
stripping executables in the CMake definition, that's completely up to how
the Release builds are set up.

Besides, since Meson was picked over CMake as the modern build setup, I am
seriously playing with the idea of abandoning Git's CMake definition (and
with that, all Visual Studio-based developers, of course).

Ciao,
Johannes

> 
>  Makefile | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git c/Makefile w/Makefile
> index 70d1543b6b..a63a4adbc7 100644
> --- c/Makefile
> +++ w/Makefile
> @@ -682,6 +682,7 @@ LIB_OBJS =
>  LIBGIT_PUB_OBJS =
>  SCALAR_OBJS =
>  OBJECTS =
> +OTHER_ARTIFACTS =
>  OTHER_PROGRAMS =
>  PROGRAM_OBJS =
>  PROGRAMS =
> @@ -2499,7 +2500,7 @@ 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
> +strip: $(PROGRAMS) $(OTHER_PROGRAMS)
>  	$(STRIP) $(STRIP_OPTS) $^
>  
>  ### Target-specific flags and dependencies
> @@ -3697,10 +3698,11 @@ rpm::
>  .PHONY: rpm
>  
>  ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
> -OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll t/unit-tests/bin/*.dll)
> +OTHER_ARTIFACTS += $(shell echo *.dll t/helper/*.dll t/unit-tests/bin/*.dll)
>  endif
>  
>  artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
> +		$(OTHER_ARTIFACTS) \
>  		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
>  		$(UNIT_TEST_PROGS) $(CLAR_TEST_PROG) $(MOFILES)
>  	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
> 
> 

Re: [PATCH] make strip: include `scalar`

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > -strip: $(PROGRAMS) git$X
>> > +strip: $(PROGRAMS) git$X scalar$X
>> >  	$(STRIP) $(STRIP_OPTS) $^
>> 
>> I wonder why the original names git$X here explicitly, instead of
>> using say $(OTHER_PROGRAMS) that covers both of these.  I know that
>> the undocumented INCLUDE_DLLS_IN_ARTIFACTS knob uses OTHER_PROGRAMS
>> by throwing in non-programs like DLLs to it, so that artifacts-tar
>> target would include them, but perhaps instead of working around the
>> misdesign of that target, wouldn't it be better to correct its use
>> of OTHER_PROGRAMS and use it here instead?
>> 
>> The change (including the "strip scalar, too!" part) should look
>> like this, I think.
>
> Sure.
>
>> Also do we need a matching change to CMake and meson?
>
> I am unfamiliar with Meson, and do not see anything about stripping in
> `meson.build` apart from a `--strip` option that is mentioned in a comment
> (and which I would assume already handles all executables, otherwise the
> move to Meson really is not worth all the hassle).

That's a great point.

Anyway, the original patch that started this thread is not wrong, so
let me queue it as-is.  Those who want to improve on it can build on
top.

Thanks.

Re: [PATCH] make strip: include `scalar`

[Patrick Steinhardt]

On Tue, Nov 25, 2025 at 02:54:09PM -0800, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > -strip: $(PROGRAMS) git$X
> >> > +strip: $(PROGRAMS) git$X scalar$X
> >> >  	$(STRIP) $(STRIP_OPTS) $^
> >> 
> >> I wonder why the original names git$X here explicitly, instead of
> >> using say $(OTHER_PROGRAMS) that covers both of these.  I know that
> >> the undocumented INCLUDE_DLLS_IN_ARTIFACTS knob uses OTHER_PROGRAMS
> >> by throwing in non-programs like DLLs to it, so that artifacts-tar
> >> target would include them, but perhaps instead of working around the
> >> misdesign of that target, wouldn't it be better to correct its use
> >> of OTHER_PROGRAMS and use it here instead?
> >> 
> >> The change (including the "strip scalar, too!" part) should look
> >> like this, I think.
> >
> > Sure.
> >
> >> Also do we need a matching change to CMake and meson?
> >
> > I am unfamiliar with Meson, and do not see anything about stripping in
> > `meson.build` apart from a `--strip` option that is mentioned in a comment
> > (and which I would assume already handles all executables, otherwise the
> > move to Meson really is not worth all the hassle).
> 
> That's a great point.
> 
> Anyway, the original patch that started this thread is not wrong, so
> let me queue it as-is.  Those who want to improve on it can build on
> top.

Yup, Dscho is exactly right. Meson handles stripping natively via the
`--strip` option that you can pass at setup time. If so, it knows to
strip all binaries when installing.

So there's nothing we need to do for Meson, thanks!

Patrick