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