859830 - B2G Emulator-x86: build error in android-signal-defs.h, need newer NDK or platform version

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

In android-signal-defs.h, we have:

   9 // All NDK platform versions have asm/sigcontext.h for ARM
  10 // Only NDK >= 6, platform >= 9 have asm/sigcontext.h for x86
  11 // Only NDK >= 8, platform >= 9 have asm/sigcontext.h for MIPS
  12 #if defined(__arm__) || defined(__thumb__) || ANDROID_VERSION >= 9
  13 #include <asm/sigcontext.h>

Gonk has api level 10 defined in bionic/libc/include/android/api-level.h. However we don't include it but rely on an autoconf variable ANDROID_VERSION, which is not assigned when we're configured with |--with-gonk=DIR|.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

After digging more automake configs, that's related to multiple components, not Gecko Profiler only. I suggest we always include api-level.h and deprecate ANDROID_VERSION because api-level.h is always available in both NDK and PDK.

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch

fix typo. https://tbpl.mozilla.org/?tree=Try&rev=c69a9645ee3a

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 735281 [details] [diff] [review]
patch

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

For nsprpub/configure.in, you'll want a separate patch, and do the corresponding change in nsprpub/configure.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 735281 [details] [diff] [review]
patch

Ted would have a more informed opinion on this than I

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 735281 [details] [diff] [review]
patch

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

The NSPR changes look fine. If you can give me a patch against https://hg.mozilla.org/projects/nspr/ I can land it there for you.

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: nspr patch

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Attachment: m-c patch

Should I keep nspr changes in this patch as well? Sorry I don't really know nspr landing rules.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 735740 [details] [diff] [review]
m-c patch

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

::: configure.in
@@ -9333,5 @@
>      _SUBDIR_CONFIG_ARGS="`echo $_SUBDIR_CONFIG_ARGS | sed -e 's/--enable-wrap-malloc *//'`"
>  fi
>  
>  if test -z "$MOZ_NATIVE_NSPR"; then
> -    ac_configure_args="$_SUBDIR_CONFIG_ARGS --with-dist-prefix=$MOZ_BUILD_ROOT/dist --with-mozilla --with-android-version=$ANDROID_VERSION"

While you're here, could you move this --with-android-version in android.m4, using NSPR_CONFIGURE_ARGS, like in https://bug860184.bugzilla.mozilla.org/attachment.cgi?id=735641 ?

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 735737 [details] [diff] [review]
nspr patch

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

I'll leave this one to ted.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 735737 [details] [diff] [review]
nspr patch

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

Typically we only land NSPR changes in the NSPR repo, and then import snapshots of that into m-c. I'll land this in NSPR for you shortly.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Attachment: nspr patch

Update r=ted only.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Attachment: m-c patch : v2

Address review comment.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Wait for bug 860184.

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 736145 [details] [diff] [review]
nspr patch

Pushed to NSPR:
https://hg.mozilla.org/projects/nspr/rev/59246254cba9

You don't really need this to land as a prerequisite to your other patches, right? Nothing in NSPR actually uses this define.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15)
> You don't really need this to land as a prerequisite to your other patches,
> right? Nothing in NSPR actually uses this define.

Yes. I had tested 1) with m-c only, 2) with nspr only, 3) with both patches, and all builds successfully in both b2g and fennec. I'm actually waiting for m-c part because it will need a rebase.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: m-c patch : v3

Rebased.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee95ad865eab

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee95ad865eab