On Thu, Sep 02 2021, Bagas Sanjaya wrote: > From: Junio C Hamano <gitster@xxxxxxxxx> > > Add $(INSTALL_STRIP), which allows passing stripping options to > $(INSTALL). > > For this to work, installing executables must be split to installing > compiled binaries and scripts portions, since $(INSTALL_STRIP) is only > meaningful to the former. > > Users can set this variable depending on their system. For example, > Linux users can use `-s --strip-program=strip`, while FreeBSD users can > simply set to `-s` and choose strip program with $STRIPBIN. > > Signed-off-by: Bagas Sanjaya <bagasdotme@xxxxxxxxx> > --- > > Changes from v3 [1]: > - Squash suggestion patch from Junio, which suggests dropping > install-stripped target and rename $(INSTALL_OPTS) to > $(INSTALL_STRIP). > - Describe $(INSTALL_STRIP) usage on the top of Makefile > > Note: In the future, we may add global $(INSTALL_OPTS), which applies > to both compiled binaries and scripts. When such happens, we should > rename $(INSTALL_STRIP) to $(INSTALL_STRIP_OPTS). I see this landed on "master" already, but in case you're interested on follow-ing up: I think the suggestion in https://lore.kernel.org/git/xmqqo89cqkt9.fsf@gitster.g/ is to make this a boolean variable, and in any case I think that makes more sense here, since.... > [1]: https://lore.kernel.org/git/xmqqo89cqkt9.fsf@gitster.g/T/#t > > Makefile | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/Makefile b/Makefile > index d1feab008f..ebef4da50c 100644 > --- a/Makefile > +++ b/Makefile > @@ -465,6 +465,9 @@ all:: > # the global variable _wpgmptr containing the absolute path of the current > # executable (this is the case on Windows). > # > +# INSTALL_STRIP can be set to "-s" to strip binaries during installation, > +# if your $(INSTALL) command supports the option. > +# > # Define GENERATE_COMPILATION_DATABASE to "yes" to generate JSON compilation > # database entries during compilation if your compiler supports it, using the > # `-MJ` flag. The JSON entries will be placed in the `compile_commands/` > @@ -3004,7 +3007,8 @@ mergetools_instdir = $(prefix)/$(mergetoolsdir) > endif > mergetools_instdir_SQ = $(subst ','\'',$(mergetools_instdir)) > > -install_bindir_programs := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) $(BINDIR_PROGRAMS_NO_X) > +install_bindir_xprograms := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) > +install_bindir_programs := $(install_bindir_xprograms) $(BINDIR_PROGRAMS_NO_X) > > .PHONY: profile-install profile-fast-install > profile-install: profile > @@ -3013,12 +3017,17 @@ profile-install: profile > profile-fast-install: profile-fast > $(MAKE) install > > +INSTALL_STRIP = > + > install: all > $(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)' > + $(INSTALL) $(INSTALL_STRIP) $(PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' > + $(INSTALL) $(SCRIPTS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' > $(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' > - $(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)' > + $(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)' > + $(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)' > + > ifdef MSVC > # We DO NOT install the individual foo.o.pdb files because they > # have already been rolled up into the exe's pdb file. > > base-commit: 6c40894d2466d4e7fddc047a05116aa9d14712ee ...this really is not an INSTALL_STRIP but (using some combination of your own naming) a "INSTALL_XPROGRAMS_OPTS" or "INSTALL_XOPTS". I.e. you can supply arbitrary options to "install" with this, but only for binaries. Also doesn't this misbehave under MSVC when combined with *.pdb files? See dce7d295514 (msvc: support building Git using MS Visual C++, 2019-06-25) and a8b5355d808 (msvc: copy the correct `.pdb` files in the Makefile target `install`, 2020-09-21) , i.e. the code at the start of your diff context. Does stripping the main binary while having a *.pdb file error or MSCV, or make the *.pdb file useless, or is *.pdb an unconditional equivalent of INSTALL_STRIP=-s on MSCV that we should disable if this hopefully-then-boolean INSTALL_STRIP option is enabled?
