Add a global --tag option to filter targets based on their tags.

--tag=foo,bar includes only targets that have at least one of those tags.
--tag=-foo,bar includes only targets that have none of those tags.
--tag=foo,bar --tag=-baz includes only targets that have at least one
  of the tags foo or bar, and do not have the tag baz.

The filtering is on target roots, of course, similar to spec_excludes and
exclude_target_regexp. Dependencies are included in the targets passed to
Task.execute() regardless of their tags.

We already had similar filtering functionality in the Filter task. So I
refactored some of that into a new filtering.py file under util.

While doing so, I noticed that the help text on Filter's options was wrong:
In --tag=-foo,bar the '-' prefix applies to both foo and bar, and
--tag=-foo,+bar is not sensible. While fixing up the help strings I realized
that it would be more succinct to move the common help text (the part
explaining about the prefix and how multiple filters are specified) into the
goal description.  Then I realized that it would be pretty verbose to put that
in the with_description() text in register.py, so I added a little thing that
uses the task's docstring as the description, if one isn't provided explicitly.

Note that Filter still has its own --tag option, which we can think
about deprecating in a future change.

IMPORTANT: While testing, I noticed that we had a longstanding bug, where
multiple filters of the same type wouldn't work, because in the loop the
filter() function was capturing the same references in its closure on each
iteration. See the test I added in test_filter.py - that test fails with no
other changes at HEAD.

Also note that the test marked as xfail is behaving as intended: according to
the implementation, separate specifications of the same option are ANDed
(while comma-separated values in a single specification are ORed). If we want
the behavior to be different then we can discuss, but for now at least that xfail
was inappropriate and misleading.

Testing Done:
CI passes: https://travis-ci.org/pantsbuild/pants/builds/66695055

Reviewed at https://rbcommons.com/s/twitter/r/2362/

src/python/pants/backend/core/register.py

@@ -95,6 +95,9 @@ def build_file_aliases():
 
 
 def register_goals():
+  # TODO: Most of these (and most tasks in other backends) can probably have their
+  # with_description() removed, as their docstring will be used instead.
+
   # Getting help.
   task(name='goals', action=ListGoals).install().with_description('List all documented goals.')
 
@@ -154,8 +157,7 @@ def execute(self):
   task(name='minimize', action=MinimalCover).install().with_description(
       'Print the minimal cover of the given targets.')
 
-  task(name='filter', action=Filter).install().with_description(
-      'Filter the input targets based on various criteria.')
+  task(name='filter', action=Filter).install()
 
   task(name='sort', action=SortTargets).install().with_description(
       'Topologically sort the targets.')

src/python/pants/backend/core/tasks/BUILD

@@ -148,6 +148,7 @@ python_library(
     'src/python/pants/base:build_environment',
     'src/python/pants/base:cmd_line_spec_parser',
     'src/python/pants/base:target',
+    'src/python/pants/util:filtering',
   ],
 )
 

src/python/pants/backend/core/tasks/filter.py

@@ -5,7 +5,6 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
-import operator
 import re
 
 from pants.backend.core.tasks.console_task import ConsoleTask
@@ -14,61 +13,36 @@
 from pants.base.cmd_line_spec_parser import CmdLineSpecParser
 from pants.base.exceptions import TaskError
 from pants.base.target import Target
-
-
-_identity = lambda x: x
-
-
-def _extract_modifier(value):
-  if value.startswith('+'):
-    return _identity, value[1:]
-  elif value.startswith('-'):
-    return operator.not_, value[1:]
-  else:
-    return _identity, value
-
-
-def _create_filters(list_option, predicate):
-  for value in list_option:
-    modifier, value = _extract_modifier(value)
-    predicates = map(predicate, value.split(','))
-    def filter(target):
-      return modifier(any(map(lambda predicate: predicate(target), predicates)))
-    yield filter
+from pants.util.filtering import create_filters, wrap_filters
 
 
 class Filter(ConsoleTask):
-  """Filters targets based on various criteria."""
+  """Filter the input targets based on various criteria.
+
+  Each of the filtering options below is a comma-separated list of filtering criteria, with an
+  implied logical OR between them, so that a target passes the filter if it matches any of the
+  criteria in the list.  A '-' prefix inverts the sense of the entire comma-separated list, so that
+  a target passes the filter only if it matches none of the criteria in the list.
 
+  Each of the filtering options may be specified multiple times, with an implied logical AND
+  between them.
+  """
   @classmethod
   def register_options(cls, register):
     super(Filter, cls).register_options(register)
-    register('--type', action='append',
-             help="Target types to include (optional '+' prefix) or exclude ('-' prefix).  "
-                  "Multiple type inclusions or exclusions can be specified in a comma-separated "
-                  "list or by using multiple instances of this flag.")
-    register('--target', action='append',
-             help="Targets to include (optional '+' prefix) or exclude ('-' prefix).  Multiple "
-                  "target inclusions or exclusions can be specified in a comma-separated list or "
-                  "by using multiple instances of this flag.")
-    register('--ancestor', action='append',
-             help="Dependency targets of targets to include (optional '+' prefix) or exclude "
-                  "('-' prefix).  Multiple ancestor inclusions or exclusions can be specified "
-                  "in a comma-separated list or by using multiple instances of this flag.")
-    register('--regex', action='append',
-             help="Regex patterns of target addresses to include (optional '+' prefix) or exclude "
-                  "('-' prefix).  Multiple target inclusions or exclusions can be specified "
-                  "in a comma-separated list or by using multiple instances of this flag.")
-    register('--tag', action='append',
-             help="Tags to include (optional '+' prefix) or exclude ('-' prefix).  Multiple "
-                  "attribute inclusions or exclusions can be specified in a comma-separated list "
-                  "or by using multiple instances of this flag. Format: "
-                  "--tag='+foo,-bar'")
-    register('--tag-regex', action='append',
-             help="Regex patterns of tags to include (optional '+' prefix) or exclude "
-                  "('-' prefix).  Multiple attribute inclusions or exclusions can be specified in "
-                  "a comma-separated list or by using multiple instances of this flag. Format: "
-                  "--tag-regex='+foo,-bar'")
+    register('--type', action='append', metavar='[+-]type1,type2,...',
+             help='Filter on these target types.')
+    register('--target', action='append', metavar='[+-]spec1,spec2,...',
+             help='Filter on these target addresses.')
+    register('--ancestor', action='append', metavar='[+-]spec1,spec2,...',
+             help='Filter on targets that these targets depend on.')
+    register('--regex', action='append', metavar='[+-]regex1,regex2,...',
+             help='Filter on target addresses matching these regexes.')
+    # TODO: Do we need this now that we have a global --tag flag? Deprecate this if not.
+    register('--tag', action='append', metavar='[+-]tag1,tag2,...',
+             help='Filter on targets with these tags.')
+    register('--tag-regex', action='append', metavar='[+-]regex1,regex2,...',
+             help='Filter on targets with tags matching these regexes.')
 
   def __init__(self, *args, **kwargs):
     super(Filter, self).__init__(*args, **kwargs)
@@ -92,7 +66,7 @@ def _get_targets(spec):
     def filter_for_address(spec):
       matches = _get_targets(spec)
       return lambda target: target in matches
-    self._filters.extend(_create_filters(self.get_options().target, filter_for_address))
+    self._filters.extend(create_filters(self.get_options().target, filter_for_address))
 
     def filter_for_type(name):
       # FIXME(pl): This should be a standard function provided by the plugin/BuildFileParser
@@ -111,43 +85,41 @@ def filter_for_type(name):
       if not issubclass(target_type, Target):
         raise TaskError('Not a Target type: {}'.format(name))
       return lambda target: isinstance(target, target_type)
-    self._filters.extend(_create_filters(self.get_options().type, filter_for_type))
+    self._filters.extend(create_filters(self.get_options().type, filter_for_type))
 
     def filter_for_ancestor(spec):
       ancestors = _get_targets(spec)
       children = set()
       for ancestor in ancestors:
         ancestor.walk(children.add)
       return lambda target: target in children
-    self._filters.extend(_create_filters(self.get_options().ancestor, filter_for_ancestor))
+    self._filters.extend(create_filters(self.get_options().ancestor, filter_for_ancestor))
 
     def filter_for_regex(regex):
       try:
         parser = re.compile(regex)
       except re.error as e:
         raise TaskError("Invalid regular expression: {}: {}".format(regex, e))
       return lambda target: parser.search(str(target.address.spec))
-    self._filters.extend(_create_filters(self.get_options().regex, filter_for_regex))
+    self._filters.extend(create_filters(self.get_options().regex, filter_for_regex))
 
     def filter_for_tag_regex(tag_regex):
       try:
         regex = re.compile(tag_regex)
       except re.error as e:
         raise TaskError("Invalid regular expression: {}: {}".format(tag_regex, e))
       return lambda target: any(map(regex.search, map(str, target.tags)))
-    self._filters.extend(_create_filters(self.get_options().tag_regex, filter_for_tag_regex))
+    self._filters.extend(create_filters(self.get_options().tag_regex, filter_for_tag_regex))
 
     def filter_for_tag(tag):
       return lambda target: tag in map(str, target.tags)
-    self._filters.extend(_create_filters(self.get_options().tag, filter_for_tag))
+    self._filters.extend(create_filters(self.get_options().tag, filter_for_tag))
 
   def console_output(self, _):
+    wrapped_filter = wrap_filters(self._filters)
     filtered = set()
     for target in self.context.target_roots:
       if target not in filtered:
         filtered.add(target)
-        for filter in self._filters:
-          if not filter(target):
-            break
-        else:
+        if wrapped_filter(target):
           yield target.address.spec

src/python/pants/bin/BUILD

@@ -25,6 +25,7 @@ python_library(
     'src/python/pants/option',
     'src/python/pants/reporting',
     'src/python/pants/subsystem',
+    'src/python/pants/util:filtering',
   ],
 )
 

src/python/pants/bin/goal_runner.py

@@ -32,6 +32,7 @@
 from pants.reporting.report import Report
 from pants.reporting.reporting import Reporting
 from pants.subsystem.subsystem import Subsystem
+from pants.util.filtering import create_filters, wrap_filters
 
 
 logger = logging.getLogger(__name__)
@@ -197,10 +198,15 @@ def _expand_goals_and_specs(self):
                                       spec_excludes=self.spec_excludes,
                                       exclude_target_regexps=self.global_options.exclude_target_regexp)
       with self.run_tracker.new_workunit(name='parse', labels=[WorkUnit.SETUP]):
+        def filter_for_tag(tag):
+          return lambda target: tag in map(str, target.tags)
+        tag_filter = wrap_filters(create_filters(self.global_options.tag, filter_for_tag))
         for spec in specs:
           for address in spec_parser.parse_addresses(spec, fail_fast):
             self.build_graph.inject_address_closure(address)
-            self.targets.append(self.build_graph.get_target(address))
+            tgt = self.build_graph.get_target(address)
+            if tag_filter(tgt):
+              self.targets.append(tgt)
     self.goals = [Goal.by_name(goal) for goal in goals]
 
   def run(self):

src/python/pants/goal/goal.py

@@ -59,11 +59,22 @@ def __init__(self, name):
     Create goals only through the Goal.by_name() factory.
     """
     self.name = name
-    self.description = None
+    self._description = ''
     self.serialize = False
     self._task_type_by_name = {}  # name -> Task subclass.
     self._ordered_task_names = []  # The task names, in the order imposed by registration.
 
+  @property
+  def description(self):
+    if self._description:
+      return self._description
+    # Return the docstring for the Task registered under the same name as this goal, if any.
+    # This is a very common case, and therefore a useful idiom.
+    namesake_task = self._task_type_by_name.get(self.name)
+    if namesake_task:
+      return namesake_task.__doc__
+    return ''
+
   def register_options(self, options):
     for task_type in sorted(self.task_types(), key=lambda cls: cls.options_scope):
       task_type.register_options_on_scope(options)
@@ -92,10 +103,13 @@ def install(self, task_registrar, first=False, replace=False, before=None, after
     # a task *instance* know its scope, but this means converting option registration from
     # a class method to an instance method, and instantiating the task much sooner in the
     # lifecycle.
-
-    subclass_name = b'{0}_{1}'.format(task_registrar.task_type.__name__,
+    superclass = task_registrar.task_type
+    subclass_name = b'{0}_{1}'.format(superclass.__name__,
                                       options_scope.replace('.', '_').replace('-', '_'))
-    task_type = type(subclass_name, (task_registrar.task_type,), {'options_scope': options_scope})
+    task_type = type(subclass_name, (superclass,), {
+      '__doc__': superclass.__doc__,
+      'options_scope': options_scope
+    })
 
     otn = self._ordered_task_names
     if replace:
@@ -121,7 +135,7 @@ def install(self, task_registrar, first=False, replace=False, before=None, after
 
   def with_description(self, description):
     """Add a description to this goal."""
-    self.description = description
+    self._description = description
     return self
 
   def uninstall_task(self, name):

src/python/pants/option/global_options.py

@@ -39,12 +39,18 @@ def register_global_options(register):
                 "are used.  Multiple constraints may be added.  They will be ORed together.")
   register('--colors', action='store_true', default=True, recursive=True,
            help='Set whether log messages are displayed in color.')
+
   register('--spec-excludes', action='append', default=[register.bootstrap.pants_workdir],
-           help='Exclude these paths when computing the command-line target specs.')
+           help='Ignore these paths when evaluating the command-line target specs.  Useful with '
+                '::, to avoid descending into unneeded directories.')
   register('--exclude-target-regexp', action='append', default=[], metavar='<regexp>',
-           help='Regex pattern to exclude from the target list (useful in conjunction with ::). '
-                'Multiple patterns may be specified by setting this flag multiple times.',
-           recursive=True)
+           help='Exclude targets that match these regexes. Useful with ::, to ignore broken '
+                'BUILD files.',
+           recursive=True)  # TODO: Does this need to be recursive? What does that even mean?
+  register('--tag', action='append', metavar='[+-]tag1,tag2,...',
+           help="Include only targets with these tags (optional '+' prefix) or without these "
+                "tags ('-' prefix).  Useful with ::, to find subsets of targets "
+                "(e.g., integration tests.)")
 
   # TODO: When we have a model for 'subsystems', create one for artifact caching and move these
   # options to there. When we do that, also drop the cumbersome word 'artifact' from these

src/python/pants/option/help_formatter.py

@@ -50,7 +50,7 @@ def _pants_arg_long_format_helper(self, action):
             invert_flag = ''
             option_flag = option_string[2:]
 
-          arg_name = action.metavar.upper() if action.metavar else action.dest.upper()
+          arg_name = action.metavar if action.metavar else action.dest
           arg_value = '' if action.nargs == 0 else '={0}'.format(arg_name)
 
           return '--{invert_flag}{heading}-{option_flag}{arg_value}\n'.format(

src/python/pants/util/BUILD

@@ -31,6 +31,12 @@ python_library(
   ],
 )
 
+python_library(
+  name = 'filtering',
+  sources = ['filtering.py'],
+  dependencies = [],
+)
+
 python_library(
   name = 'meta',
   sources = ['meta.py'],
@@ -47,4 +53,4 @@ python_library(
 python_library(
   name = 'xml_parser',
   sources = ['xml_parser.py'],
-)
+)

src/python/pants/util/filtering.py

@@ -0,0 +1,66 @@
+# coding=utf-8
+# Copyright 2015 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
+                        unicode_literals, with_statement)
+
+import copy
+import operator
+
+
+_identity = lambda x: x
+
+
+def _extract_modifier(modified_param):
+  if modified_param.startswith('+'):
+    return _identity, modified_param[1:]
+  elif modified_param.startswith('-'):
+    return operator.not_, modified_param[1:]
+  else:
+    return _identity, modified_param
+
+
+def create_filters(predicate_params, predicate_factory):
+  """Create filter functions from a list of string parameters.
+
+  :param predicate_params: A list of predicate_param arguments as in `create_filter`.
+  :param predicate_factory: As in `create_filter`.
+  """
+  filters = []
+  for predicate_param in predicate_params:
+    filters.append(create_filter(predicate_param, predicate_factory))
+  return filters
+
+
+def create_filter(predicate_param, predicate_factory):
+  """Create a filter function from a string parameter.
+
+  :param predicate_param: Create a filter for this param string. Each string is a
+                          comma-separated list of arguments to the predicate_factory.
+                          If the entire comma-separated list is prefixed by a '-' then the
+                          sense of the resulting filter is inverted.
+  :param predicate_factory: A function that takes a parameter and returns a predicate, i.e., a
+                            function that takes a single parameter (of whatever type the filter
+                            operates on) and returns a boolean.
+  :return: A filter function of one argument that is the logical OR of the predicates for each of
+           the comma-separated arguments. If the comma-separated list was prefixed by a '-',
+           the sense of the filter is inverted.
+  """
+  # NOTE: Do not inline this into create_filters above. A separate function is necessary
+  # in order to capture the different closure on each invocation.
+  modifier, param = _extract_modifier(predicate_param)
+  predicates = map(predicate_factory, param.split(','))
+  def filt(x):
+    return modifier(any(map(lambda pred: pred(x), predicates)))
+  return filt
+
+
+def wrap_filters(filters):
+  """Returns a single filter that short-circuit ANDs the specified filters."""
+  def combined_filter(x):
+    for filt in filters:
+      if not filt(x):
+        return False
+    return True
+  return combined_filter