This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH] make search.h, locale.h and libint.h optional
- From: Josh Stone <jistone at redhat dot com>
- To: groleo at gmail dot com
- Cc: systemtap at sourceware dot org, Negreanu Marius Adrian <adrian dot m dot negreanu at intel dot com>
- Date: Thu, 21 Mar 2013 13:03:35 -0700
- Subject: Re: [PATCH] make search.h, locale.h and libint.h optional
- References: <1363894024-31972-1-git-send-email-groleo at gmail dot com>
Hi,
On 03/21/2013 12:27 PM, groleo@gmail.com wrote:
> Some environments (like Android) don't have them.
I'm happy to see patches for Android, but this seems incomplete.
Excerpted comments below...
> --- a/configure.ac
> +++ b/configure.ac
> @@ -473,6 +473,10 @@ date=`date +%Y-%m-%d`
> AC_DEFINE_UNQUOTED(DATE, "$date", [Configuration/build date])
> AC_SUBST(DATE, "$date")
>
> +AC_CHECK_HEADERS([spawn.h])
> +AC_CHECK_HEADERS([libintl.h])
> +AC_CHECK_HEADERS([locale.h])
Your subject says search.h, not spawn.h - do you need both?
And I don't see that you changed any posix_spawn_* calls, only the
#include - does Android provide those functions in a different header?
On libintl.h and locale.h, I guess you have !ENABLE_NLS? I'm not sure
if we use anything else, but could we just generally move those
#includes within the #if ENABLE_NLS blocks?
> --- a/session.h
> +++ b/session.h
> @@ -10,7 +10,9 @@
> #define SESSION_H
>
> #include "config.h"
> +#ifdef HAVE_LIBINTL_H
> #include <libintl.h>
> +#endif
> #include <locale.h>
Missed HAVE_LOCALE_H?
> +#ifdef HAVE_SEARCH_H
> if (execvp ((sh_c_argv[0] == NULL ? words.we_wordv[0] : sh_c_argv[0]),
> (sh_c_argv[0] == NULL ? words.we_wordv : sh_c_argv)) < 0)
> +#else
> + if (execvp (sh_c_argv[0], sh_c_argv) < 0)
> +#endif
This one should probably be HAVE_WORDEXP_H.
> --- a/staprun/stapio.c
> +++ b/staprun/stapio.c
> @@ -25,11 +25,11 @@ char *__name__ = "stapio";
>
> int main(int argc, char **argv)
> {
> -
> +#ifdef HAVE_LOCALE_H
> setlocale (LC_ALL, "");
> bindtextdomain (PACKAGE, LOCALEDIR);
> textdomain (PACKAGE);
> -
> +#endif
Actually, I'm surprised this one is not already using #if ENABLE_NLS.
And aren't there are a few other places doing this initialization which
you didn't patch? I'm guessing you don't build those on Android, but
let's be complete here...