exclude-patterns and tag should apply only to roots (#5786)

### Problem

The `--exclude-patterns` flag currently applies to inner nodes, which causes odd errors. Moreover, tags should also apply only to roots. See #5189. 

### Solution

- added `tag` & `exclude_patterns` as params to `Specs`
- add tests for both
- modify changed tests to pass for inner node filtering 

### Result

Fixes #5189.

src/python/pants/base/specs.py

@@ -5,10 +5,11 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
+import re
 from abc import abstractmethod
 
 from pants.util.meta import AbstractClass
-from pants.util.objects import Collection, datatype
+from pants.util.objects import datatype
 
 
 class Spec(AbstractClass):
@@ -60,5 +61,11 @@ def to_spec_string(self):
     return '{}^'.format(self.directory)
 
 
-class Specs(Collection.of(Spec)):
-  """A collection of Spec subclasses."""
+class Specs(datatype(['dependencies', 'tags', ('exclude_patterns', tuple)])):
+  """A collection of Specs representing Spec subclasses, tags and regex filters."""
+
+  def __new__(cls, dependencies, tags=tuple(), exclude_patterns=tuple()):
+    return super(Specs, cls).__new__(cls, dependencies, tags, exclude_patterns)
+
+  def exclude_patterns_memo(self):
+    return [re.compile(pattern) for pattern in set(self.exclude_patterns or [])]

src/python/pants/bin/goal_runner.py

@@ -22,7 +22,6 @@
 from pants.java.nailgun_executor import NailgunProcessGroup
 from pants.option.ranked_value import RankedValue
 from pants.task.task import QuietTaskMixin
-from pants.util.filtering import create_filters, wrap_filters
 
 
 logger = logging.getLogger(__name__)
@@ -70,7 +69,7 @@ def _handle_help(self, help_request):
       result = help_printer.print_help()
       self._exiter(result)
 
-  def _init_graph(self, target_roots, graph_helper):
+  def _init_graph(self, target_roots, graph_helper, exclude_target_regexps, tags):
     """Determine the BuildGraph, AddressMapper and spec_roots for a given run.
 
     :param TargetRoots target_roots: The existing `TargetRoots` object, if any.
@@ -86,12 +85,13 @@ def _init_graph(self, target_roots, graph_helper):
                                                                     self._global_options,
                                                                     self._build_config)
       graph_helper = graph_scheduler_helper.new_session()
-
     target_roots = target_roots or TargetRootsCalculator.create(
       options=self._options,
-      build_root=self._root_dir,
       session=graph_helper.scheduler_session,
-      symbol_table=graph_helper.symbol_table
+      build_root=self._root_dir,
+      symbol_table=graph_helper.symbol_table,
+      exclude_patterns=tuple(exclude_target_regexps),
+      tags=tuple(tags)
     )
     graph, address_mapper = graph_helper.create_build_graph(target_roots,
                                                             self._root_dir)
@@ -112,16 +112,10 @@ def _determine_goals(self, requested_goals):
   def _roots_to_targets(self, target_roots):
     """Populate the BuildGraph and target list from a set of input TargetRoots."""
     with self._run_tracker.new_workunit(name='parse', labels=[WorkUnitLabel.SETUP]):
-      def filter_for_tag(tag):
-        return lambda target: tag in map(str, target.tags)
-
-      tag_filter = wrap_filters(create_filters(self._tag, filter_for_tag))
 
       def generate_targets():
         for address in self._build_graph.inject_roots_closure(target_roots, self._fail_fast):
-          target = self._build_graph.get_target(address)
-          if tag_filter(target):
-            yield target
+          yield self._build_graph.get_target(address)
 
       return list(generate_targets())
 
@@ -139,6 +133,8 @@ def _setup_context(self):
       self._build_graph, self._address_mapper, scheduler, target_roots = self._init_graph(
         self._target_roots,
         self._daemon_graph_helper,
+        self._global_options.exclude_target_regexp,
+        self._global_options.tag
       )
 
       goals = self._determine_goals(self._requested_goals)

src/python/pants/engine/BUILD

@@ -71,6 +71,7 @@ python_library(
     'src/python/pants/base:project_tree',
     'src/python/pants/build_graph',
     'src/python/pants/util:dirutil',
+    'src/python/pants/util:filtering',
     'src/python/pants/util:objects',
   ]
 )

src/python/pants/engine/build_files.py

@@ -7,6 +7,7 @@
 
 import collections
 import functools
+import logging
 from os.path import dirname, join
 
 import six
@@ -24,9 +25,13 @@
 from pants.engine.selectors import Get, Select
 from pants.engine.struct import Struct
 from pants.util.dirutil import fast_relpath_optional, recursive_dirname
+from pants.util.filtering import create_filters, wrap_filters
 from pants.util.objects import TypeConstraintError, datatype
 
 
+logger = logging.getLogger(__name__)
+
+
 class ResolvedTypeMismatchError(ResolveError):
   """Indicates a resolved object was not of the expected type."""
 
@@ -225,23 +230,28 @@ def by_directory():
   def raise_empty_address_family(spec):
     raise ResolveError('Path "{}" does not contain any 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)
+  def exclude_address(spec):
+    if specs.exclude_patterns:
+      return any(p.search(spec) is not None for p in specs.exclude_patterns_memo())
     return False
 
+  def filter_for_tag(tag):
+    return lambda t: tag in map(str, t.kwargs().get("tags", []))
+
+  include_target = wrap_filters(create_filters(specs.tags if specs.tags else '', filter_for_tag))
+
   addresses = []
   included = set()
   def include(address_families, predicate=None):
     matched = False
     for af in address_families:
-      for a in af.addressables.keys():
-        if not exclude_address(a) and (predicate is None or predicate(a)):
-          matched = True
-          if a not in included:
-            addresses.append(a)
-            included.add(a)
+      for (a, t) in af.addressables.items():
+        if (predicate is None or predicate(a)):
+          if include_target(t) and (not exclude_address(a.spec)):
+            matched = True
+            if a not in included:
+              addresses.append(a)
+              included.add(a)
     return matched
 
   for spec in specs.dependencies:
@@ -263,8 +273,11 @@ def include(address_families, predicate=None):
       address_family = by_directory().get(spec.directory)
       if not address_family:
         raise_empty_address_family(spec)
+      # spec.name here is generally the root node specified on commandline. equality here implies
+      # a root node i.e. node specified on commandline.
       if not include([address_family], predicate=lambda a: a.target_name == spec.name):
-        _raise_did_you_mean(address_family, spec.name)
+        if len(addresses) == 0:
+          _raise_did_you_mean(address_family, spec.name)
     elif type(spec) is AscendantAddresses:
       include(
         af
@@ -273,7 +286,6 @@ def include(address_families, predicate=None):
       )
     else:
       raise ValueError('Unrecognized Spec type: {}'.format(spec))
-
   yield BuildFileAddresses(addresses)
 
 

src/python/pants/engine/legacy/graph.py

@@ -203,14 +203,17 @@ def inject_addresses_closure(self, addresses):
     addresses = set(addresses) - set(self._target_by_address.keys())
     if not addresses:
       return
-    for _ in self._inject_specs([SingleAddress(a.spec_path, a.target_name) for a in addresses]):
+    dependencies = tuple(SingleAddress(a.spec_path, a.target_name) for a in addresses)
+    specs = [Specs(dependencies=tuple(dependencies))]
+    for _ in self._inject_specs(specs):
       pass
 
   def inject_roots_closure(self, target_roots, fail_fast=None):
     for address in self._inject_specs(target_roots.specs):
       yield address
 
   def inject_specs_closure(self, specs, fail_fast=None):
+    specs = [Specs(dependencies=tuple(specs))]
     # Request loading of these specs.
     for address in self._inject_specs(specs):
       yield address
@@ -258,9 +261,8 @@ def _inject_specs(self, subjects):
 
     logger.debug('Injecting specs to %s: %s', self, subjects)
     with self._resolve_context():
-      specs = tuple(subjects)
       thts, = self._scheduler.product_request(TransitiveHydratedTargets,
-                                              [Specs(specs)])
+                                              subjects)
 
     self._index(thts.closure)
 

src/python/pants/engine/legacy/source_mapper.py

@@ -79,11 +79,11 @@ def iter_target_addresses_for_sources(self, sources):
     """Bulk, iterable form of `target_addresses_for_source`."""
     # Walk up the buildroot looking for targets that would conceivably claim changed sources.
     sources_set = set(sources)
-    specs = tuple(AscendantAddresses(directory=d) for d in self._unique_dirs_for_sources(sources_set))
+    dependencies = tuple(AscendantAddresses(directory=d) for d in self._unique_dirs_for_sources(sources_set))
 
     # Uniqify all transitive hydrated targets.
     hydrated_target_to_address = {}
-    hydrated_targets, = self._scheduler.product_request(HydratedTargets, [Specs(specs)])
+    hydrated_targets, = self._scheduler.product_request(HydratedTargets, [Specs(dependencies=dependencies)])
     for hydrated_target in hydrated_targets.dependencies:
       if hydrated_target not in hydrated_target_to_address:
         hydrated_target_to_address[hydrated_target] = hydrated_target.adaptor.address
@@ -93,3 +93,4 @@ def iter_target_addresses_for_sources(self, sources):
       if (LegacyAddressMapper.any_is_declaring_file(legacy_address, sources_set) or
           self._owns_any_source(sources_set, hydrated_target)):
         yield legacy_address
+

src/python/pants/engine/scheduler.py

@@ -423,7 +423,7 @@ def execution_request(self, products, subjects):
       :class:`pants.engine.fs.PathGlobs` objects.
     :returns: An ExecutionRequest for the given products and subjects.
     """
-    roots = tuple((s, p) for s in subjects for p in products)
+    roots = (tuple((s, p) for s in subjects for p in products))
     native_execution_request = self._scheduler._native.new_execution_request()
     for subject, product in roots:
       self._scheduler.add_root_selection(native_execution_request, subject, product)

src/python/pants/init/engine_initializer.py

@@ -9,7 +9,6 @@
 
 from pants.base.build_environment import get_buildroot
 from pants.base.file_system_project_tree import FileSystemProjectTree
-from pants.base.specs import Specs
 from pants.engine.build_files import create_graph_rules
 from pants.engine.fs import create_fs_rules
 from pants.engine.isolated_process import create_process_rules
@@ -92,7 +91,9 @@ def warm_product_graph(self, target_roots):
     :param TargetRoots target_roots: The targets root of the request.
     """
     logger.debug('warming target_roots for: %r', target_roots)
-    subjects = [Specs(tuple(target_roots.specs))]
+    subjects = target_roots.specs
+    if not subjects:
+      subjects = []
     request = self.scheduler_session.execution_request([TransitiveHydratedTargets], subjects)
     result = self.scheduler_session.execute(request)
     if result.error:

src/python/pants/init/target_roots_calculator.py

@@ -101,7 +101,7 @@ class TargetRootsCalculator(object):
   """Determines the target roots for a given pants run."""
 
   @classmethod
-  def parse_specs(cls, target_specs, build_root=None):
+  def parse_specs(cls, target_specs, build_root=None, exclude_patterns=None, tags=None):
     """Parse string specs into unique `Spec` objects.
 
     :param iterable target_specs: An iterable of string specs.
@@ -110,18 +110,30 @@ def parse_specs(cls, target_specs, build_root=None):
     """
     build_root = build_root or get_buildroot()
     spec_parser = CmdLineSpecParser(build_root)
-    return OrderedSet(spec_parser.parse_spec(spec_str) for spec_str in target_specs)
+
+    dependencies = tuple(OrderedSet(spec_parser.parse_spec(spec_str) for spec_str in target_specs))
+    if not dependencies:
+      return None
+    return [Specs(
+      dependencies=dependencies,
+      exclude_patterns=exclude_patterns if exclude_patterns else tuple(),
+      tags=tags)
+    ]
 
   @classmethod
-  def create(cls, options, session, symbol_table, build_root=None):
+  def create(cls, options, session, symbol_table, build_root=None, exclude_patterns=None, tags=None):
     """
     :param Options options: An `Options` instance to use.
     :param session: The Scheduler session
     :param symbol_table: The symbol table
     :param string build_root: The build root.
     """
     # Determine the literal target roots.
-    spec_roots = cls.parse_specs(options.target_specs, build_root)
+    spec_roots = cls.parse_specs(
+      target_specs=options.target_specs,
+      build_root=build_root,
+      exclude_patterns=exclude_patterns,
+      tags=tags)
 
     # Determine `Changed` arguments directly from options to support pre-`Subsystem`
     # initialization paths.
@@ -135,8 +147,8 @@ def create(cls, options, session, symbol_table, build_root=None):
     logger.debug('changed_request is: %s', changed_request)
     logger.debug('owned_files are: %s', owned_files)
     scm = get_scm()
-    change_calculator = ChangeCalculator(session, symbol_table, scm) if scm else None
-    owner_calculator = OwnerCalculator(session, symbol_table) if owned_files else None
+    change_calculator = ChangeCalculator(scheduler=session, symbol_table=symbol_table, scm=scm) if scm else None
+    owner_calculator = OwnerCalculator(scheduler=session, symbol_table=symbol_table) if owned_files else None
     targets_specified = sum(1 for item
                          in (changed_request.is_actionable(), owned_files, spec_roots)
                          if item)
@@ -153,14 +165,16 @@ def create(cls, options, session, symbol_table, build_root=None):
       # alternate target roots.
       changed_addresses = change_calculator.changed_target_addresses(changed_request)
       logger.debug('changed addresses: %s', changed_addresses)
-      return TargetRoots(tuple(SingleAddress(a.spec_path, a.target_name) for a in changed_addresses))
+      dependencies = tuple(SingleAddress(a.spec_path, a.target_name) for a in changed_addresses)
+      return TargetRoots([Specs(dependencies=dependencies, exclude_patterns=exclude_patterns, tags=tags)])
 
     if owner_calculator and owned_files:
       # We've been provided no spec roots (e.g. `./pants list`) AND a owner request. Compute
       # alternate target roots.
       owner_addresses = owner_calculator.owner_target_addresses(owned_files)
       logger.debug('owner addresses: %s', owner_addresses)
-      return TargetRoots(tuple(SingleAddress(a.spec_path, a.target_name) for a in owner_addresses))
+      dependencies = tuple(SingleAddress(a.spec_path, a.target_name) for a in owner_addresses)
+      return TargetRoots([Specs(dependencies=dependencies, exclude_patterns=exclude_patterns, tags=tags)])
 
     return TargetRoots(spec_roots)
 

src/python/pants/pantsd/service/scheduler_service.py

@@ -170,12 +170,13 @@ def warm_product_graph(self, options, target_roots_calculator):
       self._watchman_is_running.wait()
 
     session = self._graph_helper.new_session()
-
     with self.fork_lock:
       target_roots = target_roots_calculator.create(
-        options,
-        session.scheduler_session,
-        session.symbol_table,
+        options=options,
+        session=session.scheduler_session,
+        symbol_table=session.symbol_table,
+        exclude_patterns=tuple(options.for_global_scope().exclude_target_regexp) if options.for_global_scope().exclude_target_regexp else tuple(),
+        tags=tuple(options.for_global_scope().tag) if options.for_global_scope().tag else tuple()
       )
       session.warm_product_graph(target_roots)
       return session, target_roots

testprojects/src/python/python_targets/test_library.py

@@ -1,4 +1,4 @@
-# coding=utf-8
+#  coding=utf-8
 # Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 

tests/python/pants_test/engine/BUILD

@@ -139,6 +139,7 @@ python_tests(
     ':scheduler_test_base',
     'src/python/pants/build_graph',
     'src/python/pants/engine:build_files',
+    'src/python/pants/engine/legacy:structs',
     'src/python/pants/engine:mapper',
     'src/python/pants/engine:parser',
     'src/python/pants/engine:scheduler',

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

@@ -285,7 +285,32 @@ def run_list(self, extra_args, success=True):
     pants_run = self.do_command(*list_args, success=success)
     return pants_run.stdout_data
 
-  def test_test_changed_exclude_target(self):
+  def test_changed_exclude_root_targets_only(self):
+    changed_src = 'src/python/python_targets/test_library.py'
+    exclude_target_regexp = r'_[0-9]'
+    excluded_set = {'src/python/python_targets:test_library_transitive_dependee_2',
+                    'src/python/python_targets:test_library_transitive_dependee_3',
+                    'src/python/python_targets:test_library_transitive_dependee_4'}
+    expected_set = set(self.TEST_MAPPING[changed_src]['transitive']) - excluded_set
+
+    with create_isolated_git_repo() as worktree:
+      with mutated_working_copy([os.path.join(worktree, changed_src)]):
+        pants_run = self.run_pants([
+          '-ldebug',   # This ensures the changed target names show up in the pants output.
+          '--exclude-target-regexp={}'.format(exclude_target_regexp),
+          '--changed-parent=HEAD',
+          '--changed-include-dependees=transitive',
+          'test',
+        ])
+
+      self.assert_success(pants_run)
+      for expected_item in expected_set:
+        self.assertIn(expected_item, pants_run.stdout_data)
+
+      for excluded_item in excluded_set:
+        self.assertNotIn(excluded_item, pants_run.stdout_data)
+
+  def test_changed_not_exclude_inner_targets(self):
     changed_src = 'src/python/python_targets/test_library.py'
     exclude_target_regexp = r'_[0-9]'
     excluded_set = {'src/python/python_targets:test_library_transitive_dependee_2',