[engine] Improve error messages for missing/empty dirs (pantsbuild#4517)

### Problem
When the engine has an issue finding BUILD files to fulfill a request for targets, it should have a good message even if the requested spec has no associated BUILD files.

### Solution
This patch adds another error raising path to addresses_from_address_families so that missing directories and empty directories have improved error messages.
It also
* adds an error case for address mapper for unrelated Throws so that their associated exception's text will be reported if one is encountered instead of a missing field error.
* removes a needless call to scheduler.root_entries, which calls into the native engine.
### Result
The cases in pantsbuild#3912 for missing directories or directories without BUILD files have ok errors.

src/python/pants/engine/build_files.py

@@ -234,7 +234,15 @@ def _hydrate(item_type, spec_path, **kwargs):
       [SelectDependencies(AddressFamily, BuildDirs, field_types=(Dir,)),
        Select(_SPECS_CONSTRAINT)])
 def addresses_from_address_families(address_families, spec):
-  """Given a list of AddressFamilies and a Spec, return matching Addresses."""
+  """Given a list of AddressFamilies and a Spec, return matching Addresses.
+
+  Raises a ResolveError if:
+     - there were no matching AddressFamilies, or
+     - the Spec matches no addresses for SingleAddresses.
+  """
+  if not address_families:
+    raise ResolveError('Path "{}" contains no BUILD files.'.format(spec.directory))
+
   if type(spec) in (DescendantAddresses, SiblingAddresses, AscendantAddresses):
     addresses = tuple(a for af in address_families for a in af.addressables.keys())
   elif type(spec) is SingleAddress:

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

@@ -73,17 +73,24 @@ def _internal_scan_specs(self, specs, fail_fast=True, missing_is_fatal=True):
     result = self._engine.execute(request)
     if result.error:
       raise self.BuildFileScanError(str(result.error))
-    root_entries = self._scheduler.root_entries(request)
+    root_entries = result.root_products
 
     addresses = set()
     for (spec, _), state in root_entries.items():
-      if missing_is_fatal:
-        if isinstance(state, Throw) and isinstance(state.exc, ResolveError):
-          raise self.BuildFileScanError(
-            'Spec `{}` does not match any targets.\n{}'.format(spec.to_spec_string(), str(state.exc)))
-        elif not state.value.dependencies:
-          raise self.BuildFileScanError(
-            'Spec `{}` does not match any targets.'.format(spec.to_spec_string()))
+      if isinstance(state, Throw):
+        if isinstance(state.exc, ResolveError):
+          if missing_is_fatal:
+            raise self.BuildFileScanError(
+              'Spec `{}` does not match any targets.\n{}'.format(spec.to_spec_string(), str(state.exc)))
+          else:
+            # NB: ignore Throws containing ResolveErrors because they are due to missing targets / files
+            continue
+        else:
+          raise self.BuildFileScanError(str(state.exc))
+      elif missing_is_fatal and not state.value.dependencies:
+        raise self.BuildFileScanError(
+          'Spec `{}` does not match any targets.'.format(spec.to_spec_string()))
+
       addresses.update(state.value.dependencies)
     return addresses
 

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

@@ -14,7 +14,9 @@
 from pants.bin.engine_initializer import EngineInitializer
 from pants.build_graph.address import Address, BuildFileAddress
 from pants.build_graph.address_mapper import AddressMapper
+from pants.engine.engine import Engine
 from pants.engine.legacy.address_mapper import LegacyAddressMapper
+from pants.engine.nodes import Throw
 from pants.util.contextutil import temporary_dir
 from pants.util.dirutil import safe_file_dump, safe_mkdir
 from pants_test.engine.util import init_native
@@ -182,3 +184,20 @@ def test_scan_addresses_bad_dir(self):
       mapper = self.create_address_mapper(build_root)
       addresses = mapper.scan_addresses(os.path.join(build_root, 'foo'))
       self.assertEqual(addresses, set())
+
+  def test_other_throw_is_fail(self):
+    # scan_addresses() should raise an error if the scheduler returns an error it can't ignore.
+    class ThrowReturningScheduler(object):
+      def execution_request(self, *args):
+        pass
+
+    class ThrowResultingEngine(object):
+      def execute(self, *args):
+        return Engine.Result(None, {('some-thing', None): Throw(Exception('just an exception'))})
+
+    with temporary_dir() as build_root:
+      mapper = LegacyAddressMapper(ThrowReturningScheduler(), ThrowResultingEngine(), build_root)
+
+      with self.assertRaises(LegacyAddressMapper.BuildFileScanError) as cm:
+        mapper.scan_addresses(os.path.join(build_root, 'foo'))
+      self.assertIn('just an exception', str(cm.exception))

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

@@ -54,31 +54,63 @@ def _make_setup_args(self, specs):
     return options
 
   @contextmanager
-  def open_scheduler(self, specs, symbol_table_cls=None):
+  def graph_helper(self, symbol_table_cls=None):
     with temporary_dir() as work_dir:
       path_ignore_patterns = ['.*']
-      target_roots = TargetRoots.create(options=self._make_setup_args(specs))
       graph_helper = EngineInitializer.setup_legacy_graph(path_ignore_patterns,
                                                           work_dir,
                                                           symbol_table_cls=symbol_table_cls,
                                                           native=self._native)
-      graph = graph_helper.create_build_graph(target_roots)[0]
+      yield graph_helper
+
+  @contextmanager
+  def open_scheduler(self, specs, symbol_table_cls=None):
+    with self.graph_helper(symbol_table_cls) as graph_helper:
+      graph, target_roots = self.create_graph_from_specs(graph_helper, specs)
       addresses = tuple(graph.inject_specs_closure(target_roots.as_specs()))
       yield graph, addresses, graph_helper.scheduler
 
+  def create_graph_from_specs(self, graph_helper, specs):
+    target_roots = self.create_target_roots(specs)
+    graph = graph_helper.create_build_graph(target_roots)[0]
+    return graph, target_roots
+
+  def create_target_roots(self, specs):
+    return TargetRoots.create(options=self._make_setup_args(specs))
+
 
 class GraphTargetScanFailureTests(GraphTestBase):
 
   def test_with_missing_target_in_existing_build_file(self):
     with self.assertRaises(AddressLookupError) as cm:
-      with self.open_scheduler(['3rdparty/python:rutabaga']) as (_, _, scheduler):
+      with self.graph_helper() as graph_helper:
+        self.create_graph_from_specs(graph_helper, ['3rdparty/python:rutabaga'])
         self.fail('Expected an exception.')
 
     self.assertIn('"rutabaga" was not found in namespace "3rdparty/python". Did you mean one of:\n'
                   '  :psutil\n'
                   '  :isort',
                   str(cm.exception))
 
+  def test_with_missing_directory_fails(self):
+    with self.assertRaises(AddressLookupError) as cm:
+      with self.graph_helper() as graph_helper:
+        self.create_graph_from_specs(graph_helper, ['no-such-path:'])
+        self.fail('Expected an exception.')
+
+    self.assertIn('Path "no-such-path" contains no BUILD files',
+                  str(cm.exception))
+
+  def test_with_existing_directory_with_no_build_files_fails(self):
+    with self.assertRaises(AddressLookupError) as cm:
+      with self.graph_helper() as graph_helper:
+        self.create_graph_from_specs(graph_helper, ['build-support/bin::'])
+        self.fail('Expected an exception.')
+
+    self.assertIn('Path "build-support/bin" contains no BUILD files',
+                  str(cm.exception))
+
+
 
 class GraphInvalidationTest(GraphTestBase):
 

tests/python/pants_test/engine/test_mapper.py

@@ -259,21 +259,25 @@ def resolve_multi(self, spec):
 
   def test_no_address_no_family(self):
     spec = SingleAddress('a/c', 'c')
+
     # Does not exist.
-    self.assertEqual(0, len(self.resolve(spec)))
+    with self.assertRaises(Exception):
+      self.resolve(spec)
 
-    # Exists on disk, but not yet in memory.
-    directory = 'a/c'
-    build_file = os.path.join(self.build_root, directory, 'c.BUILD.json')
+    build_file = os.path.join(self.build_root, 'a/c', 'c.BUILD.json')
     with safe_open(build_file, 'w') as fp:
       fp.write('{"type_alias": "struct", "name": "c"}')
-    self.assertEqual(0, len(self.resolve(spec)))
+
+    # Exists on disk, but not yet in memory.
+    with self.assertRaises(Exception):
+      self.resolve(spec)
+
+    self.scheduler.invalidate_files(['a/c'])
 
     # Success.
-    self.scheduler.invalidate_files([directory])
     resolved = self.resolve(spec)
     self.assertEqual(1, len(resolved))
-    self.assertEqual(Struct(name='c', type_alias='struct'), resolved[0].struct)
+    self.assertEqual([Struct(name='c', type_alias='struct')], [r.struct for r in resolved])
 
   def test_resolve(self):
     resolved = self.resolve(SingleAddress('a/b', 'b'))