Closed Bug 577635 Opened 10 years ago Closed 10 years ago

android_stub.h broken for android platform version 8

Categories

(Firefox Build System :: General, defect)

ARM
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: blassey, Assigned: blassey)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch patchSplinter Review
Dl_info and dladdr are both included in ndk version 8, so stubbing them out breaks the build
Attachment #456554 - Flags: review?(ted.mielczarek)
Attachment #456554 - Flags: review?(ted.mielczarek) → review?(me)
Comment on attachment 456554 [details] [diff] [review]
patch

>-MOZ_ARG_WITH_STRING(android-platform,
>-[  --with-android-platform=DIR
>-                          location of NDK platform dir, default NDK/build/platforms/android-5/arch-arm],
>-    android_platform=$withval)
>+
>+MOZ_ARG_WITH_STRING(android-version,
>+[  --with-android-version=VER
>+                          android platform version, default 5],
>+    android_version=$withval)
>+
>+MOZ_ARG_WITH_STRING(android-arch,
>+[  --with-android-arch=ARCH
>+                          android platform architecture, default arm],
>+    android_arch=$withval)

Supply the default android_version and android_arch here.  E.g.
MOZ_ARG_WITH_STRING(android-arch,
[  --with-android-arch=ARCH
                          android platform architecture, default arm],
    android_arch=$withval,
    android-arch=arm)

> MOZ_ARG_WITH_STRING(android-sdk,
> [  --with-android-sdk=DIR
>@@ -286,17 +292,25 @@ if test "$target" = "arm-android-eabi" ;
>         android_toolchain="$android_ndk"/build/prebuilt/`uname -s | tr "[[:upper:]]" "[[:lower:]]"`-x86/arm-eabi-4.4.0
>     fi
> 
>+    if test -z "$android_version" ; then
>+       android_version=5
>+    fi
>+
>+    if test -z "$android_arch" ; then
>+       android_arch=arm
>+    fi
>+

Again, move the defaults above.

>     if test -z "$android_platform" ; then
>-       android_platform="$android_ndk"/build/platforms/android-5/arch-arm
>+       android_platform="$android_ndk"/build/platforms/android-"$android_version"/arch-"$android_arch"
>     fi
> 
>     dnl set up compilers
>-    AS="$android_toolchain"/bin/arm-eabi-as
>-    CC="$android_toolchain"/bin/arm-eabi-gcc
>-    CXX="$android_toolchain"/bin/arm-eabi-g++
>-    CPP="$android_toolchain"/bin/arm-eabi-cpp
>-    LD="$android_toolchain"/bin/arm-eabi-ld
>-    android-toolchainAR="$android_toolchain"/bin/arm-eabi-ar
>+    if test -z "$AS"; then AS="$android_toolchain"/bin/arm-eabi-as; fi
>+    if test -z "$CC"; then CC="$android_toolchain"/bin/arm-eabi-gcc; fi
>+    if test -z "$CXX"; then CXX="$android_toolchain"/bin/arm-eabi-g++; fi
>+    if test -z "$CPP"; then CPP="$android_toolchain"/bin/arm-eabi-cpp; fi
>+    if test -z "$LD"; then LD="$android_toolchain"/bin/arm-eabi-ld; fi
>+    if test -z "$AR"; then AR="$android_toolchain"/bin/arm-eabi-ar; fi
>     RANLIB="$android_toolchain"/bin/arm-eabi-ranlib
>     STRIP="$android_toolchain"/bin/arm-eabi-strip

Is there a reason we're allowing everything but RANLIB and STRIP to be overridden?
 
>+#include "dlfcn.h"
>+#if ANDROID_VERSION >= 8
> /* because dladdr isn't supported in android 2.1 and older.
>  * however, it exists in the android repos so.. maybe someday. */
> typedef struct {
>@@ -47,6 +49,7 @@ typedef struct {
> } Dl_info;
> 
> #define dladdr(foo, bar) 0
>+#endif
> 
> /* sysinfo is defined >@@ -331,6 +345,7 @@ if test "$target" = "arm-android-eabi" ;
>     ANDROID_TOOLS="${android_tools}"
> 
>     AC_DEFINE(ANDROID)
>+    AC_DEFINE_UNQUOTED(ANDROID_VERSION, $android_version)
>     CROSS_COMPILE=1
>     MOZ_CHROME_FILE_FORMAT=omni
> fibut not implemented.>diff -r 0116958bbf99 -r fc9fb4ebfd7f security/manager/android_stub.h
>--- a/security/manager/android_stub.h	Fri Jul 09 01:29:04 2010 -0700
>+++ b/security/manager/android_stub.h	Fri Jul 09 01:29:06 2010 -0700
>@@ -40,6 +40,8 @@
> #ifndef ANDROID_STUB_H
> #define ANDROID_STUB_H
> 
>  * we may be able to implement it ourselves. */

Maybe I misunderstand what's going on, but don't you want to stub these out on ANDROID_VERSION < 8?

You also need to duplicate all of the configure changes in js/src.

r=me with those.
Attachment #456554 - Flags: review?(me) → review+
> > /* sysinfo is defined >@@ -331,6 +345,7 @@ if test "$target" = "arm-android-eabi" ;
> >     ANDROID_TOOLS="${android_tools}"
> > 
> >     AC_DEFINE(ANDROID)
> >+    AC_DEFINE_UNQUOTED(ANDROID_VERSION, $android_version)
> >     CROSS_COMPILE=1
> >     MOZ_CHROME_FILE_FORMAT=omni
> > fibut not implemented.>diff -r 0116958bbf99 -r fc9fb4ebfd7f security/manager/android_stub.h
> >--- a/security/manager/android_stub.h	Fri Jul 09 01:29:04 2010 -0700
> >+++ b/security/manager/android_stub.h	Fri Jul 09 01:29:06 2010 -0700
> >@@ -40,6 +40,8 @@
> > #ifndef ANDROID_STUB_H
> > #define ANDROID_STUB_H
> > 
> >  * we may be able to implement it ourselves. */

Ignore this whole quoted block at the end.  Somehow I managed to rearrange this text rather than delete it.
As discussed on IRC, I had two issues with this patch. --with-android-arch duplicates information normally passed with --target, and dropping the dladdr stub may not actually be the right thing to do since it doesn't seem to be implemented in bionic. Just mentioning it for the record.
I'll do a build without --allow-shlib-undefined to confirm that dladder from android-8 is working properly.

as mentioned on irc, we only really support arm right now anyway, so I can drop the --with-android-arch and just hard code arm if that sounds better to everyone
pushed http://hg.mozilla.org/mozilla-central/rev/0f1fddce41e1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
ANDROID_VERSION isn't being defined in nss.  This worked for me before only because of the typo.  Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch that breaks loader.c (obsolete) — Splinter Review
yea, I stuck DEFINES="-DANDROID_VERSION=$(ANDROID_VERSION)" \ into the rest of the DEFAULT_GMAKE_FLAGS that we add for android but then loader.c doesn't compile because SHLIB_PREFIX isn't defined. That kinda baffles me.

Also, in order for that to work needed to add ANDROID_VERSION to autoconf.mk.in so the substitution works. Here's what I've got, maybe you can tell me what's going wrong. (I know white space needs fixing)
Attachment #458105 - Flags: feedback?(me)
Attached patch patchSplinter Review
mwu's suggestion of using DSO_CFLAGS did the trick
Attachment #458105 - Attachment is obsolete: true
Attachment #458107 - Flags: review?(me)
Attachment #458105 - Flags: feedback?(me)
Comment on attachment 458107 [details] [diff] [review]
patch

That's an interesting hole to fall into.
Attachment #458107 - Flags: review?(me) → review+
Ah so this blew up because passing make values on the command line overrides anything the makefile tries to do.

http://www.gnu.org/software/make/manual/html_node/Overriding.html#Overriding
pushed http://hg.mozilla.org/mozilla-central/rev/42b67d331734 should be fixed for real now
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Assignee: nobody → blassey.bugs
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.