make strip: include `scalar`
PATCH 1/1 make strip: include `scalar`
From: Johannes Schindelin <[email protected]>
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.
---
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(-)
Makefile +1 -1
@@ -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
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 7 lines · writes:
From: Johannes Schindelin <[email protected]>
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 relevantMakefiletargets except for thestriptarget.Let's correct that.
The motivation makes perfect sense.
Makefile please_set_SHELL_PATH_to_a_more_modern_shell: · line 2569
Quoted 11 lines
Makefile +1
-1@@ -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(-)
Makefile +4 -2
@@ -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) \
Hi Junio,
Quoted 33 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 7 lines · writes:
From: Johannes Schindelin <[email protected]>
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 relevantMakefiletargets except for thestriptarget.Let's correct that.
The motivation makes perfect sense.
Makefile please_set_SHELL_PATH_to_a_more_modern_shell: · line 2569
Quoted 11 lines
Makefile +1
-1@@ -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_ARTIFACTSknob usesOTHER_PROGRAMSby 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 ofOTHER_PROGRAMSand use it here instead?
Quoted 2 lines
The change (including the "strip scalar, too!" part) should look like this, I think.
Sure.
Quoted 1 line
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
Quoted 40 lines
Makefile | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)Makefile +4
-2@@ -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) \
Quoted 15 lines · Johannes Schindelin
Quoted 12 lines
Makefile please_set_SHELL_PATH_to_a_more_modern_shell: · line 2504
Quoted 3 lines
Makefile +1
-1-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_ARTIFACTSknob usesOTHER_PROGRAMSby 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 ofOTHER_PROGRAMSand use it here instead?Quoted 2 lines
The change (including the "strip scalar, too!" part) should look like this, I think.
Quoted 8 lines
Sure.
Quoted 1 line
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.buildapart from a--stripoption 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.
Quoted 26 lines · Junio C Hamano
Quoted 15 lines · Johannes Schindelin
Quoted 12 lines
Makefile please_set_SHELL_PATH_to_a_more_modern_shell: · line 2504
Quoted 3 lines
Makefile +1
-1-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_ARTIFACTSknob usesOTHER_PROGRAMSby 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 ofOTHER_PROGRAMSand use it here instead?Quoted 2 lines
The change (including the "strip scalar, too!" part) should look like this, I think.
Quoted 8 lines
Sure.
Quoted 1 line
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.buildapart from a--stripoption 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).
Quoted 5 lines
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