[PATCH] some fixes for waf issues

[Andrew Bartlett]

On Wed, 2014-01-08 at 20:40 +0100, Christian Ambach wrote:
> Hi Jelmer,
> 
> Am 12.12.13 23:32, schrieb Jelmer Vernooij:
> 
> thank you very much for your review comments.
> 
> >> -        conf.DEFINE('DEFAULT_DOS_CHARSET', default_dos_charset, quote=True)
> >> -        conf.DEFINE('DEFAULT_UNIX_CHARSET', default_unix_charset, quote=True)
> >> +        if default_dos_charset is False or default_unix_charset is False:
> >> +        # we found iconv, but it failed to convert anything (e.g. on AIX)
> >> +            conf.undefine('HAVE_NATIVE_ICONV');
> >> +            conf.DEFINE('DEFAULT_DOS_CHARSET', "ASCII", quote=True)
> >> +            conf.DEFINE('DEFAULT_UNIX_CHARSET', "UTF8", quote=True)
> > ^^^  You might want to keep the code as is, but just set
> > default_dos_charset = "ASCII" and default_unix_charset = "UTF-8" here.
> 
> Changed patch is in the attached set.
> 
> >> Subject: [PATCH 2/4] waf:lib/replace fix up libintl related checks
> >>
> >> checklibc=True and link=True gives back false-positives
> >> on AIX for gettext/dgettext and so the build fails later when
> >> libintl.h is not available
> >>
> >> So let's check this very pedantically
> >
> > My understanding is that this makes us skip the link step, which would
> > actually make the check less pedantic rather than more (since we
> > wouldn't see any potential linking issues).
> >
> > Isn't this just a bug in CHECK_FUNCS_IN when link=True is set?
> 
> I am not sure how we should correctly deal with this.
> Examining the configure logs on AIX I we might want to change our
> detection strategy:
> The special case on AIX is that libintl.a exists, but libintl.h not.
> Without the link check, waf fails to detect gettext and friends because
> there is no header around that would declare them when it tries to
> compile code like this:
> int main(void){void *__x = (void *)dgettext;return (int)__x;return 0;}
> 
> When enabling the link check (that is enabled by default), it compiles
> and links the following code:
> #define gettext __fake__gettext
> [...]
> #undef gettext
> #if defined __stub_gettext || defined __stub___gettext
> #error "bad glibc stub"
> #endif
> extern char gettext();
> int main() { return gettext(); }
> 
> This compiles (as gettext is declared) and links (as the symbol can be
> found in libintl.a).
> The bad thing for the "real" code is that we still do not have
> prototypes around and so compilation fails (because those functions do
> not return int, as assumed when the prototype is lacking).
> This might also happen with other libraries we depend on, but for some
> cases we need this functionality (e.g. when accessing functions in the
> Kerberos libs that are not declared in the public headers).
> 
> I see two options here:
> 1. refine the configuration checks so they fail if no prototype could be
> found, a positive link check should not be good enough
> 2. declare the prototypes in lib/replace when they are missing (as for
> fdatasync)

We really should avoid that, except where we are very, very sure of the
prototype. 

> I would lean towards option 2 as it is less intrusive than changing
> the behavior of configure checks for everything that we check for.
> This is what you can find in the attached patchset and what is reported
> to fix the build on AIX by the bug reporter.

I think 1 is the better option, unless there is a very pressing need for
intl on AIX and really no way to install the correct headers. 

> >> +        # any extra path given to look at?
> >> +        if Options.options.gettext_location:
> >> +           conf.env['CFLAGS'].extend("-I%s" % Options.options.gettext_location);
> >> +           conf.env['LDFLAGS'].extend("-L%s" % Options.options.gettext_location);
> > ^^^ This seems wrong. Shouldn't it be .extend(["-I..."]) or
> > .append("-I...") ?
> 
> Changed to the extend variant, see attached patch set.
> My previous patches in that area used the same wrong style and so broke
> the build on MacOS (as Björn pointed out in Bug 9911).
> A patch for that issue is included as well (reported to fix the build 
> break).

I've been doing some build farm work, and disabled gettext on
SerNet-imini.  If MacOS now builds, we may need to unpatch the build
farm.  I just mention this so we don't forget. 

The link of 'optional, but you really want' features that we now require
you to specify --without-xxx for is currently:
 - acl-support
 - gettext
 - ldap
 - ads-support

If we extend this, we just need to make sure to patch the build farm to
match, by reading the configure or config.h output presented there. 

> >> +
> >> +    # did the user insist on gettext (--with-gettext)?
> >> +    if Options.options.gettext_location and not conf.env['HAVE_GETTEXT'] and conf.env['HAVE_DGETTEXT']:
> >> +            conf.fatal('library gettext not found at specified location')
> > ^^^ Please use four-space indents.
> 
> done, see attachment.

Thanks for your work on the build system.  

Andrew Bartlett
-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba