Re: [PATCH] Makefile: drop GEN_HDRS

From: Emily Shaffer <emilyshaffer@google.com>
Date: Dec 17, 2019, 01:27 UTC
Message-ID: <20191217012756.GQ135450@google.com>
In-Reply-To: <20191216192014.GA2678964@coredump.intra.peff.net>

On Mon, Dec 16, 2019 at 02:20:14PM -0500, Jeff King wrote:

Show 17 quoted lines

> On Mon, Dec 16, 2019 at 10:55:40AM -0800, Junio C Hamano wrote:
> 
> > LIB_H could contain command-list.h (and other GENERATED_H files) if
> > we did this, but dups in dependency does not hurt in general, and I
> > did not find anything potentially problematic in the existing use of
> > $(LIB_H) in our Makefile.
> > 
> > How about doing this as a further clean-up?  I am reasonably sure
> > the status-quo description is correct, but I find the justification
> > a bit weak (in other words, I do not have a good answer to "who
> > cares if those that depend on $(LIB_H) are not rebuilt when
> > command-list.h gets rebuilt?")
> 
> Yeah, I don't think there's any change in behavior here, since with the
> exception of hdr-check, every mention of $(LIB_H) also mentioned
> $(GENERATED_H). And in the case of hdr-check, we explicitly exclude the
> only item found in $(GENERATED_H).

To check my understanding - hdr-check just says "the headers are syntactically correct", right? $HCO's target '-o /dev/null' says "don't save the output", '-c' says "just compile, don't link", and '-xc' says "in C"; it expands to a target for each file ending in .h but not in $EXCEPT_HDRS, and hdr-check calls each one of those expanded targets, so I think I understand hdr-check is compiling each header...

Show 6 quoted lines

> 
> But this would enable us to start checking command-list.h. I'm on the
> fence on whether that's useful or not; the patch below makes it pass,
> but I'm not sure if that is really turning up any useful problems. I
> suppose somebody besides help.c could include command-list.h, in which
> case some of those MAYBE_UNUSED bits could become useful.

Firstly, I think if someone besides help.c includes command-list.h it blows up because there's no include guards :)

My gut wants to say, "I need to be sure my generated header is correct!" But it seems that will also be checked when I try to include that header from something actually important. So maybe it's not actually useful. But then it seems like hdr-check target isn't that useful for anybody, since those headers should always be included sometime down the road (or why have them). So that makes me think I must still be missing something, like maybe I parsed hdr-check wrong.

Show 76 quoted lines

> 
> I actually wonder if the whole thing would be simpler if command-list.h
> was a static tracked file with the declarations, and we generated
> command-list.c with "extern const char *command_list[]", etc.
> 
> > --- >8 ---
> > Makefile: include GENERATED_H in LIB_H
> > 
> > $(LIB_H), which is meant to be the list of header files that can
> > affect (hence trigger recompilation) the objects that go in
> > libgit.a, in a directory extracted from a tarball is computed by
> > running "find \*.h" but instead computed with "ls-files \*.h" in a
> > working tree managed by a git repository.  The former can include
> > generated header files after a build, and omit them in a clean
> > state.  The latter would not, as generated header files are by
> > definition not tracked.
> > 
> > Explicitly add $(GENERATED_H) to $(LIB_H) to make things consistent.
> 
> I do think this is slightly simpler to reason about than the existing
> setup (though see my "should it just be a C file?" above).
> 
> Here's the patch that would make hdr-check work:
> 
> ---
> diff --git a/Makefile b/Makefile
> index 87b68962ed..1eac8e7a7a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2780,7 +2780,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>  .PHONY: sparse $(SP_OBJ)
>  sparse: $(SP_OBJ)
>  
> -GEN_HDRS := command-list.h unicode-width.h
> +GEN_HDRS := unicode-width.h
>  EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/%
>  ifndef GCRYPT_SHA256
>  	EXCEPT_HDRS += sha256/gcrypt.h
> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> index 71158f7d8b..7b0751e3e1 100755
> --- a/generate-cmdlist.sh
> +++ b/generate-cmdlist.sh
> @@ -48,6 +48,7 @@ define_categories () {
>  define_category_names () {
>  	echo
>  	echo "/* Category names */"
> +	echo "MAYBE_UNUSED"
>  	echo "static const char *category_names[] = {"
>  	bit=0
>  	category_list "$1" |
> @@ -61,6 +62,7 @@ define_category_names () {
>  }
>  
>  print_command_list () {
> +	echo "MAYBE_UNUSED"
>  	echo "static struct cmdname_help command_list[] = {"
>  
>  	command_list "$1" |
> @@ -78,6 +80,7 @@ print_command_list () {
>  
>  print_config_list () {
>  	cat <<EOF
> +MAYBE_UNUSED
>  static const char *config_name_list[] = {
>  EOF
>  	grep -h '^[a-zA-Z].*\..*::$' Documentation/*config.txt Documentation/config/*.txt |
> @@ -101,7 +104,8 @@ do
>  	shift
>  done
>  
> -echo "/* Automatically generated by generate-cmdlist.sh */
> +echo "#include \"gettext.h\"
> +/* Automatically generated by generate-cmdlist.sh */
>  struct cmdname_help {
>  	const char *name;
>  	const char *help;