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

From: Martin Ågren <martin.agren@gmail.com>
Date: Sep 20, 2018, 05:53 UTC
Message-ID: <CAN0heSq8U7ue-SB-1iP1ZdW2hgrR0fwuKh_q68KvJ4K1dphpCw@mail.gmail.com>
In-Reply-To: <89478060-85c1-416a-0ad9-54d3f8f1111b@ramsayjones.plus.com>

On Wed, 19 Sep 2018 at 22:15, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:

Show 10 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

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