{"thread":{"id":"56383","subject":"[PATCH 0/2] make: install stripped Git","startedAt":"2021-08-26T11:38:44Z","lastAt":"2021-08-27T12:41:42Z","messageCount":8,"participants":["Bagas Sanjaya","Ævar Arnfjörð Bjarmason","Junio C Hamano","Đoàn Trần Công Danh"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"433821","messageId":"20210826113824.50078-1-bagasdotme@gmail.com","threadId":"56383","inReplyTo":null,"subject":"[PATCH 0/2] make: install stripped Git","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-08-26T11:38:22Z","receivedAt":"2021-08-26T11:38:44Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"This two-patch series adds convenient make target to install Git with\nstripped executables (programs). The first patch adds such target\n(called install-stripped), while the second one deletes the\nnow-redundant strip target.\n\nUnlike previous attempts [1] and [2], stripping is done after installing\nGit into installation prefix, without touching working directory where\nGit is compiled. The advantage of it is unstripped programs can be\ninstalled at the same prefix (thus overwriting already installed\nstripped ones), particularly useful for debugging and development\npurposes.\n\n[1]:\nhttps://lore.kernel.org/git/20210820105052.30631-1-bagasdotme@gmail.com/\n[2]:\nhttps://lore.kernel.org/git/20210817110728.55842-1-bagasdotme@gmail.com/\n\nBagas Sanjaya (2):\n  make: add install-stripped target\n  make: delete strip target\n\n Makefile | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\n\nbase-commit: c4203212e360b25a1c69467b5a8437d45a373cac\n-- \n2.25.1\n\n"},{"id":"433822","messageId":"20210826113824.50078-2-bagasdotme@gmail.com","threadId":"56383","inReplyTo":"20210826113824.50078-1-bagasdotme@gmail.com","subject":"[PATCH 1/2] make: add install-stripped target","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-08-26T11:38:23Z","receivedAt":"2021-08-26T11:38:51Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"Add the target that install Git with stripped executables\n\nThe executables that are going to be stripped are all of $(PROGRAMS) and\ngit. Because they are installed over various directories (bin and\nlibexec/git-core) within installation prefix, the location of each\nprogram needs to be found and pass it to $(STRIP) program.\n\nSigned-off-by: Bagas Sanjaya <bagasdotme@gmail.com>\n---\n Makefile | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex d1feab008f..b8a3a64422 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3102,7 +3102,12 @@ endif\n \tdone && \\\n \t./check_bindir \"z$$bindir\" \"z$$execdir\" \"$$bindir/git-add$X\"\n \n-.PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf\n+install-stripped: install\n+\tfor f in $(PROGRAMS) git$X; do \\\n+\t\tfind $$prefix -type f -name $$f -exec $(STRIP) $(STRIP_OPTS) {} \\; ; \\\n+\tdone\n+\n+.PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf install-stripped\n .PHONY: quick-install-doc quick-install-man quick-install-html\n install-gitweb:\n \t$(MAKE) -C gitweb install\n-- \n2.25.1\n\n"},{"id":"433823","messageId":"20210826113824.50078-3-bagasdotme@gmail.com","threadId":"56383","inReplyTo":"20210826113824.50078-1-bagasdotme@gmail.com","subject":"[PATCH 2/2] make: delete strip target","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-08-26T11:38:24Z","receivedAt":"2021-08-26T11:38:53Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"The target isn't needed anymore since stripping is done in install-strip\ntarget (in previous patch).\n\nSigned-off-by: Bagas Sanjaya <bagasdotme@gmail.com>\n---\n Makefile | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex b8a3a64422..027b052a0c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2170,8 +2170,6 @@ please_set_SHELL_PATH_to_a_more_modern_shell:\n \n shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n \n-strip: $(PROGRAMS) git$X\n-\t$(STRIP) $(STRIP_OPTS) $^\n \n ### Flags affecting all rules\n \n-- \n2.25.1\n\n"},{"id":"433865","messageId":"87bl5kyp4e.fsf@evledraar.gmail.com","threadId":"56383","inReplyTo":"20210826113824.50078-3-bagasdotme@gmail.com","subject":"Re: [PATCH 2/2] make: delete strip target","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-26T19:36:05Z","receivedAt":"2021-08-26T19:36:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Aug 26 2021, Bagas Sanjaya wrote:\n\n> The target isn't needed anymore since stripping is done in install-strip\n> target (in previous patch).\n>\n> Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>\n> ---\n>  Makefile | 2 --\n>  1 file changed, 2 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index b8a3a64422..027b052a0c 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2170,8 +2170,6 @@ please_set_SHELL_PATH_to_a_more_modern_shell:\n>  \n>  shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n>  \n> -strip: $(PROGRAMS) git$X\n> -\t$(STRIP) $(STRIP_OPTS) $^\n>  \n>  ### Flags affecting all rules\n\nThis doesn't remove the phony \"strip\" target (nor does the first patch),\nand in any case I think this would be more readable with the two patches\nsquashed together. Let's remove the old target & add the new one\natomically.\n"},{"id":"433866","messageId":"xmqq8s0odl5l.fsf@gitster.g","threadId":"56383","inReplyTo":"20210826113824.50078-2-bagasdotme@gmail.com","subject":"Re: [PATCH 1/2] make: add install-stripped target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-26T20:08:06Z","receivedAt":"2021-08-26T20:08:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bagas Sanjaya <bagasdotme@gmail.com> writes:\n\n> +install-stripped: install\n> +\tfor f in $(PROGRAMS) git$X; do \\\n> +\t\tfind $$prefix -type f -name $$f -exec $(STRIP) $(STRIP_OPTS) {} \\; ; \\\n> +\tdone\n> +\n\nThis sounds awfully wasteful.\n\nThe recipe for the install target knows exactly each of these\nprograms are installed, but yet the above is running around inside\n$prefix to find them after the fact.\n\nIt also looks incorrect, too.\n\nIt is not guaranteed that $prefix does not contain any $IFS\nwhitespace in it, and worse yet, $prefix may not contain $bindir or\n$libexecdir in it, so find may never reach these binaries.\n\nIt also depends on \"strip\" not to break handlinks to the same\nbinary.  \"git\" is linked to many built-in command binary like\n\"git-cat-file\" and \"git-remote-$curl\" for various protocols are\ninstalled by creating links to \"git-remote-http\".  It seems that the\n\"strip\" command from GNU binutils package strips such a binary\nin-place, but I do not think there is no fundamental reason to\nbelieve that everybody else's \"strip\" would behave that way.\n\nI would have expected that 'install-stripped' and 'install' targets\nwould run the same recipe, and when $(install_bindir_programs) are\ninstalled in $(bindir) using $(INSTALL), we would optionally pass\nthe '--strip' option to the $(INSTALL) program when the recipe is\nrun for the install-stripped target.  All the tricky symlinking,\nhardlinking and copying happens only on the result of that step, and\nthe strip step should happen before that, I would think.\n\n> +.PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf install-stripped\n\nSplit the overly long line like this into two or more.\n\n>  .PHONY: quick-install-doc quick-install-man quick-install-html\n>  install-gitweb:\n>  \t$(MAKE) -C gitweb install\n"},{"id":"433867","messageId":"xmqq4kbcdl0t.fsf@gitster.g","threadId":"56383","inReplyTo":"20210826113824.50078-3-bagasdotme@gmail.com","subject":"Re: [PATCH 2/2] make: delete strip target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-26T20:10:58Z","receivedAt":"2021-08-26T20:11:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bagas Sanjaya <bagasdotme@gmail.com> writes:\n\n> The target isn't needed anymore since stripping is done in install-strip\n> target (in previous patch).\n\nThat is not a valid justification.  \n\nPeople's automation may have been using a perfectly valid\n\n\t#!/bin/sh\n\tmake test &&\n\tmake doc &&\n\tmake strip &&\n\tmake install install-doc\n\nand this patch will break them for no good reason.\n\nWe need to remember that just because we (think we) came up with a\nbetter way does not necessarily mean that we can immediately force\nour users to adopt the new way.\n\n> Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>\n> ---\n>  Makefile | 2 --\n>  1 file changed, 2 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index b8a3a64422..027b052a0c 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2170,8 +2170,6 @@ please_set_SHELL_PATH_to_a_more_modern_shell:\n>  \n>  shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n>  \n> -strip: $(PROGRAMS) git$X\n> -\t$(STRIP) $(STRIP_OPTS) $^\n>  \n>  ### Flags affecting all rules\n"},{"id":"433913","messageId":"b2005b0f-9181-526b-ebf5-58fca44299b5@gmail.com","threadId":"56383","inReplyTo":"xmqq8s0odl5l.fsf@gitster.g","subject":"Re: [PATCH 1/2] make: add install-stripped target","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-08-27T07:57:56Z","receivedAt":"2021-08-27T07:58:04Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 27/08/21 03.08, Junio C Hamano wrote:\n> It also depends on \"strip\" not to break handlinks to the same\n> binary.  \"git\" is linked to many built-in command binary like\n> \"git-cat-file\" and \"git-remote-$curl\" for various protocols are\n> installed by creating links to \"git-remote-http\".  It seems that the\n> \"strip\" command from GNU binutils package strips such a binary\n> in-place, but I do not think there is no fundamental reason to\n> believe that everybody else's \"strip\" would behave that way.\n> \n\nMaybe hardlinks?\n\n> I would have expected that 'install-stripped' and 'install' targets\n> would run the same recipe, and when $(install_bindir_programs) are\n> installed in $(bindir) using $(INSTALL), we would optionally pass\n> the '--strip' option to the $(INSTALL) program when the recipe is\n> run for the install-stripped target.  All the tricky symlinking,\n> hardlinking and copying happens only on the result of that step, and\n> the strip step should happen before that, I would think.\n> \n\nDid you mean copying recipe of 'install' to 'install-stripped' and the \nlatter s/$(INSTALL)/$(INSTALL -s --strip-program=\"$(STRIP)\"/)?\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"433929","messageId":"YSjdc/tNlhbCosC2@danh.dev","threadId":"56383","inReplyTo":"b2005b0f-9181-526b-ebf5-58fca44299b5@gmail.com","subject":"Re: [PATCH 1/2] make: add install-stripped target","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2021-08-27T12:41:37Z","receivedAt":"2021-08-27T12:41:42Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2021-08-27 14:57:56+0700, Bagas Sanjaya <bagasdotme@gmail.com> wrote:\n> On 27/08/21 03.08, Junio C Hamano wrote:\n> > It also depends on \"strip\" not to break handlinks to the same\n> > binary.  \"git\" is linked to many built-in command binary like\n> > \"git-cat-file\" and \"git-remote-$curl\" for various protocols are\n> > installed by creating links to \"git-remote-http\".  It seems that the\n> > \"strip\" command from GNU binutils package strips such a binary\n> > in-place, but I do not think there is no fundamental reason to\n> > believe that everybody else's \"strip\" would behave that way.\n> > \n> \n> Maybe hardlinks?\n> \n> > I would have expected that 'install-stripped' and 'install' targets\n> > would run the same recipe, and when $(install_bindir_programs) are\n> > installed in $(bindir) using $(INSTALL), we would optionally pass\n> > the '--strip' option to the $(INSTALL) program when the recipe is\n> > run for the install-stripped target.  All the tricky symlinking,\n> > hardlinking and copying happens only on the result of that step, and\n> > the strip step should happen before that, I would think.\n> > \n> \n> Did you mean copying recipe of 'install' to 'install-stripped' and the\n> latter s/$(INSTALL)/$(INSTALL -s --strip-program=\"$(STRIP)\"/)?\n\nI think Junio meant something like this:\n\n---- 8< ----\n\ndiff --git a/Makefile b/Makefile\nindex 429c276058..70b7ef9ce1 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3004,7 +3004,8 @@ mergetools_instdir = $(prefix)/$(mergetoolsdir)\n endif\n mergetools_instdir_SQ = $(subst ','\\'',$(mergetools_instdir))\n \n-install_bindir_programs := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) $(BINDIR_PROGRAMS_NO_X)\n+install_bindir_xprograms := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X))\n+install_bindir_programs := $(install_bindir_xprograms) $(BINDIR_PROGRAMS_NO_X)\n \n .PHONY: profile-install profile-fast-install\n profile-install: profile\n@@ -3013,12 +3014,18 @@ profile-install: profile\n profile-fast-install: profile-fast\n \t$(MAKE) install\n \n-install: all\n+INSTALL_OPTS :=\n+\n+install-strip: INSTALL_OPTS := -s --strip-program=$(STRIP)\n+\n+install-strip install: all\n \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)'\n \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n-\t$(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n+\t$(INSTALL) $(INSTALL_OPTS) $(PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n+\t$(INSTALL) $(SCRIPTS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n \t$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n-\t$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'\n+\t$(INSTALL) $(INSTALL_OPTS) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'\n+\t$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'\n ifdef MSVC\n \t# We DO NOT install the individual foo.o.pdb files because they\n \t# have already been rolled up into the exe's pdb file.\n---- >8 -----\n\n-- \nDanh\n"}]}