On Mon, Mar 04, 2019 at 01:38:13PM +0100, Johannes Schindelin wrote:
> > So, tl;dr: soon, I will be submitting a patch to remove the
> > 'hdr-check' target completely, for now anyway.
>
> You mentioned later that you might be the only person using that target,
> and if that were so, I would agree.
>
> However, I started looking into adding `hdr-check` to the Azure Pipeline
> we have (which would make a *ton* of sense, if you ask me), along with the
> `sparse` thing we talked about.
>
> So I am a bit opposed to removing that target.
Yeah, I agree. If it's a linting check we expect developers to follow,
then we need to make it easy for developers to run it.
> > Yep, for me too. There is a similar problem if you build with
> > NO_CURL and don't have the 'curl/curl.h' header file, etc ...
>
> I think all it needs to do is to pass `$(ALL_CFLAGS)` to gcc (although we
> might need to add something like `#if defined(SHA256_GCRYPT) ... #endif`
> guards to `sha256/gcrypt.h` to make it work in Peff's case).
>
> But given that this target really is meant to catch all kinds of errors,
> I'd be in favor of declaring that target requiring things like libgcrypt's
> header files to be installed. We can easily make that happen in our CI
> builds.
I don't think that really scales, though. For CI, perhaps it's not too
bad, but IMHO there's real value making it possible for everyday
developers to run linting and tests locally, because it keeps the
feedback cycle tight. In other words, imagine my changes go through a
sequence like this:
1. I run "make" locally
2. I run "make test" locally
3. I push, and CI runs "make && make test" on more platforms, or with
more knobs tweaked.
it's preferable to find out about problems in the early steps, because
the cost to run each of the subsequent steps is much higher.
So if we're adding more linting, it would ideally be part of the build.
The obvious reasons not to are that it's expensive, or that it's hard
for the local user to set up. But I think at least a hdr-check that
checks _your_ platform would be valuable to run as part of step 1 or 2.
I also think it points to a weakness in the current hdr-check scheme. It
omits compat/* entirely, so we can't detect any problems there. But if
we are going to add those back in, then making it run in CI is not as
simple as "just make sure we have all libraries installed". It's
inherently going to be a different set per platform (and we'd want to
run "make hdr-check" on each of the platforms where we already run "make
test").
> > Anyway, for now, since I seem to be the only person using this
> > target, I think we should just remove it while I think again.
> > (I can put it in my config.mak, so there will be no loss for me).
>
> As I said above, I would rather keep it, with the `ls-files` and `:=`
> fixup.
>
> If others *really* feel strongly about lazy-evaluating `LIB_H`, then I
> would *additionally* use Peff's sub-make hack. But only if others feel
> strongly about it, because I, for one, certainly don't.
My measurements show it's not a big expense at least on my system (even
with the 3x runs). So I can live with just moving it to ls-files to
bound the filesystem access, and calling that good enough.
-Peff
