Re: [Acl-devel] [PATCH attr 2/8] build: Do not assume we have a config.h header around

[Mike Frysinger]

On 17 Nov 2022 01:08, Guillem Jover wrote:
> On Sat, 2022-11-12 at 21:30:27 +0700, Mike Frysinger wrote:
> > On 20 Jul 2019 04:14, Guillem Jover wrote:
> > > We are not installing it anyway, and to be able to use it we would need
> > > a config.h generated during a configure step, which we do not provide.
> > > Just remove the conditionals and assume everything required is present.
> > 
> > i think there's value in leveraging the system settings found in there,
> > like the various system extensions.  the normal way to make this logic
> > conditional is to check for HAVE_CONFIG_H defined.
> 
> I've never been a fan of the HAVE_CONFIG_H pattern, because that means
> if the build system is misconfigured (bad -I or similar), you might
> end up with files that do not include the config.h that apparently is
> being included, which can be rather confusing to debug.

if the build is broken, then the build is broken.  fix the build.

> > we could also integrate building of the tool so you could run `make
> > examples/copyattr` ...
> 
> Sure, I guess the main question is what's the purpose of this code,
> and what expectations it needs to abide by.
> 
> If this is intended to be a compilable example and not leave the source
> tree, then integrating more tightly with the build system would make
> more sense yes. Part of the patch already matches acl/examples/copyperm.c,
> but I'd indeed revert the removal of <config.h>, and hook it into the
> build system to build it, and remove the Makefile.
> 
> If this is intended to be a simple «project» example alongside the
> documentation that would ideally be installed as part of the development
> files, then instead of this patch the build system would install it and
> its Makefile, but in my mind not providing a simple configure.ac would be
> a bit confusing, but then that also might seem too much scaffolding?

the examples are meant to be examples of using the current source project.
having it build from within the source tree makes it trivial for the
maintainers to verify against the current source tree (regardless of what
is currently installed), and trivial for users to build against the current
package (which might not be installed).  having it build w/out the rest of
the source package is good for users who only installed the package via
their distro.  hence we want to support both.

there is no expectation that the examples are part of an autotooled project,
or that people building the examples have set up such a thing first.

backing up a bit, a more compelling argument is the fact that (1) we don't
create these defines in our own config.h currently, so the tests would never
pass, and (2) if the headers don't exist, the code doesn't compile to anything
useful (basically just an error message saying "couldn't find the library").
also maybe (3) the libacl examples don't do anything like this either.

so for those reasons, i'm fine with merging your original patch.  i didn't
actually read the example code before to see what it was doing, just going by
the general vibe of how the code was written & assuming the tests were useful.

i still think we should integrate the logic into examples/Makemodule.am for
quick local testing, but that shouldn't block this of course.
-mike

signature.asc
Description: PGP signature