Re: [PATCH v3 4/4] Documentation: add lint-fsck-msgids

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 26, 2022, 11:35 UTC
Message-ID: <221026.8635badbz5.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y1jG8p1aARb9+CV+@coredump.intra.peff.net>

On Wed, Oct 26 2022, Jeff King wrote:

Show 23 quoted lines

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

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

Show 8 quoted lines

>> 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* :)

Show 13 quoted lines

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