headers
wayne | 12 Apr 2012 23:59

Patch for Android NDK build

Hi everyone,

I have noticed a problem with building c-ares on the Android NDK. The
problem is that c-ares currently only checks for #ifdef ANDROID but it
really also needs to check for __ANDROID__ as well.

I noticed recently that a patch was submitted by Cedric Deltheil to the
CURL sources on 20 Dec 2011, which is designed to correctly test for
building with the Android NDK. I have included a copy of his comments and
patches for everyone to review at the bottom of this email.

I think a similar patch should also be applied to c-ares as well:

diff --git a/ares_init.c b/ares_init.c
index ea2a978..3d29cfe 100644
--- a/ares_init.c
+++ b/ares_init.c
 <at>  <at>  -60,7 +60,7  <at>  <at> 
 #include <ctype.h>
 #include <time.h>

-#ifdef ANDROID
+#if defined(ANDROID) || defined(__ANDROID__)
 #include <sys/system_properties.h>
 #endif

 <at>  <at>  -971,7 +971,7  <at>  <at>  DhcpNameServer
   }
   status = ARES_EOF;
(Continue reading)

Permalink | Reply | 10 comments |
headers
Guenter | 16 Apr 2012 01:34

Re: Patch for Android NDK build

Hi Wayne,
Am 12.04.2012 23:59, schrieb wayne@...:
> I have noticed a problem with building c-ares on the Android NDK. The
> problem is that c-ares currently only checks for #ifdef ANDROID but it
> really also needs to check for __ANDROID__ as well.
>
> I noticed recently that a patch was submitted by Cedric Deltheil to the
> CURL sources on 20 Dec 2011, which is designed to correctly test for
> building with the Android NDK. I have included a copy of his comments and
> patches for everyone to review at the bottom of this email.
>
> I think a similar patch should also be applied to c-ares as well:
>
> diff --git a/ares_init.c b/ares_init.c
> index ea2a978..3d29cfe 100644
> --- a/ares_init.c
> +++ b/ares_init.c
>  <at>  <at>  -60,7 +60,7  <at>  <at> 
>   #include<ctype.h>
>   #include<time.h>
>
> -#ifdef ANDROID
> +#if defined(ANDROID) || defined(__ANDROID__)
>   #include<sys/system_properties.h>
>   #endif
>
>  <at>  <at>  -971,7 +971,7  <at>  <at>  DhcpNameServer
>     }
>     status = ARES_EOF;
>
(Continue reading)

Permalink | Reply | 9 comments |
headers
Gerald Combs | 16 Apr 2012 01:55
Favicon
Gravatar

Re: Patch for Android NDK build

On 4/15/12 4:34 PM, Guenter wrote:
> Thanks! But can you please explain to me why its not enough to check for
> __ANDROID__ only since the NDK toolchain defines it; and everyone who
> wants to use another toolchain can still define it self?

According to David Turner you should only check for __ANDROID__:

http://groups.google.com/group/android-ndk/browse_thread/thread/71ff7f7f548b5e5b/573450fddf2b8dab
Permalink | Reply | 8 comments |
headers
Wayne Piekarski | 16 Apr 2012 02:37

Re: Patch for Android NDK build

After reading the discussion in the included link, it seems as though 
some of the older NDKs do not define __ANDROID__ properly.

So if we want c-ares to be compatible with older NDK releases, then we 
should probably test for both __ANDROID__ or ANDROID.

If we want to be compatible only with the latest NDK releases, then only 
__ANDROID__ should be fine.

I'm not sure if there was any similar discussion when the similar patch 
was made to CURL. Perhaps the safest bet would be to check for both?

regards,
Wayne

On 15/04/2012 4:55 PM, Gerald Combs wrote:
> On 4/15/12 4:34 PM, Guenter wrote:
>> Thanks! But can you please explain to me why its not enough to check for
>> __ANDROID__ only since the NDK toolchain defines it; and everyone who
>> wants to use another toolchain can still define it self?
>
> According to David Turner you should only check for __ANDROID__:
>
> http://groups.google.com/group/android-ndk/browse_thread/thread/71ff7f7f548b5e5b/573450fddf2b8dab
>
Permalink | Reply | 7 comments |
headers
Guenter | 16 Apr 2012 10:10

Re: Patch for Android NDK build

Hi Wayne,
Am 16.04.2012 02:37, schrieb Wayne Piekarski:
> After reading the discussion in the included link, it seems as though
> some of the older NDKs do not define __ANDROID__ properly.
>
> So if we want c-ares to be compatible with older NDK releases, then we
> should probably test for both __ANDROID__ or ANDROID.
>
> If we want to be compatible only with the latest NDK releases, then only
> __ANDROID__ should be fine.
>
> I'm not sure if there was any similar discussion when the similar patch
> was made to CURL. Perhaps the safest bet would be to check for both?
yes, sure, its no prob checking for both; however I was just on exactly 
this topic (I did setup autobuilds for c-ares and curl the last days):
http://curl.haxx.se/dev/builds.html
and I did two other commits to c-ares which only check __ANDROID__ ...
It was my impression that perhaps those who added initially Android 
support have used a standard toolchain rather than the NDK one and then 
just choosed ANDROID ...
also I would then like to know about my other commits if they are needed 
for older toolchains as well, and how it worked without these changes 
for others?
Fix for Android to include sys/select.h for fd_set:
https://github.com/bagder/c-ares/commit/4ef145cede844ba0a690219b36fc585d62863bf8
Fix for Android to disable useless arpa/nameser.h:
https://github.com/bagder/c-ares/commit/0d6ef42b5fd717f646b17623d35640871621e448

so thats the reason why I ask ...
(Continue reading)

Permalink | Reply | 6 comments |
headers
Guenter | 16 Apr 2012 10:37

Re: Patch for Android NDK build

Hi Wayne,
Am 16.04.2012 10:10, schrieb Guenter:
> Am 16.04.2012 02:37, schrieb Wayne Piekarski:
>> After reading the discussion in the included link, it seems as though
>> some of the older NDKs do not define __ANDROID__ properly.
>>
>> So if we want c-ares to be compatible with older NDK releases, then we
>> should probably test for both __ANDROID__ or ANDROID.
>>
>> If we want to be compatible only with the latest NDK releases, then only
>> __ANDROID__ should be fine.
>>
>> I'm not sure if there was any similar discussion when the similar patch
>> was made to CURL. Perhaps the safest bet would be to check for both?
commited now after I've read the discussion and the curl fix:
https://github.com/bagder/c-ares/commit/995fa144d97a80f87b40a1e335eb72a938026d48

also I've added the ANDROID macro check for my recent changes since at 
least the one for sys/select.h is exactly the same as what was in the 
curl patch:
https://github.com/bagder/c-ares/commit/7d3f34115657eaf47a21b1a42d515fd2f9776227

thanks, Gün.
Permalink | Reply | 5 comments |
headers
Guenter | 16 Apr 2012 11:34

missing INSTALL file

Hi Daniel,
I just found that we do not yet have an INSTALL file as we have with 
curl - what do you think about copying it over to c-ares? I think that 
most if not all in it applies to c-ares as well, and since we have now 
c-ares living as standalone project I believe it makes sense to have it 
in the c-ares repo too ...
we can just copy it over, and tweak a bit if needed once its in ...

Gün.
Permalink | Reply | 3 comments |
headers
Guenter | 16 Apr 2012 11:38

Re: Patch for Android NDK build

Am 16.04.2012 10:37, schrieb Guenter:
  commited now after I've read the discussion and the curl fix:
> https://github.com/bagder/c-ares/commit/995fa144d97a80f87b40a1e335eb72a938026d48
autobuilds are really great ;-)
follow-up commit to fix c&p error:
58e62c58794500cb65e7684f1d09c7d951b8b42d

Gün.
Permalink | Reply | 0 comments |
headers
Wayne Piekarski | 16 Apr 2012 16:15

Re: Patch for Android NDK build

Hi Guenter,

Thanks for adding patches for all of these issues. After we resolved the __ANDROID__ issue the sys/select
and nameserver problems were going to be the next thing I was going to ask about :)

Thanks for the quick response in getting this fixed. I look forward to the next release!

regards,
Wayne

On Apr 16, 2012, at 1:37 AM, Guenter wrote:

> Hi Wayne,
> Am 16.04.2012 10:10, schrieb Guenter:
>> Am 16.04.2012 02:37, schrieb Wayne Piekarski:
>>> After reading the discussion in the included link, it seems as though
>>> some of the older NDKs do not define __ANDROID__ properly.
>>> 
>>> So if we want c-ares to be compatible with older NDK releases, then we
>>> should probably test for both __ANDROID__ or ANDROID.
>>> 
>>> If we want to be compatible only with the latest NDK releases, then only
>>> __ANDROID__ should be fine.
>>> 
>>> I'm not sure if there was any similar discussion when the similar patch
>>> was made to CURL. Perhaps the safest bet would be to check for both?
> commited now after I've read the discussion and the curl fix:
> https://github.com/bagder/c-ares/commit/995fa144d97a80f87b40a1e335eb72a938026d48
> 
> also I've added the ANDROID macro check for my recent changes since at least the one for sys/select.h is
(Continue reading)

Permalink | Reply | 3 comments |
headers
Daniel Stenberg | 16 Apr 2012 22:25
Picon
Favicon
Gravatar

Re: missing INSTALL file

On Mon, 16 Apr 2012, Guenter wrote:

> I just found that we do not yet have an INSTALL file as we have with curl - 
> what do you think about copying it over to c-ares? I think that most if not 
> all in it applies to c-ares as well, and since we have now c-ares living as 
> standalone project I believe it makes sense to have it in the c-ares repo 
> too ... we can just copy it over, and tweak a bit if needed once its in ...

Makes perfect sense, yes!

--

-- 

  / daniel.haxx.se
Permalink | Reply | 2 comments |

Gmane