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

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

[SZEDER Gábor]

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

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

[Ævar Arnfjörð Bjarmason]

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

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

[Jeff King]

On Thu, May 26, 2022 at 02:14:33AM +0200, Ævar Arnfjörð Bjarmason 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):
> [...]
> 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:

Yes, I don't think building gitweb is worth the performance cost.
Speeding things up was part of my original goal in e25c7cc146. It would
be one thing if this were a recent change and somebody might be broken
or confused by it not being built by default. But after 7 years, I think
the question is: why _would_ we want to change the status quo and build
gitweb by default?

To exercise its Makefile for bugs, I guess, but IMHO it is not worth
inflicting that on random developers. People who care about gitweb (if
any) can build it themselves. I'd be even happier if it were just
carried in its own tree.

-Peff

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

[SZEDER Gábor]

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

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

[Ævar Arnfjörð Bjarmason]

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'
	

[PATCH v2 0/7] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns

[Ævar Arnfjörð Bjarmason]

The $subject is a proposed re-roll of SZEDER's
https://lore.kernel.org/git/20220525205651.825669-1-szeder.dev@gmail.com;
As noted downthread of that fix having the Makefile invoke "make -C
gitweb" again would slow us down on NOOP runs by quite a bit.

This series fixes the same regression with an amended 7/7 version of
SZEDER's fix, but starts out by having the top-level Makefile include
the gitweb/Makefile.

This series is smaller than it looks, most of the commits preceding
5-6/7 are there to make that diff smaller and easier to read, by
splitting up earlier changes into non-functional changes.

For this re-roll the equivalent of the "git hyperfine" command I
posted in [2] will return;

	Summary
	  'make NO_TCLTK=Y' in 'origin/master' ran
	    1.00 ± 0.11 times faster than 'make NO_TCLTK=Y' in 'HEAD~0'
	    2.64 ± 0.26 times faster than 'make NO_TCLTK=' in 'origin/master'
	    2.64 ± 0.24 times faster than 'make NO_TCLTK=' in 'HEAD~0'

I.e. we are no slower or faster than before, but now "make && sudo
make install-gitweb" will only copy already-generated files from the
"make" command in the "sudo" step, as intended.

1. https://lore.kernel.org/git/20220525205651.825669-1-szeder.dev@gmail.com
2. https://lore.kernel.org/git/220526.86k0a96sv2.gmgdl@evledraar.gmail.com/

SZEDER Gábor (1):
  Makefile: build 'gitweb' in the default target

Ævar Arnfjörð Bjarmason (6):
  gitweb/Makefile: define all .PHONY prerequisites inline
  gitweb/Makefile: add a $(GITWEB_ALL) variable
  gitweb/Makefile: clear up and de-duplicate the gitweb.{css,js} vars
  gitweb/Makefile: prepare to merge into top-level Makefile
  gitweb: remove "test" and "test-installed" targets
  gitweb/Makefile: include in top-level Makefile

 Makefile        |  24 ++++----
 gitweb/Makefile | 143 +++++++++++++++---------------------------------
 t/Makefile      |   4 --
 3 files changed, 59 insertions(+), 112 deletions(-)

Range-diff against v1:
1:  1bbffa8a2b6 < -:  ----------- Makefile: build 'gitweb' in the default target
-:  ----------- > 1:  14361617ca6 gitweb/Makefile: define all .PHONY prerequisites inline
-:  ----------- > 2:  7d920a13518 gitweb/Makefile: add a $(GITWEB_ALL) variable
-:  ----------- > 3:  e14a5b73061 gitweb/Makefile: clear up and de-duplicate the gitweb.{css,js} vars
-:  ----------- > 4:  02e26ca8ce2 gitweb/Makefile: prepare to merge into top-level Makefile
-:  ----------- > 5:  caf376f3dd9 gitweb: remove "test" and "test-installed" targets
-:  ----------- > 6:  b423cd58f6b gitweb/Makefile: include in top-level Makefile
-:  ----------- > 7:  69428540886 Makefile: build 'gitweb' in the default target
-- 
2.36.1.1103.g036c05811b0

[PATCH v2 1/7] gitweb/Makefile: define all .PHONY prerequisites inline

[Ævar Arnfjörð Bjarmason]

Move the '.PHONY' definition so that it's split up and accompanies the
relevant as they're defined. This will make a subsequent diff smaller
as we'll remove some of these, and won't need to re-edit the
now-removed '.PHONY' line.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 gitweb/Makefile | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/gitweb/Makefile b/gitweb/Makefile
index f13e23c4de4..abb5c9f9ab6 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -1,5 +1,6 @@
 # The default target of this Makefile is...
 all::
+.PHONY: all
 
 # Define V=1 to have a more verbose compile.
 #
@@ -45,6 +46,7 @@ HIGHLIGHT_BIN = highlight
 -include config.mak
 
 # determine version
+.PHONY: .FORCE-GIT-VERSION-FILE
 ../GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE
 
@@ -152,6 +154,7 @@ GITWEB_REPLACE = \
 	-e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
 	-e 's|++HIGHLIGHT_BIN++|$(HIGHLIGHT_BIN)|g'
 
+.PHONY: FORCE
 GITWEB-BUILD-OPTIONS: FORCE
 	@rm -f $@+
 	@echo "x" '$(PERL_PATH_SQ)' $(GITWEB_REPLACE) "$(JSMIN)|$(CSSMIN)" >$@+
@@ -171,15 +174,18 @@ static/gitweb.js: $(GITWEB_JSLIB_FILES)
 
 ### Testing rules
 
+.PHONY: test
 test:
 	$(MAKE) -C ../t gitweb-test
 
+.PHONY: test-installed
 test-installed:
 	GITWEB_TEST_INSTALLED='$(DESTDIR_SQ)$(gitwebdir_SQ)' \
 		$(MAKE) -C ../t gitweb-test
 
 ### Installation rules
 
+.PHONY: install
 install: all
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebdir_SQ)'
 	$(INSTALL) -m 755 $(GITWEB_PROGRAMS) '$(DESTDIR_SQ)$(gitwebdir_SQ)'
@@ -188,10 +194,8 @@ install: all
 
 ### Cleaning rules
 
+.PHONY: clean
 clean:
 	$(RM) gitweb.cgi static/gitweb.js \
 		static/gitweb.min.js static/gitweb.min.css \
 		GITWEB-BUILD-OPTIONS
-
-.PHONY: all clean install test test-installed .FORCE-GIT-VERSION-FILE FORCE
-
-- 
2.36.1.1103.g036c05811b0

[PATCH v2 2/7] gitweb/Makefile: add a $(GITWEB_ALL) variable

[Ævar Arnfjörð Bjarmason]

Declare the targets that the "all" target depends on with a new
$(GITWEB_ALL) variable. This will help to reduce churn in subsequent
commits.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 gitweb/Makefile | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gitweb/Makefile b/gitweb/Makefile
index abb5c9f9ab6..733b60f925e 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -54,6 +54,11 @@ ifneq ($(MAKECMDGOALS),clean)
 -include ../GIT-VERSION-FILE
 endif
 
+# What targets we'll add to 'all' for "make gitweb"
+GITWEB_ALL =
+GITWEB_ALL += gitweb.cgi
+GITWEB_ALL += static/gitweb.js
+
 ### Build rules
 
 SHELL_PATH ?= $(SHELL)
@@ -92,7 +97,7 @@ ifndef V
 endif
 endif
 
-all:: gitweb.cgi static/gitweb.js
+all:: $(GITWEB_ALL)
 
 GITWEB_PROGRAMS = gitweb.cgi
 
-- 
2.36.1.1103.g036c05811b0

[PATCH v2 3/7] gitweb/Makefile: clear up and de-duplicate the gitweb.{css,js} vars

[Ævar Arnfjörð Bjarmason]

Change the variable definitions for the $(GITWEB_CSS) and $(GITWEB_JS)
so that we have a clear separation between what we use as "in" files,
v.s. our "min" files. We can now make the appending to $(GITWEB_FILES)
unconditional, since $(GITWEB_{JS,CSS}) is either the "min" or
non-"min" version. This reduces the duplication within the file.

While we're at it let's initialize "GITWEB_JSLIB_FILES" as we normally
do with such variables.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 gitweb/Makefile | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/gitweb/Makefile b/gitweb/Makefile
index 733b60f925e..a8752c1f11c 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -31,10 +31,12 @@ GITWEB_STRICT_EXPORT =
 GITWEB_BASE_URL =
 GITWEB_LIST =
 GITWEB_HOMETEXT = indextext.html
-GITWEB_CSS = static/gitweb.css
+GITWEB_CSS_IN = static/gitweb.css
+GITWEB_CSS = $(GITWEB_CSS_IN)
 GITWEB_LOGO = static/git-logo.png
 GITWEB_FAVICON = static/git-favicon.png
-GITWEB_JS = static/gitweb.js
+GITWEB_JS_IN = static/gitweb.js
+GITWEB_JS = $(GITWEB_JS_IN)
 GITWEB_SITE_HTML_HEAD_STRING =
 GITWEB_SITE_HEADER =
 GITWEB_SITE_FOOTER =
@@ -57,7 +59,7 @@ endif
 # What targets we'll add to 'all' for "make gitweb"
 GITWEB_ALL =
 GITWEB_ALL += gitweb.cgi
-GITWEB_ALL += static/gitweb.js
+GITWEB_ALL += $(GITWEB_JS)
 
 ### Build rules
 
@@ -101,25 +103,23 @@ all:: $(GITWEB_ALL)
 
 GITWEB_PROGRAMS = gitweb.cgi
 
+GITWEB_JS_MIN = static/gitweb.min.js
 ifdef JSMIN
-GITWEB_FILES += static/gitweb.min.js
-GITWEB_JS = static/gitweb.min.js
-all:: static/gitweb.min.js
-static/gitweb.min.js: static/gitweb.js GITWEB-BUILD-OPTIONS
+GITWEB_JS = $(GITWEB_JS_MIN)
+all:: $(GITWEB_JS_MIN)
+$(GITWEB_JS_MIN): $(GITWEB_JS_IN) GITWEB-BUILD-OPTIONS
 	$(QUIET_GEN)$(JSMIN) <$< >$@
-else
-GITWEB_FILES += static/gitweb.js
 endif
+GITWEB_FILES += $(GITWEB_JS)
 
+GITWEB_CSS_MIN = static/gitweb.min.css
 ifdef CSSMIN
-GITWEB_FILES += static/gitweb.min.css
-GITWEB_CSS = static/gitweb.min.css
-all:: static/gitweb.min.css
-static/gitweb.min.css: static/gitweb.css GITWEB-BUILD-OPTIONS
+GITWEB_CSS = $(GITWEB_CSS_MIN)
+all:: $(GITWEB_CSS_MIN)
+$(GITWEB_CSS_MIN): $(GITWEB_CSS_IN) GITWEB-BUILD-OPTIONS
 	$(QUIET_GEN)$(CSSMIN) <$< >$@
-else
-GITWEB_FILES += static/gitweb.css
 endif
+GITWEB_FILES += $(GITWEB_CSS)
 
 GITWEB_FILES += static/git-logo.png static/git-favicon.png
 
@@ -127,6 +127,7 @@ GITWEB_FILES += static/git-logo.png static/git-favicon.png
 #
 # js/lib/common-lib.js should be always first, then js/lib/*.js,
 # then the rest of files; js/gitweb.js should be last (if it exists)
+GITWEB_JSLIB_FILES =
 GITWEB_JSLIB_FILES += static/js/lib/common-lib.js
 GITWEB_JSLIB_FILES += static/js/lib/datetime.js
 GITWEB_JSLIB_FILES += static/js/lib/cookies.js
@@ -201,6 +202,6 @@ install: all
 
 .PHONY: clean
 clean:
-	$(RM) gitweb.cgi static/gitweb.js \
-		static/gitweb.min.js static/gitweb.min.css \
+	$(RM) gitweb.cgi $(GITWEB_JS_IN) \
+		$(GITWEB_JS_MIN) $(GITWEB_CSS_MIN) \
 		GITWEB-BUILD-OPTIONS
-- 
2.36.1.1103.g036c05811b0

[PATCH v2 4/7] gitweb/Makefile: prepare to merge into top-level Makefile

[Ævar Arnfjörð Bjarmason]

Since the "gitweb/Makefile" was split out from the top-level Makefile
in 62331ef1637 (gitweb: Makefile improvements, 2010-01-30) we've kept
the inter-dependencies between the two, and worse have dealt with a
lot of duplication as a result.

In preparation for merging the two again add a MAK_DIR_GITWEB variable
to various rules in it. This will allow us to set this variable to
"gitweb/" as we include it in the top-level Makefile, which will
minimize the size of the subsequent diff.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 gitweb/Makefile | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/gitweb/Makefile b/gitweb/Makefile
index a8752c1f11c..74e896767e9 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -2,6 +2,8 @@
 all::
 .PHONY: all
 
+MAK_DIR_GITWEB =
+
 # Define V=1 to have a more verbose compile.
 #
 # Define JSMIN to point to JavaScript minifier that functions as
@@ -106,8 +108,9 @@ GITWEB_PROGRAMS = gitweb.cgi
 GITWEB_JS_MIN = static/gitweb.min.js
 ifdef JSMIN
 GITWEB_JS = $(GITWEB_JS_MIN)
-all:: $(GITWEB_JS_MIN)
-$(GITWEB_JS_MIN): $(GITWEB_JS_IN) GITWEB-BUILD-OPTIONS
+all:: $(MAK_DIR_GITWEB)$(GITWEB_JS_MIN)
+$(MAK_DIR_GITWEB)$(GITWEB_JS_MIN): $(MAK_DIR_GITWEB)GITWEB-BUILD-OPTIONS
+$(MAK_DIR_GITWEB)$(GITWEB_JS_MIN): $(MAK_DIR_GITWEB)$(GITWEB_JS_IN)
 	$(QUIET_GEN)$(JSMIN) <$< >$@
 endif
 GITWEB_FILES += $(GITWEB_JS)
@@ -115,8 +118,9 @@ GITWEB_FILES += $(GITWEB_JS)
 GITWEB_CSS_MIN = static/gitweb.min.css
 ifdef CSSMIN
 GITWEB_CSS = $(GITWEB_CSS_MIN)
-all:: $(GITWEB_CSS_MIN)
-$(GITWEB_CSS_MIN): $(GITWEB_CSS_IN) GITWEB-BUILD-OPTIONS
+all:: $(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN)
+$(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN): $(MAK_DIR_GITWEB)GITWEB-BUILD-OPTIONS
+$(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN): $(MAK_DIR_GITWEB)$(GITWEB_CSS_IN)
 	$(QUIET_GEN)$(CSSMIN) <$< >$@
 endif
 GITWEB_FILES += $(GITWEB_CSS)
@@ -161,19 +165,20 @@ GITWEB_REPLACE = \
 	-e 's|++HIGHLIGHT_BIN++|$(HIGHLIGHT_BIN)|g'
 
 .PHONY: FORCE
-GITWEB-BUILD-OPTIONS: FORCE
+$(MAK_DIR_GITWEB)GITWEB-BUILD-OPTIONS: FORCE
 	@rm -f $@+
 	@echo "x" '$(PERL_PATH_SQ)' $(GITWEB_REPLACE) "$(JSMIN)|$(CSSMIN)" >$@+
 	@cmp -s $@+ $@ && rm -f $@+ || mv -f $@+ $@
 
-gitweb.cgi: gitweb.perl GITWEB-BUILD-OPTIONS
+$(MAK_DIR_GITWEB)gitweb.cgi: $(MAK_DIR_GITWEB)GITWEB-BUILD-OPTIONS
+$(MAK_DIR_GITWEB)gitweb.cgi: $(MAK_DIR_GITWEB)gitweb.perl
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
 		$(GITWEB_REPLACE) $< >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
 
-static/gitweb.js: $(GITWEB_JSLIB_FILES)
+$(MAK_DIR_GITWEB)static/gitweb.js: $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_JSLIB_FILES))
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	cat $^ >$@+ && \
 	mv $@+ $@
@@ -194,14 +199,16 @@ test-installed:
 .PHONY: install
 install: all
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebdir_SQ)'
-	$(INSTALL) -m 755 $(GITWEB_PROGRAMS) '$(DESTDIR_SQ)$(gitwebdir_SQ)'
+	$(INSTALL) -m 755 $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_PROGRAMS)) \
+		'$(DESTDIR_SQ)$(gitwebdir_SQ)'
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
-	$(INSTALL) -m 644 $(GITWEB_FILES) '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
+	$(INSTALL) -m 644 $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_FILES)) \
+		'$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
 
 ### Cleaning rules
 
 .PHONY: clean
 clean:
-	$(RM) gitweb.cgi $(GITWEB_JS_IN) \
+	$(RM) $(addprefix $(MAK_DIR_GITWEB),gitweb.cgi $(GITWEB_JS_IN) \
 		$(GITWEB_JS_MIN) $(GITWEB_CSS_MIN) \
-		GITWEB-BUILD-OPTIONS
+		GITWEB-BUILD-OPTIONS)
-- 
2.36.1.1103.g036c05811b0

[PATCH v2 5/7] gitweb: remove "test" and "test-installed" targets

[Ævar Arnfjörð Bjarmason]

Remove the special "test" targets for gitweb added in
958a8467216 (gitweb/Makefile: Add 'test' and 'test-installed' targets,
2010-09-26). Unlike e.g. "contrib/scalar" and "contrib/subtree" the
"gitweb" tests themselves live in our top-level t/ directory.

It therefore doesn't make sense to maintain this indirection, no more
than it would to have a "git-send-email-test". By dropping it we'll
also free other tests to use the t95*.sh prefix.

These removed targets are unlikely to be used by anyone, and to the
extent that they are we can easily use an invocation like this
instead:

	make test T='t[0-9]*gitweb*.sh'

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 gitweb/Makefile | 11 -----------
 t/Makefile      |  4 ----
 2 files changed, 15 deletions(-)

diff --git a/gitweb/Makefile b/gitweb/Makefile
index 74e896767e9..eaf0cfcf80e 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -183,17 +183,6 @@ $(MAK_DIR_GITWEB)static/gitweb.js: $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_JSLIB_
 	cat $^ >$@+ && \
 	mv $@+ $@
 
-### Testing rules
-
-.PHONY: test
-test:
-	$(MAKE) -C ../t gitweb-test
-
-.PHONY: test-installed
-test-installed:
-	GITWEB_TEST_INSTALLED='$(DESTDIR_SQ)$(gitwebdir_SQ)' \
-		$(MAKE) -C ../t gitweb-test
-
 ### Installation rules
 
 .PHONY: install
diff --git a/t/Makefile b/t/Makefile
index 056ce55dcc9..7f56e52f767 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -35,7 +35,6 @@ TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
 CHAINLINTTMP_SQ = $(subst ','\'',$(CHAINLINTTMP))
 
 T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
-TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
 THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))
 TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
 CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
@@ -112,9 +111,6 @@ aggregate-results:
 		echo "$$f"; \
 	done | '$(SHELL_PATH_SQ)' ./aggregate-results.sh
 
-gitweb-test:
-	$(MAKE) $(TGITWEB)
-
 valgrind:
 	$(MAKE) GIT_TEST_OPTS="$(GIT_TEST_OPTS) --valgrind"
 
-- 
2.36.1.1103.g036c05811b0

[PATCH v2 6/7] gitweb/Makefile: include in top-level Makefile

[Ævar Arnfjörð Bjarmason]

Include the gitweb/Makefile in the top-level Makefile rather than
calling it as a sub-Makefile. As noted in the thread starting at at
[1] (in particular [2]) we'll pay a high cost on NOOP runs of "make"
just to figure out that we have nothing to do for "make gitweb".

The "gitweb" script also isn't maintained out-of-tree, unlike
"gitk-git" or "git-gui", which both have their own "Makefile". Other
parts of it are already integrated into our main Makefiles, e.g. the
documentation is built by Documentation/Makefile since
07ea4df2780 (gitweb: Add gitweb(1) manpage for gitweb itself,
2011-10-16).

1. https://lore.kernel.org/git/20220525205651.825669-1-szeder.dev@gmail.com/
2. https://lore.kernel.org/git/220526.86k0a96sv2.gmgdl@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Perhaps a better solution is to simply move all of this code to the
top-level gitweb/Makefile, but on the other hand being able to include
the gitweb/Makefile to fold it into the top-level one without a larger
diff makes this sort of change much easier to review.

And, as shown here we'll have "make -C gitweb" now $(error) out, so
there's no chance of confusion in practice.

 Makefile        | 23 +++++++------
 gitweb/Makefile | 91 +++++++++----------------------------------------
 2 files changed, 29 insertions(+), 85 deletions(-)

diff --git a/Makefile b/Makefile
index 18ca6744a50..d251838c554 100644
--- a/Makefile
+++ b/Makefile
@@ -538,6 +538,7 @@ gitexecdir = libexec/git-core
 mergetoolsdir = $(gitexecdir)/mergetools
 sharedir = $(prefix)/share
 gitwebdir = $(sharedir)/gitweb
+gitwebstaticdir = $(gitwebdir)/static
 perllibdir = $(sharedir)/perl5
 localedir = $(sharedir)/locale
 template_dir = share/git-core/templates
@@ -556,7 +557,7 @@ localedir_relative = $(patsubst $(prefix)/%,%,$(localedir))
 htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
 perllibdir_relative = $(patsubst $(prefix)/%,%,$(perllibdir))
 
-export prefix bindir sharedir sysconfdir gitwebdir perllibdir localedir
+export prefix bindir sharedir sysconfdir perllibdir localedir
 
 # Set our default programs
 CC = cc
@@ -2071,6 +2072,7 @@ htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
 prefix_SQ = $(subst ','\'',$(prefix))
 perllibdir_relative_SQ = $(subst ','\'',$(perllibdir_relative))
 gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
+gitwebstaticdir_SQ = $(subst ','\'',$(gitwebstaticdir))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH))
@@ -2399,10 +2401,6 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
 perllibdir:
 	@echo '$(perllibdir_SQ)'
 
-.PHONY: gitweb
-gitweb:
-	$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all
-
 git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	chmod +x $@+ && \
@@ -3036,6 +3034,15 @@ coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.c
 
 .PHONY: coccicheck coccicheck-pending
 
+# "Sub"-Makefiles, not really because they can't be run stand-alone,
+# only there to contain directory-specific rules and variables
+## gitweb/Makefile inclusion:
+MAK_DIR_GITWEB = gitweb/
+include gitweb/Makefile
+
+.PHONY: gitweb
+gitweb: $(MAK_DIR_GITWEB_ALL)
+
 ### Installation rules
 
 ifneq ($(filter /%,$(firstword $(template_dir))),)
@@ -3108,7 +3115,6 @@ ifndef NO_PERL
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perllibdir_SQ)'
 	(cd perl/build/lib && $(TAR) cf - .) | \
 	(cd '$(DESTDIR_SQ)$(perllibdir_SQ)' && umask 022 && $(TAR) xof -)
-	$(MAKE) -C gitweb install
 endif
 ifndef NO_TCLTK
 	$(MAKE) -C gitk-git install
@@ -3163,10 +3169,8 @@ endif
 		  cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
 	done
 
-.PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf
+.PHONY: install-doc install-man install-man-perl install-html install-info install-pdf
 .PHONY: quick-install-doc quick-install-man quick-install-html
-install-gitweb:
-	$(MAKE) -C gitweb install
 
 install-doc: install-man-perl
 	$(MAKE) -C Documentation install
@@ -3310,7 +3314,6 @@ clean: profile-clean coverage-clean cocciclean
 	$(MAKE) -C Documentation/ clean
 	$(RM) Documentation/GIT-EXCLUDED-PROGRAMS
 ifndef NO_PERL
-	$(MAKE) -C gitweb clean
 	$(RM) -r perl/build/
 endif
 	$(MAKE) -C templates/ clean
diff --git a/gitweb/Makefile b/gitweb/Makefile
index eaf0cfcf80e..66fceb9e94f 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -1,11 +1,8 @@
-# The default target of this Makefile is...
-all::
-.PHONY: all
-
-MAK_DIR_GITWEB =
+ifndef MAK_DIR_GITWEB
+$(error do not run gitweb/Makefile stand-alone anymore. The "gitweb" and \
+"install-gitweb" targets now live in the top-level Makefile)
+endif
 
-# Define V=1 to have a more verbose compile.
-#
 # Define JSMIN to point to JavaScript minifier that functions as
 # a filter to have static/gitweb.js minified.
 #
@@ -13,13 +10,6 @@ MAK_DIR_GITWEB =
 # version of static/gitweb.css
 #
 
-prefix ?= $(HOME)
-bindir ?= $(prefix)/bin
-gitwebdir ?= /var/www/cgi-bin
-
-RM ?= rm -f
-INSTALL ?= install
-
 # default configuration for gitweb
 GITWEB_CONFIG = gitweb_config.perl
 GITWEB_CONFIG_SYSTEM = /etc/gitweb.conf
@@ -44,71 +34,19 @@ GITWEB_SITE_HEADER =
 GITWEB_SITE_FOOTER =
 HIGHLIGHT_BIN = highlight
 
-# include user config
--include ../config.mak.autogen
--include ../config.mak
--include config.mak
-
-# determine version
-.PHONY: .FORCE-GIT-VERSION-FILE
-../GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
-	$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE
-
-ifneq ($(MAKECMDGOALS),clean)
--include ../GIT-VERSION-FILE
-endif
-
 # What targets we'll add to 'all' for "make gitweb"
 GITWEB_ALL =
 GITWEB_ALL += gitweb.cgi
 GITWEB_ALL += $(GITWEB_JS)
 
-### Build rules
-
-SHELL_PATH ?= $(SHELL)
-PERL_PATH  ?= /usr/bin/perl
-
-# Shell quote;
-bindir_SQ = $(subst ','\'',$(bindir))#'
-gitwebdir_SQ = $(subst ','\'',$(gitwebdir))#'
-gitwebstaticdir_SQ = $(subst ','\'',$(gitwebdir)/static)#'
-SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))#'
-PERL_PATH_SQ  = $(subst ','\'',$(PERL_PATH))#'
-DESTDIR_SQ    = $(subst ','\'',$(DESTDIR))#'
-
-# Quiet generation (unless V=1)
-QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
-QUIET_SUBDIR1  =
-
-ifneq ($(findstring $(MAKEFLAGS),w),w)
-PRINT_DIR = --no-print-directory
-else # "make -w"
-NO_SUBDIR = :
-endif
-
-ifneq ($(findstring $(MAKEFLAGS),s),s)
-ifndef V
-	QUIET          = @
-	QUIET_GEN      = $(QUIET)echo '   ' GEN $@;
-	QUIET_SUBDIR0  = +@subdir=
-	QUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
-	                 $(MAKE) $(PRINT_DIR) -C $$subdir
-	export V
-	export QUIET
-	export QUIET_GEN
-	export QUIET_SUBDIR0
-	export QUIET_SUBDIR1
-endif
-endif
-
-all:: $(GITWEB_ALL)
+MAK_DIR_GITWEB_ALL = $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_ALL))
 
 GITWEB_PROGRAMS = gitweb.cgi
 
 GITWEB_JS_MIN = static/gitweb.min.js
 ifdef JSMIN
 GITWEB_JS = $(GITWEB_JS_MIN)
-all:: $(MAK_DIR_GITWEB)$(GITWEB_JS_MIN)
+GITWEB_ALL += $(MAK_DIR_GITWEB)$(GITWEB_JS_MIN)
 $(MAK_DIR_GITWEB)$(GITWEB_JS_MIN): $(MAK_DIR_GITWEB)GITWEB-BUILD-OPTIONS
 $(MAK_DIR_GITWEB)$(GITWEB_JS_MIN): $(MAK_DIR_GITWEB)$(GITWEB_JS_IN)
 	$(QUIET_GEN)$(JSMIN) <$< >$@
@@ -118,7 +56,7 @@ GITWEB_FILES += $(GITWEB_JS)
 GITWEB_CSS_MIN = static/gitweb.min.css
 ifdef CSSMIN
 GITWEB_CSS = $(GITWEB_CSS_MIN)
-all:: $(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN)
+GITWEB_ALL += $(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN)
 $(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN): $(MAK_DIR_GITWEB)GITWEB-BUILD-OPTIONS
 $(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN): $(MAK_DIR_GITWEB)$(GITWEB_CSS_IN)
 	$(QUIET_GEN)$(CSSMIN) <$< >$@
@@ -185,19 +123,22 @@ $(MAK_DIR_GITWEB)static/gitweb.js: $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_JSLIB_
 
 ### Installation rules
 
-.PHONY: install
-install: all
+.PHONY: install-gitweb
+install-gitweb: $(MAK_DIR_GITWEB_ALL)
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebdir_SQ)'
-	$(INSTALL) -m 755 $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_PROGRAMS)) \
-		'$(DESTDIR_SQ)$(gitwebdir_SQ)'
+	$(INSTALL) -m 755 $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_PROGRAMS)) '$(DESTDIR_SQ)$(gitwebdir_SQ)'
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
 	$(INSTALL) -m 644 $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_FILES)) \
 		'$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
+ifndef NO_PERL
+install: install-gitweb
+endif
 
 ### Cleaning rules
 
-.PHONY: clean
-clean:
+.PHONY: gitweb-clean
+gitweb-clean:
 	$(RM) $(addprefix $(MAK_DIR_GITWEB),gitweb.cgi $(GITWEB_JS_IN) \
 		$(GITWEB_JS_MIN) $(GITWEB_CSS_MIN) \
 		GITWEB-BUILD-OPTIONS)
+clean: gitweb-clean
-- 
2.36.1.1103.g036c05811b0

[PATCH v2 7/7] Makefile: build 'gitweb' in the default target

[Ævar Arnfjörð Bjarmason]

From: SZEDER Gábor <szeder.dev@gmail.com>

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>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index d251838c554..63057b10634 100644
--- a/Makefile
+++ b/Makefile
@@ -3042,6 +3042,7 @@ include gitweb/Makefile
 
 .PHONY: gitweb
 gitweb: $(MAK_DIR_GITWEB_ALL)
+all:: gitweb
 
 ### Installation rules
 
-- 
2.36.1.1103.g036c05811b0

Re: [PATCH v2 0/7] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The $subject is a proposed re-roll of SZEDER's
> https://lore.kernel.org/git/20220525205651.825669-1-szeder.dev@gmail.com;
> As noted downthread of that fix having the Makefile invoke "make -C
> gitweb" again would slow us down on NOOP runs by quite a bit.

It would be nice to hear comments SZEDER and others, even if the
comments are clear negative or positive.

Thanks.

Re: [PATCH v2 0/7] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns

[SZEDER Gábor]

On Mon, Jun 06, 2022 at 10:44:54AM -0700, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
> > The $subject is a proposed re-roll of SZEDER's
> > https://lore.kernel.org/git/20220525205651.825669-1-szeder.dev@gmail.com;
> > As noted downthread of that fix having the Makefile invoke "make -C
> > gitweb" again would slow us down on NOOP runs by quite a bit.
> 
> It would be nice to hear comments SZEDER and others, even if the
> comments are clear negative or positive.

Well, my itch is scratched, so I'm fine with it :)

I think Peff has a point by questioning whether we should build and
install gitweb by default...  I don't have an opinion about that, but
if we do want to build it by default, then IMO doing it in the main
Makefile is the way to go, so I think in that case this patch series
goes in the right direction.


On a related note: this is not the first time when something was not
generated during a regular 'make' but only during 'sudo make install',
see e.g. 2530afd351 (Makefile: generate Git(3pm) as dependency of the
'doc' and 'man' targets, 2018-02-15).  Perhaps it would be worth to
extend the CI jobs with a 'make install/install-doc' command, and
error out if it generated any files that were not present before, so
we could catch similar issues early on.

Re: [PATCH v2 0/7] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns

[Jeff King]

On Mon, Jun 20, 2022 at 10:32:02AM +0200, SZEDER Gábor wrote:

> On Mon, Jun 06, 2022 at 10:44:54AM -0700, Junio C Hamano wrote:
> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> > 
> > > The $subject is a proposed re-roll of SZEDER's
> > > https://lore.kernel.org/git/20220525205651.825669-1-szeder.dev@gmail.com;
> > > As noted downthread of that fix having the Makefile invoke "make -C
> > > gitweb" again would slow us down on NOOP runs by quite a bit.
> > 
> > It would be nice to hear comments SZEDER and others, even if the
> > comments are clear negative or positive.
> 
> Well, my itch is scratched, so I'm fine with it :)
> 
> I think Peff has a point by questioning whether we should build and
> install gitweb by default...  I don't have an opinion about that, but
> if we do want to build it by default, then IMO doing it in the main
> Makefile is the way to go, so I think in that case this patch series
> goes in the right direction.

I hadn't realized the full situation when I was arguing earlier that "we
have not been building it for several years". You raised the point that
we do auto-build it in "make install", so it would be a change of
behavior to stop doing so.

I still find it hard to care too much about backwards compatibility for
building gitweb (or really gitweb at all, for that matter). But my main
complaint was foisting another recursive Makefile and its performance
and troubles on developers at large, and I think Ævar's patches deal
with it. So I'm OK with the direction.

I admit I didn't look _too_ closely at them, but they overall seemed
sensible to me. Two things I noted:

  - I wondered if "make NO_PERL=1" would complain about "gitweb" being
    in the default targets. It doesn't, but it does actually build
    gitweb, which seems a little weird. I don't think we actually rely
    on perl during the build (e.g., no "perl -c" checks or anything),
    and the t950x tests seem to respect NO_PERL and avoid running the
    generated file. So maybe it's OK?

  - Speaking of backwards compatibility: after this series, "cd gitweb
    && make" yields an error. It's got a nice message telling you what
    to do, but it's likely breaking distro scripts. Again, I'm not sure
    I care, but if the point of the exercise was to avoid breaking
    things, well...

-Peff

Re: [PATCH v2 0/7] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 21 2022, Jeff King wrote:

> On Mon, Jun 20, 2022 at 10:32:02AM +0200, SZEDER Gábor wrote:
>
>> On Mon, Jun 06, 2022 at 10:44:54AM -0700, Junio C Hamano wrote:
>> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>> > 
>> > > The $subject is a proposed re-roll of SZEDER's
>> > > https://lore.kernel.org/git/20220525205651.825669-1-szeder.dev@gmail.com;
>> > > As noted downthread of that fix having the Makefile invoke "make -C
>> > > gitweb" again would slow us down on NOOP runs by quite a bit.
>> > 
>> > It would be nice to hear comments SZEDER and others, even if the
>> > comments are clear negative or positive.
>> 
>> Well, my itch is scratched, so I'm fine with it :)
>> 
>> I think Peff has a point by questioning whether we should build and
>> install gitweb by default...  I don't have an opinion about that, but
>> if we do want to build it by default, then IMO doing it in the main
>> Makefile is the way to go, so I think in that case this patch series
>> goes in the right direction.
>
> I hadn't realized the full situation when I was arguing earlier that "we
> have not been building it for several years". You raised the point that
> we do auto-build it in "make install", so it would be a change of
> behavior to stop doing so.
>
> I still find it hard to care too much about backwards compatibility for
> building gitweb (or really gitweb at all, for that matter). But my main
> complaint was foisting another recursive Makefile and its performance
> and troubles on developers at large, and I think Ævar's patches deal
> with it. So I'm OK with the direction.
>
> I admit I didn't look _too_ closely at them, but they overall seemed
> sensible to me. Two things I noted:
>
>   - I wondered if "make NO_PERL=1" would complain about "gitweb" being
>     in the default targets. It doesn't, but it does actually build
>     gitweb, which seems a little weird. I don't think we actually rely
>     on perl during the build (e.g., no "perl -c" checks or anything),
>     and the t950x tests seem to respect NO_PERL and avoid running the
>     generated file. So maybe it's OK?

I think it's arguably a bug, but as you note we build/test etc. without
errors, and I think it's restoring the state before e25c7cc146
(Makefile: drop dependency between git-instaweb and gitweb, 2015-05-29).

Arguably we should replace with a stub script like git-svn et al, and
arguably we should leave it, as you're more likely to e.g. run gitweb on
a webserver, so even if you build a "no perl" package, perhaps it's
convenient to have "gitweb" part of it, and then on that one box that
runs it you'll install perl...

>   - Speaking of backwards compatibility: after this series, "cd gitweb
>     && make" yields an error. It's got a nice message telling you what
>     to do, but it's likely breaking distro scripts. Again, I'm not sure
>     I care, but if the point of the exercise was to avoid breaking
>     things, well...

I think that's OK, having maintained those sorts of build scripts in a
past life.

I.e. when you upgrade the package it's a minor hassle, and the error
tells you exactly what to do, and the fix is a 2-3 lines in your recipe
at most.

I could make gitweb/Makefile "fake it", but as argued in the patches I
think this trade-off makes more sense. Having it run in some "dual mode"
would be a maintenance hassle.

Most of the reason for keeping gitweb/Makefile around (as opposed to the
top-level Makefile absorbing it) was to be able to emit that message to
be friendly to downstream packagers.

Re: [PATCH v2 0/7] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>   - I wondered if "make NO_PERL=1" would complain about "gitweb" being
>>     in the default targets. It doesn't, but it does actually build
>>     gitweb, which seems a little weird. I don't think we actually rely
>>     on perl during the build (e.g., no "perl -c" checks or anything),
>>     and the t950x tests seem to respect NO_PERL and avoid running the
>>     generated file. So maybe it's OK?
>
> I think it's arguably a bug, but as you note we build/test etc. without
> errors, and I think it's restoring the state before e25c7cc146
> (Makefile: drop dependency between git-instaweb and gitweb, 2015-05-29).
>
> Arguably we should replace with a stub script like git-svn et al, and
> arguably we should leave it, as you're more likely to e.g. run gitweb on
> a webserver, so even if you build a "no perl" package, perhaps it's
> convenient to have "gitweb" part of it, and then on that one box that
> runs it you'll install perl...

It is perfectly acceptable to "make DESTDIR=... install" and tar up
the result on a host with NO_PERL and extract it on the target that
is capable to run gitweb, isn't it?  As long as "make NO_PERL=1"
gives exactly the gitweb as a build without NO_PERL, that should be
OK, I would think.  I would think what you have is in a good state.

>>   - Speaking of backwards compatibility: after this series, "cd gitweb
>>     && make" yields an error. It's got a nice message telling you what
>>     to do, but it's likely breaking distro scripts. Again, I'm not sure
>>     I care, but if the point of the exercise was to avoid breaking
>>     things, well...
>
> I think that's OK, having maintained those sorts of build scripts in a
> past life.
>
> I.e. when you upgrade the package it's a minor hassle, and the error
> tells you exactly what to do, and the fix is a 2-3 lines in your recipe
> at most.

We could easily add "cd .. && make gitweb" to gitweb/Makefile with
the same "minor hassle" but that needs to be done just once, instead
of having to be done once per packager, so I am not sure the above
argues for a good tradeoff.

Thanks.

Re: [PATCH v2 0/7] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 22 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>>   - I wondered if "make NO_PERL=1" would complain about "gitweb" being
>>>     in the default targets. It doesn't, but it does actually build
>>>     gitweb, which seems a little weird. I don't think we actually rely
>>>     on perl during the build (e.g., no "perl -c" checks or anything),
>>>     and the t950x tests seem to respect NO_PERL and avoid running the
>>>     generated file. So maybe it's OK?
>>
>> I think it's arguably a bug, but as you note we build/test etc. without
>> errors, and I think it's restoring the state before e25c7cc146
>> (Makefile: drop dependency between git-instaweb and gitweb, 2015-05-29).
>>
>> Arguably we should replace with a stub script like git-svn et al, and
>> arguably we should leave it, as you're more likely to e.g. run gitweb on
>> a webserver, so even if you build a "no perl" package, perhaps it's
>> convenient to have "gitweb" part of it, and then on that one box that
>> runs it you'll install perl...
>
> It is perfectly acceptable to "make DESTDIR=... install" and tar up
> the result on a host with NO_PERL and extract it on the target that
> is capable to run gitweb, isn't it?  As long as "make NO_PERL=1"
> gives exactly the gitweb as a build without NO_PERL, that should be
> OK, I would think.  I would think what you have is in a good state.

I think so, but it is inconsistent with how we treat the rest of the
*.perl scripts, which I think is what Jeff is correctly pointing
out.

I.e. if you do this your git-send-email, git-cvsserver, git-svn
etc. will be replaced by this shellscript:

	#!/bin/sh

	echo >&2 "fatal: git was built without support for $(basename $0) (NO_PERL=Y)."
	exit 128

>>>   - Speaking of backwards compatibility: after this series, "cd gitweb
>>>     && make" yields an error. It's got a nice message telling you what
>>>     to do, but it's likely breaking distro scripts. Again, I'm not sure
>>>     I care, but if the point of the exercise was to avoid breaking
>>>     things, well...
>>
>> I think that's OK, having maintained those sorts of build scripts in a
>> past life.
>>
>> I.e. when you upgrade the package it's a minor hassle, and the error
>> tells you exactly what to do, and the fix is a 2-3 lines in your recipe
>> at most.
>
> We could easily add "cd .. && make gitweb" to gitweb/Makefile with
> the same "minor hassle" but that needs to be done just once, instead
> of having to be done once per packager, so I am not sure the above
> argues for a good tradeoff.

True, but I think critically in this case we've never documented that
you should be running gitweb/Makefile directly. I.e. the gitweb/INSTALL
has always documented and assumed that you run these from the top-level.

There's no "make gitweb" target in the sub-Makefile, and the
instructions make it clear that it's the top-level, as we also suggest:

    make configure &&
    [... no cd-ing to gitweb/ ...] &&
    make gitweb

(and there's no "configure" there either).

So I think this will Just Work for anyone who's packaging this already,

I was just going above & beyond in being friendly by emitting that new
message saying you should be running "make" at the top-level in case
anyone built this in a way that we never documented, but which happened
to work before.

Re: [PATCH v2 0/7] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> We could easily add "cd .. && make gitweb" to gitweb/Makefile with
>> the same "minor hassle" but that needs to be done just once, instead
>> of having to be done once per packager, so I am not sure the above
>> argues for a good tradeoff.
>
> True, but I think critically in this case we've never documented that
> you should be running gitweb/Makefile directly. I.e. the gitweb/INSTALL
> has always documented and assumed that you run these from the top-level.

Well, I do not think Makefiles document much of their targets in
general.  If its first/default target has a reasonable name, like
"all", people expect "cd there && make all" would do the right
thing.

So I do not think "we never documented" is a good excuse.  What the
current users have been doing and are expecting to keep working is
what counts.  If they are used to see "cd gitweb && make" working,
perhaps instead of giving an unfriendly $(error do not run) at the
beginning of gitweb/Makefile that is designed to trigger only when
they did that (instead of running 'make gitweb' from the top), it
would be trivial to have the rule to "cd .. && $(MAKE) gitweb"
there, no?

Re: [PATCH v2 0/7] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 23 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> We could easily add "cd .. && make gitweb" to gitweb/Makefile with
>>> the same "minor hassle" but that needs to be done just once, instead
>>> of having to be done once per packager, so I am not sure the above
>>> argues for a good tradeoff.
>>
>> True, but I think critically in this case we've never documented that
>> you should be running gitweb/Makefile directly. I.e. the gitweb/INSTALL
>> has always documented and assumed that you run these from the top-level.
>
> Well, I do not think Makefiles document much of their targets in
> general.  If its first/default target has a reasonable name, like
> "all", people expect "cd there && make all" would do the right
> thing.
>
> So I do not think "we never documented" is a good excuse.  What the
> current users have been doing and are expecting to keep working is
> what counts.  If they are used to see "cd gitweb && make" working,
> perhaps instead of giving an unfriendly $(error do not run) at the
> beginning of gitweb/Makefile that is designed to trigger only when
> they did that (instead of running 'make gitweb' from the top), it
> would be trivial to have the rule to "cd .. && $(MAKE) gitweb"
> there, no?

I can re-roll it with that change if you insist. It would close the door
on further unifying the two Makefiles in the future (well, we could keep
the wrapper in place).

I have a script I use to see how big the impact of this sort of thing
would be in practice, i.e. I download downstream package recipies, which
are found at (name, relative path & urls). I also manually get the AIX
package:
	
	freebsd-ports   devel/git       https://github.com/freebsd/freebsd-ports.git 
	openbsd-ports   devel/git       https://github.com/openbsd/ports.git 
	netbsd-pkgsrc   devel/git-base  https://github.com/NetBSD/pkgsrc.git 
	dragonflybsd-dports     devel/git       https://github.com/DragonFlyBSD/DPorts.git 
	fedora  .       https://src.fedoraproject.org/rpms/git 
	debian  debian  https://repo.or.cz/git/debian.git 
	gentoo  dev-vcs/git     https://github.com/gentoo/gentoo.git 
	arch    git/trunk       https://github.com/archlinux/svntogit-packages.git 
	nix     pkgs/applications/version-management/git-and-tools/git  https://github.com/NixOS/nixpkgs.git 
	alpine  main/git        https://github.com/alpinelinux/aports.git       https://git.alpinelinux.org/aports 
	git_osx_installer       .       https://github.com/timcharper/git_osx_installer.git 
	homebrew-core   Formula/git.rb  https://github.com/Homebrew/homebrew-core.git 
	macports-ports  devel/git       https://github.com/macports/macports-ports.git

Looking through all of those none of them do anything with
gitweb/Makefile. I.e. all "make gitweb" at the top-level, or simply rely
on "make install" to install it.

FreeBSD and NetBSD are monkeypatching our Makefile to emulate a "I don't
want gitweb please!", which they'll need to do before/after this series
(but we could helpfully provide them a config knob).

Anyway, if you want "make gitweb" and "make gitweb-install" in the
subdirectory to work I can patch it, but per the above I think it would
be useful to pretty much nobody.

I could use around the same amount of effort to give FreeBSD and NetBSD
a "don't install gitweb please" know instead, what do you think?:)

Re: [PATCH v2 0/7] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> would be in practice, i.e. I download downstream package recipies, which
> are found at (name, relative path & urls). I also manually get the AIX
> package:
> ...
> 	macports-ports  devel/git       https://github.com/macports/macports-ports.git
>
> Looking through all of those none of them do anything with
> gitweb/Makefile. I.e. all "make gitweb" at the top-level, or simply rely
> on "make install" to install it.

Now we are talking.  Giving that as a datapoint upfront in the
proposed log message would have saved needless round-trip, I would
think.

Thanks.

[PATCH v3 0/8] gitweb: fix "make" not including "gitweb" without NOOP run slowdowns

[Ævar Arnfjörð Bjarmason]

The $subject is a mostly trivial re-roll of
https://lore.kernel.org/git/cover-v2-0.7-00000000000-20220531T173805Z-avarab@gmail.com/.

Changes since v3:

 * Correct the commit message of 7/8 to account for SZEDER's commit
   being included in this (that part was also incorrect in v2).

 * As noted in
   https://lore.kernel.org/git/220624.86bkuikidi.gmgdl@evledraar.gmail.com/
   I went over various git packages in the wild to see if my changes
   here would break things

   I did find that not having a wrapper gitweb/Makefile would break
   OpenBSD's package, but it's a one-line fix.

   But to make up for it I saw that a semi-common pattern was to
   manually munge our Makefile to get rid of "gitweb" or
   "gitweb-install" targets. All of {Free,Net,Dragonfly}BSD were doing
   that. They'll now happily be able to use a NO_GITWEB=Y flag
   instead, so hopefully the small amount of disruption here makes up
   for itself.

SZEDER Gábor (1):
  Makefile: build 'gitweb' in the default target

Ævar Arnfjörð Bjarmason (7):
  gitweb/Makefile: define all .PHONY prerequisites inline
  gitweb/Makefile: add a $(GITWEB_ALL) variable
  gitweb/Makefile: clear up and de-duplicate the gitweb.{css,js} vars
  gitweb/Makefile: prepare to merge into top-level Makefile
  gitweb: remove "test" and "test-installed" targets
  gitweb/Makefile: include in top-level Makefile
  gitweb/Makefile: add a "NO_GITWEB" parameter

 Makefile        |  31 +++++++----
 gitweb/Makefile | 145 ++++++++++++++++--------------------------------
 t/Makefile      |   4 --
 3 files changed, 68 insertions(+), 112 deletions(-)

Range-diff against v2:
1:  14361617ca6 = 1:  8e85151cf3d gitweb/Makefile: define all .PHONY prerequisites inline
2:  7d920a13518 = 2:  5c9895949aa gitweb/Makefile: add a $(GITWEB_ALL) variable
3:  e14a5b73061 = 3:  2f4db54923d gitweb/Makefile: clear up and de-duplicate the gitweb.{css,js} vars
4:  02e26ca8ce2 = 4:  d38b553a2e6 gitweb/Makefile: prepare to merge into top-level Makefile
5:  caf376f3dd9 = 5:  6c2d7b30524 gitweb: remove "test" and "test-installed" targets
6:  b423cd58f6b = 6:  5640587b9ae gitweb/Makefile: include in top-level Makefile
7:  69428540886 ! 7:  571c9c10319 Makefile: build 'gitweb' in the default target
    @@ Commit message
         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
    +    doesn't depend on the 'gitweb' target, it has a dependency on the
    +    'install-gitweb' target, 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.
-:  ----------- > 8:  0c8f26ee876 gitweb/Makefile: add a "NO_GITWEB" parameter
-- 
2.37.0.880.gf07d56b18ba

[PATCH v3 1/8] gitweb/Makefile: define all .PHONY prerequisites inline

[Ævar Arnfjörð Bjarmason]

Move the '.PHONY' definition so that it's split up and accompanies the
relevant as they're defined. This will make a subsequent diff smaller
as we'll remove some of these, and won't need to re-edit the
now-removed '.PHONY' line.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 gitweb/Makefile | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/gitweb/Makefile b/gitweb/Makefile
index f13e23c4de4..abb5c9f9ab6 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -1,5 +1,6 @@
 # The default target of this Makefile is...
 all::
+.PHONY: all
 
 # Define V=1 to have a more verbose compile.
 #
@@ -45,6 +46,7 @@ HIGHLIGHT_BIN = highlight
 -include config.mak
 
 # determine version
+.PHONY: .FORCE-GIT-VERSION-FILE
 ../GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE
 
@@ -152,6 +154,7 @@ GITWEB_REPLACE = \
 	-e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
 	-e 's|++HIGHLIGHT_BIN++|$(HIGHLIGHT_BIN)|g'
 
+.PHONY: FORCE
 GITWEB-BUILD-OPTIONS: FORCE
 	@rm -f $@+
 	@echo "x" '$(PERL_PATH_SQ)' $(GITWEB_REPLACE) "$(JSMIN)|$(CSSMIN)" >$@+
@@ -171,15 +174,18 @@ static/gitweb.js: $(GITWEB_JSLIB_FILES)
 
 ### Testing rules
 
+.PHONY: test
 test:
 	$(MAKE) -C ../t gitweb-test
 
+.PHONY: test-installed
 test-installed:
 	GITWEB_TEST_INSTALLED='$(DESTDIR_SQ)$(gitwebdir_SQ)' \
 		$(MAKE) -C ../t gitweb-test
 
 ### Installation rules
 
+.PHONY: install
 install: all
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebdir_SQ)'
 	$(INSTALL) -m 755 $(GITWEB_PROGRAMS) '$(DESTDIR_SQ)$(gitwebdir_SQ)'
@@ -188,10 +194,8 @@ install: all
 
 ### Cleaning rules
 
+.PHONY: clean
 clean:
 	$(RM) gitweb.cgi static/gitweb.js \
 		static/gitweb.min.js static/gitweb.min.css \
 		GITWEB-BUILD-OPTIONS
-
-.PHONY: all clean install test test-installed .FORCE-GIT-VERSION-FILE FORCE
-
-- 
2.37.0.880.gf07d56b18ba

[PATCH v3 2/8] gitweb/Makefile: add a $(GITWEB_ALL) variable

[Ævar Arnfjörð Bjarmason]

Declare the targets that the "all" target depends on with a new
$(GITWEB_ALL) variable. This will help to reduce churn in subsequent
commits.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 gitweb/Makefile | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gitweb/Makefile b/gitweb/Makefile
index abb5c9f9ab6..733b60f925e 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -54,6 +54,11 @@ ifneq ($(MAKECMDGOALS),clean)
 -include ../GIT-VERSION-FILE
 endif
 
+# What targets we'll add to 'all' for "make gitweb"
+GITWEB_ALL =
+GITWEB_ALL += gitweb.cgi
+GITWEB_ALL += static/gitweb.js
+
 ### Build rules
 
 SHELL_PATH ?= $(SHELL)
@@ -92,7 +97,7 @@ ifndef V
 endif
 endif
 
-all:: gitweb.cgi static/gitweb.js
+all:: $(GITWEB_ALL)
 
 GITWEB_PROGRAMS = gitweb.cgi
 
-- 
2.37.0.880.gf07d56b18ba

[PATCH v3 3/8] gitweb/Makefile: clear up and de-duplicate the gitweb.{css,js} vars

[Ævar Arnfjörð Bjarmason]

Change the variable definitions for the $(GITWEB_CSS) and $(GITWEB_JS)
so that we have a clear separation between what we use as "in" files,
v.s. our "min" files. We can now make the appending to $(GITWEB_FILES)
unconditional, since $(GITWEB_{JS,CSS}) is either the "min" or
non-"min" version. This reduces the duplication within the file.

While we're at it let's initialize "GITWEB_JSLIB_FILES" as we normally
do with such variables.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 gitweb/Makefile | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/gitweb/Makefile b/gitweb/Makefile
index 733b60f925e..a8752c1f11c 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -31,10 +31,12 @@ GITWEB_STRICT_EXPORT =
 GITWEB_BASE_URL =
 GITWEB_LIST =
 GITWEB_HOMETEXT = indextext.html
-GITWEB_CSS = static/gitweb.css
+GITWEB_CSS_IN = static/gitweb.css
+GITWEB_CSS = $(GITWEB_CSS_IN)
 GITWEB_LOGO = static/git-logo.png
 GITWEB_FAVICON = static/git-favicon.png
-GITWEB_JS = static/gitweb.js
+GITWEB_JS_IN = static/gitweb.js
+GITWEB_JS = $(GITWEB_JS_IN)
 GITWEB_SITE_HTML_HEAD_STRING =
 GITWEB_SITE_HEADER =
 GITWEB_SITE_FOOTER =
@@ -57,7 +59,7 @@ endif
 # What targets we'll add to 'all' for "make gitweb"
 GITWEB_ALL =
 GITWEB_ALL += gitweb.cgi
-GITWEB_ALL += static/gitweb.js
+GITWEB_ALL += $(GITWEB_JS)
 
 ### Build rules
 
@@ -101,25 +103,23 @@ all:: $(GITWEB_ALL)
 
 GITWEB_PROGRAMS = gitweb.cgi
 
+GITWEB_JS_MIN = static/gitweb.min.js
 ifdef JSMIN
-GITWEB_FILES += static/gitweb.min.js
-GITWEB_JS = static/gitweb.min.js
-all:: static/gitweb.min.js
-static/gitweb.min.js: static/gitweb.js GITWEB-BUILD-OPTIONS
+GITWEB_JS = $(GITWEB_JS_MIN)
+all:: $(GITWEB_JS_MIN)
+$(GITWEB_JS_MIN): $(GITWEB_JS_IN) GITWEB-BUILD-OPTIONS
 	$(QUIET_GEN)$(JSMIN) <$< >$@
-else
-GITWEB_FILES += static/gitweb.js
 endif
+GITWEB_FILES += $(GITWEB_JS)
 
+GITWEB_CSS_MIN = static/gitweb.min.css
 ifdef CSSMIN
-GITWEB_FILES += static/gitweb.min.css
-GITWEB_CSS = static/gitweb.min.css
-all:: static/gitweb.min.css
-static/gitweb.min.css: static/gitweb.css GITWEB-BUILD-OPTIONS
+GITWEB_CSS = $(GITWEB_CSS_MIN)
+all:: $(GITWEB_CSS_MIN)
+$(GITWEB_CSS_MIN): $(GITWEB_CSS_IN) GITWEB-BUILD-OPTIONS
 	$(QUIET_GEN)$(CSSMIN) <$< >$@
-else
-GITWEB_FILES += static/gitweb.css
 endif
+GITWEB_FILES += $(GITWEB_CSS)
 
 GITWEB_FILES += static/git-logo.png static/git-favicon.png
 
@@ -127,6 +127,7 @@ GITWEB_FILES += static/git-logo.png static/git-favicon.png
 #
 # js/lib/common-lib.js should be always first, then js/lib/*.js,
 # then the rest of files; js/gitweb.js should be last (if it exists)
+GITWEB_JSLIB_FILES =
 GITWEB_JSLIB_FILES += static/js/lib/common-lib.js
 GITWEB_JSLIB_FILES += static/js/lib/datetime.js
 GITWEB_JSLIB_FILES += static/js/lib/cookies.js
@@ -201,6 +202,6 @@ install: all
 
 .PHONY: clean
 clean:
-	$(RM) gitweb.cgi static/gitweb.js \
-		static/gitweb.min.js static/gitweb.min.css \
+	$(RM) gitweb.cgi $(GITWEB_JS_IN) \
+		$(GITWEB_JS_MIN) $(GITWEB_CSS_MIN) \
 		GITWEB-BUILD-OPTIONS
-- 
2.37.0.880.gf07d56b18ba

[PATCH v3 4/8] gitweb/Makefile: prepare to merge into top-level Makefile

[Ævar Arnfjörð Bjarmason]

Since the "gitweb/Makefile" was split out from the top-level Makefile
in 62331ef1637 (gitweb: Makefile improvements, 2010-01-30) we've kept
the inter-dependencies between the two, and worse have dealt with a
lot of duplication as a result.

In preparation for merging the two again add a MAK_DIR_GITWEB variable
to various rules in it. This will allow us to set this variable to
"gitweb/" as we include it in the top-level Makefile, which will
minimize the size of the subsequent diff.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 gitweb/Makefile | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/gitweb/Makefile b/gitweb/Makefile
index a8752c1f11c..74e896767e9 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -2,6 +2,8 @@
 all::
 .PHONY: all
 
+MAK_DIR_GITWEB =
+
 # Define V=1 to have a more verbose compile.
 #
 # Define JSMIN to point to JavaScript minifier that functions as
@@ -106,8 +108,9 @@ GITWEB_PROGRAMS = gitweb.cgi
 GITWEB_JS_MIN = static/gitweb.min.js
 ifdef JSMIN
 GITWEB_JS = $(GITWEB_JS_MIN)
-all:: $(GITWEB_JS_MIN)
-$(GITWEB_JS_MIN): $(GITWEB_JS_IN) GITWEB-BUILD-OPTIONS
+all:: $(MAK_DIR_GITWEB)$(GITWEB_JS_MIN)
+$(MAK_DIR_GITWEB)$(GITWEB_JS_MIN): $(MAK_DIR_GITWEB)GITWEB-BUILD-OPTIONS
+$(MAK_DIR_GITWEB)$(GITWEB_JS_MIN): $(MAK_DIR_GITWEB)$(GITWEB_JS_IN)
 	$(QUIET_GEN)$(JSMIN) <$< >$@
 endif
 GITWEB_FILES += $(GITWEB_JS)
@@ -115,8 +118,9 @@ GITWEB_FILES += $(GITWEB_JS)
 GITWEB_CSS_MIN = static/gitweb.min.css
 ifdef CSSMIN
 GITWEB_CSS = $(GITWEB_CSS_MIN)
-all:: $(GITWEB_CSS_MIN)
-$(GITWEB_CSS_MIN): $(GITWEB_CSS_IN) GITWEB-BUILD-OPTIONS
+all:: $(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN)
+$(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN): $(MAK_DIR_GITWEB)GITWEB-BUILD-OPTIONS
+$(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN): $(MAK_DIR_GITWEB)$(GITWEB_CSS_IN)
 	$(QUIET_GEN)$(CSSMIN) <$< >$@
 endif
 GITWEB_FILES += $(GITWEB_CSS)
@@ -161,19 +165,20 @@ GITWEB_REPLACE = \
 	-e 's|++HIGHLIGHT_BIN++|$(HIGHLIGHT_BIN)|g'
 
 .PHONY: FORCE
-GITWEB-BUILD-OPTIONS: FORCE
+$(MAK_DIR_GITWEB)GITWEB-BUILD-OPTIONS: FORCE
 	@rm -f $@+
 	@echo "x" '$(PERL_PATH_SQ)' $(GITWEB_REPLACE) "$(JSMIN)|$(CSSMIN)" >$@+
 	@cmp -s $@+ $@ && rm -f $@+ || mv -f $@+ $@
 
-gitweb.cgi: gitweb.perl GITWEB-BUILD-OPTIONS
+$(MAK_DIR_GITWEB)gitweb.cgi: $(MAK_DIR_GITWEB)GITWEB-BUILD-OPTIONS
+$(MAK_DIR_GITWEB)gitweb.cgi: $(MAK_DIR_GITWEB)gitweb.perl
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
 		$(GITWEB_REPLACE) $< >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
 
-static/gitweb.js: $(GITWEB_JSLIB_FILES)
+$(MAK_DIR_GITWEB)static/gitweb.js: $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_JSLIB_FILES))
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	cat $^ >$@+ && \
 	mv $@+ $@
@@ -194,14 +199,16 @@ test-installed:
 .PHONY: install
 install: all
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebdir_SQ)'
-	$(INSTALL) -m 755 $(GITWEB_PROGRAMS) '$(DESTDIR_SQ)$(gitwebdir_SQ)'
+	$(INSTALL) -m 755 $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_PROGRAMS)) \
+		'$(DESTDIR_SQ)$(gitwebdir_SQ)'
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
-	$(INSTALL) -m 644 $(GITWEB_FILES) '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
+	$(INSTALL) -m 644 $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_FILES)) \
+		'$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
 
 ### Cleaning rules
 
 .PHONY: clean
 clean:
-	$(RM) gitweb.cgi $(GITWEB_JS_IN) \
+	$(RM) $(addprefix $(MAK_DIR_GITWEB),gitweb.cgi $(GITWEB_JS_IN) \
 		$(GITWEB_JS_MIN) $(GITWEB_CSS_MIN) \
-		GITWEB-BUILD-OPTIONS
+		GITWEB-BUILD-OPTIONS)
-- 
2.37.0.880.gf07d56b18ba

[PATCH v3 5/8] gitweb: remove "test" and "test-installed" targets

[Ævar Arnfjörð Bjarmason]

Remove the special "test" targets for gitweb added in
958a8467216 (gitweb/Makefile: Add 'test' and 'test-installed' targets,
2010-09-26). Unlike e.g. "contrib/scalar" and "contrib/subtree" the
"gitweb" tests themselves live in our top-level t/ directory.

It therefore doesn't make sense to maintain this indirection, no more
than it would to have a "git-send-email-test". By dropping it we'll
also free other tests to use the t95*.sh prefix.

These removed targets are unlikely to be used by anyone, and to the
extent that they are we can easily use an invocation like this
instead:

	make test T='t[0-9]*gitweb*.sh'

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 gitweb/Makefile | 11 -----------
 t/Makefile      |  4 ----
 2 files changed, 15 deletions(-)

diff --git a/gitweb/Makefile b/gitweb/Makefile
index 74e896767e9..eaf0cfcf80e 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -183,17 +183,6 @@ $(MAK_DIR_GITWEB)static/gitweb.js: $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_JSLIB_
 	cat $^ >$@+ && \
 	mv $@+ $@
 
-### Testing rules
-
-.PHONY: test
-test:
-	$(MAKE) -C ../t gitweb-test
-
-.PHONY: test-installed
-test-installed:
-	GITWEB_TEST_INSTALLED='$(DESTDIR_SQ)$(gitwebdir_SQ)' \
-		$(MAKE) -C ../t gitweb-test
-
 ### Installation rules
 
 .PHONY: install
diff --git a/t/Makefile b/t/Makefile
index 056ce55dcc9..7f56e52f767 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -35,7 +35,6 @@ TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
 CHAINLINTTMP_SQ = $(subst ','\'',$(CHAINLINTTMP))
 
 T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
-TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
 THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))
 TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
 CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
@@ -112,9 +111,6 @@ aggregate-results:
 		echo "$$f"; \
 	done | '$(SHELL_PATH_SQ)' ./aggregate-results.sh
 
-gitweb-test:
-	$(MAKE) $(TGITWEB)
-
 valgrind:
 	$(MAKE) GIT_TEST_OPTS="$(GIT_TEST_OPTS) --valgrind"
 
-- 
2.37.0.880.gf07d56b18ba

[PATCH v3 6/8] gitweb/Makefile: include in top-level Makefile

[Ævar Arnfjörð Bjarmason]

Include the gitweb/Makefile in the top-level Makefile rather than
calling it as a sub-Makefile. As noted in the thread starting at at
[1] (in particular [2]) we'll pay a high cost on NOOP runs of "make"
just to figure out that we have nothing to do for "make gitweb".

The "gitweb" script also isn't maintained out-of-tree, unlike
"gitk-git" or "git-gui", which both have their own "Makefile". Other
parts of it are already integrated into our main Makefiles, e.g. the
documentation is built by Documentation/Makefile since
07ea4df2780 (gitweb: Add gitweb(1) manpage for gitweb itself,
2011-10-16).

1. https://lore.kernel.org/git/20220525205651.825669-1-szeder.dev@gmail.com/
2. https://lore.kernel.org/git/220526.86k0a96sv2.gmgdl@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile        | 23 +++++++------
 gitweb/Makefile | 91 +++++++++----------------------------------------
 2 files changed, 29 insertions(+), 85 deletions(-)

diff --git a/Makefile b/Makefile
index 04d0fd1fe60..a5f29c11681 100644
--- a/Makefile
+++ b/Makefile
@@ -544,6 +544,7 @@ gitexecdir = libexec/git-core
 mergetoolsdir = $(gitexecdir)/mergetools
 sharedir = $(prefix)/share
 gitwebdir = $(sharedir)/gitweb
+gitwebstaticdir = $(gitwebdir)/static
 perllibdir = $(sharedir)/perl5
 localedir = $(sharedir)/locale
 template_dir = share/git-core/templates
@@ -562,7 +563,7 @@ localedir_relative = $(patsubst $(prefix)/%,%,$(localedir))
 htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
 perllibdir_relative = $(patsubst $(prefix)/%,%,$(perllibdir))
 
-export prefix bindir sharedir sysconfdir gitwebdir perllibdir localedir
+export prefix bindir sharedir sysconfdir perllibdir localedir
 
 # Set our default programs
 CC = cc
@@ -2089,6 +2090,7 @@ htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
 prefix_SQ = $(subst ','\'',$(prefix))
 perllibdir_relative_SQ = $(subst ','\'',$(perllibdir_relative))
 gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
+gitwebstaticdir_SQ = $(subst ','\'',$(gitwebstaticdir))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH))
@@ -2417,10 +2419,6 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
 perllibdir:
 	@echo '$(perllibdir_SQ)'
 
-.PHONY: gitweb
-gitweb:
-	$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all
-
 git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	chmod +x $@+ && \
@@ -3149,6 +3147,15 @@ coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.c
 
 .PHONY: coccicheck coccicheck-pending
 
+# "Sub"-Makefiles, not really because they can't be run stand-alone,
+# only there to contain directory-specific rules and variables
+## gitweb/Makefile inclusion:
+MAK_DIR_GITWEB = gitweb/
+include gitweb/Makefile
+
+.PHONY: gitweb
+gitweb: $(MAK_DIR_GITWEB_ALL)
+
 ### Installation rules
 
 ifneq ($(filter /%,$(firstword $(template_dir))),)
@@ -3221,7 +3228,6 @@ ifndef NO_PERL
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perllibdir_SQ)'
 	(cd perl/build/lib && $(TAR) cf - .) | \
 	(cd '$(DESTDIR_SQ)$(perllibdir_SQ)' && umask 022 && $(TAR) xof -)
-	$(MAKE) -C gitweb install
 endif
 ifndef NO_TCLTK
 	$(MAKE) -C gitk-git install
@@ -3276,10 +3282,8 @@ endif
 		  cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
 	done
 
-.PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf
+.PHONY: install-doc install-man install-man-perl install-html install-info install-pdf
 .PHONY: quick-install-doc quick-install-man quick-install-html
-install-gitweb:
-	$(MAKE) -C gitweb install
 
 install-doc: install-man-perl
 	$(MAKE) -C Documentation install
@@ -3425,7 +3429,6 @@ clean: profile-clean coverage-clean cocciclean
 	$(MAKE) -C Documentation/ clean
 	$(RM) Documentation/GIT-EXCLUDED-PROGRAMS
 ifndef NO_PERL
-	$(MAKE) -C gitweb clean
 	$(RM) -r perl/build/
 endif
 	$(MAKE) -C templates/ clean
diff --git a/gitweb/Makefile b/gitweb/Makefile
index eaf0cfcf80e..66fceb9e94f 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -1,11 +1,8 @@
-# The default target of this Makefile is...
-all::
-.PHONY: all
-
-MAK_DIR_GITWEB =
+ifndef MAK_DIR_GITWEB
+$(error do not run gitweb/Makefile stand-alone anymore. The "gitweb" and \
+"install-gitweb" targets now live in the top-level Makefile)
+endif
 
-# Define V=1 to have a more verbose compile.
-#
 # Define JSMIN to point to JavaScript minifier that functions as
 # a filter to have static/gitweb.js minified.
 #
@@ -13,13 +10,6 @@ MAK_DIR_GITWEB =
 # version of static/gitweb.css
 #
 
-prefix ?= $(HOME)
-bindir ?= $(prefix)/bin
-gitwebdir ?= /var/www/cgi-bin
-
-RM ?= rm -f
-INSTALL ?= install
-
 # default configuration for gitweb
 GITWEB_CONFIG = gitweb_config.perl
 GITWEB_CONFIG_SYSTEM = /etc/gitweb.conf
@@ -44,71 +34,19 @@ GITWEB_SITE_HEADER =
 GITWEB_SITE_FOOTER =
 HIGHLIGHT_BIN = highlight
 
-# include user config
--include ../config.mak.autogen
--include ../config.mak
--include config.mak
-
-# determine version
-.PHONY: .FORCE-GIT-VERSION-FILE
-../GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
-	$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE
-
-ifneq ($(MAKECMDGOALS),clean)
--include ../GIT-VERSION-FILE
-endif
-
 # What targets we'll add to 'all' for "make gitweb"
 GITWEB_ALL =
 GITWEB_ALL += gitweb.cgi
 GITWEB_ALL += $(GITWEB_JS)
 
-### Build rules
-
-SHELL_PATH ?= $(SHELL)
-PERL_PATH  ?= /usr/bin/perl
-
-# Shell quote;
-bindir_SQ = $(subst ','\'',$(bindir))#'
-gitwebdir_SQ = $(subst ','\'',$(gitwebdir))#'
-gitwebstaticdir_SQ = $(subst ','\'',$(gitwebdir)/static)#'
-SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))#'
-PERL_PATH_SQ  = $(subst ','\'',$(PERL_PATH))#'
-DESTDIR_SQ    = $(subst ','\'',$(DESTDIR))#'
-
-# Quiet generation (unless V=1)
-QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
-QUIET_SUBDIR1  =
-
-ifneq ($(findstring $(MAKEFLAGS),w),w)
-PRINT_DIR = --no-print-directory
-else # "make -w"
-NO_SUBDIR = :
-endif
-
-ifneq ($(findstring $(MAKEFLAGS),s),s)
-ifndef V
-	QUIET          = @
-	QUIET_GEN      = $(QUIET)echo '   ' GEN $@;
-	QUIET_SUBDIR0  = +@subdir=
-	QUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
-	                 $(MAKE) $(PRINT_DIR) -C $$subdir
-	export V
-	export QUIET
-	export QUIET_GEN
-	export QUIET_SUBDIR0
-	export QUIET_SUBDIR1
-endif
-endif
-
-all:: $(GITWEB_ALL)
+MAK_DIR_GITWEB_ALL = $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_ALL))
 
 GITWEB_PROGRAMS = gitweb.cgi
 
 GITWEB_JS_MIN = static/gitweb.min.js
 ifdef JSMIN
 GITWEB_JS = $(GITWEB_JS_MIN)
-all:: $(MAK_DIR_GITWEB)$(GITWEB_JS_MIN)
+GITWEB_ALL += $(MAK_DIR_GITWEB)$(GITWEB_JS_MIN)
 $(MAK_DIR_GITWEB)$(GITWEB_JS_MIN): $(MAK_DIR_GITWEB)GITWEB-BUILD-OPTIONS
 $(MAK_DIR_GITWEB)$(GITWEB_JS_MIN): $(MAK_DIR_GITWEB)$(GITWEB_JS_IN)
 	$(QUIET_GEN)$(JSMIN) <$< >$@
@@ -118,7 +56,7 @@ GITWEB_FILES += $(GITWEB_JS)
 GITWEB_CSS_MIN = static/gitweb.min.css
 ifdef CSSMIN
 GITWEB_CSS = $(GITWEB_CSS_MIN)
-all:: $(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN)
+GITWEB_ALL += $(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN)
 $(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN): $(MAK_DIR_GITWEB)GITWEB-BUILD-OPTIONS
 $(MAK_DIR_GITWEB)$(GITWEB_CSS_MIN): $(MAK_DIR_GITWEB)$(GITWEB_CSS_IN)
 	$(QUIET_GEN)$(CSSMIN) <$< >$@
@@ -185,19 +123,22 @@ $(MAK_DIR_GITWEB)static/gitweb.js: $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_JSLIB_
 
 ### Installation rules
 
-.PHONY: install
-install: all
+.PHONY: install-gitweb
+install-gitweb: $(MAK_DIR_GITWEB_ALL)
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebdir_SQ)'
-	$(INSTALL) -m 755 $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_PROGRAMS)) \
-		'$(DESTDIR_SQ)$(gitwebdir_SQ)'
+	$(INSTALL) -m 755 $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_PROGRAMS)) '$(DESTDIR_SQ)$(gitwebdir_SQ)'
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
 	$(INSTALL) -m 644 $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_FILES)) \
 		'$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
+ifndef NO_PERL
+install: install-gitweb
+endif
 
 ### Cleaning rules
 
-.PHONY: clean
-clean:
+.PHONY: gitweb-clean
+gitweb-clean:
 	$(RM) $(addprefix $(MAK_DIR_GITWEB),gitweb.cgi $(GITWEB_JS_IN) \
 		$(GITWEB_JS_MIN) $(GITWEB_CSS_MIN) \
 		GITWEB-BUILD-OPTIONS)
+clean: gitweb-clean
-- 
2.37.0.880.gf07d56b18ba

[PATCH v3 7/8] Makefile: build 'gitweb' in the default target

[Ævar Arnfjörð Bjarmason]

From: SZEDER Gábor <szeder.dev@gmail.com>

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 has a dependency on the
'install-gitweb' target, 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>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index a5f29c11681..d150be4dc32 100644
--- a/Makefile
+++ b/Makefile
@@ -3155,6 +3155,7 @@ include gitweb/Makefile
 
 .PHONY: gitweb
 gitweb: $(MAK_DIR_GITWEB_ALL)
+all:: gitweb
 
 ### Installation rules
 
-- 
2.37.0.880.gf07d56b18ba

[PATCH v3 8/8] gitweb/Makefile: add a "NO_GITWEB" parameter

[Ævar Arnfjörð Bjarmason]

From looking at the {Free,Net,Dragonfly}BSD packages for git[1]
they've been monkeypatching "gitweb" out of the Makefile, let's be
nicer and provide a NO_GITWEB=Y for their use.

For the "all" target this allows for optionally restoring what's been
the status quo before the preceding commit, but now we'll also behave
correctly on the subsequent "make install".

As before our installation of gitweb can be suppressed with
NO_PERL. For backwards compatibility the NO_PERL=Y flag by itself
still doesn't change whether or not we build gitweb, unlike the new
NO_GITWEB=Y flag.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile        | 7 +++++++
 gitweb/Makefile | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/Makefile b/Makefile
index d150be4dc32..92bea603394 100644
--- a/Makefile
+++ b/Makefile
@@ -309,6 +309,11 @@ include shared.mak
 # distributions that want to use their packaged versions of Perl
 # modules, instead of the fallbacks shipped with Git.
 #
+# Define NO_GITWEB if you do not want to build or install
+# 'gitweb'. Note that defining NO_PERL currently has the same effect
+# on not installing gitweb, but not on whether it's built in the
+# gitweb/ directory.
+#
 # Define PYTHON_PATH to the path of your Python binary (often /usr/bin/python
 # but /usr/bin/python2.7 or /usr/bin/python3 on some platforms).
 #
@@ -3155,7 +3160,9 @@ include gitweb/Makefile
 
 .PHONY: gitweb
 gitweb: $(MAK_DIR_GITWEB_ALL)
+ifndef NO_GITWEB
 all:: gitweb
+endif
 
 ### Installation rules
 
diff --git a/gitweb/Makefile b/gitweb/Makefile
index 66fceb9e94f..3b68ab2d672 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -130,9 +130,11 @@ install-gitweb: $(MAK_DIR_GITWEB_ALL)
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
 	$(INSTALL) -m 644 $(addprefix $(MAK_DIR_GITWEB),$(GITWEB_FILES)) \
 		'$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
+ifndef NO_GITWEB
 ifndef NO_PERL
 install: install-gitweb
 endif
+endif
 
 ### Cleaning rules
 
-- 
2.37.0.880.gf07d56b18ba