[PATCH 0/2] make: install stripped Git

[PATCH 0/2] make: install stripped Git

[Bagas Sanjaya]

This two-patch series adds convenient make target to install Git with
stripped executables (programs). The first patch adds such target
(called install-stripped), while the second one deletes the
now-redundant strip target.

Unlike previous attempts [1] and [2], stripping is done after installing
Git into installation prefix, without touching working directory where
Git is compiled. The advantage of it is unstripped programs can be
installed at the same prefix (thus overwriting already installed
stripped ones), particularly useful for debugging and development
purposes.

[1]:
https://lore.kernel.org/git/20210820105052.30631-1-bagasdotme@gmail.com/
[2]:
https://lore.kernel.org/git/20210817110728.55842-1-bagasdotme@gmail.com/

Bagas Sanjaya (2):
  make: add install-stripped target
  make: delete strip target

 Makefile | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)


base-commit: c4203212e360b25a1c69467b5a8437d45a373cac
-- 
2.25.1

[PATCH 1/2] make: add install-stripped target

[Bagas Sanjaya]

Add the target that install Git with stripped executables

The executables that are going to be stripped are all of $(PROGRAMS) and
git. Because they are installed over various directories (bin and
libexec/git-core) within installation prefix, the location of each
program needs to be found and pass it to $(STRIP) program.

Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
 Makefile | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index d1feab008f..b8a3a64422 100644
--- a/Makefile
+++ b/Makefile
@@ -3102,7 +3102,12 @@ endif
 	done && \
 	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
 
-.PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf
+install-stripped: install
+	for f in $(PROGRAMS) git$X; do \
+		find $$prefix -type f -name $$f -exec $(STRIP) $(STRIP_OPTS) {} \; ; \
+	done
+
+.PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf install-stripped
 .PHONY: quick-install-doc quick-install-man quick-install-html
 install-gitweb:
 	$(MAKE) -C gitweb install
-- 
2.25.1

Re: [PATCH 1/2] make: add install-stripped target

[Junio C Hamano]

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> +install-stripped: install
> +	for f in $(PROGRAMS) git$X; do \
> +		find $$prefix -type f -name $$f -exec $(STRIP) $(STRIP_OPTS) {} \; ; \
> +	done
> +

This sounds awfully wasteful.

The recipe for the install target knows exactly each of these
programs are installed, but yet the above is running around inside
$prefix to find them after the fact.

It also looks incorrect, too.

It is not guaranteed that $prefix does not contain any $IFS
whitespace in it, and worse yet, $prefix may not contain $bindir or
$libexecdir in it, so find may never reach these binaries.

It also depends on "strip" not to break handlinks to the same
binary.  "git" is linked to many built-in command binary like
"git-cat-file" and "git-remote-$curl" for various protocols are
installed by creating links to "git-remote-http".  It seems that the
"strip" command from GNU binutils package strips such a binary
in-place, but I do not think there is no fundamental reason to
believe that everybody else's "strip" would behave that way.

I would have expected that 'install-stripped' and 'install' targets
would run the same recipe, and when $(install_bindir_programs) are
installed in $(bindir) using $(INSTALL), we would optionally pass
the '--strip' option to the $(INSTALL) program when the recipe is
run for the install-stripped target.  All the tricky symlinking,
hardlinking and copying happens only on the result of that step, and
the strip step should happen before that, I would think.

> +.PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf install-stripped

Split the overly long line like this into two or more.

>  .PHONY: quick-install-doc quick-install-man quick-install-html
>  install-gitweb:
>  	$(MAKE) -C gitweb install

Re: [PATCH 1/2] make: add install-stripped target

[Bagas Sanjaya]

On 27/08/21 03.08, Junio C Hamano wrote:
> It also depends on "strip" not to break handlinks to the same
> binary.  "git" is linked to many built-in command binary like
> "git-cat-file" and "git-remote-$curl" for various protocols are
> installed by creating links to "git-remote-http".  It seems that the
> "strip" command from GNU binutils package strips such a binary
> in-place, but I do not think there is no fundamental reason to
> believe that everybody else's "strip" would behave that way.
> 

Maybe hardlinks?

> I would have expected that 'install-stripped' and 'install' targets
> would run the same recipe, and when $(install_bindir_programs) are
> installed in $(bindir) using $(INSTALL), we would optionally pass
> the '--strip' option to the $(INSTALL) program when the recipe is
> run for the install-stripped target.  All the tricky symlinking,
> hardlinking and copying happens only on the result of that step, and
> the strip step should happen before that, I would think.
> 

Did you mean copying recipe of 'install' to 'install-stripped' and the 
latter s/$(INSTALL)/$(INSTALL -s --strip-program="$(STRIP)"/)?

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH 1/2] make: add install-stripped target

[Đoàn Trần Công Danh]

On 2021-08-27 14:57:56+0700, Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> On 27/08/21 03.08, Junio C Hamano wrote:
> > It also depends on "strip" not to break handlinks to the same
> > binary.  "git" is linked to many built-in command binary like
> > "git-cat-file" and "git-remote-$curl" for various protocols are
> > installed by creating links to "git-remote-http".  It seems that the
> > "strip" command from GNU binutils package strips such a binary
> > in-place, but I do not think there is no fundamental reason to
> > believe that everybody else's "strip" would behave that way.
> > 
> 
> Maybe hardlinks?
> 
> > I would have expected that 'install-stripped' and 'install' targets
> > would run the same recipe, and when $(install_bindir_programs) are
> > installed in $(bindir) using $(INSTALL), we would optionally pass
> > the '--strip' option to the $(INSTALL) program when the recipe is
> > run for the install-stripped target.  All the tricky symlinking,
> > hardlinking and copying happens only on the result of that step, and
> > the strip step should happen before that, I would think.
> > 
> 
> Did you mean copying recipe of 'install' to 'install-stripped' and the
> latter s/$(INSTALL)/$(INSTALL -s --strip-program="$(STRIP)"/)?

I think Junio meant something like this:

---- 8< ----

diff --git a/Makefile b/Makefile
index 429c276058..70b7ef9ce1 100644
--- a/Makefile
+++ b/Makefile
@@ -3004,7 +3004,8 @@ mergetools_instdir = $(prefix)/$(mergetoolsdir)
 endif
 mergetools_instdir_SQ = $(subst ','\'',$(mergetools_instdir))
 
-install_bindir_programs := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) $(BINDIR_PROGRAMS_NO_X)
+install_bindir_xprograms := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X))
+install_bindir_programs := $(install_bindir_xprograms) $(BINDIR_PROGRAMS_NO_X)
 
 .PHONY: profile-install profile-fast-install
 profile-install: profile
@@ -3013,12 +3014,18 @@ profile-install: profile
 profile-fast-install: profile-fast
 	$(MAKE) install
 
-install: all
+INSTALL_OPTS :=
+
+install-strip: INSTALL_OPTS := -s --strip-program=$(STRIP)
+
+install-strip install: all
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+	$(INSTALL) $(INSTALL_OPTS) $(PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+	$(INSTALL) $(SCRIPTS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) $(INSTALL_OPTS) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'
 ifdef MSVC
 	# We DO NOT install the individual foo.o.pdb files because they
 	# have already been rolled up into the exe's pdb file.
---- >8 -----

-- 
Danh

[PATCH 2/2] make: delete strip target

[Bagas Sanjaya]

The target isn't needed anymore since stripping is done in install-strip
target (in previous patch).

Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
 Makefile | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Makefile b/Makefile
index b8a3a64422..027b052a0c 100644
--- a/Makefile
+++ b/Makefile
@@ -2170,8 +2170,6 @@ 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) $(STRIP_OPTS) $^
 
 ### Flags affecting all rules
 
-- 
2.25.1

Re: [PATCH 2/2] make: delete strip target

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 26 2021, Bagas Sanjaya wrote:

> The target isn't needed anymore since stripping is done in install-strip
> target (in previous patch).
>
> Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
> ---
>  Makefile | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index b8a3a64422..027b052a0c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2170,8 +2170,6 @@ 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) $(STRIP_OPTS) $^
>  
>  ### Flags affecting all rules

This doesn't remove the phony "strip" target (nor does the first patch),
and in any case I think this would be more readable with the two patches
squashed together. Let's remove the old target & add the new one
atomically.

Re: [PATCH 2/2] make: delete strip target

[Junio C Hamano]

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> The target isn't needed anymore since stripping is done in install-strip
> target (in previous patch).

That is not a valid justification.  

People's automation may have been using a perfectly valid

	#!/bin/sh
	make test &&
	make doc &&
	make strip &&
	make install install-doc

and this patch will break them for no good reason.

We need to remember that just because we (think we) came up with a
better way does not necessarily mean that we can immediately force
our users to adopt the new way.

> Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
> ---
>  Makefile | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index b8a3a64422..027b052a0c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2170,8 +2170,6 @@ 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) $(STRIP_OPTS) $^
>  
>  ### Flags affecting all rules