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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: May 26, 2022, 21:33 UTC
Message-ID: <20220526213305.GA1707@szeder.dev>
In-Reply-To: <220526.86k0a96sv2.gmgdl@evledraar.gmail.com>

On Thu, May 26, 2022 at 02:14:33AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 56 quoted lines

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

Show 28 quoted lines

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

Show 5 quoted lines

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