Обсуждение: Proposal: common explicit lists for installed headers

Hello

This patch is based on an idea in the recent headerscheck for meson
discussion[1]:
What if, instead of the current completely different ways we handle
installed headers in the make and meson builds, we used a common list
as the source of truth, explicitly listing all files that need to be
installed?

This has several advantages:

* removes the questionable install_subdir usage in meson
* removes the also questionable glob usage in make
* provides a clear list of includes we need in the installation directory
* this list could also later be used by headerscheck, which could then
work the same way in both meson and make

I have currently split this into three separate patches:

* the first one is the bulk of the changes, it handles everything in src/include
* the second adds the remaining src directories not included in the
first one. This is less useful for the unification goal, but it would
help with the headerscheck goal
* the third one is a completely optional unification. With the first
two patches, the public and internal headers are defined in two
separate lists because they come from different source directories.
This patch moves them into a single file instead, but with the
downside that the file now references headers in a different
directory. This is definitely questionable, I included it only as a
possible alternative, and ignoring it is probably better

What isn't included:

* contrib modules are unchanged - I didn't want to include them in a
common list, and adding separate list files per target would just add
complexity
* generated headers are similarly already listed explicitly in both
meson and make, I didn't change that

This could be a later question for headerscheck if we go in this
direction and want it to check these headers - but for now, manually
replicating them into lists doesn't seem practical.

I verified with a few different configurations that these changes
result in the same installed headers in both the make and meson builds
as before.

Thoughts? Would this be a useful refactoring?

[1]: https://www.postgresql.org/message-id/33wusv6jgyvm4tqw7ibjsh5vutnh3gyleyerkdzcv4mhf2ijmh%40wkirx27m67ku

Zsolt Parragi <zsolt.parragi@percona.com> writes:
> What if, instead of the current completely different ways we handle
> installed headers in the make and meson builds, we used a common list
> as the source of truth, explicitly listing all files that need to be
> installed?

While I agree that it's not great that the two build systems do this
differently, I think you've chosen the worst-common-denominator
solution.  There is nothing to like about an explicit list of
installed files.  It requires useless make-work from patch
authors and committers, and there is a 100% gold-plated certainty
that we will sometimes forget to add some new header to the list.
We won't notice such omissions until some end user complains,
maybe years later.

What we really want is "install everything named like include/.../*.h".
I don't know if it's possible to make meson do that, though.

There was some discussion recently of using "git ls-files" for
a similar purpose, could that help?  At least for me,
    git ls-files 'src/include/*.h'
seems to produce a pretty usable list of server headers.  You'd
also need to account for generated headers, but the build systems
necessarily know about all of those.

            Regards, tom lane

> I think you've chosen the worst-common-denominator
> solution. There is nothing to like about an explicit list of
> installed files.

This is a difficult and very opinionated question. Personally, I like
to use CMake's glob_recurse + configure_depends even for C/C++ source
files in my small personal projects for simplicity, so I completely
understand and somewhat agree with your point.

In practice however, both Meson[1] and CMake[2], the two most popular
build systems, actively discourage any type of globbing/automatic file
discovery magic, and they have good reasons for doing so.

The main problem with it, which was also why Andreas Freund suggested
doing this in the headerscheck thread is that this breaks dependency
resolution:
if a new header file appears, that won't result in a build script
change, which means that the build system won't pick it up. It won't
get installed, it won't get verified by headerscheck, etc.

This means that it can:
* lead to CI failures/misses
* lead to developers missing build errors locally, wasting CI time
* break git bisect

The workaround for it (e.g. cmake's configure_depends) is to instead
of running the glob expression / git ls-files / etc during configure
time, run it for every build invocation. That's fine on fast NVMEs,
terrible on spinning disks / slow fs with large projects like
postgres.

> that we will sometimes forget to add some new header to the list.
> We won't notice such omissions until some end user complains,
> maybe years later.

This already happens, I just submitted a patch for a fix like this
yesterday, which I noticed while working on this [3]. We only use
glob/add_subdir for some directories, not all of them. The current
behavior is inconsistent, and because of that, it is easier to miss. I
think having a clear rule, and following it everywhere would improve
things.

> At least for me,
> git ls-files 'src/include/*.h'
> seems to produce a pretty usable list of server headers.

I was thinking about adding a configure-time step that verifies the
files: during configuration, it checks if we missed any headers (e.g.
they are not in the list files), and reports an error. Still keeping
the explicit requirement, but adding a guardrail - also, CI would fail
quickly if something is missed.

For now I left it out to:

a. keep the first patch simpler
b. keep things consistent, because while this is true for src/include,
it's not true for the other directories, we do not install all header
files from those (e.g. src/interfaces/libpq). If I wanted to add
automatic checks there, I would have to also add exclude lists.

[1]: https://mesonbuild.com/FAQ.html#why-cant-i-specify-target-files-with-a-wildcard
[2]: https://cmake.org/cmake/help/latest/command/file.html#filesystem
[3]: https://www.postgresql.org/message-id/CAN4CZFP6NOjv__4Mx%2BiQD8StdpbHvzDAatEQn2n15UKJ%3DMySSQ%40mail.gmail.com