Closed Bug 1428182 Opened 6 years ago Closed 6 years ago

Fennec doesn't build with NDK r16

Categories

(Firefox Build System :: Android Studio and Gradle Integration, enhancement)

Unspecified
Android
enhancement
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(11 files)

59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
ted
: review+
Details
59 bytes, text/x-review-board-request
jseward
: review+
Details
59 bytes, text/x-review-board-request
esawin
: review+
Details
59 bytes, text/x-review-board-request
lsalzman
: review+
Details
59 bytes, text/x-review-board-request
lsalzman
: review+
Details
59 bytes, text/x-review-board-request
ted
: review+
Details
NDK r16 got rid of per-API-level headers (under /platforms/android-*) in favor of unified headers (under /sysroot), so we need to add support for unified headers.
Just making sure froydnj is aware of this.
Flags: needinfo?(nfroyd)
Fun!  Do we want to switch to toolchain jobs for Android along with doing this?
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #2)
> Fun!  Do we want to switch to toolchain jobs for Android along with doing
> this?

I'll leave that up to the sucker^H^H^H person doing the work.  If it was me, I would :)

How hard would it be to use clang-* toolchain tasks and avoid the shipped NDK entirely?
FWIW, I have a set of patches that make us compile again under stock NDK r16b.
(In reply to Nick Alexander :nalexander (less responsive until Jan 3, 2018) from comment #3)
> How hard would it be to use clang-* toolchain tasks and avoid the shipped
> NDK entirely?

There are two different levels of toolchain tasks for Android:

1) Move the NDK packaging that we do now into taskcluster, and make the Android jobs depend on that "toolchain".  This gets the NDK out of tooltool (which is good) and also means that we might have a better shot at, say, keeping the NDK from `mach bootstrap` and the NDK we use in automation consistent.  I don't *think* we can simply have `mach bootstrap` download our NDK from taskcluster due to legal issues around redistributing the NDK...?

2) Make Android use clang-* toolchain tasks, i.e. compile in automation with the clang that we build ourselves.  This probably (?) requires some variant of #1, because we need the NDK headers/libraries/etc. to point our builds at anyway.  We may need to do this to get static analysis builds (bug 1428158) working anyway.  I'm not super-happy about doing this, because it makes our automation builds differ substantially from a "normal" developer build...but perhaps if we went this route, we'd just have `mach bootstrap` download our custom clang toolchain and use *that* instead of the NDK clang?  That would at least make things more consistent...

#1 is probably a day or two of work.  #2 could be pretty easy, dropping in one clang for another, or it could be a rathole.  Thoughts?
I think fixing this is actually necessary to fix bug 1423238 properly.  jchen, do you have builds working, and are patches necessary?  I *think* we ought to be able to replace the -isystem directories we use, but perhaps there are other gotchas lurking?
Flags: needinfo?(nchen)
(In reply to Nathan Froyd [:froydnj] from comment #6)
> I think fixing this is actually necessary to fix bug 1423238 properly. 
> jchen, do you have builds working, and are patches necessary?  I *think* we
> ought to be able to replace the -isystem directories we use, but perhaps
> there are other gotchas lurking?

We actually can't use $ndk/sysroot as our sysroot for NDK r15c because of issues that are fixed in NDK r16. But I'm hoping the quota.h patch in this bug helps with that bug.
Flags: needinfo?(nchen)
Comment on attachment 8941518 [details]
Bug 1428182 - 8. Don't redefine keycodes when using unified headers;

https://reviewboard.mozilla.org/r/211780/#review217594
Attachment #8941518 - Flags: review?(esawin) → review+
Comment on attachment 8941515 [details]
Bug 1428182 - 5. Update libevent patch for Android builds;

https://reviewboard.mozilla.org/r/211774/#review217598
Attachment #8941515 - Flags: review?(nfroyd) → review+
Comment on attachment 8941511 [details]
Bug 1428182 - 1. Support unified headers for Android builds;

https://reviewboard.mozilla.org/r/211766/#review217602

This looks fine to me, but I don't understand the `__ANDROID_API__` bit entirely.  If that's complicated, get froydnj to review as well.  Otherwise, thanks for making this happen!

::: build/moz.configure/android-ndk.configure:171
(Diff revision 1)
> +
> +    for test_dir, sysroot_dir in search_dirs:
> +        if isdir(test_dir):
> +            return sysroot_dir
> +
> +    die("Android sysroot directory not found in %s." % str(zip(*search_dirs)[1]))

This `str(zip(*search_dirs)...` is a little fancy.  Can we just have `[d for d, _ in search_dirs]` or similar?
Attachment #8941511 - Flags: review?(nalexander) → review+
Comment on attachment 8941520 [details]
Bug 1428182 - 10. Add <stdlib.h> include in pixman patch;

https://reviewboard.mozilla.org/r/211784/#review217612
Attachment #8941520 - Flags: review?(lsalzman) → review+
Comment on attachment 8941519 [details]
Bug 1428182 - 9. Apply Skia upstream commit to support NDK r16;

https://reviewboard.mozilla.org/r/211782/#review217614
Attachment #8941519 - Flags: review?(lsalzman) → review+
Comment on attachment 8941521 [details]
Bug 1428182 - 6a. Apply Breakpad upstream commit for building with NDK r16;

https://reviewboard.mozilla.org/r/211786/#review217650

::: nsprpub/pr/src/md/unix/unix.c:2708
(Diff revision 1)
>      else errno = EFBIG;  /* we can't go there */
>      return rv;
>  }  /* _MD_Unix_lseek64 */
>  
>  static void* _MD_Unix_mmap64(
>      void *addr, PRSize len, PRIntn prot, PRIntn flags,

Note that NSPR is maintained as a separate project. Firefox uses a release snapshot/copy of NSPR. We don't patch the copy. The proper way to add fixes to NSPR is: Report a bug against the NSPR project, and submit a patch for the https://hg.mozilla.org/projects/nspr repository. Once reviewed and landed there, a new NSPR snapshot will be uplifted into m-i  by the NSPR maintainers.
Comment on attachment 8941521 [details]
Bug 1428182 - 6a. Apply Breakpad upstream commit for building with NDK r16;

https://reviewboard.mozilla.org/r/211786/#review217652

::: nsprpub/pr/src/md/unix/unix.c:2802
(Diff revision 1)
>      _md_iovector._open64 = open;
>  #else
>      _md_iovector._open64 = open64;
>  #endif
>      _md_iovector._mmap64 = mmap64;
> +#if (defined(ANDROID) && __ANDROID_API__ < 21)

The current code uses (f)stat64 uncondititionally, is my understanding correct? If that worked on Android before, why is it necessary to change old Android versions to use (f)stat? Are you making this for consistency?
Comment on attachment 8941521 [details]
Bug 1428182 - 6a. Apply Breakpad upstream commit for building with NDK r16;

https://reviewboard.mozilla.org/r/211786/#review217650

> Note that NSPR is maintained as a separate project. Firefox uses a release snapshot/copy of NSPR. We don't patch the copy. The proper way to add fixes to NSPR is: Report a bug against the NSPR project, and submit a patch for the https://hg.mozilla.org/projects/nspr repository. Once reviewed and landed there, a new NSPR snapshot will be uplifted into m-i  by the NSPR maintainers.

I can land this patch on NSPR when it is reviewed. Or should I open a separate bug for NSPR?
Comment on attachment 8941521 [details]
Bug 1428182 - 6a. Apply Breakpad upstream commit for building with NDK r16;

https://reviewboard.mozilla.org/r/211786/#review217652

> The current code uses (f)stat64 uncondititionally, is my understanding correct? If that worked on Android before, why is it necessary to change old Android versions to use (f)stat? Are you making this for consistency?

So the old NDK header essentially does this:

    #if __ANDROID_API__ >= 21
    int stat64(...);
    int fstat64(...);
    #else
    #define stat64  stat
    #define fstat64 fstat
    #endif

But the new NDK header doesn't include those extra lines for unsupported platforms:

    #if __ANDROID_API__ >= 21
    int stat64(...);
    int fstat64(...);
    #endif

So we need to manually map `(f)stat64` to `(f)stat` on unsupported platforms.
Thanks for the clarification! r=kaie for the NSPR change.

Is your target for this bug Firefox 59 (devel phase ending in a few days) or Firefox 60?
Flags: needinfo?(nchen)
These should be okay for either 59 or 60, but I will probably wait for 60. I assume once the NSPR patch is checked into NSPR, the next sync to m-c will happen in 60?
Flags: needinfo?(nchen)
We're rather late in the current dev cycle. If this was urgent, we could have justified including it into the NSPR release that targets Firefox 59. But if the fixes for this bug will wait for the 60 cycle anyway, it's easier to postpone the NSPR fix to 60, too. In that case the NSPR uplift to mozilla-central will be delayed until the merge period for FF60 is completely over, I guess that means Jan 22.
Comment on attachment 8941512 [details]
Bug 1428182 - 2. Check both quota.h and quotactl();

https://reviewboard.mozilla.org/r/211768/#review218086
Attachment #8941512 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8941517 [details]
Bug 1428182 - 7. Include <link.h> in LUL for Android;

https://reviewboard.mozilla.org/r/211778/#review218088

::: tools/profiler/lul/LulElfInt.h:57
(Diff revision 1)
>  
>  
>  // (derived from)
>  // elfutils.h: Utilities for dealing with ELF files.
>  //
> +# include <link.h>

Nit: pls remove the space between the # and the
'include', since this is no longer conditionalised (IIUC).
Attachment #8941517 - Flags: review?(jseward) → review+
Comment on attachment 8941513 [details]
Bug 1428182 - 3. Only include <linux/elf.h> for non-unified headers;

https://reviewboard.mozilla.org/r/211770/#review218090
Attachment #8941513 - Flags: review?(mh+mozilla) → review+
Depends on: 1432473
Depends on: 1432480
Comment on attachment 8941516 [details]
Bug 1428182 - 6b. Support unified headers in Breakpad code;

https://reviewboard.mozilla.org/r/211776/#review220614

::: toolkit/crashreporter/google-breakpad/src/common/android/include/link.h:46
(Diff revision 1)
>  
>  #ifdef __cplusplus
>  extern "C" {
>  #endif  // __cplusplus
>  
> -#if defined(ANDROID) && ANDROID_VERSION <= 20
> +#if defined(ANDROID) && __ANDROID_API__ < 21 && !defined(__ANDROID_API_L__)

Files under google-breakpad are from upstream (the stuff under breakpad-client is forked), so these changes need to go upstream or they'll get lost in a future sync from upstream.

It looks like similar changes have already been made upstream:
https://chromium.googlesource.com/breakpad/breakpad/+/afa9c52715db1e4bfaa4b01c9aec40cc249b689b
Diff:
https://chromium.googlesource.com/breakpad/breakpad/+/afa9c52715db1e4bfaa4b01c9aec40cc249b689b%5E%21/#F0

Does cherry-picking that change from upstream fix your issue? If so, we should just do that.
Attachment #8941516 - Flags: review?(ted) → review-
Comment on attachment 8941516 [details]
Bug 1428182 - 6b. Support unified headers in Breakpad code;

https://reviewboard.mozilla.org/r/211776/#review220614

> Files under google-breakpad are from upstream (the stuff under breakpad-client is forked), so these changes need to go upstream or they'll get lost in a future sync from upstream.
> 
> It looks like similar changes have already been made upstream:
> https://chromium.googlesource.com/breakpad/breakpad/+/afa9c52715db1e4bfaa4b01c9aec40cc249b689b
> Diff:
> https://chromium.googlesource.com/breakpad/breakpad/+/afa9c52715db1e4bfaa4b01c9aec40cc249b689b%5E%21/#F0
> 
> Does cherry-picking that change from upstream fix your issue? If so, we should just do that.

Updated to cherry-pick that commit.
Comment on attachment 8941521 [details]
Bug 1428182 - 6a. Apply Breakpad upstream commit for building with NDK r16;

https://reviewboard.mozilla.org/r/211786/#review222130

Thanks!
Attachment #8941521 - Flags: review?(ted) → review+
Comment on attachment 8941516 [details]
Bug 1428182 - 6b. Support unified headers in Breakpad code;

https://reviewboard.mozilla.org/r/211776/#review222126

::: toolkit/crashreporter/google-breakpad/src/common/linux/moz.build:43
(Diff revision 2)
>  ]
>  
>  if CONFIG['OS_TARGET'] == 'Android':
>      DEFINES['ANDROID_NDK_MAJOR_VERSION'] = CONFIG['ANDROID_NDK_MAJOR_VERSION']
>      DEFINES['ANDROID_NDK_MINOR_VERSION'] = CONFIG['ANDROID_NDK_MINOR_VERSION']
> -    LOCAL_INCLUDES += [
> +    COMPILE_FLAGS["OS_INCLUDES"] += [

nit: we use single quotes in moz.build files
Attachment #8941516 - Flags: review?(ted) → review+
Comment on attachment 8941520 [details]
Bug 1428182 - 10. Add <stdlib.h> include in pixman patch;

https://reviewboard.mozilla.org/r/211784/#review222174
Attachment #8941520 - Flags: review+
Comment on attachment 8941514 [details]
Bug 1428182 - 4. Fix a gfx warning;

https://reviewboard.mozilla.org/r/211772/#review222176
Attachment #8941514 - Flags: review?(nchen) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17c0e373d921
1. Support unified headers for Android builds; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/377eada51b3c
2. Check both quota.h and quotactl(); r=glandium
https://hg.mozilla.org/integration/autoland/rev/4655e1b1b237
3. Only include <linux/elf.h> for non-unified headers; r=glandium
https://hg.mozilla.org/integration/autoland/rev/55891ffb3768
4. Fix a gfx warning; r=jchen
https://hg.mozilla.org/integration/autoland/rev/1d1278b289b7
5. Update libevent patch for Android builds; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/fbbb0745b139
6a. Apply Breakpad upstream commit for building with NDK r16; r=ted
https://hg.mozilla.org/integration/autoland/rev/4dd7eaff3ab5
6b. Support unified headers in Breakpad code; r=ted
https://hg.mozilla.org/integration/autoland/rev/092662eab5eb
7. Include <link.h> in LUL for Android; r=jseward
https://hg.mozilla.org/integration/autoland/rev/c576e9d1f68f
8. Don't redefine keycodes when using unified headers; r=esawin
https://hg.mozilla.org/integration/autoland/rev/429433caa78c
9. Apply Skia upstream commit to support NDK r16; r=lsalzman
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d7a9a8c1e46e
1. Support unified headers for Android builds; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/5b50e77aee6f
2. Check both quota.h and quotactl(); r=glandium
https://hg.mozilla.org/integration/autoland/rev/827f7841bd63
3. Only include <linux/elf.h> for non-unified headers; r=glandium
https://hg.mozilla.org/integration/autoland/rev/d732dedb3844
4. Fix a gfx warning; r=jchen
https://hg.mozilla.org/integration/autoland/rev/18e9c4bdc3a9
5. Update libevent patch for Android builds; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/fb4a8fe6bb1a
6a. Apply Breakpad upstream commit for building with NDK r16; r=ted
https://hg.mozilla.org/integration/autoland/rev/6f260b258a97
6b. Support unified headers in Breakpad code; r=ted
https://hg.mozilla.org/integration/autoland/rev/7771ab0cabc5
7. Include <link.h> in LUL for Android; r=jseward
https://hg.mozilla.org/integration/autoland/rev/2eb97f7b99e0
8. Don't redefine keycodes when using unified headers; r=esawin
https://hg.mozilla.org/integration/autoland/rev/95b744f261bf
9. Apply Skia upstream commit to support NDK r16; r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/7e16f570704f
10. Add <stdlib.h> include in pixman patch; r=lsalzman
Flags: needinfo?(nchen)
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 60 → mozilla60
You need to log in before you can comment on or make changes to this bug.