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)
Firefox Build System
Android Studio and Gradle Integration
Unspecified
Android
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.
Comment 2•6 years ago
|
||
Fun! Do we want to switch to toolchain jobs for Android along with doing this?
Flags: needinfo?(nfroyd)
Comment 3•6 years ago
|
||
(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?
Assignee | ||
Comment 4•6 years ago
|
||
FWIW, I have a set of patches that make us compile again under stock NDK r16b.
Comment 5•6 years ago
|
||
(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?
Comment 6•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•6 years ago
|
||
(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 19•6 years ago
|
||
mozreview-review |
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 20•6 years ago
|
||
mozreview-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 21•6 years ago
|
||
mozreview-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 22•6 years ago
|
||
mozreview-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 23•6 years ago
|
||
mozreview-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 24•6 years ago
|
||
mozreview-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 25•6 years ago
|
||
mozreview-review |
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?
Assignee | ||
Comment 26•6 years ago
|
||
mozreview-review-reply |
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?
Assignee | ||
Comment 27•6 years ago
|
||
mozreview-review-reply |
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.
Comment 28•6 years ago
|
||
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)
Assignee | ||
Comment 29•6 years ago
|
||
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)
Comment 30•6 years ago
|
||
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 31•6 years ago
|
||
mozreview-review |
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 32•6 years ago
|
||
mozreview-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 33•6 years ago
|
||
mozreview-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+
Comment 34•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 46•6 years ago
|
||
mozreview-review-reply |
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 47•6 years ago
|
||
mozreview-review |
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 48•6 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 60•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 61•6 years ago
|
||
mozreview-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+
Comment 62•6 years ago
|
||
Backed out for build bustages on pixman-inlines.h:29:10. Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=159201768&repo=autoland&lineNumber=14319 Treeherder push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=84c767de620288047f879277bdcae2dfc805b7b6&selectedJob=159201758 Backout link: https://hg.mozilla.org/integration/autoland/rev/8d50a77f84379016f33e0754fd356e0e1cc26dda
Flags: needinfo?(nchen)
Comment 63•6 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 75•6 years ago
|
||
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
Comment 76•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d7a9a8c1e46e https://hg.mozilla.org/mozilla-central/rev/5b50e77aee6f https://hg.mozilla.org/mozilla-central/rev/827f7841bd63 https://hg.mozilla.org/mozilla-central/rev/d732dedb3844 https://hg.mozilla.org/mozilla-central/rev/18e9c4bdc3a9 https://hg.mozilla.org/mozilla-central/rev/fb4a8fe6bb1a https://hg.mozilla.org/mozilla-central/rev/6f260b258a97 https://hg.mozilla.org/mozilla-central/rev/7771ab0cabc5 https://hg.mozilla.org/mozilla-central/rev/2eb97f7b99e0 https://hg.mozilla.org/mozilla-central/rev/95b744f261bf https://hg.mozilla.org/mozilla-central/rev/7e16f570704f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(nchen)
Updated•5 years ago
|
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.
Description
•