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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 22, 2022, 09:27 UTC
Message-ID: <220622.86r13hkp2c.gmgdl@evledraar.gmail.com>
In-Reply-To: <YrFphmtLuHVkI7yr@coredump.intra.peff.net>

On Tue, Jun 21 2022, Jeff King wrote:

Show 41 quoted lines

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

Show 5 quoted lines

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