Closed Bug 1360291 Opened 3 years ago Closed 3 years ago

Build system fixes for AArch64 support

Categories

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

50 Branch
Other
Android
enhancement
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

No description provided.
64-bit is only supported in android-21 and above in the NDK.
Attachment #8862669 - Flags: review?(nalexander)
Detect the NDK major/minor version numbers, and feed that to Breakpad.
For AArch64, some Breakpad headers try to workaround NDK oddities by
checking the ANDROID_NDK_MAJOR_VERSION and ANDROID_NDK_MINOR_VERSION
macros.
Attachment #8862670 - Flags: review?(nalexander)
Add AArch64 support to the script that produces android version codes.
Use the same scheme for AArch64 as x86, since the two architectures
don't overlap, and AArch64 should override ARM just like x86 should
override ARM.
Attachment #8862671 - Flags: review?(nalexander)
Comment on attachment 8862671 [details] [diff] [review]
Produce android version codes for AArch64 Fennec (v1)

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

Looks good.  Can you say AArch64/ARM64 in the commit message summary and body?  I want to make it easy to grep for.

::: python/mozbuild/mozbuild/android_version_code.py
@@ +50,5 @@
>      The bits labelled 'x', 'p', and 'g' are feature flags.
>  
> +    The bit labelled 'x' is 1 if the build is for an x86 or ARM64 architecture,
> +    and 0 otherwise, which means the build is for a (32-bit) ARM architecture.
> +    (Fennec no longer supports ARMv6, so ARM is equivalent to ARMv7.)

nit: add "ARM64 is also known as AArch64; it is logically ARMv8."  (Or whatever wording you prefer.)

@@ +145,5 @@
>                                       add_help=False)
>      parser.add_argument('--verbose', action='store_true',
>                          default=False,
>                          help='Be verbose')
>      parser.add_argument('--with-android-cpu-arch', dest='cpu_arch',

nit: since we hit our line limit, let's go "long vertical", one per line.

::: python/mozbuild/mozbuild/test/test_android_version_code.py
@@ +22,5 @@
>          self.assertEqual(android_version_code_v0(buildid, cpu_arch='x86', min_sdk=9, max_sdk=None), x86_api9)
>  
>      def test_android_version_code_v1(self):
>          buildid = '20150825141628'
>          arm_api15 = 0b01111000001000000001001001110001

nit: align the 0b so these are easier to compare.
Attachment #8862671 - Flags: review?(nalexander) → review+
Comment on attachment 8862669 [details] [diff] [review]
Set minimum NDK platform version to 21 for 64-bit builds (v1)

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

I think this is OK, but I'm a bit concerned that we're specially handling CPU architectures here that I doubt we handle correctly elsewhere in the tree.  (x86_64, for example.)  I trust you to think through the consequences of this.
Attachment #8862669 - Flags: review?(nalexander) → review+
Comment on attachment 8862670 [details] [diff] [review]
Detect NDK version number (v1)

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

I have some style nits, which I am happy to revisit if you disagree.  Otherwise, it looks great!

::: build/moz.configure/android-ndk.configure
@@ +63,5 @@
> +def ndk_version(ndk):
> +    with open(os.path.join(ndk, 'source.properties'), 'r') as f:
> +        for line in f:
> +            if line.startswith('Pkg.Revision'):
> +                return line.partition('=')[-1].strip()

`partition` is not as well known as `split`, and it returns `(s, '', '')` if the separator.  This will subtly fail if `line` is not of the form you expect.  I'd prefer `(_, version) = line.split('=')` and an `if version: return version` to fallthrough.  That asserts that the line has exactly the form you expect.

@@ +68,5 @@
> +    die('Cannot determine NDK version')
> +
> +@depends(ndk_version)
> +def ndk_major_version(ndk_version):
> +    return ndk_version.split('.')[0]

Again, prefer being strict: `(major, minor) = ...split('.')`, and `die` if you don't find a version.

@@ +74,5 @@
> +set_config('ANDROID_NDK_MAJOR_VERSION', ndk_major_version);
> +
> +@depends(ndk_version)
> +def ndk_minor_version(ndk_version):
> +    return ndk_version.split('.')[1]

Comment that you never get Major.Minor.Patch here.
Attachment #8862670 - Flags: review?(nalexander) → review+
Depends on: 1360629
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50c060199b9a
Set minimum NDK platform version to 21 for 64-bit builds; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b48b5adae0b
Detect NDK version number; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/47c7a63f002e
Produce android version codes for AArch64/ARM64 Fennec; r=nalexander
https://hg.mozilla.org/mozilla-central/rev/50c060199b9a
https://hg.mozilla.org/mozilla-central/rev/1b48b5adae0b
https://hg.mozilla.org/mozilla-central/rev/47c7a63f002e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 55 → mozilla55
You need to log in before you can comment on or make changes to this bug.