From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] Makefile: build 'gitweb' in the default target
Date: Thu, 26 May 2022 23:33:05 +0200 [thread overview]
Message-ID: <20220526213305.GA1707@szeder.dev> (raw)
In-Reply-To: <220526.86k0a96sv2.gmgdl@evledraar.gmail.com>
On Thu, May 26, 2022 at 02:14:33AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, May 25 2022, SZEDER Gábor wrote:
>
> > Our Makefile's default target used to build 'gitweb', though
> > indirectly: the 'all' target depended on 'git-instaweb', which in turn
> > depended on 'gitweb'. Then e25c7cc146 (Makefile: drop dependency
> > between git-instaweb and gitweb, 2015-05-29) removed the latter
> > dependency, and for good reasons (quoting its commit message):
> >
> > "1. git-instaweb has no build-time dependency on gitweb; it
> > is a run-time dependency
> >
> > 2. gitweb is a directory that we want to recursively make
> > in. As a result, its recipe is marked .PHONY, which
> > causes "make" to rebuild git-instaweb every time it is
> > run."
> >
> > Since then a simple 'make' doesn't build 'gitweb'.
> >
> > Luckily, installing 'gitweb' is not broken: although 'make install'
> > doesn't depend on the 'gitweb' target, it runs 'make -C gitweb
> > install' unconditionally, which does generate all the necessary files
> > for 'gitweb' and installs them. However, if someone runs 'make &&
> > sudo make install', then those files in the 'gitweb' directory will be
> > generated and owned by root, which is not nice.
> >
> > List 'gitweb' as a direct dependency of the default target, so a plain
> > 'make' will build it.
> >
> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> > ---
> > Makefile | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/Makefile b/Makefile
> > index f8bccfab5e..ee74892b33 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2188,6 +2188,8 @@ ifneq (,$X)
> > $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
> > endif
> >
> > +all:: gitweb
> > +
> > all::
> > ifndef NO_TCLTK
> > $(QUIET_SUBDIR0)git-gui $(QUIET_SUBDIR1) gitexecdir='$(gitexec_instdir_SQ)' all
>
> In various recent patches & some upcoming ones I plan to submit I've
> been trying to get the runtime of a noop "make" runs down, which really
> helps e.g. with "git rebase -x make ..." running faster on a large
> series.
>
> While you're right that this wasn't intentional to begin with, we have
> lacked the "gitweb" as part of the default target since v2.4.5 now, and
> adding it back is a major performance regression on noop "make" runs:
I think that generating stuff, potentially as root, during 'make
install' is a more severe regression, than this noop make slowdown,
which in practice tends to be lost in the noise anyway. Even in an
unrealistic case (it doesn't modify any C source files explicitly, let
alone a frequently included header file) like this:
$ git checkout fddc3b420f^
$ make
[...]
$ for i in {1..10} ; do git commit --allow-empty -q -m $i ; done
$ time git rebase -x 'make -j8 NO_TCLTK=Y >/dev/null' HEAD~10
[...]
real 0m31.026s
user 0m46.897s
sys 0m11.492s
$ git checkout fddc3b420f
$ for i in {1..10} ; do git commit --allow-empty -q -m $i ; done
$ time git rebase -x 'make -j8 NO_TCLTK=Y >/dev/null' HEAD~10
[...]
real 0m30.865s
user 0m48.315s
sys 0m12.125s
Hrm, it actually ended up slightly faster.
> $ git hyperfine -L rev HEAD~1,HEAD~0 -L t Y, -s 'make' 'make NO_TCLTK={t}' --warmup 1 -r 5
> Benchmark 1: make NO_TCLTK=Y' in 'HEAD~1
> Time (mean ± σ): 103.6 ms ± 1.1 ms [User: 83.8 ms, System: 32.1 ms]
> Range (min … max): 102.2 ms … 105.2 ms 5 runs
>
> Benchmark 2: make NO_TCLTK=Y' in 'HEAD~0
> Time (mean ± σ): 191.4 ms ± 1.6 ms [User: 151.0 ms, System: 60.5 ms]
> Range (min … max): 189.2 ms … 193.3 ms 5 runs
>
> Benchmark 3: make NO_TCLTK=' in 'HEAD~1
> Time (mean ± σ): 272.0 ms ± 5.0 ms [User: 206.3 ms, System: 83.3 ms]
> Range (min … max): 266.7 ms … 277.3 ms 5 runs
>
> Benchmark 4: make NO_TCLTK=' in 'HEAD~0
> Time (mean ± σ): 358.3 ms ± 1.4 ms [User: 282.7 ms, System: 104.0 ms]
> Range (min … max): 356.6 ms … 360.0 ms 5 runs
>
> Summary
> 'make NO_TCLTK=Y' in 'HEAD~1' ran
> 1.85 ± 0.02 times faster than 'make NO_TCLTK=Y' in 'HEAD~0'
> 2.63 ± 0.06 times faster than 'make NO_TCLTK=' in 'HEAD~1'
> 3.46 ± 0.04 times faster than 'make NO_TCLTK=' in 'HEAD~0'
>
> I.e. this is with your patch here applied as HEAD~0 and HEAD~1 being
> 'master'.
>
> I think given that that a better solution would be to just declare this
> as a feature at this point
As long as 'make install' installs 'gitweb', I don't think that's an
option.
> especially as gitweb/INSTALL notes that the
> way to install it is:
>
> $ make prefix=/usr gitweb ;# as yourself
> # make gitwebdir=/var/www/cgi-bin install-gitweb ;# as root
Or are you suggesting not to install 'gitweb' during 'make install'?
I'm fine with that, but I doubt I will argue about it convincingly in
a commit message.
> Or we could just fold gitweb/Makefile into the main Makefile, unlike
> gitk and git-gui it's not externally maintained, and most of it is
> shimmying to work around not being part of the main Makefile (which it
> strongly inter-depends on anyway).
next prev parent reply other threads:[~2022-05-26 21:33 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-25 20:56 [PATCH] Makefile: build 'gitweb' in the default target SZEDER Gábor
2022-05-26 0:14 ` Ævar Arnfjörð Bjarmason
2022-05-26 7:57 ` Jeff King
2022-05-26 21:33 ` SZEDER Gábor [this message]
2022-05-27 9:23 ` Ævar Arnfjörð Bjarmason
2022-05-31 17:45 ` [PATCH v2 0/7] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns Ævar Arnfjörð Bjarmason
2022-05-31 17:45 ` [PATCH v2 1/7] gitweb/Makefile: define all .PHONY prerequisites inline Ævar Arnfjörð Bjarmason
2022-05-31 17:45 ` [PATCH v2 2/7] gitweb/Makefile: add a $(GITWEB_ALL) variable Ævar Arnfjörð Bjarmason
2022-05-31 17:45 ` [PATCH v2 3/7] gitweb/Makefile: clear up and de-duplicate the gitweb.{css,js} vars Ævar Arnfjörð Bjarmason
2022-05-31 17:45 ` [PATCH v2 4/7] gitweb/Makefile: prepare to merge into top-level Makefile Ævar Arnfjörð Bjarmason
2022-05-31 17:45 ` [PATCH v2 5/7] gitweb: remove "test" and "test-installed" targets Ævar Arnfjörð Bjarmason
2022-05-31 17:45 ` [PATCH v2 6/7] gitweb/Makefile: include in top-level Makefile Ævar Arnfjörð Bjarmason
2022-05-31 17:46 ` [PATCH v2 7/7] Makefile: build 'gitweb' in the default target Ævar Arnfjörð Bjarmason
2022-06-06 17:44 ` [PATCH v2 0/7] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns Junio C Hamano
2022-06-20 8:32 ` SZEDER Gábor
2022-06-21 6:47 ` Jeff King
2022-06-22 9:27 ` Ævar Arnfjörð Bjarmason
2022-06-22 15:37 ` Junio C Hamano
2022-06-23 10:29 ` Ævar Arnfjörð Bjarmason
2022-06-23 23:25 ` Junio C Hamano
2022-06-23 23:45 ` Ævar Arnfjörð Bjarmason
2022-06-24 1:14 ` Junio C Hamano
2022-06-28 10:15 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
2022-06-28 10:15 ` [PATCH v3 1/8] gitweb/Makefile: define all .PHONY prerequisites inline Ævar Arnfjörð Bjarmason
2022-06-28 10:15 ` [PATCH v3 2/8] gitweb/Makefile: add a $(GITWEB_ALL) variable Ævar Arnfjörð Bjarmason
2022-06-28 10:15 ` [PATCH v3 3/8] gitweb/Makefile: clear up and de-duplicate the gitweb.{css,js} vars Ævar Arnfjörð Bjarmason
2022-06-28 10:15 ` [PATCH v3 4/8] gitweb/Makefile: prepare to merge into top-level Makefile Ævar Arnfjörð Bjarmason
2022-06-28 10:15 ` [PATCH v3 5/8] gitweb: remove "test" and "test-installed" targets Ævar Arnfjörð Bjarmason
2022-06-28 10:16 ` [PATCH v3 6/8] gitweb/Makefile: include in top-level Makefile Ævar Arnfjörð Bjarmason
2022-06-28 10:16 ` [PATCH v3 7/8] Makefile: build 'gitweb' in the default target Ævar Arnfjörð Bjarmason
2022-06-28 10:16 ` [PATCH v3 8/8] gitweb/Makefile: add a "NO_GITWEB" parameter Ævar Arnfjörð Bjarmason
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220526213305.GA1707@szeder.dev \
--to=szeder.dev@gmail.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
/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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).