Fail to build with unbound Info.plist substitutions instead of silently dropping them.

build/config/mac/gen_plist.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import argparse
 import plistlib
 import os
 import re
 import subprocess
 import sys
 import tempfile
 import shlex
 
 
 # Xcode substitutes variables like ${PRODUCT_NAME} when compiling Info.plist.
 # It also supports supports modifiers like :identifier or :rfc1034identifier.
 # SUBST_RE matches a variable substitution pattern with an optional modifier,
 # while IDENT_RE matches all characters that are not valid in an "identifier"
 # value (used when applying the modifier).
 SUBST_RE = re.compile(r'\$\{(?P<id>[^}]*?)(?P<modifier>:[^}]*)?\}')
 IDENT_RE = re.compile(r'[_/\s]')
 
 
+class SubstitutionError(Exception):
+  def __init__(self, key):
+    super(SubstitutionError, self).__init__()
+    self.key = key
+
+
 class ArgumentParser(argparse.ArgumentParser):
   """Subclass of argparse.ArgumentParser to work with GN response files.
 
   GN response file writes all the arguments on a single line and assumes
   that the python script uses shlext.split() to extract them. Since the
   default ArgumentParser expects a single argument per line, we need to
   provide a subclass to have the correct support for @{{response_file_name}}.
   """
 
   def convert_arg_line_to_args(self, arg_line):
     return shlex.split(arg_line)
 
 
-def InterpolateList(values, substitutions):
-  """Interpolates variable references into |value| using |substitutions|.
-
-  Inputs:
-    values: a list of values
-    substitutions: a mapping of variable names to values
-
-  Returns:
-    A new list of values with all variables references ${VARIABLE} replaced
-    by their value in |substitutions| or None if any of the variable has no
-    subsitution.
-  """
-  result = []
-  for value in values:
-    interpolated = InterpolateValue(value, substitutions)
-    if interpolated is None:
-      return None
-    result.append(interpolated)
-  return result
-
-
 def InterpolateString(value, substitutions):
   """Interpolates variable references into |value| using |substitutions|.
 
   Inputs:
     value: a string
     substitutions: a mapping of variable names to values
 
   Returns:
     A new string with all variables references ${VARIABLES} replaced by their
-    value in |substitutions| or None if any of the variable has no substitution.
+    value in |substitutions|. Raises SubstitutionError if a variable has no
+    substitution.
   """
-  result = value
-  for match in reversed(list(SUBST_RE.finditer(value))):
+  def repl(match):
     variable = match.group('id')
     if variable not in substitutions:
-      return None
+      raise SubstitutionError(variable)
     # Some values need to be identifier and thus the variables references may
     # contains :modifier attributes to indicate how they should be converted
     # to identifiers ("identifier" replaces all invalid characters by '_' and
     # "rfc1034identifier" replaces them by "-" to make valid URI too).
     modifier = match.group('modifier')
     if modifier == ':identifier':
-      interpolated = IDENT_RE.sub('_', substitutions[variable])
+      return IDENT_RE.sub('_', substitutions[variable])
     elif modifier == ':rfc1034identifier':
-      interpolated = IDENT_RE.sub('-', substitutions[variable])
+      return IDENT_RE.sub('-', substitutions[variable])
     else:
-      interpolated = substitutions[variable]
-    result = result[:match.start()] + interpolated + result[match.end():]
-  return result
+      return substitutions[variable]
+  return SUBST_RE.sub(repl, value)
 
 
-def InterpolateValue(value, substitutions):
+def Interpolate(value, substitutions, map_fn=map):
   """Interpolates variable references into |value| using |substitutions|.
 
   Inputs:
     value: a value, can be a dictionary, list, string or other
     substitutions: a mapping of variable names to values
 
   Returns:
     A new value with all variables references ${VARIABLES} replaced by their
-    value in |substitutions| or None if any of the variable has no substitution.
+    value in |substitutions|. Raises SubstitutionError if a variable has no
+    substitution.
   """
   if isinstance(value, dict):
-    return Interpolate(value, substitutions)
+    return dict(map_fn(lambda (k, v): (k, Interpolate(v, substitutions,
+      map_fn=map_fn)), value.iteritems()))
   if isinstance(value, list):
-    return InterpolateList(value, substitutions)
+    return list(map_fn(lambda v: Interpolate(v, substitutions, map_fn=map_fn),
+      value.iteritems()))
   if isinstance(value, str):
     return InterpolateString(value, substitutions)

[sdefresne, 2016/09/27 08:26:00]

I think call may lead to uncaught SubstitutionError on iOS even if
--skip-unbound-variables is true. Should you use map_fn here too? Or add an
explicit catch (but then you'll have to pass the boolean flag here too.

[Sidney San Martín, 2016/09/27 20:15:22]

I think the current behavior might be correct, but let me know if my reasoning
doesn't make sense —

As it is, Interpolate never returns None. map_fn lets the caller take control of
collections (e.g. swallowing values when a SubstitutionError is raised). If
map_fn were wrapped around InterpolateString, what should Interpolate return
when it swallows the value? In practice, Interpolate is only ever called from
outside with lists/dicts. If someone *did* call it with a string with bad
substitutions, getting a SubstitutionError makes as much or more sense than it
just returning None, since they both have to be handled specially.

   return value
 
 
-def Interpolate(plist, substitutions):
-  """Interpolates variable references into |value| using |substitutions|.
-
-  Inputs:
-    plist: a dictionary representing a Property List (.plist) file
-    substitutions: a mapping of variable names to values
-
-  Returns:
-    A new plist with all variables references ${VARIABLES} replaced by their
-    value in |substitutions|. All values that contains references with no
-    substitutions will be removed and the corresponding key will be cleared
-    from the plist (not recursively).
-  """
-  result = {}
-  for key in plist:
-    value = InterpolateValue(plist[key], substitutions)
-    if value is not None:
-      result[key] = value
-  return result
-
-
 def LoadPList(path):
   """Loads Plist at |path| and returns it as a dictionary."""
   fd, name = tempfile.mkstemp()
   try:
     subprocess.check_call(['plutil', '-convert', 'xml1', '-o', name, path])
     with os.fdopen(fd, 'r') as f:
       return plistlib.readPlist(f)
   finally:
     os.unlink(name)
 
@@ skipping 18 matching lines @@
 
   Args:
     plist1: a dictionary representing a Property List (.plist) file
     plist2: a dictionary representing a Property List (.plist) file
 
   Returns:
     A new dictionary representing a Property List (.plist) file by merging
     |plist1| with |plist2|. If any value is a dictionary, they are merged
     recursively, otherwise |plist2| value is used.
   """
-  if not isinstance(plist1, dict) or not isinstance(plist2, dict):
-    if plist2 is not None:
-      return plist2
-    else:
-      return plist1
-  result = {}
-  for key in set(plist1) | set(plist2):
-    if key in plist2:
-      value = plist2[key]
-    else:
-      value = plist1[key]
+  result = plist1.copy()
+  for key, value in plist2.iteritems():
     if isinstance(value, dict):
-      value = MergePList(plist1.get(key, None), plist2.get(key, None))
+      old_value = result.get(key)
+      if isinstance(old_value, dict):
+        value = MergePList(old_value, value)
     result[key] = value
   return result
 
 
 def main():
   parser = ArgumentParser(
       description='A script to generate iOS application Info.plist.',
       fromfile_prefix_chars='@')
   parser.add_argument('-o', '--output', required=True,
                       help='Path to output plist file.')
   parser.add_argument('-s', '--subst', action='append', default=[],
                       help='Substitution rule in the format "key=value".')
   parser.add_argument('-f', '--format', required=True,
                       help='Plist format (e.g. binary1, xml1) to output.')
+  parser.add_argument('--skip-unbound-variables', action='store_true',
+                      help="""When an item uses an unbound variable, skip it
+                      instead of raising an error.
+                      """)
   parser.add_argument('path', nargs="+", help='Path to input plist files.')
   args = parser.parse_args()
   substitutions = {}
   for subst in args.subst:
     key, value = subst.split('=', 1)
     substitutions[key] = value
   data = {}
+  if args.skip_unbound_variables:
+    def interpolate_fn(*args):
+      def map_fn(f, iterable):
+        for v in iterable:
+          try:
+            yield f(v)
+          except SubstitutionError:
+            pass
+      return Interpolate(*args, map_fn=map_fn)
+  else:
+    interpolate_fn = Interpolate
+
   for filename in args.path:
-    data = MergePList(data, LoadPList(filename))
-  data = Interpolate(data, substitutions)
+    plist = LoadPList(filename)
+    try:
+      data = MergePList(data, interpolate_fn(plist, substitutions))
+    except SubstitutionError as e:
+      print >>sys.stderr, (
+        "SubstitutionError: No substitution found for '{0}' in {1}"
+        .format(e.key, filename))
+      return 1
+
   SavePList(args.output, args.format, data)
   return 0
 
 if __name__ == '__main__':
   sys.exit(main())