threads / patch / 56310

patchmake: add install-strip target

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

## tl;dr

9 messages between Aug 17, 2021 and Aug 24, 2021. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Bagas Sanjaya· Aug 17, 2021, 11:07 UTC · lore

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(-)
Show changes to 2 files +9 −1

INSTALL, Makefile

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
Johannes Schindelin· Aug 17, 2021, 21:28 UTC · re: Bagas Sanjaya · lore

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

Hi Bagas,
On Tue, 17 Aug 2021, Bagas Sanjaya wrote:
Show 9 quoted lines
> 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

Show 20 quoted lines
> +	$(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
>
>
Eric Sunshine· Aug 17, 2021, 21:48 UTC · re: Johannes Schindelin · lore

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

On Tue, Aug 17, 2021 at 5:29 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 8 quoted lines
> 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`?

Johannes Schindelin· Aug 18, 2021, 10:25 UTC · re: Eric Sunshine · lore

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

Hi Eric,
On Tue, 17 Aug 2021, Eric Sunshine wrote:
Show 14 quoted lines
> 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 --
Show changes to Makefile +1 −2
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
Junio C Hamano· Aug 19, 2021, 19:54 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 12 quoted lines
> -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.
Johannes Schindelin· Aug 24, 2021, 13:15 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Thu, 19 Aug 2021, Junio C Hamano wrote:
Show 22 quoted lines
> 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

Junio C Hamano· Aug 17, 2021, 22:13 UTC · re: Johannes Schindelin · lore

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

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").

Johannes Schindelin· Aug 18, 2021, 10:30 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Tue, 17 Aug 2021, Junio C Hamano wrote:
Show 10 quoted lines
> 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

Junio C Hamano· Aug 19, 2021, 19:42 UTC · re: Johannes Schindelin · lore

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

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.

← back to recent threads