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@xxxxxxxxx> writes: >> > >> > > The $subject is a proposed re-roll of SZEDER's >> > > https://lore.kernel.org/git/20220525205651.825669-1-szeder.dev@xxxxxxxxx; >> > > 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.
