Makes the analyzer output the set of targets needing a build

test/analyzer/gyptest-analyzer.py

Index: test/analyzer/gyptest-analyzer.py
===================================================================
--- test/analyzer/gyptest-analyzer.py	(revision 1965)
+++ test/analyzer/gyptest-analyzer.py	(working copy)
@@ -10,12 +10,15 @@
 import TestGyp
 
 found = 'Found dependency'
+found_all = 'Found dependency (all)'
 not_found = 'No dependencies'
 
 
-def _CreateTestFile(files, targets):
+def _CreateTestFile(files, targets, ignore_targets=[]):
   f = open('test_file', 'w')

[scottmg, 2014/08/15 21:30:24]

could you add a doc comment here? I find it confusing what's being created.
|files| and |targets| are both what's noted to have changed? Or files are what's
changed and we would like to build |targets|?

[sky, 2014/08/15 22:39:37]

Done.

-  to_write = {'files': files, 'targets': targets }
+  to_write = {'files': files,
+              'targets': targets,
+              'ignore_targets': ignore_targets }
   json.dump(to_write, f)
   f.close()
 
@@ -37,22 +40,36 @@
 # over a bug in pylint (E1002).
 test = TestGyp.TestGypCustom(format='analyzer')
 
+def CommonArgs():
+  return ('-Gconfig_path=test_file',
+           '-Ganalyzer_output_path=analyzer_output')
 
+
 def run_analyzer(*args, **kw):
   """Runs the test specifying a particular config and output path."""
-  args += ('-Gconfig_path=test_file',
-           '-Ganalyzer_output_path=analyzer_output')
+  args += CommonArgs()
   test.run_gyp('test.gyp', *args, **kw)
 
 
 def run_analyzer2(*args, **kw):
-  """Runs the test specifying a particular config and output path."""
-  args += ('-Gconfig_path=test_file',
-           '-Ganalyzer_output_path=analyzer_output')
+  """Same as run_analyzer(), but passes in test2.gyp instead of test.gyp."""
+  args += CommonArgs()
   test.run_gyp('test2.gyp', *args, **kw)
 
 
-def EnsureContains(targets=set(), matched=False):
+def run_analyzer3(*args, **kw):
+  """Same as run_analyzer(), but passes in test3.gyp instead of test.gyp."""
+  args += CommonArgs()
+  test.run_gyp('test3.gyp', *args, **kw)
+
+
+def run_analyzer4(*args, **kw):
+  """Same as run_analyzer(), but passes in test3.gyp instead of test.gyp."""
+  args += CommonArgs()
+  test.run_gyp('test4.gyp', *args, **kw)
+
+
+def EnsureContains(targets=set(), matched=False, affected_targets=set()):
   """Verifies output contains |targets|."""
   result = _ReadOutputFileContents()
   if result.get('error', None):
@@ -68,6 +85,12 @@
     print 'actual targets:', actual_targets, '\nexpected targets:', targets
     test.fail_test()
 
+  actual_affected_targets = set(result['affected_targets'])
+  if actual_affected_targets != affected_targets:
+    print 'actual affected_targets:', actual_affected_targets, \
+           '\nexpected affected_targets:', affected_targets
+    test.fail_test()
+
   if matched and result['status'] != found:
     print 'expected', found, 'got', result['status']
     test.fail_test()
@@ -76,6 +99,26 @@
     test.fail_test()
 
 
+def EnsureMatchedAll(targets):
+  result = _ReadOutputFileContents()
+  if result.get('error', None):
+    print 'unexpected error', result.get('error')
+    test.fail_test()
+
+  if result.get('warning', None):
+    print 'unexpected warning', result.get('warning')
+    test.fail_test()
+
+  if result['status'] != found_all:
+    print 'expected', found_all, 'got', result['status']
+    test.fail_test()
+
+  actual_targets = set(result['targets'])
+  if actual_targets != targets:
+    print 'actual targets:', actual_targets, '\nexpected targets:', targets
+    test.fail_test()
+
+
 def EnsureError(expected_error_string):
   """Verifies output contains the error string."""
   result = _ReadOutputFileContents()
@@ -110,7 +153,7 @@
              '-Ganalyzer_output_path=analyzer_output')
 EnsureError('Unable to open file bogus_file')
 
-# Verify get error when bad target is specified.
+# Verify get warning when bad target is specified.
 _CreateTestFile(['exe2.c'], ['bad_target'])
 run_analyzer()
 EnsureWarning('Unable to find all targets')
@@ -123,7 +166,7 @@
 # Trivial test of a source.
 _CreateTestFile(['foo.c'], [])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'all'})
 
 # Conditional source that is excluded.
 _CreateTestFile(['conditional_source.c'], [])
@@ -133,7 +176,7 @@
 # Conditional source that is included by way of argument.
 _CreateTestFile(['conditional_source.c'], [])
 run_analyzer('-Dtest_variable=1')
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'all'})
 
 # Two unknown files.
 _CreateTestFile(['unknown1.c', 'unoknow2.cc'], [])
@@ -148,12 +191,12 @@
 # Included dependency
 _CreateTestFile(['unknown1.c', 'subdir/subdir_source.c'], [])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'exe', 'exe3'})
 
 # Included inputs to actions.
 _CreateTestFile(['action_input.c'], [])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'all'})
 
 # Don't consider outputs.
 _CreateTestFile(['action_output.c'], [])
@@ -163,7 +206,7 @@
 # Rule inputs.
 _CreateTestFile(['rule_input.c'], [])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'all'})
 
 # Ignore path specified with PRODUCT_DIR.
 _CreateTestFile(['product_dir_input.c'], [])
@@ -173,70 +216,140 @@
 # Path specified via a variable.
 _CreateTestFile(['subdir/subdir_source2.c'], [])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'all'})
 
 # Verifies paths with // are fixed up correctly.
 _CreateTestFile(['parent_source.c'], [])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'exe', 'exe3'})
 
 # Verifies relative paths are resolved correctly.
 _CreateTestFile(['subdir/subdir_source.h'], [])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'exe'})
 
 # Various permutations when passing in targets.
 _CreateTestFile(['exe2.c', 'subdir/subdir2b_source.c'], ['exe', 'exe3'])
 run_analyzer()
-EnsureContains(matched=True, targets={'exe3'})
+EnsureContains(matched=True, targets={'exe3'},
+               affected_targets={'exe2', 'exe3'})
 
 _CreateTestFile(['exe2.c', 'subdir/subdir2b_source.c'], ['exe'])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'exe2', 'subdir2a'})
 
 # Verifies duplicates are ignored.
 _CreateTestFile(['exe2.c', 'subdir/subdir2b_source.c'], ['exe', 'exe'])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'exe2', 'subdir2a'})
 
 _CreateTestFile(['exe2.c'], ['exe'])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'exe2'})
 
 _CreateTestFile(['exe2.c'], [])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'exe2'})
 
 _CreateTestFile(['subdir/subdir2b_source.c', 'exe2.c'], [])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'exe2', 'subdir2a'})
 
+_CreateTestFile(['subdir/subdir2b_source.c'], ['exe3'])
+run_analyzer()
+EnsureContains(matched=True, targets={'exe3'}, affected_targets={'exe3'})
+
 _CreateTestFile(['exe2.c'], [])
 run_analyzer()
-EnsureContains(matched=True)
+EnsureContains(matched=True, affected_targets={'exe2'})
 
+_CreateTestFile(['foo.c'], [], ignore_targets=['all'])
+run_analyzer()
+EnsureContains(matched=True, affected_targets={'exe'})
+
 # Assertions when modifying build (gyp/gypi) files, especially when said files
 # are included.
 _CreateTestFile(['subdir2/d.cc'], ['exe', 'exe2', 'foo', 'exe3'])
 run_analyzer2()
-EnsureContains(matched=True, targets={'exe', 'foo'})
+EnsureContains(matched=True, targets={'exe', 'foo'},
+               affected_targets={'exe'})
 
 _CreateTestFile(['subdir2/subdir.includes.gypi'],
                 ['exe', 'exe2', 'foo', 'exe3'])
 run_analyzer2()
-EnsureContains(matched=True, targets={'exe', 'foo'})
+EnsureContains(matched=True, targets={'exe', 'foo'},
+               affected_targets={'exe'})
 
 _CreateTestFile(['subdir2/subdir.gyp'], ['exe', 'exe2', 'foo', 'exe3'])
 run_analyzer2()
-EnsureContains(matched=True, targets={'exe', 'foo'})
+EnsureContains(matched=True, targets={'exe', 'foo'},
+               affected_targets={'exe'})
 
 _CreateTestFile(['test2.includes.gypi'], ['exe', 'exe2', 'foo', 'exe3'])
 run_analyzer2()
-EnsureContains(matched=True, targets={'exe', 'exe2', 'exe3'})
+EnsureContains(matched=True, targets={'exe', 'exe2', 'exe3'},
+               affected_targets={'exe', 'exe2', 'exe3'})
 
 # Verify modifying a file included makes all targets dirty.
 _CreateTestFile(['common.gypi'], ['exe', 'exe2', 'foo', 'exe3'])
 run_analyzer2('-Icommon.gypi')
-EnsureContains(matched=True, targets={'exe', 'foo', 'exe2', 'exe3'})
+EnsureMatchedAll({'exe', 'exe2', 'foo', 'exe3'})
 
+# Assertions from test3.gyp.
+_CreateTestFile(['d.c', 'f.c'], ['a'])
+run_analyzer3()
+EnsureContains(matched=True, targets={'a'}, affected_targets={'a', 'b'})

[scottmg, 2014/08/15 21:30:24]

Huh, I hadn't seen set notation before
https://docs.python.org/2/library/stdtypes.html#set . I think there was still
some CrOS bots that use 2.6 or something (it's fine I think, but just in case
someone complains something broke)

+
+_CreateTestFile(['f.c'], ['a'])
+run_analyzer3()
+EnsureContains(matched=True, targets={'a'}, affected_targets={'a'})
+
+_CreateTestFile(['f.c'], [])
+run_analyzer3()
+EnsureContains(matched=True, affected_targets={'d'})

[scottmg, 2014/08/15 21:30:24]

could you explain this one? f.c changed, so 'f' is affected. why up only 1 to
'd'?

all the targets are executable, does this one need to change if d is a
shared_library, i.e. a/b might need a relink in that case?

[sky, 2014/08/15 22:39:37]

My assumption, which could certainly be bogus, is that only the targets that
depend upon matched targets need to be recompiled. This detection isn't about
the actual running phase, just the minimal set of targets to compile.

Lets simplify this to A depends on B and B depends on C. If some file in C
changes, I certainly need to recompile C, and I need to recompile B because
files in B depend upon C. But do I also need to recompile A? None of the files
in A or its dependency B have changed. If A were an executable and I wanted to
run A, then certainly I would need to compile A. This phase isn't about running,
just about compiling. On the chromium side if I cared about actually running A I
would pass in A as one of the targets, which would trigger A being returned both
as a target and affected_target because one its descendant dependencies was
changed. What I'm trying to cover with affected_targets is only compiling
targets that are not in the set of tests/executables but targets that need to be
recompiled because of a change.

Does that make sense? It could certainly be my logic is wrong here. At the end
of the day what I want to ensure is that we build as little as possible, but is
still correct. That if you were to build the set of targets returned, then build
all you don't get any compile errors. No doubt if dependencies are wrong that
won't happen, but all of this assumes dependencies are set up correctly.

+
+_CreateTestFile(['c.c', 'e.c'], [])
+run_analyzer3()
+EnsureContains(matched=True, affected_targets={'a', 'b'})
+
+_CreateTestFile(['d.c'], ['a'])
+run_analyzer3()
+EnsureContains(matched=True, targets={'a'}, affected_targets={'a', 'b'})
+
+_CreateTestFile(['a.c'], ['a', 'b'], ignore_targets=['all'])
+run_analyzer3()
+EnsureContains(matched=True, targets={'a'}, affected_targets={'a'})
+
+_CreateTestFile(['a.c'], ['a', 'b'])
+run_analyzer3()
+EnsureContains(matched=True, targets={'a'}, affected_targets={'all'})
+
+_CreateTestFile(['d.c'], ['a', 'b'])
+run_analyzer3()
+EnsureContains(matched=True, targets={'a', 'b'}, affected_targets={'a', 'b'})
+
+_CreateTestFile(['f.c'], ['a'])
+run_analyzer3()
+EnsureContains(matched=True, targets={'a'}, affected_targets={'a'})
+
+_CreateTestFile(['a.c'], ['a'], ignore_targets=['all'])
+run_analyzer3()
+EnsureContains(matched=True, targets={'a'}, affected_targets={'a'})
+
+_CreateTestFile(['a.c'], [], ignore_targets=['all'])
+run_analyzer3()
+EnsureContains(matched=True, affected_targets={'a'})
+
+_CreateTestFile(['d.c'], [])
+run_analyzer3()
+EnsureContains(matched=True, affected_targets={'a', 'b'})
+
+_CreateTestFile(['f.c'], [])

[scottmg, 2014/08/15 21:30:24]

comment here that we switched to test4.gyp

[sky, 2014/08/15 22:39:37]

Done.

+run_analyzer4()
+EnsureContains(matched=True, affected_targets={'e'})
+
+_CreateTestFile(['d.c'], [])
+run_analyzer4()
+EnsureContains(matched=True, affected_targets={'c'})
+
 test.pass_test()