[PATCH] make: add install-strip target

[PATCH] make: add install-strip target

[Bagas Sanjaya]

Previously to install Git with stripped binaries, users have to do `make
all` then `make strip` before doing `make install`. It is nice to have
`install-strip` target for convenience, so that they can simply type
`make install-strip` and have Git with stripped binaries installed.
On some environments where disk space and resources is limited (such as
embedded systems), installed size can be smaller that with non-stripped
binaries.

Also mention the target in INSTALL.

Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
 INSTALL  | 5 +++++
 Makefile | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/INSTALL b/INSTALL
index 66389ce059..6e6303d482 100644
--- a/INSTALL
+++ b/INSTALL
@@ -25,6 +25,11 @@ set up install paths (via config.mak.autogen), so you can write instead
 	$ make all doc ;# as yourself
 	# make install install-doc install-html;# as root
 
+If you're tight on space (common on embedded systems), you can install
+with debugging info stripped with
+
+	# make install-strip
+
 If you're willing to trade off (much) longer build time for a later
 faster git you can also do a profile feedback build with
 
diff --git a/Makefile b/Makefile
index 9573190f1d..8c4633ba8e 100644
--- a/Makefile
+++ b/Makefile
@@ -3093,6 +3093,9 @@ endif
 	done && \
 	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
 
+install-strip: all strip
+	$(MAKE) install
+
 .PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf
 .PHONY: quick-install-doc quick-install-man quick-install-html
 install-gitweb:
@@ -3265,7 +3268,7 @@ ifdef MSVC
 	$(RM) compat/vcbuild/MSVC-DEFS-GEN
 endif
 
-.PHONY: all install profile-clean cocciclean clean strip
+.PHONY: all install install-strip profile-clean cocciclean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
 .PHONY: FORCE cscope
 

base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
-- 
2.25.1

Re: [PATCH] make: add install-strip target

[Johannes Schindelin]

Hi Bagas,

On Tue, 17 Aug 2021, Bagas Sanjaya wrote:

> diff --git a/Makefile b/Makefile
> index 9573190f1d..8c4633ba8e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3093,6 +3093,9 @@ endif
>  	done && \
>  	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
>
> +install-strip: all strip

Would those `all` and `strip` targets interfere with one another if `make
-j2` was called? If not, wouldn't it be sufficient to let `install-strip`
depend on `strip` alone?

Ciao,
Dscho

> +	$(MAKE) install
> +
>  .PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf
>  .PHONY: quick-install-doc quick-install-man quick-install-html
>  install-gitweb:
> @@ -3265,7 +3268,7 @@ ifdef MSVC
>  	$(RM) compat/vcbuild/MSVC-DEFS-GEN
>  endif
>
> -.PHONY: all install profile-clean cocciclean clean strip
> +.PHONY: all install install-strip profile-clean cocciclean clean strip
>  .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
>  .PHONY: FORCE cscope
>
>
> base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
> --
> 2.25.1
>
>

Re: [PATCH] make: add install-strip target

[Eric Sunshine]

On Tue, Aug 17, 2021 at 5:29 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Tue, 17 Aug 2021, Bagas Sanjaya wrote:
> > diff --git a/Makefile b/Makefile
> > @@ -3093,6 +3093,9 @@ endif
> > +install-strip: all strip
>
> Would those `all` and `strip` targets interfere with one another if `make
> -j2` was called? If not, wouldn't it be sufficient to let `install-strip`
> depend on `strip` alone?

A more pertinent question, perhaps, is why would we need
`install-strip` at all? What benefit does it provide over simply
typing `make strip install`?

Re: [PATCH] make: add install-strip target

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Would those `all` and `strip` targets interfere with one another if `make
> -j2` was called? If not, wouldn't it be sufficient to let `install-strip`
> depend on `strip` alone?

Good question.

I would have expected that this will *not* be a new target, but some
sort of make variable (e.g. "make INSTALL_STRIP=yes install").

Re: [PATCH] make: add install-strip target

[Johannes Schindelin]

Hi Eric,

On Tue, 17 Aug 2021, Eric Sunshine wrote:

> On Tue, Aug 17, 2021 at 5:29 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Tue, 17 Aug 2021, Bagas Sanjaya wrote:
> > > diff --git a/Makefile b/Makefile
> > > @@ -3093,6 +3093,9 @@ endif
> > > +install-strip: all strip
> >
> > Would those `all` and `strip` targets interfere with one another if `make
> > -j2` was called? If not, wouldn't it be sufficient to let `install-strip`
> > depend on `strip` alone?
>
> A more pertinent question, perhaps, is why would we need
> `install-strip` at all? What benefit does it provide over simply
> typing `make strip install`?

That would require an order-only prerequisite (see
https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html)
for `make -j2 strip install` to work correctly, i.e. something like this:

-- snip --
diff --git a/Makefile b/Makefile
index 2d5c822f7a8..9987f3b2c13 100644
--- a/Makefile
+++ b/Makefile
@@ -2990,7 +2990,7 @@ profile-install: profile
 profile-fast-install: profile-fast
 	$(MAKE) install

-install: all
+install: all | strip
 	$(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)'
-- snap --

I am not quite certain that this is compatible with other `make`
implementations we still might support (if there are any, I remember that
we often have to rely on `gmake` because the native `make` does not
understand our `Makefile`?), so that might need to be conditional on GNU
Make.

Ciao,
Dscho

Re: [PATCH] make: add install-strip target

[Johannes Schindelin]

Hi Junio,

On Tue, 17 Aug 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Would those `all` and `strip` targets interfere with one another if `make
> > -j2` was called? If not, wouldn't it be sufficient to let `install-strip`
> > depend on `strip` alone?
>
> Good question.
>
> I would have expected that this will *not* be a new target, but some
> sort of make variable (e.g. "make INSTALL_STRIP=yes install").

That would work, too. At the same time: wouldn't it be nicer to let `make
-j15 strip install` Do The Right Thing?

Ciao,
Dscho

Re: [PATCH] make: add install-strip target

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> That would work, too. At the same time: wouldn't it be nicer to let `make
> -j15 strip install` Do The Right Thing?

Oh, absolutely.  With "make all install" and "make strip install",
building (and optional stripping) should complete before the
"install" target kicks in.

It may be a bit tricky to implement, though.  Making 'install'
depend unconditionally on 'all' is trivial, but we want it to depend
on 'strip' only when 'strip' is part of the targets requested.

Re: [PATCH] make: add install-strip target

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> -install: all
> +install: all | strip
>  	$(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)'
> -- snap --
>
> I am not quite certain that this is compatible with other `make`
> implementations we still might support (if there are any, I remember that
> we often have to rely on `gmake` because the native `make` does not
> understand our `Makefile`?), so that might need to be conditional on GNU
> Make.

I think we are pretty-much dependent on GNU make already (it is
possible to raise a weather balloon to confirm by renaming Makefile
to GNUmakefile and observing if anybody complains, I think).

But I am not sure what such a rule does for a .PHONY target like
'strip'.  Does it do the right thing, i.e. "install recipe is run
after 'strip' recipe has run, iff 'strip' is also asked for"?

Thanks.

Re: [PATCH] make: add install-strip target

[Johannes Schindelin]

Hi Junio,

On Thu, 19 Aug 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > -install: all
> > +install: all | strip
> >  	$(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)'
> > -- snap --
> >
> > I am not quite certain that this is compatible with other `make`
> > implementations we still might support (if there are any, I remember that
> > we often have to rely on `gmake` because the native `make` does not
> > understand our `Makefile`?), so that might need to be conditional on GNU
> > Make.
>
> I think we are pretty-much dependent on GNU make already (it is
> possible to raise a weather balloon to confirm by renaming Makefile
> to GNUmakefile and observing if anybody complains, I think).
>
> But I am not sure what such a rule does for a .PHONY target like
> 'strip'.  Does it do the right thing, i.e. "install recipe is run
> after 'strip' recipe has run, iff 'strip' is also asked for"?

My reading of the documentation is that just as with regular dependencies,
it does not matter whether order-only dependencies are .PHONY or not.

The only difference between order-only vs regular dependencies seems to be
that order-only dependencies are not necessarily built. But if they are,
they are guaranteed to be built before the order-only dependencee.

Granted, I did not have time to test it, but from an implementation point
of view, I would be surprised if there was any more to it.

Ciao,
Dscho