Re: [PATCH] Makefile: build 'gitweb' in the default target

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, May 26 2022, SZEDER Gábor wrote:

> 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.

It really isn't lost in the noise, e.g. a common use-case of mine is to
have let's say a 10 patch series that I want to frequently run a spot
check like this on:

    git rebase -i -x 'make'

On my system before your patch (this is with my config.mak, so it has
e.g. NO_TCLTK=Y) it's ~105ms per run, with your change ~200ms[1].
	
So that's 1s before of "make" overhead, now 2s. Is it the end of the
world? No, but it does add up. In particular with ccache we can end up
mostly spending time on make itself deciding what it's going to do.

For comparison running EDITOR=cat git rebase -i -x 'echo make' HEAD~10
takes around 100ms in total for me.

> unrealistic case (it doesn't modify any C source files explicitly, let
> alone a frequently included header file) like this:

Yeah that's fair, of course no-op runs are where it matters the
most. But e.g. for forcing a hook.c change (including re-linking "git")
it's 10% slower[2] overall if we use ccache.

>   $ 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.

I should have said that I create a "version" file in my checkout dir,
e.g.: 2.36.GIT-dev

Otherwise you end up with a guarnteed recompile-re-link every time HEAD
changes, so it's likely lost in that noise.

I just run this (via a script):

    echo $(/usr/bin/git grep -h -o -P '(?<=^DEF_VER=v).*' 'HEAD:GIT-VERSION-GEN')-dev

And echo it to the top-level "version". Aside from what we're discussing
here you might want to try that, your "git version" won't be correct,
but for that low cost (I do generate it for the version I actually
install) you'll get actual no-op runs in the case of e.g. only modifying
tests.

>> 	$ 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.

Yes, I'm not saying that we shouldn't fix this no matter what, but just
suggesting that perhaps we come up with a better solution.

>> 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.

We could also:

 * $(error out if install is in MAKECMDGOALS and we don't have those
   generated files, that's a few-line ifdef change to the Makefile,
   "build it first".

 * Just fold gitweb/Makefile into the top-level Makefile

>> 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).

1.
	$ git hyperfine  -L rev HEAD~1,HEAD~0 -s 'make' 'make'
	Benchmark 1: make' in 'HEAD~1
	  Time (mean ± σ):     106.9 ms ±   1.3 ms    [User: 86.0 ms, System: 33.1 ms]
	  Range (min … max):   105.3 ms … 110.2 ms    27 runs
	
	Benchmark 2: make' in 'HEAD~0
	  Time (mean ± σ):     199.6 ms ±   2.5 ms    [User: 162.7 ms, System: 57.3 ms]
	  Range (min … max):   197.4 ms … 207.4 ms    15 runs
	
	Summary
	  'make' in 'HEAD~1' ran
	    1.87 ± 0.03 times faster than 'make' in 'HEAD~0'
2.
	
	$ git hyperfine -L rev HEAD~1,HEAD~0 -s 'make' 'make -W hook.c NO_TCLTK=Y CC="ccache cc"' --warmup 1 -r 5
	Benchmark 1: make -W hook.c NO_TCLTK=Y CC="ccache cc"' in 'HEAD~1
	  Time (mean ± σ):     742.7 ms ±   7.1 ms    [User: 1965.5 ms, System: 561.6 ms]
	  Range (min … max):   730.3 ms … 748.2 ms    5 runs
	 
	Benchmark 2: make -W hook.c NO_TCLTK=Y CC="ccache cc"' in 'HEAD~0
	  Time (mean ± σ):     819.2 ms ±   7.4 ms    [User: 2013.9 ms, System: 583.3 ms]
	  Range (min … max):   811.0 ms … 830.0 ms    5 runs
	 
	Summary
	  'make -W hook.c NO_TCLTK=Y CC="ccache cc"' in 'HEAD~1' ran
	    1.10 ± 0.01 times faster than 'make -W hook.c NO_TCLTK=Y CC="ccache cc"' in 'HEAD~0'