Exclude only roots for exclude-target-regexp in v2 (pantsbuild#4578)

### Problem

The `v2` engine implemented the `exclude-target-regexp` option differently from the `v1` engine, in that it completely excluded targets immediately after parsing. While that arguably aligned with the option's docstring, it did not align with the implementation in the `v1` engine, which had the effect of only filtering target roots.

Real usecases in the wild (see twitter-archive/commons#451) were depending on the behaviour of filtering only roots, and the result of filtering targets completely out of the graph when other things are depending on them is always some sort of error.

### Solution

Align the behaviour of `v2` with `v1` by only filtering root targets, clarify the doc string, and add a test that excluding a dependency still allows the dependee to be built.

### Result

The behaviour of the `v1` engine for this case is preserved in `v2`. Users who want to completely ignore a BUILD file can use the more granular `--build-ignore` option to do so. Fixes pantsbuild#4573

src/python/pants/engine/build_files.py

@@ -82,8 +82,7 @@ def parse_address_family(address_mapper, path, build_files):
     address_maps.append(AddressMap.parse(filecontent_product.path,
                                          filecontent_product.content,
                                          address_mapper.symbol_table_cls,
-                                         address_mapper.parser_cls,
-                                         address_mapper.exclude_patterns))
+                                         address_mapper.parser_cls))
   return AddressFamily.create(path.path, address_maps)
 
 
@@ -231,9 +230,10 @@ def _hydrate(item_type, spec_path, **kwargs):
 
 
 @rule(BuildFileAddresses,
-      [SelectDependencies(AddressFamily, BuildDirs, field_types=(Dir,)),
+      [Select(AddressMapper),
+       SelectDependencies(AddressFamily, BuildDirs, field_types=(Dir,)),
        Select(_SPECS_CONSTRAINT)])
-def addresses_from_address_families(address_families, spec):
+def addresses_from_address_families(address_mapper, address_families, spec):
   """Given a list of AddressFamilies and a Spec, return matching Addresses.
 
   Raises a ResolveError if:
@@ -243,13 +243,23 @@ def addresses_from_address_families(address_families, spec):
   if not address_families:
     raise ResolveError('Path "{}" contains no BUILD files.'.format(spec.directory))
 
+  def exclude_address(address):
+    if address_mapper.exclude_patterns:
+      address_str = address.spec
+      return any(p.search(address_str) is not None for p in address_mapper.exclude_patterns)
+    return False
+
   if type(spec) in (DescendantAddresses, SiblingAddresses, AscendantAddresses):
-    addresses = tuple(a for af in address_families for a in af.addressables.keys())
+    addresses = tuple(a
+                      for af in address_families
+                      for a in af.addressables.keys()
+                      if not exclude_address(a))
   elif type(spec) is SingleAddress:
     # TODO Could assert len(address_families) == 1, as it should always be true in this case.
     addresses = tuple(a
                       for af in address_families
-                      for a in af.addressables.keys() if a.target_name == spec.name)
+                      for a in af.addressables.keys()
+                      if a.target_name == spec.name and not exclude_address(a))
     if not addresses:
       if len(address_families) == 1:
         _raise_did_you_mean(address_families[0], spec.name)

src/python/pants/engine/mapper.py

@@ -39,16 +39,8 @@ class AddressMap(datatype('AddressMap', ['path', 'objects_by_name'])):
   :param objects_by_name: A dict mapping from object name to the parsed 'thin' addressable object.
   """
 
-  @staticmethod
-  def exclude_target(target_address, exclude_patterns):
-    if exclude_patterns:
-      for pattern in exclude_patterns:
-        if pattern.search(target_address) is not None:
-          return True
-    return False
-
   @classmethod
-  def parse(cls, filepath, filecontent, symbol_table_cls, parser_cls, exclude_patterns=None):
+  def parse(cls, filepath, filecontent, symbol_table_cls, parser_cls):
     """Parses a source for addressable Serializable objects.
 
     No matter the parser used, the parsed and mapped addressable objects are all 'thin'; ie: any
@@ -61,7 +53,6 @@ def parse(cls, filepath, filecontent, symbol_table_cls, parser_cls, exclude_patt
     :type symbol_table_cls: A :class:`pants.engine.parser.SymbolTable`.
     :param parser_cls: The parser cls to use.
     :type parser_cls: A :class:`pants.engine.parser.Parser`.
-    :param list exclude_patterns: A list of compiled regular expression objects.
     """
     try:
       objects = parser_cls.parse(filepath, filecontent, symbol_table_cls)
@@ -80,10 +71,7 @@ def parse(cls, filepath, filecontent, symbol_table_cls, parser_cls, exclude_patt
       if name in objects_by_name:
         raise DuplicateNameError('An object already exists at {!r} with name {!r}: {!r}.  Cannot '
                                  'map {!r}'.format(filepath, name, objects_by_name[name], obj))
-
-      target_address = '{}:{}'.format(os.path.dirname(filepath), name)
-      if not AddressMap.exclude_target(target_address, exclude_patterns):
-        objects_by_name[name] = obj
+      objects_by_name[name] = obj
     return cls(filepath, OrderedDict(sorted(objects_by_name.items())))
 
 

src/python/pants/option/global_options.py

@@ -163,8 +163,7 @@ def register_options(cls, register):
                   "are used.  Multiple constraints may be added.  They will be ORed together.")
     register('--exclude-target-regexp', advanced=True, type=list, default=[],
              metavar='<regexp>',
-             help='Exclude targets that match these regexes.',
-             recursive=True)  # TODO: Does this need to be recursive? What does that even mean?
+             help='Exclude target roots that match these regexes.')
     # Relative pants_distdir to buildroot. Requires --pants-distdir to be bootstrapped above first.
     # e.g. '/dist/'
     rel_distdir = '/{}/'.format(os.path.relpath(register.bootstrap.pants_distdir, get_buildroot()))

testprojects/src/java/org/pantsbuild/testproject/phrases/BUILD

@@ -19,4 +19,11 @@ jvm_binary(name='ten-thousand',
 jvm_binary(name='there-was-a-duck',
   main='org.pantsbuild.testproject.phrases.ThereWasADuck',
   source='ThereWasADuck.java',
+  dependencies=[
+    ':with-her-trusty-companion',
+  ]
+)
+
+java_library(name='with-her-trusty-companion',
+  sources=['TrustyCompanion.java'],
 )

testprojects/src/java/org/pantsbuild/testproject/phrases/ThereWasADuck.java

@@ -5,6 +5,6 @@
  */
 public class ThereWasADuck {
   public static void main(String[] args) {
-    System.out.println("And also, there was a duck.");
+    System.out.println("And also, there was a duck, with her " + TrustyCompanion.VALUE +  ".");
   }
-}
+}

testprojects/src/java/org/pantsbuild/testproject/phrases/TrustyCompanion.java

@@ -0,0 +1,5 @@
+package org.pantsbuild.testproject.phrases;
+
+public class TrustyCompanion {
+  public static String VALUE = "trusty companion";
+}

tests/python/pants_test/base/BUILD

@@ -33,8 +33,8 @@ python_tests(
 )
 
 python_tests(
-  name = 'bundle_integration',
-  sources = [ 'test_bundle_integration.py' ],
+  name = 'exclude_target_regexp_integration',
+  sources = [ 'test_exclude_target_regexp_integration.py' ],
   dependencies = [
     'tests/python/pants_test:int-test',
   ],

tests/python/pants_test/base/test_bundle_integration.py → tests/python/pants_test/base/test_exclude_target_regexp_integration.py

@@ -40,13 +40,15 @@ def full_spec(self):
   ten_thousand = Bundle('ten-thousand',
       "And now you must face my army of ten thousand BUILD files.")
   there_was_a_duck = Bundle('there-was-a-duck',
-      "And also, there was a duck.")
+      "And also, there was a duck, with her trusty companion.")
+
+  trusty_companion = 'with-her-trusty-companion'
 
   all_bundles = [lesser_of_two, once_upon_a_time, ten_thousand, there_was_a_duck]
 
 
-class BundleIntegrationTest(PantsRunIntegrationTest):
-  """Tests the functionality of the --spec-exclude flag in pants."""
+class ExcludeTargetRegexpIntegrationTest(PantsRunIntegrationTest):
+  """Tests the functionality of the --exclude-target-regexp flag."""
 
   def _bundle_path(self, bundle):
     return os.path.join(get_buildroot(), 'dist',
@@ -135,12 +137,11 @@ def test_exclude_two(self):
     )
 
   @ensure_engine
-  def test_bundle_resource_ordering(self):
-    """Ensures that `resources=` ordering is respected."""
-    pants_run = self.run_pants(
-      ['-q',
-       'run',
-       'testprojects/src/java/org/pantsbuild/testproject/bundle:bundle-resource-ordering']
+  def test_only_exclude_roots(self):
+    # You cannot exclude the trusty companion (ie dependency) of an included root.
+    self._test_bundle_existences([
+          '{}:{}'.format(Bundles.phrase_path, Bundles.there_was_a_duck.spec),
+          '--exclude-target-regexp={}:{}'.format(Bundles.phrase_path, Bundles.trusty_companion),
+        ],
+        set([Bundles.there_was_a_duck]),
     )
-    self.assert_success(pants_run)
-    self.assertEquals(pants_run.stdout_data, 'Hello world from Foo\n\n')

tests/python/pants_test/engine/legacy/BUILD

@@ -31,7 +31,8 @@ python_tests(
   dependencies = [
     'tests/python/pants_test:int-test'
   ],
-  tags = {'integration'}
+  tags = {'integration'},
+  timeout = 240,
 )
 
 python_tests(
@@ -65,7 +66,7 @@ python_tests(
     'tests/python/pants_test:int-test'
   ],
   tags = {'integration'},
-  timeout = 90,
+  timeout = 30,
 )
 
 python_tests(

tests/python/pants_test/engine/legacy/test_bundle_integration.py

@@ -54,3 +54,14 @@ def test_bundle_rel_path(self):
         self.assert_success(pants_run)
         self.assertTrue(os.path.isfile(
           '{}/{}.rel_path-bundle/b/file1.txt'.format(temp_distdir, self.TARGET_PATH.replace('/', '.'))))
+
+  @ensure_engine
+  def test_bundle_resource_ordering(self):
+    """Ensures that `resources=` ordering is respected."""
+    pants_run = self.run_pants(
+      ['-q',
+       'run',
+       'testprojects/src/java/org/pantsbuild/testproject/bundle:bundle-resource-ordering']
+    )
+    self.assert_success(pants_run)
+    self.assertEquals(pants_run.stdout_data, 'Hello world from Foo\n\n')

tests/python/pants_test/engine/test_mapper.py

@@ -54,9 +54,9 @@ class AddressMapTest(unittest.TestCase):
   _symbol_table_cls = ThingTable
 
   @contextmanager
-  def parse_address_map(self, json, exclude_patterns=None):
+  def parse_address_map(self, json):
     path = '/dev/null'
-    address_map = AddressMap.parse(path, json, self._symbol_table_cls, self._parser_cls, exclude_patterns)
+    address_map = AddressMap.parse(path, json, self._symbol_table_cls, self._parser_cls)
     self.assertEqual(path, address_map.path)
     yield address_map
 
@@ -93,77 +93,6 @@ def test_duplicate_names(self):
                                   '{"type_alias": "thing", "name": "one"}'):
         self.fail()
 
-  def test_exclude_target_regexps_plain_string(self):
-    with self.parse_address_map(dedent("""
-      {
-        "type_alias": "thing",
-        "name": "one",
-        "age": 42
-      }
-      {
-        "type_alias": "thing",
-        "name": "two",
-        "age": 37
-      }
-      """), [re.compile('two')]) as address_map:
-      self.assertEqual({'one': Thing(name='one', age=42)}, address_map.objects_by_name)
-
-  def test_exclude_target_regexps_exclude_all(self):
-    with self.parse_address_map(dedent("""
-      {
-        "type_alias": "thing",
-        "name": "one",
-        "age": 42
-      }
-      {
-        "type_alias": "thing",
-        "name": "two",
-        "age": 37
-      }
-      """), [re.compile('o')]) as address_map:
-      self.assertEqual(dict(), address_map.objects_by_name)
-
-  def test_exclude_target_regexps_re_expression(self):
-    with self.parse_address_map(dedent("""
-      {
-        "type_alias": "thing",
-        "name": "one",
-        "age": 42
-      }
-      {
-        "type_alias": "thing",
-        "name": "one_two",
-        "age": 37
-      }
-      {
-        "type_alias": "thing",
-        "name": "one_two_three",
-        "age": 32
-      }
-      """), [re.compile('o.*_two$')]) as address_map:
-      self.assertEqual({'one': Thing(name='one', age=42), 'one_two_three': Thing(name='one_two_three', age=32)},
-                       address_map.objects_by_name)
-
-  def test_exclude_target_regexps_multiple_re(self):
-    with self.parse_address_map(dedent("""
-      {
-        "type_alias": "thing",
-        "name": "one",
-        "age": 42
-      }
-      {
-        "type_alias": "thing",
-        "name": "one_two",
-        "age": 37
-      }
-      {
-        "type_alias": "thing",
-        "name": "one_two_three",
-        "age": 32
-      }
-      """), [re.compile('_.*_'), re.compile('e$')]) as address_map:
-      self.assertEqual({'one_two': Thing(name='one_two', age=37)}, address_map.objects_by_name)
-
 
 class AddressFamilyTest(unittest.TestCase):
   def test_create_single(self):
@@ -305,10 +234,3 @@ def test_walk_descendants_rel_path(self):
                       self.addr('a/d/e:e'): Target(name='e', type_alias='target'),
                       self.addr('a/d/e:e-prime'): Struct(name='e-prime', type_alias='struct')},
                      self.resolve_multi(DescendantAddresses('a/d')))
-
-  # Excludes are not implemented: expects excludes=['a/b', 'a/d/e'].
-  @unittest.expectedFailure
-  def test_walk_descendants_path_excludes(self):
-    self.assertEqual({self.addr('//:root'): Struct(name='root'),
-                      self.addr('a/d:d'): Target(name='d')},
-                     self.resolve_multi(DescendantAddresses('')))

tests/python/pants_test/help/test_help_integration.py

@@ -47,7 +47,7 @@ def test_help_all_advanced(self):
     self.assertIn('cache.test.junit advanced options:', pants_run.stdout_data)
     # Spot check to see that full args for all options are printed
     self.assertIn('--binary-dup-max-dups', pants_run.stdout_data)
-    self.assertIn('--cache-test-junit-exclude-target-regexp', pants_run.stdout_data)
+    self.assertIn('--cache-test-junit-read', pants_run.stdout_data)
     # Spot check to see that subsystem options are printing
     self.assertIn('--jvm-options', pants_run.stdout_data)
     # Spot check to see that advanced subsystem options are printing