Closed Bug 673799 Opened 13 years ago Closed 13 years ago

Improve auto-discovering of Android toolchain directory

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mounir, Assigned: mounir)

Details

(Whiteboard: [needs NSPR update before fixing])

Attachments

(2 files, 3 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
      No description provided.
Attachment #548034 - Flags: review?(khuey)
Attachment #548034 - Flags: feedback?(blassey.bugs)
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Whiteboard: [needs review]
Comment on attachment 548034 [details] [diff] [review]
Patch v1

/me delegates.

blassey is more qualified than me to review Android build stuff anyways.
Attachment #548034 - Flags: review?(khuey)
Attachment #548034 - Flags: review?(blassey.bugs)
Attachment #548034 - Flags: feedback?(blassey.bugs)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #548034 - Attachment is obsolete: true
Attachment #548034 - Flags: review?(blassey.bugs)
Attachment #548055 - Flags: review?(blassey.bugs)
Blocks: 673818
Comment on attachment 548055 [details] [diff] [review]
Patch v2

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

r=blassey for the non-nspr bits. Ted will need to review the nspr changes (and he may want them as a separate patch)
Attachment #548055 - Flags: review?(ted.mielczarek)
Attachment #548055 - Flags: review?(blassey.bugs)
Attachment #548055 - Flags: review+
Ted, note that I've open a bug to backport the NSPR part to NSPR trunk.
We don't generally backport NSPR changes ... we land them upstream and pull a tag.
Attached patch NSPR patch (obsolete) — Splinter Review
Attachment #548221 - Flags: review?(ted.mielczarek)
Attachment #548055 - Flags: review?(ted.mielczarek)
Can you move the NSPR patch over to bug 673818? It makes it easier to track NSPR changes when they're in their own bug.
Comment on attachment 548055 [details] [diff] [review]
Patch v2

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

::: configure.in
@@ +300,5 @@
> +        AC_MSG_CHECKING([for android toolchain directory])
> +
> +        kernel_name=`uname -s | tr "[[:upper:]]" "[[:lower:]]"`
> +
> +        android_toolchain="$android_ndk"/build/prebuilt/$kernel_name-x86/arm-eabi-4.4.0

NDK 6 removed the arm-eabi-4.4.0 toolchain (and the toolchain directory changed), so effectively, without the --with-android-toolchain option, you can't build with NDK 6.

Also note that at least in theory, it should be possible to have a x86-64 toolchain (and considering I'm building a new NDK with a newer binutils and gcc, I might be doing that), so the hardcoded -x86 would get in the way.
Comment on attachment 548221 [details] [diff] [review]
NSPR patch

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

Note that this needs to land in NSPR CVS, not directly in mozilla-central.

::: nsprpub/configure.in
@@ +183,5 @@
> +
> +        if test -d "$android_toolchain" ; then
> +            AC_MSG_RESULT([$android_toolchain])
> +        else
> +            AC_MSG_ERROR([not found])

Should this error message instruct you to use --with-android-toolchain?
Attachment #548221 - Flags: review?(ted.mielczarek) → review+
Attached patch NSPR PatchSplinter Review
Ted, could you push this patch to the NSPR trunk?
Attachment #548221 - Attachment is obsolete: true
Attachment #549433 - Flags: checkin?
Attached patch m-c patchSplinter Review
This patch will have to be pushed whet the NSPR patch will be in m-c.
Attachment #548055 - Attachment is obsolete: true
Whiteboard: [needs review] → [needs landing]
No longer blocks: 673818
Landed the NSPR patch:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.309; previous revision: 1.308
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.311; previous revision: 1.310
done
Attachment #549433 - Flags: checkin? → checkin+
If either patch needs to/should land in m-c directly, please request checkin? or checkin-needed keyword again, and someone will push them. From the looks of it we are already done here.
We don't land NSPR patches directly in m-c.
Ted, is there a bug for the next NSPR snapshot landing in m-c?
No, there isn't. We should probably wait until after the next Aurora uplift, so that we don't have to worry about having unreleased NSPR changes in Aurora.
Attachment #549434 - Flags: checkin+
m-c patch has been pushed to inbound. This will not work until NSPR is updated in the tree.
Whiteboard: [needs landing] → [needs NSPR update before fixing][inbound]
http://hg.mozilla.org/mozilla-central/rev/06d657efa434

Have to wait for NSPR merge now.
Flags: in-testsuite-
Whiteboard: [needs NSPR update before fixing][inbound] → [needs NSPR update before fixing]
The NSPR update landed:
http://hg.mozilla.org/mozilla-central/rev/4ab18c9e38b1
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: