Hi Ævar,
On 26 Oct 2022, at 7:35, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Oct 26 2022, Jeff King wrote:
>
>> On Wed, Oct 26, 2022 at 04:43:32AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>>>
>>> On Tue, Oct 25 2022, Junio C Hamano wrote:
>>>
>>>> During the initial development of the fsck-msgids.txt feature, it
>>>> has become apparent that it is very much error prone to make sure
>>>> the description in the documentation file are sorted and correctly
>>>> match what is in the fsck.h header file.
>>>
>>> I have local fixes for the same issues in the list of advice in our
>>> docs, some of it's missing, wrong, out of date etc.
>>>
>>> I tried to quickly adapt the generation script I had for that, which
>>> works nicely, and by line count much shorter than the lint :)
>>
>> Yeah, my instinct here was to generate rather than lint. If you make a
>> mistake and the linter hits you over the head, that is better than
>> quietly letting your mistake go. But better still is making it
>> impossible to make in the first place.
>>
>> The downside is added complexity to the build, but it doesn't seem too
>> bad in this case.
>
> Yeah, it's not, I have local patches to generate advice-{type,config}.h,
> and builtin.h. This is a quick POC to do it for fsck-msgids.h.
>
> I see I forgot the .gitignore entry, so it's a rough POC :)
>
>> (I had a similar thought after getting hit on the head by the recent
>> t0450-txt-doc-vs-help.sh).
>
> Sorry about that. FWIW I've wanted to assert that for a while, and to do
> it by e.g. having the doc *.txt blurbs generated from running "$buildin
> -h" during the build.
If we wanted to go this route of generating the docs from the code (which sounds
like a better long term solution), how would this work? Would we print out the
list of message ids in builtin/fsck.c and write it to
Documentation/fsck-msgids.txt ?
>
> But when I suggested that I gathered That Junio wasn't up for that
> approach, it does have its downsides thorugh. E.g. to build the docs
> you'd now have to compile C code, and e.g. that git-scm.com publisher
> written in Ruby would have to compile the code to generate its docs.
>
> Or we could do it the other way around, and scape the *.txt for the *.c
> generation, but then we need to run a new script for building
> builtin/*.o. Also possible, and I think eventually both are better than
> what t0450-txt-doc-vs-help.sh's doing now, but that was a less invasive
> change than both...
>
>>> Having to exhaustively list every *.c file that uses fsck.h is a bit of
>>> a bother, but we have the same with the other generated *.h's, so it
>>> shouldn't be too bad.
>>
>> It feels like this could be made much shorter by having a separate
>> fsck-msgs.h and not including it from fsck.h. Only fsck.c and mktag.c
>> need the actual list. It would probably have to stop being an "enum",
>> though.
>
> Yes, that would make it shorter, but C doesn't have forward decls of
> enums, so we'd need to make it "int", ....
>
>> Another alternative is to generate the doc from the code, rather than
>> the other way around.
>
> *nod* :)
>
>>> +# Unfortunately our dependency management of generated headers used
>>> +# from within other headers suck, so we'll need to list every user of
>>> +# fsck.h here, but not too bad...
>>> +FSCK_MSGIDS_H_BUILTINS = fsck index-pack mktag receive-pack unpack-objects
>>> +$(foreach f,$(FSCK_MSGIDS_H_BUILTINS:%=builtin/%),$f.sp $f.s $f.o): fsck-msgids.h
>>> +FSCK_MSGIDS_H_LIBS = fetch-pack fsck
>>> +$(foreach f,$(FSCK_MSGIDS_H_LIBS),$f.sp $f.s $f.o): fsck-msgids.h
>>
>> I don't understand the "used within other headers" part here. Computed
>> dependencies will get this right. It's only the initial build (before we
>> have any computed dependencies) that needs to know that the C files in
>> question depend on the generated file. But that is true whether they do
>> so directly or indirectly.
>
> I forgot the full story, but luckily I wrote it down in the WIP commits
> :) FWIW if you want to scour that it's mainly:
>
> https://github.com/avar/git/commit/a00f1cb9ea5 # add advice-type.h
> https://github.com/avar/git/commit/9e080923a11 # generate 'struct advice
>
> Also, before generating builtin.h I've got e.g. this:
>
> https://github.com/avar/git/commit/5a5360d0134 # just check with 'make' that any file is sorted
>
> To actualy generate it, very WIP:
>
> http://github.com/avar/git/commit/cf1d02fa6b2
>
> Anyway, in partial summary, why (and sorry this got so long):
>
> * Yes, once we build e.g. fsck.o we have the full dependency tree,
> yay....
> * ...but only for gcc & clang, but we support other compilers.
> * ...for other compilers (or gcc & clang without the dep detection
> enabled) what should this do:
>
> make file-that-does-not-use-generated-header.o
>
> It sucks a bit to have e.g. every *.o depend on command-list.h, when
> we only use it in 1-2 places, ditto the other headers.
>
> The approach I took to this was to manually list headers that had
> "small uses" (1-10), but say that all of *.h dependend on running the
> *.sh to generate some "widely used" headers.
>
> E.g. if we generate builtin.h we'll either need that, or add
> builtin/%.o as a dependency, with a manual listing of the handful of
> uses outside of that subdirectory.
>
> * .... or just not care about any of that, i.e. to have all of our *.sh
> run on the "first build" no matter what, which certainly simplifies
> things, but once you have e.g. 5-10 generated headers doing e.g.:
>
> make grep.o
>
> and having it build command-list.h or whatever is a bit noisy, but
> it's a trade-off of manually maintaining deps v.s. not.
>
> * Our "COMPUTED_HEADER_DEPENDENCIES" only happens when you build the
> *.o, but we have *.sp and *.s targets too. I have some local changes
> *to generalize that, so we e.g. get proper dependencies for building
> **.s files.
>
> * We also have e.g. "make hdr-check", which works just fine now, but
> it's becausue we have no *.h file that uses another generated *.h
> file.
>
> Actually, I suspect the posted WIP change is broken in that regard
> (but doing this for real on my topic branch works), i.e. we need to
> start caring about deps for those auxiliary targets.
>
> * I may be wrong (I'm just looking at some old "here be dragons" note
> of mine), but I think there's also an edge case where the dependency
> graph of .depend.d/* is correct *once you've always had it*, but if a
> file now e.g. depends on foo.h, and I make foo.h include a new
> foo-generated.h things go boom.
>
> That issue (it if even exists, sorry, I'm blanking on the details)
> was solvable by just doing a "make clean", and building again,
> i.e. once we had re-built once we were OK, it was only a problem with
> an older checkout with an existing build pulling new sources.
>
> Still, I wanted to not make that case annoying either if I added a
> generated header...
