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

[Florian Weimer <fweimer@redhat.com>]

On 11/25/2016 05:14 PM, Zack Weinberg wrote:
> On 11/25/2016 10:59 AM, Florian Weimer wrote:
>> I have split up test-skeleton.c into its components.  The new test
>> skeleton should be compatible with compilation in C90 mode.  I expect to
>> use some of these helper functions for future build support on the host
>> (that is, these routines will have to be compiled twice, once against
>> the built libc, and once against the host libc).
>
> I support this general idea, especially the "not #including
> test-skeleton.c anymore" part.
>
> 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).

> (It looks like you've set it up so libextras is not installed, so that's
> not a concern.)

Good point, I need to validate this.

To reiterate, there is no intent on my part to ship this for use in 
application code.

>> I do not propose bulk migration at this point.  Some obscure use cases
>> are not supported by the exported hooks.
>
> Could you give an example?

PREPARE with argc/argv arguments and command line parser extensions. 
The latter requires some gymnastics to support it's heavily based on 
preprocessor macros and assumes that <getopt.h> is always included.

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

>> +#ifndef EXTRAS_CHECK_H
>> +#define EXTRAS_CHECK_H
>> +
>> +#include <features.h>
>> +
>> +__BEGIN_DECLS
>> +
>> +/* Print failure message to standard output and return 1.  */
>> +#define FAIL_RET(...) \
>> +  return __extras_print_failure (__FILE__, __LINE__, __VA_ARGS__)
>
> 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.

Florian