Re: [PATCH] make: add INSTALL_STRIP variable

["Đoàn Trần Công Danh" <congdanhqx@gmail.com>]

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