Allow android_resources targets to specify files rather than dirs

Specifying directories in android_resources targets leads to subtle
failures such as the target not being rebuilt after adding new files to
it.

Change-Id: I5f5cb1d50b5d7d6a8d7184b891182658aa690afa
Bug: 1026378
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1919602
Commit-Queue: Mohamed Heikal <mheikal@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#717177}

build/android/gyp/prepare_resources.py

@@ -38,10 +38,10 @@ def _ParseArgs(args):
   input_opts.add_argument(
       '--aapt-path', required=True, help='Path to the Android aapt tool')
 
-  input_opts.add_argument('--resource-dirs',
-                        default='[]',
-                        help='A list of input directories containing resources '
-                             'for this target.')
+  input_opts.add_argument(
+      '--res-sources-path',
+      required=True,
+      help='Path to a list of input resources for this target.')
 
   input_opts.add_argument(
       '--shared-resources',
@@ -77,19 +77,27 @@ def _ParseArgs(args):
 
   resource_utils.HandleCommonOptions(options)
 
-  options.resource_dirs = build_utils.ParseGnList(options.resource_dirs)
+  with open(options.res_sources_path) as f:
+    options.sources = [line.strip() for line in f.readlines()]
+  options.resource_dirs = resource_utils.ExtractResourceDirsFromFileList(
+      options.sources)
 
   return options
 
 
-def _GenerateGlobs(pattern):
-  # This function processes the aapt ignore assets pattern into a list of globs
-  # to be used to exclude files on the python side. It removes the '!', which is
-  # used by aapt to mean 'not chatty' so it does not output if the file is
-  # ignored (we dont output anyways, so it is not required). This function does
-  # not handle the <dir> and <file> prefixes used by aapt and are assumed not to
-  # be included in the pattern string.
-  return pattern.replace('!', '').split(':')
+def _CheckAllFilesListed(resource_files, resource_dirs):
+  resource_files = set(resource_files)
+  missing_files = []
+  for path, _ in resource_utils.IterResourceFilesInDirectories(resource_dirs):
+    if path not in resource_files:
+      missing_files.append(path)
+
+  if missing_files:
+    sys.stderr.write('Error: Found files not listed in the sources list of '
+                     'the BUILD.gn target:\n')
+    for path in missing_files:
+      sys.stderr.write('{}\n'.format(path))
+    sys.exit(1)
 
 
 def _ZipResources(resource_dirs, zip_path, ignore_pattern):
@@ -100,22 +108,13 @@ def _ZipResources(resource_dirs, zip_path, ignore_pattern):
   # files that should not be part of the final resource zip.
   files_to_zip = dict()
   files_to_zip_without_generated = dict()
-  globs = _GenerateGlobs(ignore_pattern)
-  for d in resource_dirs:
-    for root, _, files in os.walk(d):
-      for f in files:
-        archive_path = f
-        parent_dir = os.path.relpath(root, d)
-        if parent_dir != '.':
-          archive_path = os.path.join(parent_dir, f)
-        path = os.path.join(root, f)
-        if build_utils.MatchesGlob(archive_path, globs):
-          continue
-        # We want the original resource dirs in the .info file rather than the
-        # generated overridden path.
-        if not path.startswith('/tmp'):
-          files_to_zip_without_generated[archive_path] = path
-        files_to_zip[archive_path] = path
+  for path, archive_path in resource_utils.IterResourceFilesInDirectories(
+      resource_dirs, ignore_pattern):
+    # We want the original resource dirs in the .info file rather than the
+    # generated overridden path.
+    if not path.startswith('/tmp'):
+      files_to_zip_without_generated[archive_path] = path
+    files_to_zip[archive_path] = path
   resource_utils.CreateResourceInfoFile(files_to_zip_without_generated,
                                         zip_path)
   build_utils.DoZip(files_to_zip.iteritems(), zip_path)
@@ -145,7 +144,7 @@ def _GenerateRTxt(options, dep_subdirs, gen_dir):
   for j in options.include_resources:
     package_command += ['-I', j]
 
-  ignore_pattern = _AAPT_IGNORE_PATTERN
+  ignore_pattern = resource_utils.AAPT_IGNORE_PATTERN
   if options.strip_drawables:
     ignore_pattern += ':*drawable*'
   package_command += [
@@ -183,7 +182,7 @@ def _GenerateResourcesZip(output_resource_zip, input_resource_dirs,
     input_resource_dirs: A list of input resource directories.
   """
 
-  ignore_pattern = _AAPT_IGNORE_PATTERN
+  ignore_pattern = resource_utils.AAPT_IGNORE_PATTERN
   if strip_drawables:
     ignore_pattern += ':*drawable*'
   _ZipResources(input_resource_dirs, output_resource_zip, ignore_pattern)
@@ -191,6 +190,8 @@ def _GenerateResourcesZip(output_resource_zip, input_resource_dirs,
 
 def _OnStaleMd5(options):
   with resource_utils.BuildContext() as build:
+    if options.sources:
+      _CheckAllFilesListed(options.sources, options.resource_dirs)
     if options.r_text_in:
       r_txt_path = options.r_text_in
     else:

build/android/gyp/util/resource_utils.py

@@ -47,6 +47,15 @@ _ALL_RESOURCE_TYPES = {
     'xml'
 }
 
+AAPT_IGNORE_PATTERN = ':'.join([
+    '*OWNERS',  # Allow OWNERS files within res/
+    '*.py',  # PRESUBMIT.py sometimes exist.
+    '*.pyc',
+    '*~',  # Some editors create these as temp files.
+    '.*',  # Never makes sense to include dot(files/dirs).
+    '*.d.stamp',  # Ignore stamp files
+])
+
 
 def ToAndroidLocaleName(chromium_locale):
   """Convert an Chromium locale name into a corresponding Android one."""
@@ -156,6 +165,53 @@ _TextSymbolEntry = collections.namedtuple('RTextEntry',
     ('java_type', 'resource_type', 'name', 'value'))
 
 
+def _GenerateGlobs(pattern):
+  # This function processes the aapt ignore assets pattern into a list of globs
+  # to be used to exclude files using build_utils.MatchesGlob. It removes the
+  # '!', which is used by aapt to mean 'not chatty' so it does not output if the
+  # file is ignored (we dont output anyways, so it is not required). This
+  # function does not handle the <dir> and <file> prefixes used by aapt and are
+  # assumed not to be included in the pattern string.
+  return pattern.replace('!', '').split(':')
+
+
+def ExtractResourceDirsFromFileList(resource_files,
+                                    ignore_pattern=AAPT_IGNORE_PATTERN):
+  """Return a list of resource directories from a list of resource files."""
+  # Directory list order is important, cannot use set or other data structures
+  # that change order. This is because resource files of the same name in
+  # multiple res/ directories ellide one another (the last one passed is used).
+  # Thus the order must be maintained to prevent non-deterministic and possibly
+  # flakey builds.
+  resource_dirs = []
+  globs = _GenerateGlobs(ignore_pattern)
+  for resource_path in resource_files:
+    if build_utils.MatchesGlob(os.path.basename(resource_path), globs):
+      # Ignore non-resource files like OWNERS and the like.
+      continue
+    # Resources are always 1 directory deep under res/.
+    res_dir = os.path.dirname(os.path.dirname(resource_path))
+    if res_dir not in resource_dirs:
+      resource_dirs.append(res_dir)
+  return resource_dirs
+
+
+def IterResourceFilesInDirectories(directories,
+                                   ignore_pattern=AAPT_IGNORE_PATTERN):
+  globs = _GenerateGlobs(ignore_pattern)
+  for d in directories:
+    for root, _, files in os.walk(d):
+      for f in files:
+        archive_path = f
+        parent_dir = os.path.relpath(root, d)
+        if parent_dir != '.':
+          archive_path = os.path.join(parent_dir, f)
+        path = os.path.join(root, f)
+        if build_utils.MatchesGlob(archive_path, globs):
+          continue
+        yield path, archive_path
+
+
 def CreateResourceInfoFile(files_to_zip, zip_path):
   """Given a mapping of archive paths to their source, write an info file.
 

build/android/gyp/write_build_config.py

@@ -152,11 +152,11 @@ end up packaged into the final APK by the build system.
 
 It uses the following keys:
 
-* `deps_info['resource_dirs']`:
-List of paths to the source directories containing the resources for this
-target. This key is optional, because some targets can refer to prebuilt
-`.aar` archives.
 
+* `deps_info['res_sources_path']`:
+Path to file containing a list of resource source files used by the
+android_resources target. This replaces `deps_info['resource_dirs']` which is
+now no longer used.
 
 * `deps_info['resources_zip']`:
 *Required*. Path to the `.resources.zip` file that contains all raw/uncompiled
@@ -548,6 +548,7 @@ import sys
 import xml.dom.minidom
 
 from util import build_utils
+from util import resource_utils
 
 # Types that should never be used as a dependency of another build config.
 _ROOT_TYPES = ('android_apk', 'java_binary', 'java_annotation_processor',
@@ -828,6 +829,9 @@ def main(argv):
   parser.add_option('--android-manifest', help='Path to android manifest.')
   parser.add_option('--resource-dirs', action='append', default=[],
                     help='GYP-list of resource dirs')
+  parser.add_option(
+      '--res-sources-path',
+      help='Path to file containing a list of paths to resources.')
 
   # android_assets options
   parser.add_option('--asset-sources', help='List of asset sources.')
@@ -1237,10 +1241,10 @@ def main(argv):
     if options.package_name:
       deps_info['package_name'] = options.package_name
 
-    deps_info['resources_dirs'] = []
-    if options.resource_dirs:
-      for gyp_list in options.resource_dirs:
-        deps_info['resources_dirs'].extend(build_utils.ParseGnList(gyp_list))
+
+    deps_info['res_sources_path'] = ''
+    if options.res_sources_path:
+      deps_info['res_sources_path'] = options.res_sources_path
 
   if options.requires_android and is_java_target:
     # Lint all resources that are not already linted by a dependent library.
@@ -1251,8 +1255,12 @@ def main(argv):
       # Always use resources_dirs in favour of resources_zips so that lint error
       # messages have paths that are closer to reality (and to avoid needing to
       # extract during lint).
-      if c['resources_dirs']:
-        owned_resource_dirs.update(c['resources_dirs'])
+      if c['res_sources_path']:
+        with open(c['res_sources_path']) as f:
+          resource_files = f.readlines()
+        resource_dirs = resource_utils.ExtractResourceDirsFromFileList(
+            resource_files)
+        owned_resource_dirs.update(resource_dirs)
       else:
         owned_resource_zips.add(c['resources_zip'])
       srcjar = c.get('srcjar')

build/android/gyp/write_build_config.pydeps

 # Generated by running:
 #   build/print_python_deps.py --root build/android/gyp --output build/android/gyp/write_build_config.pydeps build/android/gyp/write_build_config.py
+../../../third_party/jinja2/__init__.py
+../../../third_party/jinja2/_compat.py
+../../../third_party/jinja2/bccache.py
+../../../third_party/jinja2/compiler.py
+../../../third_party/jinja2/defaults.py
+../../../third_party/jinja2/environment.py
+../../../third_party/jinja2/exceptions.py
+../../../third_party/jinja2/filters.py
+../../../third_party/jinja2/idtracking.py
+../../../third_party/jinja2/lexer.py
+../../../third_party/jinja2/loaders.py
+../../../third_party/jinja2/nodes.py
+../../../third_party/jinja2/optimizer.py
+../../../third_party/jinja2/parser.py
+../../../third_party/jinja2/runtime.py
+../../../third_party/jinja2/tests.py
+../../../third_party/jinja2/utils.py
+../../../third_party/jinja2/visitor.py
+../../../third_party/markupsafe/__init__.py
+../../../third_party/markupsafe/_compat.py
+../../../third_party/markupsafe/_native.py
 ../../gn_helpers.py
 util/__init__.py
 util/build_utils.py
 util/md5_check.py
+util/resource_utils.py
 write_build_config.py

build/config/android/internal_rules.gni

@@ -326,6 +326,10 @@ template("write_build_config") {
       resource_dirs = rebase_path(invoker.resource_dirs, root_build_dir)
       args += [ "--resource-dirs=$resource_dirs" ]
     }
+    if (defined(invoker.res_sources_path)) {
+      _res_sources_path = rebase_path(invoker.res_sources_path, root_build_dir)
+      args += [ "--res-sources-path=$_res_sources_path" ]
+    }
     if (defined(invoker.proto_resources_path)) {
       _rebased_proto_resources =
           rebase_path(invoker.proto_resources_path, root_build_dir)
@@ -1839,17 +1843,13 @@ if (enable_java_templates) {
   #
   #   build_config: Path to the .build_config file corresponding to the target.
   #
-  #   resource_dirs:
+  #   resource_dirs (Deprecated):
+  #     ** This is deprecated, please specify files using |sources| parameter **
   #     List of directories containing Android resources, layout should be
   #     similar to what aapt -S <dir> expects.
   #
-  #   generated_resource_dirs: (optional)
-  #     List of directories containing generated resources.
-  #
-  #   generated_resource_files: (optional)
-  #     If generated_resources_dirs is not empty, must list all the files
-  #     within these directories (the directory must appear at the start of
-  #     the file path).
+  #   sources:
+  #     List of input resource files.
   #
   #   custom_package: (optional)
   #     Package name for the generated R.java source file. Optional if
@@ -1887,6 +1887,7 @@ if (enable_java_templates) {
       forward_variables_from(invoker,
                              [
                                "deps",
+                               "sources",
                                "testonly",
                                "visibility",
                              ])
@@ -1894,28 +1895,6 @@ if (enable_java_templates) {
 
       depfile = "$target_gen_dir/${invoker.target_name}.d"
       outputs = []
-      _all_resource_dirs = []
-      sources = []
-
-      if (defined(invoker.resource_dirs)) {
-        _all_resource_dirs += invoker.resource_dirs
-
-        # Speed up "gn gen" by short-circuiting the empty directory.
-        if (invoker.resource_dirs != [ "//build/android/empty" ] &&
-            invoker.resource_dirs != []) {
-          _sources_build_rel =
-              exec_script("//build/android/gyp/find.py",
-                          rebase_path(invoker.resource_dirs, root_build_dir),
-                          "list lines")
-          sources += rebase_path(_sources_build_rel, ".", root_build_dir)
-        }
-      }
-
-      if (defined(invoker.generated_resource_dirs)) {
-        assert(defined(invoker.generated_resource_files))
-        _all_resource_dirs += invoker.generated_resource_dirs
-        sources += invoker.generated_resource_files
-      }
 
       _android_aapt_path = android_default_aapt_path
 
@@ -1924,9 +1903,9 @@ if (enable_java_templates) {
         _android_aapt_path,
       ]
 
-      _rebased_all_resource_dirs =
-          rebase_path(_all_resource_dirs, root_build_dir)
       _rebased_build_config = rebase_path(invoker.build_config, root_build_dir)
+      _rebased_res_sources_path =
+          rebase_path(invoker.res_sources_path, root_build_dir)
 
       args = [
         "--depfile",
@@ -1937,6 +1916,7 @@ if (enable_java_templates) {
         "--dependencies-res-zips=@FileArg($_rebased_build_config:deps_info:dependency_zips)",
         "--extra-res-packages=@FileArg($_rebased_build_config:deps_info:extra_package_names)",
         "--extra-r-text-files=@FileArg($_rebased_build_config:deps_info:extra_r_text_files)",
+        "--res-sources-path=$_rebased_res_sources_path",
       ]
 
       if (defined(invoker.android_manifest)) {
@@ -1950,10 +1930,6 @@ if (enable_java_templates) {
         ]
       }
 
-      if (_rebased_all_resource_dirs != []) {
-        args += [ "--resource-dirs=$_rebased_all_resource_dirs" ]
-      }
-
       if (defined(invoker.zip_path)) {
         outputs += [
           invoker.zip_path,

build/config/android/rules.gni

@@ -899,12 +899,11 @@ if (enable_java_templates) {
   #   alternative_android_sdk_dep: Optional. Alternative Android system
   #     android java target to use.
   #   resource_dirs: List of directories containing resources for this target.
-  #   generated_resource_dirs: List of directories containing resources for this
-  #     target which are *generated* by a dependency. |generated_resource_files|
-  #     must be specified if |generated_resource_dirs| is specified.
-  #   generated_resource_files: List of all files in |generated_resource_dirs|.
-  #     |generated_resource_dirs| must be specified in |generated_resource_files|
-  #     is specified.
+  #   generated_resource_dirs (deprecated):
+  #     **This is deprecated, you only need to specify generated files directly
+  #     in |generated_resource_files|**
+  #   generated_resource_files: List of all resource files for this target
+  #     which are *generated* by a dependency.
   #   android_manifest: AndroidManifest.xml for this target (optional). Will be
   #     merged into apks that directly or indirectly depend on this target.
   #   android_manifest_dep: Target that generates AndroidManifest (if applicable)
@@ -920,13 +919,19 @@ if (enable_java_templates) {
   # Example:
   #   android_resources("foo_resources") {
   #     deps = [":foo_strings_grd"]
-  #     resource_dirs = ["res"]
+  #     sources = [
+  #       "res/drawable/foo1.xml",
+  #       "res/drawable/foo2.xml",
+  #     ]
   #     custom_package = "org.chromium.foo"
   #   }
   #
   #   android_resources("foo_resources_overrides") {
   #     deps = [":foo_resources"]
-  #     resource_dirs = ["res_overrides"]
+  #     sources = [
+  #       "res_overrides/drawable/foo1.xml",
+  #       "res_overrides/drawable/foo2.xml",
+  #     ]
   #   }
   template("android_resources") {
     forward_variables_from(invoker, [ "testonly" ])
@@ -936,6 +941,14 @@ if (enable_java_templates) {
       not_needed(invoker, [ "v14_skip" ])
     }
 
+    # TODO(https://crbug.com/1026378): remove unneeded generated_resource_dirs
+    if (defined(invoker.generated_resource_dirs)) {
+      not_needed(invoker, [ "generated_resource_dirs" ])
+    }
+
+    _pass_resource_files = defined(invoker.sources)
+    _res_sources_path = "$target_gen_dir/${invoker.target_name}.res.sources"
+
     # JUnit tests use resource zip files. These must not be put in gen/
     # directory or they will not be available to tester bots.
     _resources_zip_rebased_path = rebase_path(target_gen_dir, root_gen_dir)
@@ -959,15 +972,32 @@ if (enable_java_templates) {
       _deps += [ "//third_party/android_sdk:android_sdk_java" ]
     }
 
+    if (_pass_resource_files) {
+      _resource_files = invoker.sources
+    } else {
+      _sources_build_rel = []
+
+      # Speed up "gn gen" by short-circuiting the empty directory.
+      if (invoker.resource_dirs != [ "//build/android/empty" ] &&
+          invoker.resource_dirs != []) {
+        _sources_build_rel +=
+            exec_script("//build/android/gyp/find.py",
+                        rebase_path(invoker.resource_dirs, root_build_dir),
+                        "list lines")
+      }
+      _resource_files = rebase_path(_sources_build_rel, ".", root_build_dir)
+    }
+    if (defined(invoker.generated_resource_files)) {
+      _resource_files += invoker.generated_resource_files
+    }
+    _rebased_resource_files = rebase_path(_resource_files, root_build_dir)
+    write_file(_res_sources_path, _rebased_resource_files)
+
     write_build_config(_build_config_target_name) {
       type = "android_resources"
       build_config = _build_config
       resources_zip = _zip_path
-
-      resource_dirs = invoker.resource_dirs
-      if (defined(invoker.generated_resource_dirs)) {
-        resource_dirs += invoker.generated_resource_dirs
-      }
+      res_sources_path = _res_sources_path
 
       if (defined(_srcjar_path)) {
         forward_variables_from(invoker,
@@ -995,9 +1025,6 @@ if (enable_java_templates) {
                              [
                                "android_manifest",
                                "custom_package",
-                               "generated_resource_dirs",
-                               "generated_resource_files",
-                               "resource_dirs",
                                "strip_drawables",
                              ])
       deps = _deps
@@ -1006,6 +1033,9 @@ if (enable_java_templates) {
         deps += [ invoker.android_manifest_dep ]
       }
 
+      res_sources_path = _res_sources_path
+      sources = _resource_files
+
       build_config = _build_config
       zip_path = _zip_path
       r_text_out_path = _r_text_out_path