patchmake: add INSTALL_STRIP variable

> 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
> 

>>  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

> Đ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.
> 

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

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

> 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?

> 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?

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

> 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?