[PATCH] make: add INSTALL_STRIP variable

[PATCH] make: add INSTALL_STRIP variable

[Bagas Sanjaya]

In some environments (most notably embedded systems and small production
servers), it is often desirable to have stripped Git binaries due to
tight disk space constraint.

Until now stripped Git can be built wih `make strip install`. Add
INSTALL_STRIP make variable so that they can install stripped Git
binaries with `make INSTALL_STRIP=yes install`.

Also document stripping and using INSTALL_STRIP in INSTALL.

Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
 Junio suggested me to have INSTALL_STRIP make variable [1] when
 reviewing the install-strip target patch.

 [1]:
https://lore.kernel.org/git/xmqq1r6p1ark.fsf@gitster.g/T/#mc3b8017448bdafedf9250ba407f5de767c20ad67

 INSTALL  | 8 ++++++++
 Makefile | 5 +++++
 2 files changed, 13 insertions(+)

diff --git a/INSTALL b/INSTALL
index 66389ce059..98e541ee4d 100644
--- a/INSTALL
+++ b/INSTALL
@@ -58,6 +58,14 @@ suite has to be run using only a single CPU.  In addition, the profile
 feedback build stage currently generates a lot of additional compiler
 warnings.
 
+You can also strip debug info from built binaries by:
+
+	$ make strip
+
+or for stripping and installing together:
+
+	$ make INSTALL_STRIP=yes install
+
 Issues of note:
 
  - Ancient versions of GNU Interactive Tools (pre-4.9.2) installed a
diff --git a/Makefile b/Makefile
index 9573190f1d..e486f3ab75 100644
--- a/Makefile
+++ b/Makefile
@@ -8,6 +8,8 @@ all::
 # Define SANE_TOOL_PATH to a colon-separated list of paths to prepend
 # to PATH if your tools in /usr/bin are broken.
 #
+# Define INSTALL_STRIP if you want to install with stripped binaries.
+#
 # Define SOCKLEN_T to a suitable type (such as 'size_t') if your
 # system headers do not define a socklen_t type.
 #
@@ -3005,6 +3007,9 @@ profile-fast-install: profile-fast
 	$(MAKE) install
 
 install: all
+ifdef INSTALL_STRIP
+	$(MAKE) strip
+endif
 	$(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)'

base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
-- 
2.25.1

Re: [PATCH] make: add INSTALL_STRIP variable

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

On 2021-08-20 17:50:53+0700, Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> In some environments (most notably embedded systems and small production
> servers), it is often desirable to have stripped Git binaries due to
> tight disk space constraint.
> 
> Until now stripped Git can be built wih `make strip install`. Add
> INSTALL_STRIP make variable so that they can install stripped Git
> binaries with `make INSTALL_STRIP=yes install`.
> 
> Also document stripping and using INSTALL_STRIP in INSTALL.
> 
> Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
> ---
>  Junio suggested me to have INSTALL_STRIP make variable [1] when
>  reviewing the install-strip target patch.
> 
>  [1]:
> https://lore.kernel.org/git/xmqq1r6p1ark.fsf@gitster.g/T/#mc3b8017448bdafedf9250ba407f5de767c20ad67
> 
>  INSTALL  | 8 ++++++++
>  Makefile | 5 +++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/INSTALL b/INSTALL
> index 66389ce059..98e541ee4d 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -58,6 +58,14 @@ suite has to be run using only a single CPU.  In addition, the profile
>  feedback build stage currently generates a lot of additional compiler
>  warnings.
>  
> +You can also strip debug info from built binaries by:
> +
> +	$ make strip
> +
> +or for stripping and installing together:
> +
> +	$ make INSTALL_STRIP=yes install
> +
>  Issues of note:
>  
>   - Ancient versions of GNU Interactive Tools (pre-4.9.2) installed a
> diff --git a/Makefile b/Makefile
> index 9573190f1d..e486f3ab75 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -8,6 +8,8 @@ all::
>  # Define SANE_TOOL_PATH to a colon-separated list of paths to prepend
>  # to PATH if your tools in /usr/bin are broken.
>  #
> +# Define INSTALL_STRIP if you want to install with stripped binaries.
> +#
>  # Define SOCKLEN_T to a suitable type (such as 'size_t') if your
>  # system headers do not define a socklen_t type.
>  #
> @@ -3005,6 +3007,9 @@ profile-fast-install: profile-fast
>  	$(MAKE) install
>  
>  install: all
> +ifdef INSTALL_STRIP
> +	$(MAKE) strip
> +endif

I believe it's better to write like this:

----- 8< ------
ifdef INSTALL_STRIP
install: strip
endif

install: all
	....
---- >8-------

IOW, install depends on strip, not install invoke strip.
I think it would work better for:

	make install 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)'
> 
> base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
> -- 
> 2.25.1
> 

-- 
Danh

Re: [PATCH] make: add INSTALL_STRIP variable

[Junio C Hamano]

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

>>  install: all
>> +ifdef INSTALL_STRIP
>> +	$(MAKE) strip
>> +endif
>
> I believe it's better to write like this:
>
> ----- 8< ------
> ifdef INSTALL_STRIP
> install: strip
> endif
>
> install: all
> 	....
> ---- >8-------
>
> IOW, install depends on strip, not install invoke strip.
> I think it would work better for:
>
> 	make install strip

I think you meant "it would work better than 'make install strip'",
and if so, I tend to agree.  With

	echo INSTALL_STRIP=YesPlease >>config.mak

either Bagas's or your "before installing, make sure we strip"
change lets

	make install

just work without "strip" given on the command line.

If users with such a config.mak type "make install strip", it will
make the recipe for "install" wait until "strip" is done, which is
what we want, but "strip" on the command line for them is redundant,
and there is no way for them to install unstripped binaries, which
may be a bit of downside.

But for those who do not always want to use INSTALL_STRIP, as Dscho
said after I mentioned the "make variable" thing, we probably a
wrong thing when they say "make -j strip install", as there is
nothing to make recipe for "install" to wait for "strip", so it is
not a fully satisfactory solution.

I think we want two things:

 (1) if a user says "make [-j] strip install", make sure "install"
     won't start before "strip" finishes;

 (2) if a user wants to always install stripped binary, allow some
     make variable in config.mak so that "make install" would do
     that without an explicit "strip".

Of course, if a user does not have (2) configured, "make install"
should install unstripped binaries, but that goes without saying.

And after thinking it like this, perhaps a new "install-stripped"
target that runs "strip" and then "install" as originally proposed
in the thread that triggered this discussion may be the simplest
approach.  We can control the optional dependency between "strip"
and "install", those who want to install stripped binary can use
"install-stripped" instead of "install", and they can on-demand
choose to install unstripped binary (which was a potential downside
of the "make variable" approach under discussion here).

Thanks.

Re: [PATCH] make: add INSTALL_STRIP variable

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

On 2021-08-20 11:16:37-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
> 
> >>  install: all
> >> +ifdef INSTALL_STRIP
> >> +	$(MAKE) strip
> >> +endif
> >
> > I believe it's better to write like this:
> >
> > ----- 8< ------
> > ifdef INSTALL_STRIP
> > install: strip
> > endif
> >
> > install: all
> > 	....
> > ---- >8-------
> >
> > IOW, install depends on strip, not install invoke strip.
> > I think it would work better for:
> >
> > 	make install strip
> 
> I think you meant "it would work better than 'make install strip'",
> and if so, I tend to agree.  With
> 
> 	echo INSTALL_STRIP=YesPlease >>config.mak
> 
> either Bagas's or your "before installing, make sure we strip"
> change lets
> 
> 	make install
> 
> just work without "strip" given on the command line.
> 
> If users with such a config.mak type "make install strip", it will
> make the recipe for "install" wait until "strip" is done, which is
> what we want, but "strip" on the command line for them is redundant,
> and there is no way for them to install unstripped binaries, which
> may be a bit of downside.
> 
> But for those who do not always want to use INSTALL_STRIP, as Dscho
> said after I mentioned the "make variable" thing, we probably a
> wrong thing when they say "make -j strip install", as there is
> nothing to make recipe for "install" to wait for "strip", so it is
> not a fully satisfactory solution.

In that case, we can use this construct (since we depends on GNU Make,
anyway).

---- 8< ------
ifneq ($(filter install,$(MAKECMDGOALS)),)
ifneq ($(filter strip,$(MAKECMDGOALS)),)
install: strip
endif
endif
---- >8 -----

MAKECMDGOALS is available from at least GNU Make 3.75.1 in 1997.

Anyway, maybe it's only me, but I think people may want to install
first, then strip later for debug mapping.

> 
> I think we want two things:
> 
>  (1) if a user says "make [-j] strip install", make sure "install"
>      won't start before "strip" finishes;
> 
>  (2) if a user wants to always install stripped binary, allow some
>      make variable in config.mak so that "make install" would do
>      that without an explicit "strip".
> 
> Of course, if a user does not have (2) configured, "make install"
> should install unstripped binaries, but that goes without saying.
> 
> And after thinking it like this, perhaps a new "install-stripped"
> target that runs "strip" and then "install" as originally proposed
> in the thread that triggered this discussion may be the simplest
> approach.  We can control the optional dependency between "strip"
> and "install", those who want to install stripped binary can use
> "install-stripped" instead of "install", and they can on-demand
> choose to install unstripped binary (which was a potential downside
> of the "make variable" approach under discussion here).
> 
> Thanks.
> 

-- 
Danh

Re: [PATCH] make: add INSTALL_STRIP variable

[Bagas Sanjaya]

On 20/08/21 18.36, Đoàn Trần Công Danh wrote:
> I believe it's better to write like this:
> 
> ----- 8< ------
> ifdef INSTALL_STRIP
> install: strip
> endif
> 
> install: all
> 	....
> ---- >8-------
> 
> IOW, install depends on strip, not install invoke strip.

Oh, I missed that.

> I think it would work better for:
> 
> 	make install strip
> 

Wouldn't it install unstripped binaries to the prefix then stripping 
them in the build directory?

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

Re: [PATCH] make: add INSTALL_STRIP variable

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

On 2021-08-21 15:24:22+0700, Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> On 20/08/21 18.36, Đoàn Trần Công Danh wrote:
> > I believe it's better to write like this:
> > 
> > ----- 8< ------
> > ifdef INSTALL_STRIP
> > install: strip
> > endif
> > 
> > install: all
> > 	....
> > ---- >8-------
> > 
> > IOW, install depends on strip, not install invoke strip.
> 
> Oh, I missed that.
> 
> > I think it would work better for:
> > 
> > 	make install strip
> > 
> 
> Wouldn't it install unstripped binaries to the prefix then stripping them in
> the build directory?

No, with:

	install: strip

strip is one of install's dependencies, then strip will be executed
before install.

-- 
Danh

Re: [PATCH] make: add INSTALL_STRIP variable

[Junio C Hamano]

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> In that case, we can use this construct (since we depends on GNU Make,
> anyway).
>
> ---- 8< ------
> ifneq ($(filter install,$(MAKECMDGOALS)),)
> ifneq ($(filter strip,$(MAKECMDGOALS)),)
> install: strip
> endif
> endif
> ---- >8 -----
>
> MAKECMDGOALS is available from at least GNU Make 3.75.1 in 1997.

Or the "|" thing Dscho floated earlier?

> Anyway, maybe it's only me, but I think people may want to install
> first, then strip later for debug mapping.

Perhaps.  One bad thing with the current "strip" arrangement is that
it is done in the built directory, and because "make install" would
blindly install whatever in the built directory, if you truly care
that you install unstripped binaries, you need to see if they are
stripped and rebuild them as needed, because "make strip" may or may
not have been done.  From that point of view, getting rid of the
current "make strip" and introducing either "make strip-installed"
("we've installed things earlier---go strip them") or "make
install-stripped" ("we've built (or if we haven't please build them
first), now install them and strip them in the installed directory")
may make more sense.  And for that, any idea that came up in this
discussion that relies on the current "strip" target would not help.

Thanks.

Re: [PATCH] make: add INSTALL_STRIP variable

[Bagas Sanjaya]

On 23/08/21 22.55, Junio C Hamano wrote:
> Perhaps.  One bad thing with the current "strip" arrangement is that
> it is done in the built directory, and because "make install" would
> blindly install whatever in the built directory, if you truly care
> that you install unstripped binaries, you need to see if they are
> stripped and rebuild them as needed, because "make strip" may or may
> not have been done.  From that point of view, getting rid of the
> current "make strip" and introducing either "make strip-installed"
> ("we've installed things earlier---go strip them") or "make
> install-stripped" ("we've built (or if we haven't please build them
> first), now install them and strip them in the installed directory")
> may make more sense.  And for that, any idea that came up in this
> discussion that relies on the current "strip" target would not help.

But often the installed directory (install prefix) is owned by root,
so one has to `sudo make install-strip`, right?

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

Re: [PATCH] make: add INSTALL_STRIP variable

[Johannes Schindelin]

Hi Bagas,

On Tue, 24 Aug 2021, Bagas Sanjaya wrote:

> On 23/08/21 22.55, Junio C Hamano wrote:
> > Perhaps.  One bad thing with the current "strip" arrangement is that
> > it is done in the built directory, and because "make install" would
> > blindly install whatever in the built directory, if you truly care
> > that you install unstripped binaries, you need to see if they are
> > stripped and rebuild them as needed, because "make strip" may or may
> > not have been done.  From that point of view, getting rid of the
> > current "make strip" and introducing either "make strip-installed"
> > ("we've installed things earlier---go strip them") or "make
> > install-stripped" ("we've built (or if we haven't please build them
> > first), now install them and strip them in the installed directory")
> > may make more sense.  And for that, any idea that came up in this
> > discussion that relies on the current "strip" target would not help.
>
> But often the installed directory (install prefix) is owned by root,
> so one has to `sudo make install-strip`, right?

The default for `make install` goes to `$HOME`. I would wager a bet that
that's the common case, too. It does not require `sudo`.

Ciao,
Dscho

Re: [PATCH] make: add INSTALL_STRIP variable

[Junio C Hamano]

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> On 23/08/21 22.55, Junio C Hamano wrote:
>> Perhaps.  One bad thing with the current "strip" arrangement is that
>> it is done in the built directory, and because "make install" would
>> blindly install whatever in the built directory, if you truly care
>> that you install unstripped binaries, you need to see if they are
>> stripped and rebuild them as needed, because "make strip" may or may
>> not have been done.  From that point of view, getting rid of the
>> current "make strip" and introducing either "make strip-installed"
>> ("we've installed things earlier---go strip them") or "make
>> install-stripped" ("we've built (or if we haven't please build them
>> first), now install them and strip them in the installed directory")
>> may make more sense.  And for that, any idea that came up in this
>> discussion that relies on the current "strip" target would not help.
>
> But often the installed directory (install prefix) is owned by root,
> so one has to `sudo make install-strip`, right?

Whatever you would need to do for "make install" would be necessary,
because you would be touching the system directories, so there isn't
anything new, I would think.