Re: [PATCH] extras: New test/build infrastructure

[Zack Weinberg <zackw@panix.com>]

On Fri, Nov 25, 2016 at 12:45 PM, Florian Weimer <fweimer@redhat.com> wrote:
> On 11/25/2016 05:14 PM, Zack Weinberg wrote:
>>
>> Can I ask why the new directory is called "extras"?  That makes it sound
>> like a home for extra features that we want to provide but not in the
>> core C library.  Something more obviously internal-use and
>> build/test-related would be better, I think.
>
> I plan to use bits of it for fixing localedef bugs and contributing Fedora
> changes upstream.  Those bits would then end up in installed binaries.
>
> The immediate need is for testing only and generic test support code
> (container setup, a fake DNS server implementation, and so on).

So maybe "support/" or "build-test-support/"?

>>> +libextras-static-only-routines := $(libextras-routines)
>>> +# Only build one variant of the library.
>>> +libextras-inhibit-o := .os
>>> +ifeq ($(build-shared),yes)
>>> +libextras-inhibit-o += .o
>>> +endif
>>
>> This doesn't look right if the goal is to build only the .a version of
>> the library.
>
> Could you clarify what worries you?  If there's just one variant, it has to
> be PIC, unless it's a static-only build.

I may well not understand what this combination of
-static-only-routines and -inhibit-o does, but what it *looks* like it
does is disable generation of .os (PIC) object files unconditionally,
and if shared libraries are enabled, it also disables .o (static)
object files. I would expect this to wind up either not working at
all, or producing only a *shared* library, or possibly only a
profiling library!

Also I don't know why this code would need to be PIC.

>> This library is _not_ part of the implementation and should not be using
>> __ names.  And I'm not sure it ought to be using features.h either.
>
> <features.h> is needed for __BEGIN_DECLS.  Including <sys/cdefs.h> would be
> even more extreme, I think.

Honestly I think sys/cdefs.h has a better claim to be a public
interface than features.h, since it exists on *BSD (and does in fact
define compatible __BEGIN_DECLS/__END_DECLS there too: see
https://github.com/freebsd/freebsd/blob/master/sys/sys/cdefs.h)

zw