Status

enhancement
P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

other
x86
Android
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

8 years ago
buildconfig of NSPR should support Android-x86.
Assignee

Comment 1

8 years ago
Posted patch fix v1Splinter Review
Assignee

Updated

8 years ago
Attachment #555335 - Flags: review?(wtc)

Comment 2

8 years ago
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?
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 4.9

Comment 3

8 years ago
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?
I can review these patches.
Assignee

Comment 5

8 years ago
(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

8 years ago
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.
Attachment #555335 - Flags: review?(wtc)
Attachment #555335 - Flags: review?(ted.mielczarek)
Attachment #555335 - Flags: review+
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.
Attachment #555335 - Flags: review?(ted.mielczarek) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED

Comment 9

8 years ago
Pushed to mozilla-central in the latest NSPR update: https://hg.mozilla.org/mozilla-central/rev/7e25caedd620
(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

8 years ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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...
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.
You need to log in before you can comment on or make changes to this bug.