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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 26, 2022, 00:14 UTC
Message-ID: <220526.86k0a96sv2.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220525205651.825669-1-szeder.dev@gmail.com>

On Wed, May 25 2022, SZEDER Gábor wrote:

Show 44 quoted lines

> 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:
	
	$ 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, 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 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).