681558 - NSPR supports Android-x86

Status: RESOLVED FIXED

Description

[Makoto Kato [:m_kato]]

buildconfig of NSPR should support Android-x86.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: fix v1

Comment 2

[Wan-Teh Chang]

m_kato: thank you for the patch.

Ted: who's the best person to review build system patches
for Android?  You, Brad Lassey, or Mike Hommey?

Comment 3

[Wan-Teh Chang]

Comment on attachment 555335 [details] [diff] [review]
fix v1

In pr/src/Makefile.in:

> ifeq ($(OS_ARCH),Linux)
> ifeq ($(USE_PTHREADS), 1)
>+ifeq ($(OS_TARGET),Android)
>+# Android has no libpthread.so in NDK
>+OS_LIBS		= -ldl
>+else
> OS_LIBS		= -lpthread -ldl
>+endif
> else
> OS_LIBS		= -ldl
> endif

m_kato: why doesn't Android-arm need this change?

Comment 4

[(not currently active) Ted Mielczarek]

I can review these patches.

Comment 5

[Makoto Kato [:m_kato]]

(In reply to Wan-Teh Chang from comment #3)
> Comment on attachment 555335 [details] [diff] [review]
> fix v1
> 
> In pr/src/Makefile.in:
> 
> > ifeq ($(OS_ARCH),Linux)
> > ifeq ($(USE_PTHREADS), 1)
> >+ifeq ($(OS_TARGET),Android)
> >+# Android has no libpthread.so in NDK
> >+OS_LIBS		= -ldl
> >+else
> > OS_LIBS		= -lpthread -ldl
> >+endif
> > else
> > OS_LIBS		= -ldl
> > endif
> 
> m_kato: why doesn't Android-arm need this change?

There is no libpthread.so in NDK.  On arm toolchain, -lpthread doesn't cause error even if there is no libpthread.so.  But, on x86 toolchain, it causes error due to missing library.

So we should remove -lpthread if target is Android.

Comment 6

[Wan-Teh Chang]

Comment on attachment 555335 [details] [diff] [review]
fix v1

r=wtc.

Ted, when you review this patch, please verify
that the Android NDK does not have libpthread.so.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 555335 [details] [diff] [review]
fix v1

Review of attachment 555335 [details] [diff] [review]:
-----------------------------------------------------------------

I checked, the NDK does not include a pthread library, just the headers. I'm not sure why this didn't break on the ARM build.

This patch looks fine.

Comment 8

[(not currently active) Ted Mielczarek]

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.314; previous revision: 1.313
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.316; previous revision: 1.315
done
Checking in pr/src/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/Makefile.in,v  <--  Makefile.in
new revision: 1.61; previous revision: 1.60
done

Comment 9

[:Ehsan Akhgari]

Pushed to mozilla-central in the latest NSPR update: https://hg.mozilla.org/mozilla-central/rev/7e25caedd620

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> Pushed to mozilla-central in the latest NSPR update:
> https://hg.mozilla.org/mozilla-central/rev/7e25caedd620

The change you linked to isn't the same as the patch posted here...and that patch doesn't appear in current m-c.

Comment 11

[:Ehsan Akhgari]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #10)
> (In reply to Ehsan Akhgari [:ehsan] from comment #9)
> > Pushed to mozilla-central in the latest NSPR update:
> > https://hg.mozilla.org/mozilla-central/rev/7e25caedd620
> 
> The change you linked to isn't the same as the patch posted here...and that
> patch doesn't appear in current m-c.

You're right.  Sorry, the confusion was on my part.  Ted, was this excluded from the NSPR tag you created?  If yes, I think we need to create another one...

Comment 12

[(not currently active) Ted Mielczarek]

I tagged that beta during the week of September 12th, and I landed this the 22nd, so yeah, this wouldn't have made the tag.