Re: [PATCH] make search.h, locale.h and libint.h optional

[Adrian M Negreanu <groleo at gmail dot com>]

On Thu, Mar 21, 2013 at 10:03 PM, Josh Stone <jistone@redhat.com> wrote:
> 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?
I'll check that and document it in the commit message.
>
> 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?
Will do.


>> --- 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...
fair enough :). I'll submit a v2 then.
Thanks the the quick review.



--
Regards!
http://groleo.wordpress.com