542146 - Add support for building JS on Android

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Comment 1

[Michael Wu [:mwu]]

Attachment: Add support for building JS on Android, v1

This patch rolls up all the js specific changes in http://hg.mozilla.org/users/vladimir_mozilla.com/mozilla-droid . Not sure who to request review from for the non-build parts..

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'd suggest jorendorff for the other bits.

Comment 3

[Michael Wu [:mwu]]

Vlad, is there a reason why an endianness override is used here? It seems like things won't work nicely anyway if we differ from NSPR.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It's not an override, there's just no way to know it at compile time, which is what this code wants.  It can't depend on NSPR because js only has an optional dependency on it, so having a configure override is just the easiest way to specify what it should be.  Better solutions accepted! :)

Comment 5

[Michael Wu [:mwu]]

Ah, I see. I thought NSPR was required. This seems fine to me.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 423474 [details] [diff] [review]
Add support for building JS on Android, v1

Just a few nits:

>+#ifdef ANDROID
>+    rt->thousandsSeparator = JS_strdup(cx, "'");

Let's use #ifdef HAVE_LOCALE_H or HAVE_LOCALECONV here with appropriate line in configure.in.

And please file a follow-up bug to make Number.prototype.toLocaleString do the right thing on Android.

Comment 7

[Michael Wu [:mwu]]

Attachment: Add support for building JS on Android, v2

Use HAVE_LOCALECONV.

Comment 8

[Michael Wu [:mwu]]

Comment on attachment 424643 [details] [diff] [review]
Add support for building JS on Android, v2

Some new stuff was added in the android tree. Will update this patch.

Comment 9

[Michael Wu [:mwu]]

Attachment: Add support for building JS on Android, v3

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 425374 [details] [diff] [review]
Add support for building JS on Android, v3

This is fine, but please file a follow-up bug to make Number.prototype.toLocaleString do the
right thing on Android.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 425374 [details] [diff] [review]
Add support for building JS on Android, v3

>diff --git a/js/src/config/autoconf.mk.in b/js/src/config/autoconf.mk.in
>--- a/js/src/config/autoconf.mk.in
>+++ b/js/src/config/autoconf.mk.in
>@@ -339,8 +339,10 @@ UNIVERSAL_BINARY= @UNIVERSAL_BINARY@
> HAVE_DTRACE= @HAVE_DTRACE@
> 
> VISIBILITY_FLAGS = @VISIBILITY_FLAGS@
> WRAP_SYSTEM_INCLUDES = @WRAP_SYSTEM_INCLUDES@
> 
> ENABLE_JIT = @ENABLE_JIT@
> NANOJIT_ARCH = @NANOJIT_ARCH@
> HAVE_ARM_SIMD= @HAVE_ARM_SIMD@
>+
>+ANDROID = @ANDROID@

Do you really need an $(ANDROID) variable? Can't you just use ifeq($(OS_ARCH),Android)? (Assuming you set OS_ARCH appropriately in configure.)

>diff --git a/js/src/configure.in b/js/src/configure.in
>--- a/js/src/configure.in
>+++ b/js/src/configure.in
>@@ -202,16 +202,111 @@ fi
> COMPILE_ENVIRONMENT=1
> MOZ_ARG_ENABLE_BOOL(compile-environment,
> [  --disable-compile-environment
>                            Disable compiler/library checks.],
>     COMPILE_ENVIRONMENT=1,
>     COMPILE_ENVIRONMENT= )
> 
> dnl ========================================================
>+dnl = Android uses a very custom (hacky) toolchain; we need to do this
>+dnl = here, so that the compiler checks can succeed
>+dnl ========================================================
>+
>+AC_ARG_WITH(android-ndk,
>+    [  --with-android-ndk=DIR
>+           location where the Android NDK can be found],
>+    android_ndk=$withval)
>+
>+AC_ARG_WITH(android-toolchain,
>+    [  --with-android-toolchain=DIR
>+           location of the android toolchain, default NDK/build/prebuilt/HOST/arm-eabi-4.2.1],
>+    android_toolchain=$withval)
>+
>+AC_ARG_WITH(android-platform,
>+    [  --with-android-platform=DIR
>+           location of platform dir, default NDK/build/platforms/android-4/arch-arm],
>+    android_platform=$withval)

If android-toolchain and android-platform have sane defaults, do they need to be options? Is anyone going to want to fiddle them? The less configure options we have, the better.

>+
>+if test "$target" = "arm-android-eabi" ; then

I'm curious how you decided to use this configure triple. The convention is:
$cpu-$vendor-$os (where $os can be $system or $kernel-$system, to account for linux-gnu) per http://www.gnu.org/software/hello/manual/autoconf/Specifying-Target-Triplets.html
I don't see how eabi fits in there. Is this something the Android project invented?

>+    if test -z "$android_ndk" ; then
>+       AC_MSG_ERROR([Android target, but missing --with-android-ndk option!])

I prefer that configure error messages tell you exactly what to do, so I'd like this phrased more like: "You must specify --with-android-ndk=/path/to/ndk when targeting Android."

>+    fi
>+
>+    if test -z "$android_toolchain" ; then
>+       dnl XXX don't hardcode linux-x86 as the host; we could easily support MacOS X here

If you're going to leave yourself TODOs, either file bugs on them and put the bug number in the comment, or don't leave them at all.

>+    dnl set up the various flags, but only if they're not specified earlier
>+    if test -z "$CPPFLAGS" ; then
>+        CPPFLAGS="-I$android_platform/usr/include -DANDROID"
>+    fi
>+
>+    if test -z "$CFLAGS" ; then
>+        CFLAGS="-mandroid"
>+        CFLAGS="$CFLAGS -I$android_platform/usr/include"
>+        CFLAGS="$CFLAGS -msoft-float -fno-short-enums -fno-exceptions"
>+        CFLAGS="$CFLAGS -march=armv5te -mthumb-interwork"
>+        dnl XXX don't hardcode this, especially -mthumb
>+        dnl CFLAGS="$CFLAGS -mthumb"
>+    fi

I don't think you really want to do this quite like this. It's common for people to do things like CFLAGS="-g" ./configure, which would then presumably fail to compile. Also, same note about the TODO.

>+    dnl For some reason this configure uses CFLAGS as HOST_CFLAGS.. what gives?

I think it assumes that CFLAGS won't get munged with arch-specific options until later on in the file.

>+    if test -z "$HOST_CPPFLAGS" ; then
>+        HOST_CPPFLAGS="-g"
>+    fi
>+    if test -z "$HOST_CFLAGS" ; then
>+        HOST_CFLAGS="-g"
>+    fi
>+    if test -z "$HOST_CXXFLAGS" ; then
>+        HOST_CXXFLAGS="-g"
>+    fi
>+    if test -z "$HOST_LDFLAGS" ; then
>+        HOST_LDFLAGS="-g"
>+    fi

Why are you using -g as the default? If you don't want them set, then just set them to empty.

>+
>+    ANDROID=1
>+    AC_SUBST(ANDROID)

As I said before, I'd rather just have OS_ARCH=Android rather than inventing a new make variable.

> arm)
>-    if test "$OS_ARCH" = "WINCE"; then
>-        CPU_ARCH="$OS_TEST"
>-    fi
>+    CPU_ARCH="$OS_TEST"
>     ;;

You can't do this. We specifically only set CPU_ARCH on platforms where we have a known ABI. If Android has a known ABI (and you can prove that the compiler will use it), then you can special-case it here like we did for arm-wince.

> case "$target_os" in
>-linux*)
>+# eabi is a bit of a hack for arm-android-eabi
>+linux*|eabi)

This is sort of sad. I'd rather see you change this to $target and tweak all the case statements so you could put *android-* here.
> dnl ========================================================
>+dnl Add a way to specify target endianness explicitly for jscpucfg
>+dnl ========================================================
>+
>+AC_ARG_WITH(endian,
>+    [  --with-endian=little|big  Preset endianness for jscpucfg ],

This is awful, and I don't want you to add this option. I know jscpucfg sucks, but I'd rather you do something better here, even if it's hardcoding the right values for Android. bug 467453 would be about doing this "the right way".

Comment 12

[Michael Wu [:mwu]]

(FWIW I did not write most of the things in this patch but I will attempt to address all review issues as well as I can)

(In reply to comment #11)
> (From update of attachment 425374 [details] [diff] [review])
> >diff --git a/js/src/config/autoconf.mk.in b/js/src/config/autoconf.mk.in
> >--- a/js/src/config/autoconf.mk.in
> >+++ b/js/src/config/autoconf.mk.in
> >@@ -339,8 +339,10 @@ UNIVERSAL_BINARY= @UNIVERSAL_BINARY@
> > HAVE_DTRACE= @HAVE_DTRACE@
> > 
> > VISIBILITY_FLAGS = @VISIBILITY_FLAGS@
> > WRAP_SYSTEM_INCLUDES = @WRAP_SYSTEM_INCLUDES@
> > 
> > ENABLE_JIT = @ENABLE_JIT@
> > NANOJIT_ARCH = @NANOJIT_ARCH@
> > HAVE_ARM_SIMD= @HAVE_ARM_SIMD@
> >+
> >+ANDROID = @ANDROID@
> 
> Do you really need an $(ANDROID) variable? Can't you just use
> ifeq($(OS_ARCH),Android)? (Assuming you set OS_ARCH appropriately in
> configure.)
> 
Should be possible. However, WINCE already has its own variable. It seems to make certain things easier like ifeq ($(WINCE)$(ANDROID),) in js/src/Makefile.in. I am fine with either though I'd be more inclined to remove ANDROID if we can also remove WINCE.

> >diff --git a/js/src/configure.in b/js/src/configure.in
> >--- a/js/src/configure.in
> >+++ b/js/src/configure.in
> >@@ -202,16 +202,111 @@ fi
> > COMPILE_ENVIRONMENT=1
> > MOZ_ARG_ENABLE_BOOL(compile-environment,
> > [  --disable-compile-environment
> >                            Disable compiler/library checks.],
> >     COMPILE_ENVIRONMENT=1,
> >     COMPILE_ENVIRONMENT= )
> > 
> > dnl ========================================================
> >+dnl = Android uses a very custom (hacky) toolchain; we need to do this
> >+dnl = here, so that the compiler checks can succeed
> >+dnl ========================================================
> >+
> >+AC_ARG_WITH(android-ndk,
> >+    [  --with-android-ndk=DIR
> >+           location where the Android NDK can be found],
> >+    android_ndk=$withval)
> >+
> >+AC_ARG_WITH(android-toolchain,
> >+    [  --with-android-toolchain=DIR
> >+           location of the android toolchain, default NDK/build/prebuilt/HOST/arm-eabi-4.2.1],
> >+    android_toolchain=$withval)
> >+
> >+AC_ARG_WITH(android-platform,
> >+    [  --with-android-platform=DIR
> >+           location of platform dir, default NDK/build/platforms/android-4/arch-arm],
> >+    android_platform=$withval)
> 
> If android-toolchain and android-platform have sane defaults, do they need to
> be options? Is anyone going to want to fiddle them? The less configure options
> we have, the better.
> 
Agreed. I think vlad might rely on this to use his own toolchain instead of the NDK.. will check.

> >+
> >+if test "$target" = "arm-android-eabi" ; then
> 
> I'm curious how you decided to use this configure triple. The convention is:
> $cpu-$vendor-$os (where $os can be $system or $kernel-$system, to account for
> linux-gnu) per
> http://www.gnu.org/software/hello/manual/autoconf/Specifying-Target-Triplets.html
> I don't see how eabi fits in there. Is this something the Android project
> invented?
> 
The NDK prefixes arm-eabi- to everything. I don't think they invented it - I remember seeing eabi elsewhere.

> >+    if test -z "$android_ndk" ; then
> >+       AC_MSG_ERROR([Android target, but missing --with-android-ndk option!])
> 
> I prefer that configure error messages tell you exactly what to do, so I'd like
> this phrased more like: "You must specify --with-android-ndk=/path/to/ndk when
> targeting Android."
> 
Ok, will do.

> >+    fi
> >+
> >+    if test -z "$android_toolchain" ; then
> >+       dnl XXX don't hardcode linux-x86 as the host; we could easily support MacOS X here
> 
> If you're going to leave yourself TODOs, either file bugs on them and put the
> bug number in the comment, or don't leave them at all.
> 
Sorry, this is actually fixed already. I don't know why it didn't make it into the rollup.

> >+    dnl set up the various flags, but only if they're not specified earlier
> >+    if test -z "$CPPFLAGS" ; then
> >+        CPPFLAGS="-I$android_platform/usr/include -DANDROID"
> >+    fi
> >+
> >+    if test -z "$CFLAGS" ; then
> >+        CFLAGS="-mandroid"
> >+        CFLAGS="$CFLAGS -I$android_platform/usr/include"
> >+        CFLAGS="$CFLAGS -msoft-float -fno-short-enums -fno-exceptions"
> >+        CFLAGS="$CFLAGS -march=armv5te -mthumb-interwork"
> >+        dnl XXX don't hardcode this, especially -mthumb
> >+        dnl CFLAGS="$CFLAGS -mthumb"
> >+    fi
> 
> I don't think you really want to do this quite like this. It's common for
> people to do things like CFLAGS="-g" ./configure, which would then presumably
> fail to compile. Also, same note about the TODO.
> 
Ok.

I don't know what the TODO means. Will need to ask vlad.

> >+    dnl For some reason this configure uses CFLAGS as HOST_CFLAGS.. what gives?
> 
> I think it assumes that CFLAGS won't get munged with arch-specific options
> until later on in the file.
> 
I will remove this comment. The cross compile stuff mostly makes sense to me.

> >+    if test -z "$HOST_CPPFLAGS" ; then
> >+        HOST_CPPFLAGS="-g"
> >+    fi
> >+    if test -z "$HOST_CFLAGS" ; then
> >+        HOST_CFLAGS="-g"
> >+    fi
> >+    if test -z "$HOST_CXXFLAGS" ; then
> >+        HOST_CXXFLAGS="-g"
> >+    fi
> >+    if test -z "$HOST_LDFLAGS" ; then
> >+        HOST_LDFLAGS="-g"
> >+    fi
> 
> Why are you using -g as the default? If you don't want them set, then just set
> them to empty.
> 
Don't know. Will remove.

> > arm)
> >-    if test "$OS_ARCH" = "WINCE"; then
> >-        CPU_ARCH="$OS_TEST"
> >-    fi
> >+    CPU_ARCH="$OS_TEST"
> >     ;;
> 
> You can't do this. We specifically only set CPU_ARCH on platforms where we have
> a known ABI. If Android has a known ABI (and you can prove that the compiler
> will use it), then you can special-case it here like we did for arm-wince.
> 
Yup, we have eabi. Will add a special case.

> > case "$target_os" in
> >-linux*)
> >+# eabi is a bit of a hack for arm-android-eabi
> >+linux*|eabi)
> 
> This is sort of sad. I'd rather see you change this to $target and tweak all
> the case statements so you could put *android-* here.
Ok.

> > dnl ========================================================
> >+dnl Add a way to specify target endianness explicitly for jscpucfg
> >+dnl ========================================================
> >+
> >+AC_ARG_WITH(endian,
> >+    [  --with-endian=little|big  Preset endianness for jscpucfg ],
> 
> This is awful, and I don't want you to add this option. I know jscpucfg sucks,
> but I'd rather you do something better here, even if it's hardcoding the right
> values for Android. bug 467453 would be about doing this "the right way".
I will try to find a better way that hopefully doesn't depend on NSPR.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #12)
> (FWIW I did not write most of the things in this patch but I will attempt to
> address all review issues as well as I can)
> 
> (In reply to comment #11)
> > (From update of attachment 425374 [details] [diff] [review] [details])
> > >diff --git a/js/src/config/autoconf.mk.in b/js/src/config/autoconf.mk.in
> > >--- a/js/src/config/autoconf.mk.in
> > >+++ b/js/src/config/autoconf.mk.in
> > >@@ -339,8 +339,10 @@ UNIVERSAL_BINARY= @UNIVERSAL_BINARY@
> > > HAVE_DTRACE= @HAVE_DTRACE@
> > > 
> > > VISIBILITY_FLAGS = @VISIBILITY_FLAGS@
> > > WRAP_SYSTEM_INCLUDES = @WRAP_SYSTEM_INCLUDES@
> > > 
> > > ENABLE_JIT = @ENABLE_JIT@
> > > NANOJIT_ARCH = @NANOJIT_ARCH@
> > > HAVE_ARM_SIMD= @HAVE_ARM_SIMD@
> > >+
> > >+ANDROID = @ANDROID@
> > 
> > Do you really need an $(ANDROID) variable? Can't you just use
> > ifeq($(OS_ARCH),Android)? (Assuming you set OS_ARCH appropriately in
> > configure.)
> > 
> Should be possible. However, WINCE already has its own variable. It seems to
> make certain things easier like ifeq ($(WINCE)$(ANDROID),) in
> js/src/Makefile.in. I am fine with either though I'd be more inclined to remove
> ANDROID if we can also remove WINCE.

Yeah, ifeq($(OS_ARCH),Android) is pretty ugly, and we already have a random mix of these settings.  Though WINCE is somewhat of a special case, because WINCE looks like win32 in most ways as far as the makefiles are concerned.

> > If android-toolchain and android-platform have sane defaults, do they need to
> > be options? Is anyone going to want to fiddle them? The less configure options
> > we have, the better.
> > 
> Agreed. I think vlad might rely on this to use his own toolchain instead of the
> NDK.. will check.

Yes, they should be kept as options -- no reason not to, and there are valid reasons for overriding them (which would be difficult/impossible otherwise).

> > >+
> > >+if test "$target" = "arm-android-eabi" ; then
> > 
> > I'm curious how you decided to use this configure triple. The convention is:
> > $cpu-$vendor-$os (where $os can be $system or $kernel-$system, to account for
> > linux-gnu) per
> > http://www.gnu.org/software/hello/manual/autoconf/Specifying-Target-Triplets.html
> > I don't see how eabi fits in there. Is this something the Android project
> > invented?
> > 
> The NDK prefixes arm-eabi- to everything. I don't think they invented it - I
> remember seeing eabi elsewhere.

arm-eabi- is the generic prefix for the gcc compiler; but I didn't want to use arm-eabi- because it's -too- generic.  We could use arm-google-android-eabi, but I'm not sure if our configure stuff knows how to handle the fourth bit there all that well.

> > >+    dnl set up the various flags, but only if they're not specified earlier
> > >+    if test -z "$CPPFLAGS" ; then
> > >+        CPPFLAGS="-I$android_platform/usr/include -DANDROID"
> > >+    fi
> > >+
> > >+    if test -z "$CFLAGS" ; then
> > >+        CFLAGS="-mandroid"
> > >+        CFLAGS="$CFLAGS -I$android_platform/usr/include"
> > >+        CFLAGS="$CFLAGS -msoft-float -fno-short-enums -fno-exceptions"
> > >+        CFLAGS="$CFLAGS -march=armv5te -mthumb-interwork"
> > >+        dnl XXX don't hardcode this, especially -mthumb
> > >+        dnl CFLAGS="$CFLAGS -mthumb"
> > >+    fi
> > 
> > I don't think you really want to do this quite like this. It's common for
> > people to do things like CFLAGS="-g" ./configure, which would then presumably
> > fail to compile. Also, same note about the TODO.
> > 
> Ok.
> 
> I don't know what the TODO means. Will need to ask vlad.

The TODO issue there is somewhat related to ted's comment; the problem is that without a bunch of those flags, or some equivalent set, we can't build.  So someone specifying CFLAGS on the command line had better know what they're doing, in which case they need to specify whatever set they need for the other bits.  We can't just -add- flags, because, for example, it's valid to do a build with '-msoft-float=vfp -mfpu=neon -march=armv7', which would need to override.  But, the TODO is there because there's no good way then to just -add- flags.  That todo might just go away; it could possibly just be changed to a "at some point we might want to build thumb2 by default".

> > >+    dnl For some reason this configure uses CFLAGS as HOST_CFLAGS.. what gives?
> > 
> > I think it assumes that CFLAGS won't get munged with arch-specific options
> > until later on in the file.
> > 
> I will remove this comment. The cross compile stuff mostly makes sense to me.

That's a pretty silly assumption, but ok!  (e.g. makes no sense when the host compiler is MSVC but the target compiler is gcc)


> > >+    if test -z "$HOST_CPPFLAGS" ; then
> > >+        HOST_CPPFLAGS="-g"
> > >+    fi
> > >+    if test -z "$HOST_CFLAGS" ; then
> > >+        HOST_CFLAGS="-g"
> > >+    fi
> > >+    if test -z "$HOST_CXXFLAGS" ; then
> > >+        HOST_CXXFLAGS="-g"
> > >+    fi
> > >+    if test -z "$HOST_LDFLAGS" ; then
> > >+        HOST_LDFLAGS="-g"
> > >+    fi
> > 
> > Why are you using -g as the default? If you don't want them set, then just set
> > them to empty.
> > 
> Don't know. Will remove.

Well, it's just host apps, so -g seemed like the safest thing in case someone wanted to debug.  But sure, can remove, as long as we don't end up with $CFLAGS in there.


> > > arm)
> > >-    if test "$OS_ARCH" = "WINCE"; then
> > >-        CPU_ARCH="$OS_TEST"
> > >-    fi
> > >+    CPU_ARCH="$OS_TEST"
> > >     ;;
> > 
> > You can't do this. We specifically only set CPU_ARCH on platforms where we have
> > a known ABI. If Android has a known ABI (and you can prove that the compiler
> > will use it), then you can special-case it here like we did for arm-wince.
> > 
> Yup, we have eabi. Will add a special case.

Hmm.. this defines the xpcom ABI, right?  We may need to revisit this, but special-casing it for android for now is fine.

> > > case "$target_os" in
> > >-linux*)
> > >+# eabi is a bit of a hack for arm-android-eabi
> > >+linux*|eabi)
> > 
> > This is sort of sad. I'd rather see you change this to $target and tweak all
> > the case statements so you could put *android-* here.
> Ok.

Should be good if we switch to arm-google-android-eabi.

> > > dnl ========================================================
> > >+dnl Add a way to specify target endianness explicitly for jscpucfg
> > >+dnl ========================================================
> > >+
> > >+AC_ARG_WITH(endian,
> > >+    [  --with-endian=little|big  Preset endianness for jscpucfg ],
> > 
> > This is awful, and I don't want you to add this option. I know jscpucfg sucks,
> > but I'd rather you do something better here, even if it's hardcoding the right
> > values for Android. bug 467453 would be about doing this "the right way".
> I will try to find a better way that hopefully doesn't depend on NSPR.

I understand that bug 467453 would fix it the right way, but that bug's been open for more than a year; this jscpucfg thing affects pretty much anyone who's cross compiling, and there's really no good reason for it.  The configure option is super simple, and the default is to do exactly what happens currently... it fixes a very big pain point when cross compiling in a simple way that can be removed when someone -does- fix bug 467453.  I think we should just keep it here and avoid wasting a bunch of time trying to find a perfect solution.

Comment 14

[Michael Wu [:mwu]]

FWIW, it seems that arm-android-eabi is being used by google. It doesn't make a lot of sense to me but it should be "correct".

Comment 15

[Michael Wu [:mwu]]

Attachment: Add support for building JS on Android, v4

Changes:
1. s/AC_ARG_WITH/MOZ_ARG_WITH_STRING/
2. NDK error message updated
3. Update defaults for new NDK
4. Support OSX crosscompile in addition to Linux
5. Always set a sane set of flags for android and allow user to add additional options after our default set.
6. Use a space to prevent target flags from being used as host flags.
7. Special case CPU_ARCH handling.
8. Set AVMPLUS_UNIX/AVMPLUS_LINUX based on $target instead of $target_os

Comment 16

[(not currently active) Ted Mielczarek]

(In reply to comment #13)
> I understand that bug 467453 would fix it the right way, but that bug's been
> open for more than a year; this jscpucfg thing affects pretty much anyone who's
> cross compiling, and there's really no good reason for it.  The configure

I forgot to reply to this. I'm not saying we should block on the perfect solution, I'm just saying I'd prefer that you hardcode the right answer for Android in configure instead of adding a configure option. We have too many configure options, and users will just use them to shoot themselves in the foot. Please don't give them any more bullets.

Comment 17

[(not currently active) Ted Mielczarek]

(In reply to comment #13)
> > Should be possible. However, WINCE already has its own variable. It seems to
> > make certain things easier like ifeq ($(WINCE)$(ANDROID),) in
> > js/src/Makefile.in. I am fine with either though I'd be more inclined to remove
> > ANDROID if we can also remove WINCE.
> 
> Yeah, ifeq($(OS_ARCH),Android) is pretty ugly, and we already have a random mix
> of these settings.  Though WINCE is somewhat of a special case, because WINCE
> looks like win32 in most ways as far as the makefiles are concerned.

No, WINCE is the only one that deviates. We use OS_ARCH / OS_TARGET for everything else. Let's not add one more special case just because we have one bad actor. FWIW, I would take a patch to remove the WINCE Makefile var and replace it with normal OS_TARGET stuff.

GNU Make's conditionals suck, there's no way around that, but being consistent with the way we do things at least makes it possible to express the kinds of things we need to express, like ifneq(,$(filter-out WINNT WINCE,$(OS_ARCH)).

(And no, WINCE doesn't look much like Win32 in our makefiles, it has OS_ARCH=OS_TARGET=WINCE, and OS_TARGET=WINMO for winmo.)

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 431494 [details] [diff] [review]
Add support for building JS on Android, v4

>diff --git a/js/src/Makefile.in b/js/src/Makefile.in
>--- a/js/src/Makefile.in
>+++ b/js/src/Makefile.in
>@@ -60,7 +60,8 @@
> endif
> 
> # FIXME: bug 515383 covers getting these working on wince
>-ifndef WINCE
>+# bug 530688 covers Android
>+ifeq ($(WINCE)$(ANDROID),)

ifeq (,$(filter-out WINCE ANDROID,$(OS_ARCH))

>diff --git a/js/src/config/autoconf.mk.in b/js/src/config/autoconf.mk.in
>--- a/js/src/config/autoconf.mk.in
>+++ b/js/src/config/autoconf.mk.in
>@@ -345,3 +345,5 @@
> ENABLE_TRACEJIT = @ENABLE_TRACEJIT@
> NANOJIT_ARCH = @NANOJIT_ARCH@
> HAVE_ARM_SIMD= @HAVE_ARM_SIMD@
>+
>+ANDROID = @ANDROID@

Please get rid of this per my previous comment. I don't think it adds any value over using OS_ARCH / OS_TARGET, and it propagates inconsistency.

>diff --git a/js/src/configure.in b/js/src/configure.in
>--- a/js/src/configure.in
>+++ b/js/src/configure.in
>+    CPPFLAGS="-I$android_platform/usr/include -DANDROID $CPPFLAGS"

Please use AC_DEFINE(ANDROID) instead of -DANDROID directly. It's a bit easier for me to find.

>+    ANDROID=1
>+    AC_SUBST(ANDROID)

Remove this too.

>@@ -1204,9 +1270,11 @@
>     ;;
> 
> arm)
>-    if test "$OS_ARCH" = "WINCE"; then
>-        CPU_ARCH="$OS_TEST"
>-    fi
>+    case "$OS_ARCH" in
>+        WINCE | android)
>+            CPU_ARCH="$OS_TEST"
>+        ;;
>+    esac

vlad's got a real fix for this in bug 441767, so just copy what he's doing there. (I don't think his patch touches js/src/configure.)

>-case "$target_os" in
>-linux*)
>+case "$target" in
>+*linux*|*-android-eabi)
>     AC_DEFINE(AVMPLUS_UNIX)
>     AC_DEFINE(AVMPLUS_LINUX)
>     ;;

Pedantic, but I think you should make these like *-linux* instead of just *linux*.

> dnl ========================================================
>+dnl Add a way to specify target endianness explicitly for jscpucfg
>+dnl ========================================================
>+
>+AC_ARG_WITH(endian,
>+    [  --with-endian=little|big  Preset endianness for jscpucfg ],
>+    [ if test "$withval" = "little" ; then AC_DEFINE(FORCE_LITTLE_ENDIAN)
>+      elif test "$withval" = "big" ; then AC_DEFINE(FORCE_LITTLE_ENDIAN)
>+      else AC_MSG_ERROR([--with-endian must be either "little" or "big"!]) ; fi
>+])

Please just hardcode the right answer in the android block instead. Anything but this.

Pretty close, if you fix all those issues I think it should be r+ next time.

Comment 19

[Michael Wu [:mwu]]

Attachment: Add support for building JS on Android, v5

All review comments addressed.

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 436075 [details] [diff] [review]
Add support for building JS on Android, v5

>diff --git a/js/src/Makefile.in b/js/src/Makefile.in
>--- a/js/src/Makefile.in
>+++ b/js/src/Makefile.in
>@@ -60,7 +60,8 @@ DIRS += shell
> endif
> 
> # FIXME: bug 515383 covers getting these working on wince
>-ifndef WINCE
>+# bug 530688 covers Android
>+ifeq (,$(filter-out WINCE ANDROID,$(OS_ARCH)))

Oops, this should be ifneq, I guess I told you the wrong thing.

r=me with that change.

Comment 21

[Michael Wu [:mwu]]

(In reply to comment #20)
> (From update of attachment 436075 [details] [diff] [review])
> > # FIXME: bug 515383 covers getting these working on wince
> >-ifndef WINCE
> >+# bug 530688 covers Android
> >+ifeq (,$(filter-out WINCE ANDROID,$(OS_ARCH)))
> 
> Oops, this should be ifneq, I guess I told you the wrong thing.
> 
> r=me with that change.
Isn't that right? If we can't find wince or android, allow tests to be built.

Comment 22

[(not currently active) Ted Mielczarek]

http://www.gnu.org/software/make/manual/make.html#Text-Functions
"$(filter-out pattern...,text)
    Returns all whitespace-separated words in text that do not match any of the pattern words, removing the words that do match one or more. This is the exact opposite of the filter function. "

so $(filter-out WINCE ANDROID,ANDROID) == ""
if you don't have an empty string, build these tests.

Comment 23

[Michael Wu [:mwu]]

Attachment: Add support for building JS on Android, v6

Alright. I'll RTFM next time. Thanks Ted!

Comment 24

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/36dcd18c334d

Comment 25

[Boris Zbarsky [:bzbarsky]]

> Let's use #ifdef HAVE_LOCALE_H or HAVE_LOCALECONV here with appropriate line in
> configure.in.

Said appropriate line never happened, thus breaking toLocaleString across the board.  See bug 558394.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Er, nevermind.  AC_HAVE_FUNCS(localeconv) is there... but seems to not work at least on Windows.

Comment 27

[Jim Blandy :jimb]

Yeah, configure.in tries never to run compiler tests on Windows.  Everything has to be specified explicitly for Windows.  I've attached an untested patch to bug 558394.