[PATCH 1/9] Makefile: add a hdr-check target

[PATCH 1/9] Makefile: add a hdr-check target

[Ramsay Jones]

Commit ef3ca95475 ("Add missing includes and forward declarations",
2018-08-15) resulted from the author employing a manual method to
create a C file consisting of a pair of pre-processor #include
lines (for 'git-compat-util.h' and a given toplevel header), and
fixing any resulting compiler errors or warnings.

Add a Makefile target to automate this process. This implementation
relies on the '-include' and '-xc' arguments to the 'gcc' and 'clang'
compilers, which allows us to effectively create the required C
compilation unit on-the-fly. This limits the portability of this
solution to those systems which have such a compiler.

The new 'hdr-check' target can be used to check most header files in
the project (for various reasons, the 'compat' and 'xdiff' directories
are not included). Also, note that individual header files can be
checked directly using the '.hco' extension (read: Hdr-Check Object)
like so:

    $ make config.hco
        HDR config.h
    $

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 Makefile | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Makefile b/Makefile
index b567ccca45..835030e22b 100644
--- a/Makefile
+++ b/Makefile
@@ -1793,6 +1793,7 @@ ifndef V
 	QUIET_MSGFMT   = @echo '   ' MSGFMT $@;
 	QUIET_GCOV     = @echo '   ' GCOV $@;
 	QUIET_SP       = @echo '   ' SP $<;
+	QUIET_HDR      = @echo '   ' HDR $<;
 	QUIET_RC       = @echo '   ' RC $@;
 	QUIET_SUBDIR0  = +@subdir=
 	QUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
@@ -2675,6 +2676,17 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
+GEN_HDRS := command-list.h unicode-width.h
+EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
+CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
+HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
+
+$(HCO): %.hco: %.h FORCE
+	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
+
+.PHONY: hdr-check $(HCO)
+hdr-check: $(HCO)
+
 .PHONY: style
 style:
 	git clang-format --style file --diff --extensions c,h
-- 
2.19.0

Re: [PATCH 1/9] Makefile: add a hdr-check target

[Martin Ågren]

Hi Ramsay,

On Wed, 19 Sep 2018 at 02:07, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> @@ -2675,6 +2676,17 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>  .PHONY: sparse $(SP_OBJ)
>  sparse: $(SP_OBJ)
>
> +GEN_HDRS := command-list.h unicode-width.h

Most of the things happening around here are obvious steps towards the
end-goal and seem to logically belong here, together. This one though,
"generated headers"(?) seems like it is more general in nature, so could
perhaps live somewhere else.

Actually, we have a variable `GENERATED_H` which seems to try to do more
or less the same thing. It lists just one file, though, command-list.h.
And unicode-width.h seems to be tracked in git.git.

Maybe use `GENERATED_H` instead, and list unicode-width.h on the next
line instead? Or am I completely misreading "GEN_HDRS"?

> +EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
> +CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
> +HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
> +
> +$(HCO): %.hco: %.h FORCE
> +       $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
> +
> +.PHONY: hdr-check $(HCO)
> +hdr-check: $(HCO)
> +
>  .PHONY: style
>  style:
>         git clang-format --style file --diff --extensions c,h

Re: [PATCH 1/9] Makefile: add a hdr-check target

[Ramsay Jones]

On 19/09/18 18:49, Martin Ågren wrote:
> Hi Ramsay,
> 
> On Wed, 19 Sep 2018 at 02:07, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>> @@ -2675,6 +2676,17 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>>  .PHONY: sparse $(SP_OBJ)
>>  sparse: $(SP_OBJ)
>>
>> +GEN_HDRS := command-list.h unicode-width.h
> 
> Most of the things happening around here are obvious steps towards the
> end-goal and seem to logically belong here, together. This one though,
> "generated headers"(?) seems like it is more general in nature, so could
> perhaps live somewhere else.

Yes, generated headers, but no, not more general. ;-)

I originally included those headers directly in the
EXCEPT_HDRS macro, along with the list of excluded
directories (which was much longer at one point).

The 'command-list.h' is generated as part of the build
(and so may or may not be part of the LIB_H macro), whereas
the unicode-width.h header is only re-generated when someone
runs the 'contrib/update-unicode/update_unicode.sh' script
(presumably when a new standard version is announced) and
commits the result.

Both headers fail the 'hdr-check', although both generator
scripts could be 'fixed' so that they passed. I just didn't
think it was worth the effort - neither header was likely
to be #included anywhere else. I guess I could eliminate
that macro, absorbing it into EXCEPT_HDRS, if that would
lead to less confusion ...

[I suspect the fact that LIB_H (almost always) contains
'command-list.h' has not been noticed ... :-P ]

> Actually, we have a variable `GENERATED_H` which seems to try to do more
> or less the same thing. It lists just one file, though, command-list.h.
> And unicode-width.h seems to be tracked in git.git.

Hmm, GENERATED_H seems only to be used by the i18n part of the
makefile and, as a result of its appearance in LIB_H, sometimes
results in command-list.h appearing twice in LOCALIZED_C.
(which is probably not a problem).

ATB,
Ramsay Jones

> Maybe use `GENERATED_H` instead, and list unicode-width.h on the next
> line instead? Or am I completely misreading "GEN_HDRS"?
> 
>> +EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
>> +CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
>> +HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
>> +
>> +$(HCO): %.hco: %.h FORCE
>> +       $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
>> +
>> +.PHONY: hdr-check $(HCO)
>> +hdr-check: $(HCO)
>> +
>>  .PHONY: style
>>  style:
>>         git clang-format --style file --diff --extensions c,h
> 

Re: [PATCH 1/9] Makefile: add a hdr-check target

[Martin Ågren]

On Wed, 19 Sep 2018 at 22:15, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> On 19/09/18 18:49, Martin Ågren wrote:
> > On Wed, 19 Sep 2018 at 02:07, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> >> +GEN_HDRS := command-list.h unicode-width.h
> >
> > Most of the things happening around here are obvious steps towards the
> > end-goal and seem to logically belong here, together. This one though,
> > "generated headers"(?) seems like it is more general in nature, so could
> > perhaps live somewhere else.
>
> Yes, generated headers, but no, not more general. ;-)

> The 'command-list.h' is generated as part of the build
> (and so may or may not be part of the LIB_H macro), whereas
> the unicode-width.h header is only re-generated when someone
> runs the 'contrib/update-unicode/update_unicode.sh' script
> (presumably when a new standard version is announced) and
> commits the result.

Ah, I wasn't aware of how unicode-width.h works, thanks.

> Both headers fail the 'hdr-check', although both generator
> scripts could be 'fixed' so that they passed. I just didn't
> think it was worth the effort - neither header was likely
> to be #included anywhere else.

With the above background, I'd tend to agree.

> I guess I could eliminate
> that macro, absorbing it into EXCEPT_HDRS, if that would
> lead to less confusion ...

I'm just a single data point, so don't trust my experience too much.

> > Actually, we have a variable `GENERATED_H` which seems to try to do more
> > or less the same thing. It lists just one file, though, command-list.h.
>
> Hmm, GENERATED_H seems only to be used by the i18n part of the
> makefile and, as a result of its appearance in LIB_H, sometimes
> results in command-list.h appearing twice in LOCALIZED_C.

Just thinking out loud, I suppose you could use $(GENERATED_H) instead
of hard-coding command-list.h in your patch. But with the background you
explained above, I think there's a good counter-argument to be made:

When we gain new auto-generated headers, we wouldn't actually mind `make
hdr-check` failing. We'd get the opportunity to decide whether making
the new header pass the check is worth it, or whether the correct
solution is to blacklist the auto-generated header. That's probably
better than just blacklisting the new header by default so that we don't
even notice that it has a potential problem.

So I think your approach makes sense. I stumbled on the similarity of
the name to something we already have. Maybe something like

  # Ignore various generated files, rather than updating the
  # generating scripts for little or no benefit.
  GEN_HDRS := ...

or a remark in the commit message, or rolling this into EXCEPT_HDRS, but
I'd be perfectly fine just leaving this as it is. Please trust your own
judgment more than mine.

Thanks
Martin

Re: [PATCH 1/9] Makefile: add a hdr-check target

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> Commit ef3ca95475 ("Add missing includes and forward declarations",
> 2018-08-15) resulted from the author employing a manual method to
> create a C file consisting of a pair of pre-processor #include
> lines (for 'git-compat-util.h' and a given toplevel header), and
> fixing any resulting compiler errors or warnings.

It makes sense to have tool do what we do not have to do manually.

One thing that makes me wonder with the patch is that the new check
command does not seem to need to see what is on CFLAGS and friends.
Having seen that "make DEVELOPER=1" adds more -W... on the command
line of the compiler and makes a build fail on a source that
otherwise would build, I am wondering if there are some (subset of)
options that the header-check command line wants to give to the
compiler.

Of course, there are also conditionally compiled sections of code,
which are affected by the choice of -DMACRO=VALUE; how does this new
feature handle that?

> Add a Makefile target to automate this process. This implementation
> relies on the '-include' and '-xc' arguments to the 'gcc' and 'clang'
> compilers, which allows us to effectively create the required C
> compilation unit on-the-fly. This limits the portability of this
> solution to those systems which have such a compiler.

Re: [PATCH 1/9] Makefile: add a hdr-check target

[Ramsay Jones]

On 20/09/18 15:26, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> Commit ef3ca95475 ("Add missing includes and forward declarations",
>> 2018-08-15) resulted from the author employing a manual method to
>> create a C file consisting of a pair of pre-processor #include
>> lines (for 'git-compat-util.h' and a given toplevel header), and
>> fixing any resulting compiler errors or warnings.
> 
> It makes sense to have tool do what we do not have to do manually.
> 
> One thing that makes me wonder with the patch is that the new check
> command does not seem to need to see what is on CFLAGS and friends.
> Having seen that "make DEVELOPER=1" adds more -W... on the command
> line of the compiler and makes a build fail on a source that
> otherwise would build, I am wondering if there are some (subset of)
> options that the header-check command line wants to give to the
> compiler.

Yes, this was one of my first concerns (I even asked Elijah what
compiler options he used), but I was getting useful results without
passing CFLAGS, so I just ignored that issue ... :-D

[The 'on-the-fly' compilation units don't correspond to any _actual_
compilation unit, so it's not easy to use existing rules ... but we
could use 'hco' rule specific definitions to add flags, I suppose ...]

> Of course, there are also conditionally compiled sections of code,
> which are affected by the choice of -DMACRO=VALUE; how does this new
> feature handle that?

Indeed. This bothered me as well. The 'compat' directory does not
follow the 'usual pattern' of the main headers and is particularly
sensitive to the lack of various -DMACROs. I had initially included
_all_ sub-directories in the 'exclude list' (to follow what Elijah
had done), but then removed one at a time ...

I am open to suggestions for improvements. ;-)

ATB,
Ramsay Jones

Re: [PATCH 1/9] Makefile: add a hdr-check target

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> Yes, this was one of my first concerns (I even asked Elijah what
> compiler options he used), but I was getting useful results without
> passing CFLAGS, so I just ignored that issue ... :-D
> ...
> Indeed. This bothered me as well. The 'compat' directory does not
> follow the 'usual pattern' of the main headers and is particularly
> sensitive to the lack of various -DMACROs. I had initially included
> _all_ sub-directories in the 'exclude list' (to follow what Elijah
> had done), but then removed one at a time ...
>
> I am open to suggestions for improvements. ;-)

Perhaps it is sufficient to add a mention of these two issues in log
message and an in-code comment nearby the .hco rule in Makefile, so
that people can come back later to discover what design choice we
made (i.e. "without worrying about these two issues, we are getting
useful enough results, so we decided it is OK at least for now not
to worry about them").

Thanks.