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 23, 2022, 10:29 UTC
Message-ID: <220623.865ykrll0j.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqq7d587lqx.fsf@gitster.g>

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

Show 24 quoted lines

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

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