Rewrite iOS FAT libraries build script in Python

tools-webrtc/ios/build_ios_libs.py

+#!/usr/bin/env python
+
+# 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.
+
+"""WebRTC iOS FAT libraries build script.
+Each architecture is compiled separately before being merged together.
+By default, the library is created in out_ios_libs/. (Change with -o.)
+The headers will be copied to out_ios_libs/include.
+"""
+
+import argparse
+import distutils.dir_util
+import logging
+import os
+import shutil
+import subprocess
+import sys
+
+
+SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__))
+WEBRTC_BASE_DIR = os.path.abspath(os.path.join(SCRIPT_DIR, '..', '..'))
+SDK_OUTPUT_DIR = os.path.join(WEBRTC_BASE_DIR, 'out_ios_libs')
+SDK_LIB_NAME = 'librtc_sdk_objc.a'
+SDK_FRAMEWORK_NAME = 'WebRTC.framework'
+
+BUILD_FLAVOR = 'release'
+ENABLED_ARCHITECTURES = ['arm', 'arm64', 'x64']
+IOS_DEPLOYMENT_TARGET = '8.0'
+LIBVPX_BUILD_VP9 = False
+CUSTOM_GN_OPTS = [] # example: ['some_option=foo bar', 'other_option=true']
+
+
+def _ParseArgs():
+  parser = argparse.ArgumentParser(description=__doc__)
+  parser.add_argument('-b', '--build_type', default='framework',
+      choices=['framework', 'static_only'],
+      help='The build type. Can be "framework" or "static_only". '
+           'Defaults to "framework".')
+  parser.add_argument('-c', '--clean', action='store_true', default=False,
+      help='Removes the previously generated build output, if any.')
+  parser.add_argument('-o', '--output-dir', default=SDK_OUTPUT_DIR,
+      help='Specifies a directory to output the build artifacts to. '
+           'If specified together with -c, deletes the dir.')
+  parser.add_argument('-r', '--revision', type=int, default=0,
+      help='Specifies a revision number to embed if building the framework.')
+  parser.add_argument('-e', '--bitcode', action='store_true', default=False,
+      help='Compile with bitcode.')
+  return parser.parse_args()
+
+
+def _CleanArtifacts(output_dir):
+  if os.path.isdir(output_dir):
+    logging.info('Deleting %s', output_dir)
+    shutil.rmtree(output_dir, True)

[kjellander_webrtc, 2017/01/31 21:39:54]

I'm not sure we want to ignore errors here. Since this is run only on Mac, we
shouldn't really get errors when deleting the dir, and if we do I think we might
be interested in seeing them as an exception.

[oprypin_webrtc, 2017/02/06 08:39:14]

Done.

+
+
+def BuildWebRTC(output_dir, target_arch, flavor, build_type,
+                ios_deployment_target, libvpx_build_vp9, use_bitcode,
+                custom_gn_options=[]):

[kjellander_webrtc, 2017/01/31 21:39:53]

empty lists as a default parameters in Python are scary, let's avoid it:
http://stackoverflow.com/questions/366422/what-is-the-pythonic-way-to-avoid-d...

[oprypin_webrtc, 2017/02/06 08:39:14]

Done.

+  output_dir = os.path.join(output_dir, target_arch + '_libs')
+  gn_args = ['target_os="ios"', 'ios_enable_code_signing=false',
+             'use_xcode_clang=true', 'is_component_build=false']
+
+  # Add flavor option.

[kjellander_webrtc, 2017/01/31 21:39:54]

I know you're just preserving the behavior of the old script here, but let's
improve the new one by introducing a new flag instead of this hardcoded
constant:

parser.add_argument('--build_config', default='release',
       choices=['debug', 'release'],
       help='The build config. Can be "debug" or "release". '
            'Defaults to "release".')

Then you can shorten this to something like
gn_args.append('is_debug=%s' % 'true' if flavor == 'debug' else 'false')
and skip the ValueError (I assume parse_args() deals with incorrect values?).

One could argue all of the constants should be flags, but let's wait with that
at least.

[oprypin_webrtc, 2017/02/06 08:39:14]

I made the change to use a flag but I feel like the check inside the function is
still nice to have, so we don't have to rely on a check that is done outside of
it and might not always apply.

[kjellander_webrtc, 2017/02/06 08:58:24]

Fair enough. 

+  if flavor == 'debug':
+    gn_args.append('is_debug=true')
+  elif flavor == 'release':
+    gn_args.append('is_debug=false')
+  else:
+    raise ValueError('Unexpected flavor type: %s' % flavor)
+
+  # Add the specified architecture.

[kjellander_webrtc, 2017/01/31 21:39:53]

I know these comments were there in the bash script, but I'd prefer just
removing this one and the ones below (lines 78-92) as they don't really tell
anything just reading the code doesn't reveal.

[oprypin_webrtc, 2017/02/06 08:39:14]

Done.

+  gn_args.append('target_cpu="%s"' % target_arch)
+
+  # Add deployment target.
+  gn_args.append('ios_deployment_target="%s"' % ios_deployment_target)
+
+  # Add vp9 option.
+  gn_args.append('rtc_libvpx_build_vp9=' +
+                 ('true' if libvpx_build_vp9 else 'false'))
+
+  # Add bitcode option.
+  gn_args.append('enable_ios_bitcode=' +
+                 ('true' if use_bitcode else 'false'))
+
+  # Add custom options.
+  gn_args.extend(custom_gn_options)
+
+  # Generate static or dynamic.
+  if build_type == 'static_only':
+    gn_target_name = 'rtc_sdk_objc'
+  elif build_type == 'framework':
+    gn_target_name = 'rtc_sdk_framework_objc'
+    gn_args.append('enable_dsyms=true')
+    gn_args.append('enable_stripping=true')
+  else:
+    raise ValueError('Build type "%s" is not supported.' % build_type)
+
+  logging.info('Building WebRTC with args: %s', ' '.join(gn_args))
+  cmd = ['gn', 'gen', output_dir,
+         '--args=' + ' '.join(gn_args)]
+  subprocess.check_call(cmd)

[kjellander_webrtc, 2017/01/31 21:39:54]

For debugging purposes, I find it useful to wrap subprocess.check_call into a
helper function so it's convenient to log what it's executing. Can you do that?
I suggest making something like _RunCommand which is similar to the _RunGN
function at
https://chromium.googlesource.com/external/webrtc/+/master/tools-webrtc/andro...
(since we only invoke each of these tools once I think just a generic function
is needed for this script).

[oprypin_webrtc, 2017/02/06 08:39:14]

Done.

+  logging.info('Building target: %s', gn_target_name)
+  cmd = ['ninja', '-C', output_dir, gn_target_name]
+  subprocess.check_call(cmd)
+
+  # Strip debug symbols to reduce size.
+  if build_type == 'static_only':
+    gn_target_path = os.path.join(output_dir, 'obj', 'webrtc', 'sdk',
+                                  'lib%s.a' % gn_target_name)
+    cmd = ['strip', '-S', gn_target_path, '-o',
+           os.path.join(output_dir, 'lib%s.a' % gn_target_name)]
+    subprocess.check_call(cmd)
+
+
+def main():
+  args = _ParseArgs()
+
+  logging.basicConfig(level=logging.INFO)
+
+  if args.clean:
+    _CleanArtifacts(args.output_dir)
+    return 0
+
+  # Build all architectures.
+  for arch in ENABLED_ARCHITECTURES:
+    BuildWebRTC(args.output_dir, arch, BUILD_FLAVOR, args.build_type,
+                IOS_DEPLOYMENT_TARGET, LIBVPX_BUILD_VP9, args.bitcode,
+                CUSTOM_GN_OPTS)
+
+  # Ignoring x86 except for static libraries for now because of a GN build issue
+  # where the generated dynamic framework has the wrong architectures.
+
+  # Create FAT archive.
+  if args.build_type == 'static_only':
+    BuildWebRTC(args.output_dir, 'x86', BUILD_FLAVOR, args.build_type,
+                IOS_DEPLOYMENT_TARGET, LIBVPX_BUILD_VP9, args.bitcode,
+                CUSTOM_GN_OPTS)
+
+    arm_lib_path = os.path.join(args.output_dir, 'arm_libs', SDK_LIB_NAME)
+    arm64_lib_path = os.path.join(args.output_dir, 'arm64_libs', SDK_LIB_NAME)
+    x64_lib_path = os.path.join(args.output_dir, 'x64_libs', SDK_LIB_NAME)
+    x86_lib_path = os.path.join(args.output_dir, 'x86_libs', SDK_LIB_NAME)
+
+    # Combine the slices.
+    cmd = ['lipo', arm_lib_path, arm64_lib_path, x64_lib_path, x86_lib_path,
+           '-create', '-output', os.path.join(args.output_dir, SDK_LIB_NAME)]
+    subprocess.check_call(cmd)
+
+  elif args.build_type == 'framework':
+    arm_lib_path = os.path.join(args.output_dir, 'arm_libs')
+    arm64_lib_path = os.path.join(args.output_dir, 'arm64_libs')
+    x64_lib_path = os.path.join(args.output_dir, 'x64_libs')
+
+    # Combine the slices.
+    dylib_path = os.path.join(SDK_FRAMEWORK_NAME, 'WebRTC')
+    # Use distutils instead of shutil to support merging folders.
+    distutils.dir_util.copy_tree(
+        os.path.join(arm64_lib_path, SDK_FRAMEWORK_NAME), args.output_dir)
+    try:
+      os.remove(os.path.join(args.output_dir, dylib_path))

[kjellander_webrtc, 2017/01/31 21:39:54]

Use shutil.rmtree instead and check with os.path.isdir first to avoid errors if
it doesn't exist.

[oprypin_webrtc, 2017/02/06 08:39:14]

Not sure what you mean here. Seems like we are deleting a file, not a directory
tree here.

[kjellander_webrtc, 2017/02/06 08:58:24]

Ah, I thought it was a dir. Still nicer to check if the file is there and only
try to delete it if so, instead of catching an exception and do nothing.

[oprypin_webrtc, 2017/02/06 09:19:50]

Just applying the EAFP principle. Aside from how it looks, this prevents the
hypothetical race condition where the file gets deleted elsewhere after the
check, causing a "file not found" error when deleting it in the program.

[kjellander_webrtc, 2017/02/06 11:33:02]

You're right. This is fine.

+    except OSError:
+      pass
+    logging.info('Merging framework slices.')
+    cmd = ['lipo', os.path.join(arm_lib_path, dylib_path),
+           os.path.join(arm64_lib_path, dylib_path),
+           os.path.join(x64_lib_path, dylib_path),
+           '-create', '-output', os.path.join(args.output_dir, dylib_path)]
+    subprocess.check_call(cmd)
+
+    # Remove stray mobileprovision if it exists until chromium roll lands.
+    # See https://codereview.chromium.org/2397433002.

[kjellander_webrtc, 2017/01/31 21:39:54]

This was rolled in long time ago. Can you make a separate CL to remove it from
the bash script first, so we don't make that change in this CL? (but we will of
course remove it from here as well if that is not breaking anything).

Send the review of that CL to me+tkchin, who added it in
https://chromium.googlesource.com/external/webrtc/+/dd0e1e007092a6a87a3abf994...

[oprypin_webrtc, 2017/02/06 08:39:14]

https://codereview.webrtc.org/2676233002/

[kjellander_webrtc, 2017/02/06 08:58:24]

Acknowledged.

+    provision_file = os.path.join(args.output_dir, SDK_FRAMEWORK_NAME,
+                                  'embedded.mobileprovision')
+    try:
+      os.remove(provision_file)
+    except OSError:
+      pass
+
+    # Merge the dSYM slices.
+    dsym_path = 'WebRTC.dSYM/Contents/Resources/DWARF/WebRTC'

[kjellander_webrtc, 2017/01/31 21:39:53]

since we use os.path.join everywhere else, we should do that here too.

[oprypin_webrtc, 2017/02/06 08:39:14]

Done.

+    distutils.dir_util.copy_tree(os.path.join(arm64_lib_path, 'WebRTC.dSYM'),
+                                 args.output_dir)
+    os.remove(os.path.join(args.output_dir, dsym_path))
+    logging.info('Merging dSYM slices.')
+    cmd = ['lipo', os.path.join(arm_lib_path, dsym_path),
+           os.path.join(arm64_lib_path, dsym_path),
+           os.path.join(x64_lib_path, dsym_path),
+           '-create', '-output', os.path.join(args.output_dir, dsym_path)]
+    subprocess.check_call(cmd)
+
+    # Modify the version number.
+    # Format should be <Branch cut MXX>.<Hotfix #>.<Rev #>.
+    # e.g. 55.0.14986 means branch cut 55, no hotfixes, and revision 14986.
+    infoplist_path = os.path.join(args.output_dir, SDK_FRAMEWORK_NAME,
+                                  'Info.plist')
+    cmd = ['PlistBuddy', '-c',
+           'Print :CFBundleShortVersionString', infoplist_path]
+    major_minor = subprocess.check_output(cmd).strip()
+    version_number = '%s.%s' % (major_minor, args.revision)
+    logging.info('Substituting revision number: %s', version_number)
+    cmd = ['PlistBuddy', '-c',
+           'Set :CFBundleVersion ' + version_number, infoplist_path]
+    subprocess.check_call(cmd)
+    subprocess.check_call(['plutil', '-convert', 'binary1', infoplist_path])
+
+  logging.info('Done.')
+  return 0
+
+
+if __name__ == '__main__':
+  sys.exit(main())