git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Bagas Sanjaya <bagasdotme@gmail.com>,
	git@vger.kernel.org, Emily Shaffer <emilyshaffer@google.com>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH] make: add INSTALL_STRIP variable
Date: Sat, 21 Aug 2021 09:13:17 +0700	[thread overview]
Message-ID: <YSBhPdK8jYIQUNhP@danh.dev> (raw)
In-Reply-To: <xmqqwnogt20q.fsf@gitster.g>

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

  reply	other threads:[~2021-08-21  2:13 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20 10:50 [PATCH] make: add INSTALL_STRIP variable Bagas Sanjaya
2021-08-20 11:36 ` Đoàn Trần Công Danh
2021-08-20 18:16   ` Junio C Hamano
2021-08-21  2:13     ` Đoàn Trần Công Danh [this message]
2021-08-23 15:55       ` Junio C Hamano
2021-08-24  9:39         ` Bagas Sanjaya
2021-08-24  9:49           ` Johannes Schindelin
2021-08-24 18:58           ` Junio C Hamano
2021-08-21  8:24   ` Bagas Sanjaya
2021-08-21 10:35     ` Đoàn Trần Công Danh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YSBhPdK8jYIQUNhP@danh.dev \
    --to=congdanhqx@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=bagasdotme@gmail.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).