make strip: include scalar
#2004
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <[email protected]>
Member
Author
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Johannes Schindelin via GitGitGadget" <[email protected]>
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 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) \
|
|
This patch series was integrated into seen via git@f090714. |
|
This patch series was integrated into seen via git@ffdecde. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@c95930b. |
|
This patch series was integrated into seen via git@f04517c. |
|
There was a status update in the "New Topics" section about the branch "make strip" has been taught to strip "scalar" as well as "git". Will merge to 'next'? cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@7a12739. |
|
This patch series was integrated into seen via git@7295f98. |
|
This patch series was integrated into seen via git@b792336. |
|
There was a status update in the "Cooking" section about the branch "make strip" has been taught to strip "scalar" as well as "git". Will merge to 'next'? cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@af6848b. |
|
This patch series was integrated into seen via git@1f3ca7c. |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Junio,
On Mon, 17 Nov 2025, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <[email protected]>
> 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 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) \
>
> |
|
This patch series was integrated into seen via git@e2b583f. |
|
This patch series was integrated into next via git@9f2607a. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Johannes Schindelin <[email protected]> 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. |
|
There was a status update in the "Cooking" section about the branch "make strip" has been taught to strip "scalar" as well as "git". Will merge to 'master'. cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@1a42108. |
|
There was a status update in the "Cooking" section about the branch "make strip" has been taught to strip "scalar" as well as "git". Will merge to 'master'. cf. <[email protected]> source: <[email protected]> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is something I noticed while working on aligning Git for Windows better with MSYS2.