Coalesce errors when parsing BUILDS in a spec

The idea is that when you submit a pants command, it will emit an error for ALL erroneous build files in a spec.

Note: if multiple specs are given on the command line, if a given one has an error, then only the first's will be printed, then it will error out.

I handled the errors as best as I could, becasue we need to know the flag before a lot of the options parsing happens.

Testing Done:
I tested manually and it works. I need to update the unit tests but wanted to validate my approach first.

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

src/python/pants/base/cmd_line_spec_parser.py

@@ -50,7 +50,7 @@ def __init__(self, root_dir, address_mapper):
     self._root_dir = os.path.realpath(root_dir)
     self._address_mapper = address_mapper
 
-  def parse_addresses(self, specs):
+  def parse_addresses(self, specs, fail_fast=False):
     """Process a list of command line specs and perform expansion.  This method can expand a list
     of command line specs, some of which may be subtracted from the  return value if they include
     the prefix '^'
@@ -62,18 +62,17 @@ def parse_addresses(self, specs):
 
     addresses = OrderedSet()
     addresses_to_remove = set()
-
     for spec in specs:
       if spec.startswith('^'):
-        for address in self._parse_spec(spec.lstrip('^')):
+        for address in self._parse_spec(spec.lstrip('^'), fail_fast):
           addresses_to_remove.add(address)
       else:
-        for address in self._parse_spec(spec):
+        for address in self._parse_spec(spec, fail_fast):
           addresses.add(address)
     for result in addresses - addresses_to_remove:
       yield result
 
-  def _parse_spec(self, spec):
+  def _parse_spec(self, spec, fail_fast=False):
     def normalize_spec_path(path):
       is_abs = not path.startswith('//') and os.path.isabs(path)
       if is_abs:
@@ -91,6 +90,8 @@ def normalize_spec_path(path):
         normalized = ''
       return normalized
 
+    errored_out = []
+
     if spec.endswith('::'):
       addresses = set()
       spec_path = spec[:-len('::')]
@@ -99,11 +100,23 @@ def normalize_spec_path(path):
         raise self.BadSpecError('Can only recursive glob directories and {0} is not a valid dir'
                                 .format(spec_dir))
       try:
-        for build_file in BuildFile.scan_buildfiles(self._root_dir, spec_dir):
-          addresses.update(self._address_mapper.addresses_in_spec_path(build_file.spec_path))
-        return addresses
+        build_files = BuildFile.scan_buildfiles(self._root_dir, spec_dir)
       except (BuildFile.BuildFileError, AddressLookupError) as e:
         raise self.BadSpecError(e)
+
+      for build_file in build_files:
+        try:
+          addresses.update(self._address_mapper.addresses_in_spec_path(build_file.spec_path))
+        except (BuildFile.BuildFileError, AddressLookupError) as e:
+          if fail_fast:
+            raise self.BadSpecError(e)
+          errored_out.append('Exception message: {0}'.format(e.message))
+
+      if errored_out:
+        error_msg = '\n'.join(errored_out + ["Invalid BUILD files for [{0}]".format(spec)])
+        raise self.BadSpecError(error_msg)
+      return addresses
+
     elif spec.endswith(':'):
       spec_path = spec[:-len(':')]
       spec_dir = normalize_spec_path(spec_path)

src/python/pants/commands/goal_runner.py

@@ -56,6 +56,7 @@ def parse_args(args):
     goals = OrderedSet()
     specs = OrderedSet()
     explicit_multi = False
+    fail_fast = False
     logger = logging.getLogger(__name__)
     has_double_dash = u'--' in args
     goal_names = [goal.name for goal in Goal.all()]
@@ -91,11 +92,13 @@ def is_spec(spec):
         explicit_multi = True
         del args[i]
         break
+      elif '--fail-fast' == arg.lower():
+        fail_fast = True
 
     if explicit_multi:
       specs.update(arg for arg in args[len(goals):] if not arg.startswith('-'))
 
-    return goals, specs
+    return goals, specs, fail_fast
 
   # TODO(John Sirois): revisit wholesale locking when we move py support into pants new
   @classmethod
@@ -165,7 +168,7 @@ def setup_parser(self, parser, args):
     show_help = len(help_flags.intersection(args)) > 0
     non_help_args = filter(lambda f: f not in help_flags, args)
 
-    goals, specs = GoalRunner.parse_args(non_help_args)
+    goals, specs, fail_fast = GoalRunner.parse_args(non_help_args)
     if show_help:
       print_help(goals)
       sys.exit(0)
@@ -177,7 +180,7 @@ def setup_parser(self, parser, args):
       spec_parser = CmdLineSpecParser(self.root_dir, self.address_mapper)
       with self.run_tracker.new_workunit(name='parse', labels=[WorkUnit.SETUP]):
         for spec in specs:
-          for address in spec_parser.parse_addresses(spec):
+          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))
     self.goals = [Goal.by_name(goal) for goal in goals]

src/python/pants/goal/option_helpers.py

@@ -54,6 +54,10 @@ def _set_bool(option, opt_str, value, parser):
          help='Write build artifacts to cache, if possible.'),
   Option('--print-exception-stacktrace', dest='print_exception_stacktrace', action='store_true',
          default=False, help='Print to console the full exception stack trace if encountered.'),
+  Option('--fail-fast', dest='fail_fast', action='store_true',
+         default=False, help="When parsing specs, will stop on the first erronous BUILD file "
+                             "encountered. Otherwise, will parse all builds in a spec and "
+                             "then throw an Exception."),
 ]
 
 

testprojects/README.md

@@ -4,8 +4,8 @@ Here you will find projects that were created to exercise Pants functionality.
 These targets are referenced from pants test code in /tests/ and may reference
 code under /examples.
 
-It is possible that some of these projects may be intentionally  broken to
-create negative test cases or to demonstrate bugs in Pants.
+None of these projects should be broken. Broken BUILD files should be tested in
+unit tests.
 
 Ideally, none of these projects would be referenced from documentation.
 Example code intended for Pants users should be placed under /examples instead.

tests/python/pants_test/base/test_cmd_line_spec_parser.py

@@ -131,3 +131,32 @@ def sort(addresses):
 
     self.assertEqual(sort(SyntheticAddress.parse(addr) for addr in expected),
                      sort(self.spec_parser.parse_addresses(cmdline_spec_list)))
+
+class CmdLineSpecParserBadBuildTest(BaseTest):
+  def setUp(self):
+    super(CmdLineSpecParserBadBuildTest, self).setUp()
+
+    self.add_to_build_file('bad/a', 'a_is_bad')
+    self.add_to_build_file('bad/b', 'b_is_bad')
+
+    self.spec_parser = CmdLineSpecParser(self.build_root, self.address_mapper)
+
+    self.NO_FAIL_FAST_RE = """^Exception message: name 'a_is_bad' is not defined
+ while executing BUILD file (/[^/]+)*/bad/a/BUILD
+ Loading addresses from 'bad/a' failed\.
+Exception message: name 'b_is_bad' is not defined
+ while executing BUILD file (/[^/]+)*T/bad/b/BUILD
+ Loading addresses from 'bad/b' failed\.
+Invalid BUILD files for \[::\]$"""
+
+    self.FAIL_FAST_RE = """^name 'a_is_bad' is not defined
+ while executing BUILD file (/[^/]+)*/bad/a/BUILD
+ Loading addresses from 'bad/a' failed.$"""
+
+  def test_bad_build_files(self):
+    with self.assertRaisesRegexp(self.spec_parser.BadSpecError, self.NO_FAIL_FAST_RE):
+      list(self.spec_parser.parse_addresses('::'))
+
+  def test_bad_build_files_fail_fast(self):
+    with self.assertRaisesRegexp(self.spec_parser.BadSpecError, self.FAIL_FAST_RE):
+      list(self.spec_parser.parse_addresses('::', True))

tests/python/pants_test/commands/test_goal.py

@@ -24,9 +24,9 @@ def tearDown(self):
     super(GoalTest, self).tearDown()
     Goal.clear()
 
-  def assert_result(self, goals, specs, args):
-    g, s = GoalRunner.parse_args(args)
-    self.assertEquals((goals, specs), (list(g), list(s)))
+  def assert_result(self, goals, specs, args, fail_fast=False):
+    g, s, ff = GoalRunner.parse_args(args)
+    self.assertEquals((goals, specs, fail_fast), (list(g), list(s), ff))
 
   def test_top_level_dir(self):
     self.create_dir('topleveldir')