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

[Florian Weimer <fweimer@redhat.com>]

On 11/25/2016 07:44 PM, Zack Weinberg wrote:
> 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/"?

support/ works for me.  Any objects to that?

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

For shared builds, we still build .oS, that is lib_*nonshared.a.  Sure, 
it's rather strange, but it looks like this is the way it is expected to 
work.

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

We already have one use of write_message from a test DSO, in 
dlfcn/bug-atexit3-lib.cc.  This was impossible with the existing test 
skeleton.

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

Fine, <sys/cdefs.h> it is.

Thanks,
Florian