Changes semantics of analyzer

pylib/gyp/generator/analyzer.py

Index: pylib/gyp/generator/analyzer.py
diff --git a/pylib/gyp/generator/analyzer.py b/pylib/gyp/generator/analyzer.py
index c093f1c6bda4fdcd466775a7af7755eec43f86ab..d49052718bdcecf3b33650bc884f03438dfb0191 100644
--- a/pylib/gyp/generator/analyzer.py
+++ b/pylib/gyp/generator/analyzer.py
@@ -7,17 +7,27 @@ This script is intended for use as a GYP_GENERATOR. It takes as input (by way of
 the generator flag config_path) the path of a json file that dictates the files
 and targets to search for. The following keys are supported:
 files: list of paths (relative) of the files to search for.
-targets: list of targets to search for. The target names are unqualified.
+test_targets: unqualified target names to search for. Any target in this list
+that depends upon a file in |files| is output regardless of the type of target
+or chain of dependencies.
+compile_targets: Additional unqualified targets to search for. Targets in this

[Dirk Pranke, 2015/11/11 03:51:16]

"Additional" is perhaps a bit confusing in this context, since compile_targets
might contain the exact same items as test_targets. However, see the comment
below line 712; maybe we should actually change the input to call this
'additional_compile_targets' instead.

[sky, 2015/11/11 18:38:10]

I thought we agreed on compile_targets as the name? I'm leaving it as
compile_targets. I did change the wording though.

+list that depend upon a file in |files| are not necessarily output. For
+example, if the target is out type none then the target is not output (but one
+of the descendants of the target will be).
 
 The following is output:
 error: only supplied if there is an error.
-build_targets: minimal set of targets that directly depend on the changed
-  files and need to be built. The expectation is this set of targets is passed
-  into a build step. The returned values are either values in the supplied
-  targets, or have a dependency on one of the supplied targets.
+compile_targets: minimal set of targets that directly or indirectly depend on
+  the files in |files| and is one of the supplied targets or a target that one
+  of the supplied targets depends on.

[Dirk Pranke, 2015/11/11 03:51:16]

maybe note that the second clause only applies if the supplied target is of type
'none' and also depends on unaffected targets that we don't actually want to
build? Don't know if you want to use the "meta" phrase here to match the analyze
spec in MB or not.

[sky, 2015/11/11 18:38:10]

Done.

+  The expectation is this set of targets is passed into a build step.
+test_targets: set of targets from the supplied |test_targets| that either
+  directly or indictly depend upon a file in |files|. This list if useful

[Dirk Pranke, 2015/11/11 03:51:16]

typo: s/indictly/indirectly .

[sky, 2015/11/11 18:38:10]

Done.

+  if additional processing needs to be done for certain targets after the
+  build, such as running tests.
 status: outputs one of three values: none of the supplied files were found,
   one of the include files changed so that it should be assumed everything
-  changed (in this case targets and build_targets are not output) or at
+  changed (in this case test_targets and compile_targets are not output) or at
   least one file was found.
 invalid_targets: list of supplied targets thare were not found.

[Dirk Pranke, 2015/11/11 03:51:16]

typo: s/thare/that/

[sky, 2015/11/11 18:38:10]

Done.

 
@@ -28,14 +38,16 @@ Consider a graph like the following:
 B   C
 A depends upon both B and C, A is of type none and B and C are executables.
 D is an executable, has no dependencies and nothing depends on it.
-If |targets| = ["A"] and files = ["b.cc", "d.cc"] (B depends upon b.cc and D
-depends upon d.cc), then the following is output:
-|build_targets| = ["B"] B must built as it depends upon the changed file b.cc
+If |compile_targets| = ["A"], |test_targets| = ["B", "C"] and
+files = ["b.cc", "d.cc"] (B depends upon b.cc and D depends upon d.cc), then
+the following is output:
+|compile_targets| = ["B"] B must built as it depends upon the changed file b.cc
 and the supplied target A depends upon it. A is not output as a build_target
 as it is of type none with no rules and actions.
+|test_targets| = ["B"] B directly depends upon the change file b.cc.
 
 Even though the file d.cc, which D depends upon, has changed D is not output
-as none of the supplied targets (A) depend upon D.
+as it was not supplied by way of |compile_targets| or |test_targets|.
 
 If the generator flag analyzer_output_path is specified, output is written
 there. Otherwise output is written to stdout.
@@ -225,6 +237,10 @@ class Config(object):
   def __init__(self):
     self.files = []
     self.targets = set()
+    self.compile_targets = set()
+    self.test_targets = set()
+    # Needed until recipes is updated.

[Dirk Pranke, 2015/11/11 03:51:16]

Nit: "are updated" :).

[sky, 2015/11/11 18:38:10]

Done.

+    self.deprecated_mode = False
 
   def Init(self, params):
     """Initializes Config. This is a separate method as it raises an exception
@@ -244,7 +260,12 @@ class Config(object):
     if not isinstance(config, dict):
       raise Exception('config_path must be a JSON file containing a dictionary')
     self.files = config.get('files', [])
-    self.targets = set(config.get('targets', []))
+    if config.get('targets'):
+      self.targets = set(config.get('targets'))
+      self.deprecated_mode = True
+    else:
+      self.compile_targets = set(config.get('compile_targets', []))
+      self.test_targets = set(config.get('test_targets', []))
 
 
 def _WasBuildFileModified(build_file, data, files, toplevel_dir):
@@ -375,11 +396,13 @@ def _GenerateTargets(data, target_list, target_dicts, toplevel_dir, files,
 
 
 def _GetUnqualifiedToTargetMapping(all_targets, to_find):
-  """Returns a mapping (dictionary) from unqualified name to Target for all the
-  Targets in |to_find|."""
+  """Returns a tuple of the following:
+  . mapping (dictionary) from unqualified name to Target for all the
+    Targets in |to_find|.
+  . any target names not found. If this is empty all targets were found."""
   result = {}
   if not to_find:
-    return result
+    return {}, []
   to_find = set(to_find)
   for target_name in all_targets.keys():
     extracted = gyp.common.ParseQualifiedTarget(target_name)
@@ -387,11 +410,11 @@ def _GetUnqualifiedToTargetMapping(all_targets, to_find):
       to_find.remove(extracted[1])
       result[extracted[1]] = all_targets[target_name]
       if not to_find:
-        return result
-  return result
+        return result, []
+  return result, [x for x in to_find]
 
 
-def _AddBuildTargets(target, roots, result):
+def _AddBuildTargetsDeprecated(target, roots, result):
   """Recurses through all targets that depend on |target|, adding all targets
   that need to be built (and are in |roots|) to |result|.
   roots: set of root targets.
@@ -403,7 +426,7 @@ def _AddBuildTargets(target, roots, result):
   target.in_roots = target in roots
 
   for back_dep_target in target.back_deps:
-    _AddBuildTargets(back_dep_target, roots, result)
+    _AddBuildTargetsDeprecated(back_dep_target, roots, result)
     target.added_to_compile_targets |= back_dep_target.added_to_compile_targets
     target.in_roots |= back_dep_target.in_roots
     target.is_or_has_linked_ancestor |= (
@@ -429,14 +452,97 @@ def _AddBuildTargets(target, roots, result):
     target.added_to_compile_targets = True
 
 
-def _GetBuildTargets(matching_targets, roots):
+def _GetBuildTargetsDeprecated(matching_targets, roots):
   """Returns the set of Targets that require a build.
   matching_targets: targets that changed and need to be built.
   roots: set of root targets in the build files to search from."""
   result = set()
   for target in matching_targets:
     print '\tfinding build targets for match', target.name
-    _AddBuildTargets(target, roots, result)
+    _AddBuildTargetsDeprecated(target, roots, result)
+  return result
+
+
+def _DoesTargetDependOn(target):

[sky, 2015/11/10 18:41:31]

This function and the next are exactly the same as was removed here:
https://codereview.chromium.org/1402813002 .

+  """Returns true if |target| or any of its dependencies matches the supplied
+  set of paths. This updates |matches| of the Targets as it recurses.

[Dirk Pranke, 2015/11/11 03:51:16]

"the supplied set of paths" is unclear (to me) here. You're actually referring
to the set of paths passed to analyze as a whole (the 'files' parameter), which
is passed to _GenerateTargets(), right? Maybe say that explicitly? I was a bit
confused because you're not actually supplying a set of paths to this specific
function.

Put differently _DoesTargetDependOn as a name suggests something that takes two
parameters, i.e., does X depend on Y. But it's not clear what X and Y are in
this function (the "supplied paths" that aren't really supplied to this function
directly). Maybe call this _DoesTargetDependOnAffectedFiles() or something?

I was particularly thrown because I initially expected this function and
_GetTargetsDependingOn(possible_targets) to actually be looking for all of the
targets that depend on possible_targets, not all of the targets that
possible_targets are depending on that reach a matched target. I.e., you're
traversing down toward the changed leaves, but I initially expected you to be
traversing up towards the roots.

I think this is also partially confusing because you do the traversals two
different ways (downward from test_targets to  the changed_targets, but upwards
from changed_targets to compile_targets). 

Is there a reason to do the traversals in two different directions? (I probably
would've done them both in the same direction, probably both upwards from the
changed targets, but I might be biased because that's what GN makes easiest).

[sky, 2015/11/11 18:38:10]

I updated the docs and changed names.

As to the direction. In thinking about this code a bit more I think it possible
to change _AddCompileTargets to set something in Target that indicates it is
impacted by the change so that the code in this function trivially checks a bit
in Target. Since this code was working before, I was hoping to use it as is. I
can optimize later on if we see value in it (meaning we think gyp is going to be
around long enough that's it worth improving this code).

+  target: the Target to look for."""
+  if target.match_status == MATCH_STATUS_DOESNT_MATCH:
+    return False
+  if target.match_status == MATCH_STATUS_MATCHES or \
+      target.match_status == MATCH_STATUS_MATCHES_BY_DEPENDENCY:
+    return True
+  for dep in target.deps:
+    if _DoesTargetDependOn(dep):
+      target.match_status = MATCH_STATUS_MATCHES_BY_DEPENDENCY
+      print '\t', target.name, 'matches by dep', dep.name
+      return True
+  target.match_status = MATCH_STATUS_DOESNT_MATCH
+  return False
+
+
+def _GetTargetsDependingOn(possible_targets):

[Dirk Pranke, 2015/11/11 03:51:16]

See comments above for _DoesTargetDependOn() (as relating to the name of this
function, etc.)

+  """Returns the list of Targets in |possible_targets| that depend (either
+  directly on indirectly) on the matched targets.
+  possible_targets: targets to search from."""
+  found = []
+  print 'Targets that matched by dependency:'
+  for target in possible_targets:
+    if _DoesTargetDependOn(target):
+      found.append(target)
+  return found
+
+
+def _AddCompileTargets(target, roots, add_if_no_ancestor, result):

[sky, 2015/11/10 18:41:31]

And this is _AddBuildTargets from before
https://codereview.chromium.org/1402813002.

+  """Recurses through all targets that depend on |target|, adding all targets
+  that need to be built (and are in |roots|) to |result|.
+  roots: set of root targets.
+  add_if_no_ancestor: If true and there are no ancestors of |target| then add
+  |target| to |result|. |target| must still be in |roots|.
+  result: targets that need to be built are added here."""
+  if target.visited:
+    return
+
+  target.visited = True
+  target.in_roots = target in roots
+
+  for back_dep_target in target.back_deps:
+    _AddCompileTargets(back_dep_target, roots, False, result)
+    target.added_to_compile_targets |= back_dep_target.added_to_compile_targets
+    target.in_roots |= back_dep_target.in_roots
+    target.is_or_has_linked_ancestor |= (
+      back_dep_target.is_or_has_linked_ancestor)
+
+  # Always add 'executable' targets. Even though they may be built by other
+  # targets that depend upon them it makes detection of what is going to be
+  # built easier.
+  # And always add static_libraries that have no dependencies on them from
+  # linkables. This is necessary as the other dependencies on them may be
+  # static libraries themselves, which are not compile time dependencies.
+  if target.in_roots and \
+        (target.is_executable or
+         (not target.added_to_compile_targets and
+          (add_if_no_ancestor or target.requires_build)) or
+         (target.is_static_library and add_if_no_ancestor and
+          not target.is_or_has_linked_ancestor)):
+    print '\t\tadding to compile targets', target.name, 'executable', \
+           target.is_executable, 'added_to_compile_targets', \
+           target.added_to_compile_targets, 'add_if_no_ancestor', \
+           add_if_no_ancestor, 'requires_build', target.requires_build, \
+           'is_static_library', target.is_static_library, \
+           'is_or_has_linked_ancestor', target.is_or_has_linked_ancestor
+    result.add(target)
+    target.added_to_compile_targets = True
+
+
+def _GetCompileTargets(matching_targets, roots):
+  """Returns the set of Targets that require a build.
+  matching_targets: targets that changed and need to be built.
+  roots: set of root targets in the build files to search from."""

[Dirk Pranke, 2015/11/11 03:51:16]

calling this 'roots' is also a bit confusing, because this isn't necessarily the
root targets in the build graph, it's just the list of targets you care about.

[sky, 2015/11/11 18:38:10]

Done.

+  result = set()
+  for target in matching_targets:
+    print 'finding compile targets for match', target.name
+    _AddCompileTargets(target, roots, True, result)
   return result
 
 
@@ -461,6 +567,16 @@ def _WriteOutput(params, **values):
     print 'Targets that require a build:'
     for target in values['build_targets']:
       print '\t', target
+  if 'compile_targets' in values:
+    values['compile_targets'].sort()
+    print 'Targets that need to be built:'
+    for target in values['compile_targets']:
+      print '\t', target
+  if 'test_targets' in values:
+    values['test_targets'].sort()
+    print 'Test targets:'
+    for target in values['test_targets']:
+      print '\t', target
 
   output_path = params.get('generator_flags', {}).get(
       'analyzer_output_path', None)
@@ -520,58 +636,149 @@ def CalculateVariables(default_variables, params):
     default_variables.setdefault('OS', operating_system)
 
 
+def _GenerateOutputDeprecated(target_list, target_dicts, data, params, config):
+  """Old deprecated behavior, will be nuked shortly."""
+  toplevel_dir = _ToGypPath(os.path.abspath(params['options'].toplevel_dir))
+
+  if _WasGypIncludeFileModified(params, config.files):
+    result_dict = { 'status': all_changed_string,
+                    'targets': list(config.targets) }

[Dirk Pranke, 2015/11/11 03:51:16]

this is probably a nit, since this is the way the code was before, but shouldn't
this also contain 'build_targets': list(config.targets) ?

[sky, 2015/11/11 18:38:10]

I believe the recipe code ignores the targets if all changed, so I don't think
it's an issue. Given this code is going to go away, and the code was like this
before it doesn't seem worth changing.

+    _WriteOutput(params, **result_dict)
+    return
+
+  all_targets, matching_targets, root_targets = _GenerateTargets(
+    data, target_list, target_dicts, toplevel_dir, frozenset(config.files),
+    params['build_files'])
+
+  unqualified_mapping, invalid_targets = _GetUnqualifiedToTargetMapping(
+    all_targets, config.targets)
+
+  if matching_targets:
+    search_targets = _LookupTargets(config.targets, unqualified_mapping)
+    print 'supplied targets'
+    for target in config.targets:
+      print '\t', target
+    print 'expanded supplied targets'
+    for target in search_targets:
+      print '\t', target.name
+    # Reset the visited status for _GetBuildTargets.
+    for target in all_targets.itervalues():
+      target.visited = False
+    build_targets = _GetBuildTargetsDeprecated(matching_targets, search_targets)
+    build_targets = [gyp.common.ParseQualifiedTarget(target.name)[1]
+                     for target in build_targets]
+  else:
+    build_targets = []
+
+  result_dict = { 'targets': build_targets,
+                  'status': found_dependency_string if matching_targets else
+                            no_dependency_string,
+                  'build_targets': build_targets}
+  if invalid_targets:
+    result_dict['invalid_targets'] = invalid_targets
+  _WriteOutput(params, **result_dict)
+
+
 def GenerateOutput(target_list, target_dicts, data, params):
   """Called by gyp as the final stage. Outputs results."""
   config = Config()
   try:
     config.Init(params)
+
     if not config.files:
       raise Exception('Must specify files to analyze via config_path generator '
                       'flag')
 
+    if config.deprecated_mode:
+      _GenerateOutputDeprecated(target_list, target_dicts, data, params,
+                                config)
+      return
+
     toplevel_dir = _ToGypPath(os.path.abspath(params['options'].toplevel_dir))
     if debug:
       print 'toplevel_dir', toplevel_dir
 
     if _WasGypIncludeFileModified(params, config.files):
       result_dict = { 'status': all_changed_string,
-                      'targets': list(config.targets) }
+                      'test_targets': list(config.test_targets),
+                      'compile_targets': list(config.compile_targets) }
       _WriteOutput(params, **result_dict)
       return
 

[Dirk Pranke, 2015/11/11 03:51:16]

General comments on the rest of this function:

It's long and involved, longer than I personally like individual python
functions to be.

I would maybe change this to something like:

all_targets, changed_targets, root_targets = _GenerateTargets(...)
if not changed_targets:
  WriteOutput(params, test_targets=[], compile_targets=[],
status=no_dependency_string}
  return

test_targets = ComputeTestTargets(...)
compile_targets = ComputeCompileTargets(...)
WriteOutput(...)

Also, it's confusing that 'test_targets' and 'test_target_names_contains_all'
get updated over the course of the function. I'd probably prefer it if you
created new variables for them. I suggest some possible changes below, but feel
free to adapt as you prefer.

[sky, 2015/11/11 18:38:10]

I moved all of this off into a class. Hopefully it's more readable.

-    all_targets, matching_targets, _ = _GenerateTargets(
+    all_targets, changed_targets, root_targets = _GenerateTargets(

[Dirk Pranke, 2015/11/11 03:51:16]

'changed_targets' is the list of targets *directly* affected by the changed
files (and doesn't not include the targets *indirectly* affected), right? In
other words, using your example from the top of the file, it includes B but not
A?

Also, the above comments about the use of 'root' also apply here and below. I
feel like changing 'root_targets' to 'dependencies_of_all' might make the code
below clearer.

       data, target_list, target_dicts, toplevel_dir, frozenset(config.files),
       params['build_files'])
 
-    unqualified_mapping = _GetUnqualifiedToTargetMapping(all_targets,
-                                                         config.targets)
-    invalid_targets = None
-    if len(unqualified_mapping) != len(config.targets):
-      invalid_targets = _NamesNotIn(config.targets, unqualified_mapping)
-
-    if matching_targets:
-      search_targets = _LookupTargets(config.targets, unqualified_mapping)
-      print 'supplied targets'
-      for target in config.targets:
-        print '\t', target
-      print 'expanded supplied targets'
-      for target in search_targets:
+    supplied_target_names = config.compile_targets | config.test_targets

[Dirk Pranke, 2015/11/11 03:51:16]

We discussed this earlier (as to how this was safe to use supplied_target_names
for determining compile_targets but not test_targets), but I've now realized
something different as well.

In the spec I've proposed:

https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp...

the wording strongly suggests that you should only ever look at compile_targets
to figure out which things to compile, and not look at test_targets at all. It
also says that the caller should generally compute compile_targets =
test_targets + additional_test_targets but it does not say that that's
explicitly required.

This could suggest that if we passed in

{ compile_targets: ['base_unittests'], test_targets: ['base_unittests',
'net_unittests'] }

it would be *incorrect* to return 'net_unittests' in the 'compile_targets' field
of the returned output. That's strange, but it would be even stranger if we did
that and also returned 'net_unittests' in the 'test_targets' field of the
returned output.

That suggests to me that we should actually change the spec to pass in
'additional_compile_targets' instead of 'compile_targets', to make this clearer.

WDYT?

+    # 'all' is added back below.
+    supplied_targets_contains_all = 'all' in supplied_target_names
+    supplied_target_names.discard('all')
+    unqualified_mapping, invalid_targets = _GetUnqualifiedToTargetMapping(
+      all_targets, supplied_target_names)
+
+    if changed_targets:
+      # Find the test targets first. 'all' is special cased to mean all the
+      # root targets. To deal with all the supplied |test_targets| are expanded
+      # to include the root targets during lookup. If any of the root targets
+      # match, we remove it and replace it with 'all'.

[Dirk Pranke, 2015/11/11 03:51:16]

Same concerns about 'all' and 'root targets' here as in previous comments. There
should also be some explanation as to why set(test_targets) might not be a
strict subset of set(root_targets).

+      test_target_names = set(config.test_targets)
+      test_target_names_contains_all = 'all' in test_target_names
+      test_target_names.discard('all')
+      test_targets = _LookupTargets(test_target_names, unqualified_mapping)
+      original_test_targets = set(test_targets)
+      if test_target_names_contains_all:
+        test_targets = [x for x in (set(test_targets) | set(root_targets))]

[Dirk Pranke, 2015/11/11 03:51:16]

Rewrite as

test_targets = list(set(test_targets) | set(root_targets))

maybe? I think that's a bit clearer. 

I might also call this 'expanded_test_targets' to avoid having to keep
original_test_targets around and changing the contents (and meaning) of
test_targets.

+      print 'supplied test_targets'
+      for target_name in config.test_targets:
+        print '\t', target_name
+      print 'found test_targets'
+      for target in test_targets:
+        print '\t', target.name
+      print 'searching for matching test targets'
+      test_targets = _GetTargetsDependingOn(test_targets)

[Dirk Pranke, 2015/11/11 03:51:16]

maybe instead something like:

affected_test_targets = _GetTargetsDependingOn(expanded_test_targets)

+      test_targets_contains_all = (test_target_names_contains_all and
+                                   set(test_targets) & set(root_targets))

[Dirk Pranke, 2015/11/11 03:51:16]

and then rewriting this as something like:

  affected_test_targets_should_contain_all = (
    test_target_names_originally_contained_all and
    set(affected_test_targets) & set(dependencies_of_all)
  )

+      if test_targets_contains_all:
+        # Remove any of the targets for all that were not explicitly supplied,
+        # 'all' is subsequentely added to the matching names below.
+        test_targets = [x for x in (set(test_targets) &
+                                    set(original_test_targets))]

[Dirk Pranke, 2015/11/11 03:51:16]

affected_test_targets = list(set(affected_test_targets) & set(test_targets))
instead?

Actually, maybe 'original_test_targets' is better here. Not sure.

Also nit: you misspelled subsequently.

+
+      print 'matched test_targets'
+      for target in test_targets:
         print '\t', target.name
+      test_target_names = [gyp.common.ParseQualifiedTarget(target.name)[1]
+                           for target in test_targets]
+      if test_targets_contains_all:
+        test_target_names.append('all')
+        print '\tall'
+
+      # Compile targets are found by searching up from changed targets.
       # Reset the visited status for _GetBuildTargets.
       for target in all_targets.itervalues():
         target.visited = False
-      print 'Finding build targets'
-      build_targets = _GetBuildTargets(matching_targets, search_targets)
-      build_targets = [gyp.common.ParseQualifiedTarget(target.name)[1]
-                       for target in build_targets]
-    else:
-      build_targets = []
 
-    # TODO(sky): nuke 'targets'.
-    result_dict = { 'targets': build_targets,
-                    'status': found_dependency_string if matching_targets else
-                              no_dependency_string,
-                    'build_targets': build_targets}
+      supplied_targets = _LookupTargets(supplied_target_names,

[Dirk Pranke, 2015/11/11 03:51:16]

see comments above re: supplied_target_names being the union of compile_targets
+ test_targets.

+                                        unqualified_mapping)
+      if supplied_targets_contains_all:
+        supplied_targets = [x for x in (set(supplied_targets) |
+                                        set(root_targets))]
+      print 'Supplied test_targets & compile_targets'
+      for target in supplied_targets:
+        print '\t', target.name
+      print 'Finding compile targets'
+      compile_targets = _GetCompileTargets(changed_targets, supplied_targets)
+      compile_target_names = [gyp.common.ParseQualifiedTarget(target.name)[1]
+                              for target in compile_targets]
+    else:
+      compile_target_names = []
+      test_target_names = []
+
+    found_at_least_one_target = compile_target_names or test_target_names
+    result_dict = { 'test_targets': test_target_names,
+                    'status': found_dependency_string if
+                        found_at_least_one_target else no_dependency_string,
+                    'compile_targets': compile_target_names}
     if invalid_targets:
       result_dict['invalid_targets'] = invalid_targets

[Dirk Pranke, 2015/11/11 03:51:16]

Looks like you *don't* return an error if there are invalid_targets. Should you?

     _WriteOutput(params, **result_dict)