Skip to content

Conversation

@dscho
Copy link
Member

@dscho dscho commented Nov 17, 2025

This is something I noticed while working on aligning Git for Windows better with MSYS2.

When Scalar was made a canonical part of Git in 7b5c93c (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>
@dscho dscho self-assigned this Nov 17, 2025
@dscho
Copy link
Member Author

dscho commented Nov 17, 2025

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 17, 2025

Submitted as pull.2004.git.1763409086322.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2004/dscho/include-scalar-in-the-strip-Makefile-target-v1

To fetch this version to local tag pr-2004/dscho/include-scalar-in-the-strip-Makefile-target-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2004/dscho/include-scalar-in-the-strip-Makefile-target-v1

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 17, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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) \

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 17, 2025

This patch series was integrated into seen via git@f090714.

@gitgitgadget gitgitgadget bot added the seen label Nov 17, 2025
@gitgitgadget
Copy link

gitgitgadget bot commented Nov 18, 2025

This patch series was integrated into seen via git@ffdecde.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 18, 2025

This branch is now known as js/strip-scalar-too.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 18, 2025

This patch series was integrated into seen via git@c95930b.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 19, 2025

This patch series was integrated into seen via git@f04517c.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 20, 2025

There was a status update in the "New Topics" section about the branch js/strip-scalar-too on the Git mailing list:

"make strip" has been taught to strip "scalar" as well as "git".

Will merge to 'next'?
cf. <xmqq7bvoiadg.fsf@gitster.g>
source: <pull.2004.git.1763409086322.gitgitgadget@gmail.com>

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 20, 2025

This patch series was integrated into seen via git@7a12739.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant