{"thread":{"id":"64496","subject":"[PATCH] make strip: include `scalar`","startedAt":"2025-11-17T19:51:28Z","lastAt":"2025-12-01T07:58:47Z","messageCount":5,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"530833","messageId":"pull.2004.git.1763409086322.gitgitgadget@gmail.com","threadId":"64496","inReplyTo":null,"subject":"[PATCH] make strip: include `scalar`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-11-17T19:51:26Z","receivedAt":"2025-11-17T19:51:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen Scalar was made a canonical part of Git in 7b5c93c6c68 (scalar:\ninclude in standard Git build & installation, 2022-09-02), it was added\nto all relevant Makefile targets except for the `strip` target.\n\nLet's correct that.\n\nSigned-off-by: Johannes Schindelin \n---\n make strip: include scalar\n \n This is something I noticed while working on aligning Git for Windows\n better with MSYS2.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2004%2Fdscho%2Finclude-scalar-in-the-strip-Makefile-target-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2004/dscho/include-scalar-in-the-strip-Makefile-target-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2004\n\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 7e0f77e298..62f7f7bf56 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2565,7 +2565,7 @@ please_set_SHELL_PATH_to_a_more_modern_shell:\n \n shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n \n-strip: $(PROGRAMS) git$X\n+strip: $(PROGRAMS) git$X scalar$X\n \t$(STRIP) $(STRIP_OPTS) $^\n \n ### Target-specific flags and dependencies\n\nbase-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed\n-- \ngitgitgadget\n"},{"id":"530840","messageId":"xmqq7bvoiadg.fsf@gitster.g","threadId":"64496","inReplyTo":"pull.2004.git.1763409086322.gitgitgadget@gmail.com","subject":"Re: [PATCH] make strip: include `scalar`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-17T22:04:43Z","receivedAt":"2025-11-17T22:04:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When Scalar was made a canonical part of Git in 7b5c93c6c68 (scalar:\n> include in standard Git build & installation, 2022-09-02), it was added\n> to all relevant Makefile targets except for the `strip` target.\n>\n> Let's correct that.\n\nThe motivation makes perfect sense.\n\n> diff --git a/Makefile b/Makefile\n> index 7e0f77e298..62f7f7bf56 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2565,7 +2565,7 @@ please_set_SHELL_PATH_to_a_more_modern_shell:\n> \n> shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n> \n> -strip: $(PROGRAMS) git$X\n> +strip: $(PROGRAMS) git$X scalar$X\n> \t$(STRIP) $(STRIP_OPTS) $^\n\nI wonder why the original names git$X here explicitly, instead of\nusing say $(OTHER_PROGRAMS) that covers both of these. I know that\nthe undocumented INCLUDE_DLLS_IN_ARTIFACTS knob uses OTHER_PROGRAMS\nby throwing in non-programs like DLLs to it, so that artifacts-tar\ntarget would include them, but perhaps instead of working around the\nmisdesign of that target, wouldn't it be better to correct its use\nof OTHER_PROGRAMS and use it here instead?\n\nThe change (including the \"strip scalar, too!\" part) should look\nlike this, I think.\n\nAlso do we need a matching change to CMake and meson?\n\n Makefile | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git c/Makefile w/Makefile\nindex 70d1543b6b..a63a4adbc7 100644\n--- c/Makefile\n+++ w/Makefile\n@@ -682,6 +682,7 @@ LIB_OBJS =\n LIBGIT_PUB_OBJS =\n SCALAR_OBJS =\n OBJECTS =\n+OTHER_ARTIFACTS =\n OTHER_PROGRAMS =\n PROGRAM_OBJS =\n PROGRAMS =\n@@ -2499,7 +2500,7 @@ please_set_SHELL_PATH_to_a_more_modern_shell:\n \n shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n \n-strip: $(PROGRAMS) git$X\n+strip: $(PROGRAMS) $(OTHER_PROGRAMS)\n \t$(STRIP) $(STRIP_OPTS) $^\n \n ### Target-specific flags and dependencies\n@@ -3697,10 +3698,11 @@ rpm::\n .PHONY: rpm\n \n ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)\n-OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll t/unit-tests/bin/*.dll)\n+OTHER_ARTIFACTS += $(shell echo *.dll t/helper/*.dll t/unit-tests/bin/*.dll)\n endif\n \n artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \\\n+\t\t$(OTHER_ARTIFACTS) \\\n \t\tGIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \\\n \t\t$(UNIT_TEST_PROGS) $(CLAR_TEST_PROG) $(MOFILES)\n \t$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \\\n\n"},{"id":"531269","messageId":"235775ef-d12f-4b19-0b80-672c4e5e1812@gmx.de","threadId":"64496","inReplyTo":"xmqq7bvoiadg.fsf@gitster.g","subject":"Re: [PATCH] make strip: include `scalar`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-11-25T17:47:42Z","receivedAt":"2025-11-25T17:47:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 17 Nov 2025, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > From: Johannes Schindelin \n> >\n> > When Scalar was made a canonical part of Git in 7b5c93c6c68 (scalar:\n> > include in standard Git build & installation, 2022-09-02), it was added\n> > to all relevant Makefile targets except for the `strip` target.\n> >\n> > Let's correct that.\n> \n> The motivation makes perfect sense.\n> \n> > diff --git a/Makefile b/Makefile\n> > index 7e0f77e298..62f7f7bf56 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -2565,7 +2565,7 @@ please_set_SHELL_PATH_to_a_more_modern_shell:\n> > \n> > shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n> > \n> > -strip: $(PROGRAMS) git$X\n> > +strip: $(PROGRAMS) git$X scalar$X\n> > \t$(STRIP) $(STRIP_OPTS) $^\n> \n> I wonder why the original names git$X here explicitly, instead of\n> using say $(OTHER_PROGRAMS) that covers both of these. I know that\n> the undocumented INCLUDE_DLLS_IN_ARTIFACTS knob uses OTHER_PROGRAMS\n> by throwing in non-programs like DLLs to it, so that artifacts-tar\n> target would include them, but perhaps instead of working around the\n> misdesign of that target, wouldn't it be better to correct its use\n> of OTHER_PROGRAMS and use it here instead?\n> \n> The change (including the \"strip scalar, too!\" part) should look\n> like this, I think.\n\nSure.\n\n> Also do we need a matching change to CMake and meson?\n\nI am unfamiliar with Meson, and do not see anything about stripping in\n`meson.build` apart from a `--strip` option that is mentioned in a comment\n(and which I would assume already handles all executables, otherwise the\nmove to Meson really is not worth all the hassle).\n\nAbout CMake: It was always meant as a tool to help Visual Studio users to\nbuild and debug Git for Windows conveniently (something that Meson\ndistinctly fails to accomplish). As such, there is no support for\nstripping executables in the CMake definition, that's completely up to how\nthe Release builds are set up.\n\nBesides, since Meson was picked over CMake as the modern build setup, I am\nseriously playing with the idea of abandoning Git's CMake definition (and\nwith that, all Visual Studio-based developers, of course).\n\nCiao,\nJohannes\n\n> \n> Makefile | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git c/Makefile w/Makefile\n> index 70d1543b6b..a63a4adbc7 100644\n> --- c/Makefile\n> +++ w/Makefile\n> @@ -682,6 +682,7 @@ LIB_OBJS =\n> LIBGIT_PUB_OBJS =\n> SCALAR_OBJS =\n> OBJECTS =\n> +OTHER_ARTIFACTS =\n> OTHER_PROGRAMS =\n> PROGRAM_OBJS =\n> PROGRAMS =\n> @@ -2499,7 +2500,7 @@ please_set_SHELL_PATH_to_a_more_modern_shell:\n> \n> shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n> \n> -strip: $(PROGRAMS) git$X\n> +strip: $(PROGRAMS) $(OTHER_PROGRAMS)\n> \t$(STRIP) $(STRIP_OPTS) $^\n> \n> ### Target-specific flags and dependencies\n> @@ -3697,10 +3698,11 @@ rpm::\n> .PHONY: rpm\n> \n> ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)\n> -OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll t/unit-tests/bin/*.dll)\n> +OTHER_ARTIFACTS += $(shell echo *.dll t/helper/*.dll t/unit-tests/bin/*.dll)\n> endif\n> \n> artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \\\n> +\t\t$(OTHER_ARTIFACTS) \\\n> \t\tGIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \\\n> \t\t$(UNIT_TEST_PROGS) $(CLAR_TEST_PROG) $(MOFILES)\n> \t$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \\\n> \n> \n"},{"id":"531278","messageId":"xmqq4iqhraem.fsf@gitster.g","threadId":"64496","inReplyTo":"235775ef-d12f-4b19-0b80-672c4e5e1812@gmx.de","subject":"Re: [PATCH] make strip: include `scalar`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-25T22:54:09Z","receivedAt":"2025-11-25T22:54:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > -strip: $(PROGRAMS) git$X\n>> > +strip: $(PROGRAMS) git$X scalar$X\n>> > \t$(STRIP) $(STRIP_OPTS) $^\n>> \n>> I wonder why the original names git$X here explicitly, instead of\n>> using say $(OTHER_PROGRAMS) that covers both of these. I know that\n>> the undocumented INCLUDE_DLLS_IN_ARTIFACTS knob uses OTHER_PROGRAMS\n>> by throwing in non-programs like DLLs to it, so that artifacts-tar\n>> target would include them, but perhaps instead of working around the\n>> misdesign of that target, wouldn't it be better to correct its use\n>> of OTHER_PROGRAMS and use it here instead?\n>> \n>> The change (including the \"strip scalar, too!\" part) should look\n>> like this, I think.\n>\n> Sure.\n>\n>> Also do we need a matching change to CMake and meson?\n>\n> I am unfamiliar with Meson, and do not see anything about stripping in\n> `meson.build` apart from a `--strip` option that is mentioned in a comment\n> (and which I would assume already handles all executables, otherwise the\n> move to Meson really is not worth all the hassle).\n\nThat's a great point.\n\nAnyway, the original patch that started this thread is not wrong, so\nlet me queue it as-is. Those who want to improve on it can build on\ntop.\n\nThanks.\n"},{"id":"531484","messageId":"aS1KqrTRxHSGDDZY@pks.im","threadId":"64496","inReplyTo":"xmqq4iqhraem.fsf@gitster.g","subject":"Re: [PATCH] make strip: include `scalar`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-01T07:58:34Z","receivedAt":"2025-12-01T07:58:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 25, 2025 at 02:54:09PM -0800, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n> >> > -strip: $(PROGRAMS) git$X\n> >> > +strip: $(PROGRAMS) git$X scalar$X\n> >> > \t$(STRIP) $(STRIP_OPTS) $^\n> >> \n> >> I wonder why the original names git$X here explicitly, instead of\n> >> using say $(OTHER_PROGRAMS) that covers both of these. I know that\n> >> the undocumented INCLUDE_DLLS_IN_ARTIFACTS knob uses OTHER_PROGRAMS\n> >> by throwing in non-programs like DLLs to it, so that artifacts-tar\n> >> target would include them, but perhaps instead of working around the\n> >> misdesign of that target, wouldn't it be better to correct its use\n> >> of OTHER_PROGRAMS and use it here instead?\n> >> \n> >> The change (including the \"strip scalar, too!\" part) should look\n> >> like this, I think.\n> >\n> > Sure.\n> >\n> >> Also do we need a matching change to CMake and meson?\n> >\n> > I am unfamiliar with Meson, and do not see anything about stripping in\n> > `meson.build` apart from a `--strip` option that is mentioned in a comment\n> > (and which I would assume already handles all executables, otherwise the\n> > move to Meson really is not worth all the hassle).\n> \n> That's a great point.\n> \n> Anyway, the original patch that started this thread is not wrong, so\n> let me queue it as-is. Those who want to improve on it can build on\n> top.\n\nYup, Dscho is exactly right. Meson handles stripping natively via the\n`--strip` option that you can pass at setup time. If so, it knows to\nstrip all binaries when installing.\n\nSo there's nothing we need to do for Meson, thanks!\n\nPatrick\n"}]}