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