add NDK r13 support

RESOLVED FIXED in Firefox 55

Status

Firefox Build System
General
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: froydnj, Assigned: m_kato)

Tracking

Trunk
mozilla55
All
Android

Firefox Tracking Flags

(firefox50 affected, firefox55 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

I've been filing bugs on the Android NDK over at Github, and a person from the NDK team, Dan Albert, has dove in and wrote some patches to make Fennec compile with NDK r13 (beta coming later this summer, release in Q3)!

Comment 1

2 years ago
Created attachment 8766923 [details]
Bug 1283611 - Add support for NDK r13's libc++ paths.

The order of these includes is important with the updated libc++ in
NDK r13. libc++ now includes a math.h that needs to be able to
include_next the same and find definitions for all the math
functions. libandroid_support also needs to include_next. The previous
include order resulted in the following resolution when math.h was
included:

1. libandroid_support's math.h
2. libc++'s math.h
3. libc's math.h.

We enter 2 at the top of 1, so when we continue processing 2 after
including 1, we haven't picked up all the definitions we need from 1
yet, and the build fails because we're missing a huge portion of the
math definitions.

Moving the -I later gets us the resolution order we need:

1. libc++'s math.h
2. libandroid_support's math.h
3. libc's math.h

Review commit: https://reviewboard.mozilla.org/r/61642/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61642/
Attachment #8766923 - Flags: review?(nfroyd)
Attachment #8766923 - Flags: review?(nfroyd) → review+
Comment on attachment 8766923 [details]
Bug 1283611 - Add support for NDK r13's libc++ paths.

https://reviewboard.mozilla.org/r/61642/#review58578

Thanks for the explanation.

Comment 3

2 years ago
https://reviewboard.mozilla.org/r/61642/#review58578

Hmm, it's still not letting me trigger a try build. Any idea why?
(In reply to Dan Albert from comment #3)
> https://reviewboard.mozilla.org/r/61642/#review58578
> 
> Hmm, it's still not letting me trigger a try build. Any idea why?

Sorry about the PTO-induced latency here.  I asked around, and folks said that following the steps here:

https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html#manually-associating-your-ldap-account-with-mozreview

should enable you to push try builds from inside mozreview.  For generating a Bugzilla API key, you'll want to select "Preferences" from the dropdown menu on the upper right of any Bugzilla page, go to the "API Keys" sub-menu, and then generate a new API key according to the bits there.

You'll also want to set up ~/.ssh/config for reviewboard-hg.mozilla.org.

Let me know if you have any more questions.
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
Created attachment 8851302 [details] [diff] [review]
Change order include path to support NDK r13+

When using NDK r13+ with part 1, the following build error still occurs when using cmath.

 0:21.01 /mozilla/android-ndk-r14b/sources/cxx-stl/llvm-libc++/include/math.h:661:105: error: 'acosl' was not declared in this scope
 0:21.01  inline _LIBCPP_INLINE_VISIBILITY long double acos(long double __lcpp_x) _NOEXCEPT {return acosl(__lcpp_x);}

To fix this, we need change the order of include path.
Assignee: nobody → m_kato
Attachment #8851302 - Flags: review?(nfroyd)
Comment on attachment 8851302 [details] [diff] [review]
Change order include path to support NDK r13+

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

r=me assuming this still works with the NDK we use.
Attachment #8851302 - Flags: review?(nfroyd) → review+

Comment 9

a year ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7de73b4bbf32
Add support for NDK r13's libc++ paths. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/17052b6a31bf
Change order of include path to support NDK r13+ r=froydnj

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7de73b4bbf32
https://hg.mozilla.org/mozilla-central/rev/17052b6a31bf
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

2 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.