[PATCH] some fixes for waf issues
Christian Ambach
ambi at samba.org
Wed Jan 8 12:40:26 MST 2014
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)
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.
>> + # 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).
>> +
>> + # 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.
Cheers,
Christian
-------------- next part --------------
From 7aacff3a94ccb9d7d58d50d5c781fded0dd7f56e Mon Sep 17 00:00:00 2001
From: Christian Ambach <ambi at samba.org>
Date: Wed, 4 Dec 2013 22:50:11 +0100
Subject: [PATCH 1/6] waf: improve iconv checks
there are broken iconv implementations around (e.g. on AIX) that you
can compile against but that refuse any mapping requests
make sure we do the same as the autoconf-based build did and
fall back to our own code
Bug: https://bugzilla.samba.org/show_bug.cgi?id=10308
Signed-off-by: Christian Ambach <ambi at samba.org>
---
source3/build/charset.py | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/source3/build/charset.py b/source3/build/charset.py
index 44852a6..8cef7da 100644
--- a/source3/build/charset.py
+++ b/source3/build/charset.py
@@ -33,6 +33,12 @@ def CHECK_SAMBA3_CHARSET(conf, crossbuild=False):
default_unix_charset="UTF-8"
# TODO: this used to warn about the set charset on cross builds
+ 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');
+ default_dos_charset = "ASCII"
+ default_unix_charset = "UTF8"
+
conf.DEFINE('DEFAULT_DOS_CHARSET', default_dos_charset, quote=True)
conf.DEFINE('DEFAULT_UNIX_CHARSET', default_unix_charset, quote=True)
--
1.8.1.2
From 55a8f7495ec61b62b6275d535db6fbb23456b06e Mon Sep 17 00:00:00 2001
From: Christian Ambach <ambi at samba.org>
Date: Thu, 2 Jan 2014 23:28:20 +0100
Subject: [PATCH 2/6] waf:lib/replace correct detection of libiconv
add -liconv as a complete command line argument,
not all characters on their own
Bug: https://bugzilla.samba.org/show_bug.cgi?id=9911
Signed-off-by: Christian Ambach <ambi at samba.org>
---
lib/replace/wscript | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/replace/wscript b/lib/replace/wscript
index a316032..09efdc8 100644
--- a/lib/replace/wscript
+++ b/lib/replace/wscript
@@ -390,7 +390,7 @@ removeea setea
# Some hosts need lib iconv for linking with lib intl
# So we try with flags just in case it helps.
oldflags = conf.env['EXTRA_LDFLAGS'];
- conf.env['EXTRA_LDFLAGS'].extend("-liconv")
+ conf.env['EXTRA_LDFLAGS'].extend(["-liconv"])
conf.CHECK_FUNCS_IN('dgettext gettext bindtextdomain textdomain bind_textdomain_codeset',
'intl', checklibc=False, headers='libintl.h')
conf.env['EXTRA_LDFLAGS'] = oldflags
--
1.8.1.2
From e3f425855164124d45c8c9186fe156ea7f61dbb6 Mon Sep 17 00:00:00 2001
From: Christian Ambach <ambi at samba.org>
Date: Thu, 2 Jan 2014 22:23:16 +0100
Subject: [PATCH 3/6] waf:lib/replace fix up libintl related checks
on a default installation of AIX, libintl.a exists but no
libintl.h
So check for the declarations of those functions as well
to make sure that replace.h can declare those functions
if necessary
Bug: https://bugzilla.samba.org/show_bug.cgi?id=9911
Signed-off-by: Christian Ambach <ambi at samba.org>
---
lib/replace/wscript | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/replace/wscript b/lib/replace/wscript
index 09efdc8..6784dd3 100644
--- a/lib/replace/wscript
+++ b/lib/replace/wscript
@@ -371,6 +371,7 @@ removeea setea
if not Options.options.disable_gettext:
conf.CHECK_HEADERS('libintl.h')
conf.CHECK_LIB('intl')
+ conf.CHECK_DECLS('dgettext gettext bindtextdomain textdomain bind_textdomain_codeset', headers="libintl.h")
# *textdomain functions are not strictly necessary
conf.CHECK_FUNCS_IN('bindtextdomain textdomain bind_textdomain_codeset',
'', checklibc=True, headers='libintl.h')
--
1.8.1.2
From a2caabe855e2025c69ad3af956aee9c9be3c32bd Mon Sep 17 00:00:00 2001
From: Christian Ambach <ambi at samba.org>
Date: Thu, 2 Jan 2014 22:22:23 +0100
Subject: [PATCH 4/6] lib/replace add prototypes for libintl functions
one some platforms (e.g. AIX), libintl.h is not there, but libintl.a is
so the functions can be used if prototypes are there
Bug: https://bugzilla.samba.org/show_bug.cgi?id=9911
Signed-off-by: Christian Ambach <ambi at samba.org>
---
lib/replace/replace.h | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/lib/replace/replace.h b/lib/replace/replace.h
index c0b7997..8cab307 100644
--- a/lib/replace/replace.h
+++ b/lib/replace/replace.h
@@ -899,4 +899,25 @@ int usleep(useconds_t);
void rep_setproctitle(const char *fmt, ...) PRINTF_ATTRIBUTE(1, 2);
#endif
+/* prototypes of libintl functions */
+#ifndef HAVE_DECL_GETTEXT
+char* gettext(const char* msgid);
+#endif
+
+#ifndef HAVE_DECL_DGETTEXT
+char* dgettext(const char* domainname, const char* msgid);
+#endif
+
+#ifndef HAVE_DECL_BINDTEXTDOMAIN
+char* bindtextdomain(const char* domainname, const char* dirname);
+#endif
+
+#ifndef HAVE_DECL_TEXTDOMAIN
+char* textdomain(const char* domainname);
+#endif
+
+#ifndef HAVE_DECL_BIND_TEXTDOMAIN_CODESET
+char* bind_textdomain_codeset(const char* domainname, const char* codeset);
+#endif
+
#endif /* _LIBREPLACE_REPLACE_H */
--
1.8.1.2
From 68f1a465328882894694a05ed976a1a55f0e82b8 Mon Sep 17 00:00:00 2001
From: Christian Ambach <ambi at samba.org>
Date: Thu, 12 Dec 2013 22:10:36 +0100
Subject: [PATCH 5/6] waf:lib/replace fix gettext detection
if the user has specified a path for gettext, add it to CFLAGS and LDFLAGS
so we can find it during configure and build
Bug: https://bugzilla.samba.org/show_bug.cgi?id=9911
Signed-off-by: Christian Ambach <ambi at samba.org>
---
lib/replace/wscript | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/lib/replace/wscript b/lib/replace/wscript
index 6784dd3..a957cb4 100644
--- a/lib/replace/wscript
+++ b/lib/replace/wscript
@@ -369,6 +369,10 @@ removeea setea
conf.env.intl_libs=''
if not Options.options.disable_gettext:
+ # 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]);
conf.CHECK_HEADERS('libintl.h')
conf.CHECK_LIB('intl')
conf.CHECK_DECLS('dgettext gettext bindtextdomain textdomain bind_textdomain_codeset', headers="libintl.h")
--
1.8.1.2
From 9b5bf364e67760b4d5a1eb3555443a06f6d2f024 Mon Sep 17 00:00:00 2001
From: Christian Ambach <ambi at samba.org>
Date: Thu, 12 Dec 2013 22:12:07 +0100
Subject: [PATCH 6/6] waf:lib/replace change detection of gettext
convert this to an automatic check: if no option is given, try to find gettext
and if found, use it
if user has specified --with-gettext, then bail out if it could not be found
in case of --without-gettext, skip all gettext related configure checks
Bug: https://bugzilla.samba.org/show_bug.cgi?id=9911
Signed-off-by: Christian Ambach <ambi at samba.org>
Reviewed-By: Jelmer Vernooij <jelmer at samba.org>
---
lib/replace/wscript | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/lib/replace/wscript b/lib/replace/wscript
index a957cb4..f597ac6 100644
--- a/lib/replace/wscript
+++ b/lib/replace/wscript
@@ -367,6 +367,7 @@ removeea setea
headers='netinet/in.h arpa/nameser.h resolv.h')
+ # try to find libintl (if --without-gettext is not given)
conf.env.intl_libs=''
if not Options.options.disable_gettext:
# any extra path given to look at?
@@ -402,10 +403,10 @@ removeea setea
if conf.env['HAVE_GETTEXT'] and conf.env['HAVE_DGETTEXT']:
# save for dependency definitions
conf.env.intl_libs='iconv intl'
- else:
- conf.fatal('library gettext not found, try specifying the path to ' +
- 'it with --with-gettext=</path/to/gettext> or ' +
- '--without-gettext to build without''')
+
+ # 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')
conf.CHECK_FUNCS_IN('pthread_create', 'pthread', checklibc=True, headers='pthread.h')
--
1.8.1.2
More information about the samba-technical
mailing list