Android: Script for building libwebrtc.aar.

webrtc/build/android/build_aar.py

+#!/usr/bin/python3

[kjellander_webrtc, 2017/01/23 14:39:31]

Prefer  #!/usr/bin/env python
Especially Python 3 is cannot assumed be installed.
https://google.github.io/styleguide/pyguide.html?showone=Shebang_Line#Shebang...

[sakal, 2017/01/23 15:29:03]

Done.

+
+# Copyright (c) 2017 The WebRTC project authors. All Rights Reserved.
+#
+# Use of this source code is governed by a BSD-style license
+# that can be found in the LICENSE file in the root of the source
+# tree. An additional intellectual property rights grant can be found
+# in the file PATENTS.  All contributing project authors may
+# be found in the AUTHORS file in the root of the source tree.
+
+# Run from root src folder: ./webrtc/build/android/build_aar.py

[kjellander_webrtc, 2017/01/23 14:39:30]

Add a docstring that briefly explains what this script does. I think it would be
useful to list an example of how the resulting directory hierarchy of the .aar
file looks like.

[kjellander_webrtc, 2017/01/23 14:39:31]

Is it a requirement to run this from the src/ folder? That's fine, but make it
clear and make the script fail if not done so, if you want to limit it to that.

[sakal, 2017/01/23 15:29:03]

Done.

[sakal, 2017/01/23 15:29:03]

Do you have an eloquent method to check the folder? The best I can come up with
is checking the relative path to the script itself.

+
+import argparse
+import logging
+import os
+import subprocess
+import sys
+import zipfile
+
+DEFAULT_ARCHS = ['armeabi-v7a', 'x86']
+NEEDED_SO_FILES = [
+  'libc++_shared.so',

[kjellander_webrtc, 2017/01/23 14:39:31]

Is there a way around hardcoding these into the script?

[sakal, 2017/01/23 15:29:03]

Actually, it seems we only need libjingle_peerconnection_so.so when not doing a
debug build.

Yes, these could be read from the build files but I think we should keep it
simple here. This is not something that changes often and should be easy to
manually update when needed.

+  'libboringssl.cr.so',
+  'libjingle_peerconnection_so.so',
+  'libprotobuf_lite.cr.so',
+]
+JAR_FILE = 'lib.java/webrtc/sdk/android/libwebrtc.jar'
+MANIFEST_FILE = 'webrtc/sdk/android/AndroidManifest.xml'
+TARGETS = [
+  'build/android:cpplib_stripped',
+  'webrtc/sdk/android:libwebrtc',
+  'webrtc/sdk/android:libjingle_peerconnection_so'
+]
+TMP_OUTPUT = '/tmp/webrtc_android_aar_build/'

[kjellander_webrtc, 2017/01/23 14:39:31]

Please use Python's tempfile module to create a unique temporary directory
instead. 

[sakal, 2017/01/23 15:29:03]

Done.

+
+parser = argparse.ArgumentParser(description='libwebrtc.aar generator.')

[kjellander_webrtc, 2017/01/23 14:39:31]

Refactor line 36-44 into a _ParseArgs function that you call from main()

[sakal, 2017/01/23 15:29:03]

Done.

+parser.add_argument('--output', default='libwebrtc.aar',
+    help='Output file of the scipt.')
+parser.add_argument('--arch', action='append', default=[],

[kjellander_webrtc, 2017/01/23 14:39:31]

Can't you set default=DEFAULT_ARCHS here?

[sakal, 2017/01/23 15:29:03]

No, additional arch parameters would be just be appended to that list.

+    help='Architectures to build. Defaults to ' + str(DEFAULT_ARCHS))
+parser.add_argument('--use-goma', action='store_true', default=False,
+    help='Use goma.')
+parser.add_argument('--debug', action='store_true', default=False,

[kjellander_webrtc, 2017/01/23 14:39:31]

I prefer --verbose, but I won't force you.

[sakal, 2017/01/23 15:29:03]

Done.

+    help='Debug logging.')
+
+def RunGN(args):

[kjellander_webrtc, 2017/01/23 14:39:31]

Use two blank lines before top-level statements/functions:
https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines

[sakal, 2017/01/23 15:29:03]

Done.

+  cmd = ['gn']
+  cmd.extend(args)
+  logging.debug('Running: %r', cmd)
+  subprocess.check_call(cmd)
+
+def RunNinja(output_directory, args):
+  cmd = ['ninja', '-C', output_directory]
+  cmd.extend(args)
+  logging.debug('Running: %r', cmd)
+  subprocess.check_call(cmd)
+
+def EncodeForGN(value):
+  """Encodes value as a GN literal."""
+  if type(value) is str:
+    return '"' + value + '"'
+  elif type(value) is bool:
+    return repr(value).lower()
+  else:
+    return repr(value)
+
+def GetOutputDirectory(arch):
+  """Returns the GN output directory for the target architecture."""
+  return TMP_OUTPUT + arch
+
+def GetTargetCpu(arch):
+  """Returns target_cpu for the GN build with the given architecture."""
+  if arch in ['armeabi', 'armeabi-v7a']:
+    return 'arm'
+  elif arch == 'x86':
+    return 'x86'
+  else:
+    raise Exception('Unknown arch: ' + arch)
+
+def GetArmVersion(arch):
+  """Returns arm_version for the GN build with the given architecture."""
+  if arch == 'armeabi':
+    return 6
+  elif arch == 'armeabi-v7a':
+    return 7
+  elif arch == 'x86':
+    return None
+  else:
+    raise Exception('Unknown arch: ' + arch)
+
+def Build(arch, use_goma):
+  """Generates target architecture using GN and builds it using ninja."""
+  logging.info('Building: ' + arch)

[kjellander_webrtc, 2017/01/23 14:39:31]

Use string formatters:
logging.info('Building: %s', arch)

[sakal, 2017/01/23 15:29:03]

Done.

+  output_directory = GetOutputDirectory(arch)
+  gn_args = {
+    'target_os': 'android',
+    'target_cpu': GetTargetCpu(arch),
+    'use_goma': use_goma
+  }
+  arm_version = GetArmVersion(arch)
+  if arm_version:
+    gn_args['arm_version'] = arm_version
+  gn_args_str = '--args=' + ' '.join([
+      k + '=' + EncodeForGN(v) for k, v in gn_args.items()])
+
+  RunGN(['gen', output_directory, gn_args_str])
+
+  ninja_args = TARGETS
+  if use_goma:
+    ninja_args.extend(['-j', '1024'])
+  RunNinja(output_directory, ninja_args)
+
+def CollectCommon(aar, arch):

[kjellander_webrtc, 2017/01/23 14:39:30]

aar -> aar_file to make it more clear what this object is.

[sakal, 2017/01/23 15:29:03]

Done.

+  """Collects architecture independent files into the .aar-archive."""
+  logging.info('Collecting common files.')
+  output_directory = GetOutputDirectory(arch)
+  aar.write(MANIFEST_FILE, 'AndroidManifest.xml')
+  aar.write(os.path.join(output_directory, JAR_FILE), 'classes.jar')
+
+def Collect(aar, arch):
+  """Collects architecture specific files into the .aar-archive."""
+  logging.info('Collecting: ' + arch)
+  output_directory = GetOutputDirectory(arch)
+
+  jni_dir = 'jni'

[kjellander_webrtc, 2017/01/23 14:39:31]

Remove this variable, it doesn't add anything useful.

[sakal, 2017/01/23 15:29:03]

Done.

+  abi_dir = os.path.join(jni_dir, arch)
+
+  for so_file in NEEDED_SO_FILES:
+    aar.write(os.path.join(output_directory, so_file),
+        os.path.join(abi_dir, so_file))

[kjellander_webrtc, 2017/01/23 14:39:31]

+ indentation to alight with the above parenthesis.

[sakal, 2017/01/23 15:29:03]

Done.

+
+def main():
+  args = parser.parse_args()
+  if not args.arch:

[kjellander_webrtc, 2017/01/23 14:39:31]

Add a parser.error message if len(args.arch) < 1.

[sakal, 2017/01/23 15:29:03]

Wouldn't that be the case when using the default architectures.

[kjellander_webrtc, 2017/01/24 06:47:00]

Yeah, this assumed it was possible to use default=DEFAULT_ARCHS which turned out
to not be true, so just ignore my comment here :)

+    args.arch = DEFAULT_ARCHS

[kjellander_webrtc, 2017/01/23 14:39:31]

Can't you set this at line 39 instead?

[sakal, 2017/01/23 15:29:03]

No, see above.

+
+  logging.basicConfig(level=logging.INFO if not args.debug else logging.DEBUG)

[kjellander_webrtc, 2017/01/23 14:39:30]

level=logging.DEBUG if args.debug else logging.INFO
is easier to read IMO.

[sakal, 2017/01/23 15:29:03]

Done.

+
+  for arch in args.arch:
+    Build(arch, args.use_goma)
+
+  with zipfile.ZipFile(args.output, 'w') as aar:
+    CollectCommon(aar, args.arch[0])

[kjellander_webrtc, 2017/01/23 14:39:31]

This makes me wonder what different it makes which is the first architecture in
the list? If it can matter, it'd be better to pass some kind of primary arch and
"additional archs" so it's obvious what happens.

[sakal, 2017/01/23 15:29:03]

There is no difference. The common outputs are the same regardless of the
architecture. I just arbitrarily choose to take them from the first one.

[kjellander_webrtc, 2017/01/24 06:47:00]

Please add a comment explaining that to make it obvious for future readers.

+    for arch in args.arch:
+      Collect(aar, arch)
+
+if __name__ == '__main__':
+  sys.exit(main())