Make LegacyAddressMapper v2 engine backed

When testing v2 engine in OSS pants master, I found that LegacyAddressMapper is missing a scan_build_files method which is used in v1 BuildFileAddressMapper, thus causing some test failures. In addition, LegacyAddressMapper internally relies on LegacyBuildGraph to do things. This dependency is unnecessary.

This change adds the missing method to LegacyAddressMapper, and implements all methods of LAM using v2 engine directly.

A summary of this review:
1. Implement "scan_build_files" on LegacyAddressMapper. As part of the effort, add a new selection_request method in LocalScheduler class.
2. Modify all other methods of LegacyAddressMapper to use v2 engine directly, and remove the dependency on LegacyBuildGraph.
3. Remove "resolve_spec" method in AddressMapper base class, replace with a new method called "check_valid_spec", which will check if input spec is valid or not. This suffices the only use case of resolve_spec in master without returning an unused TargetAddressable.
4. Add a new "resolve_address" method in BuildGraph, which will replace the usage of "address_mapper.resolve" in master. This is done to unify the behavior between v1 and v2 engine, since in v2 engine, only LegacyBuildGraph has instantiated target objects (ProductGraph only has TargetAdaptor).
5. Modify test cases for LegacyAddressMapper
6. Add some missing dependencies in several BUILD files.
7. Fix a bug in go_buildgen()

Testing Done:
https://travis-ci.org/pantsbuild/pants/builds/163454699

Bugs closed: 3871, 3875

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

contrib/go/src/python/pants/contrib/go/tasks/go_buildgen.py

@@ -249,7 +249,7 @@ def _materialize(self, generation_result):
     def gather_go_buildfiles(rel_path):
       address_mapper = self.context.address_mapper
       for build_file in address_mapper.scan_build_files(base_path=rel_path):
-        existing_go_buildfiles.add(build_file.relpath)
+        existing_go_buildfiles.add(build_file)
 
     gather_go_buildfiles(generation_result.local_root)
     if remote and generation_result.remote_root != generation_result.local_root:
@@ -275,9 +275,9 @@ def gather_go_buildfiles(rel_path):
       for existing_go_buildfile in existing_go_buildfiles:
         spec_path = os.path.dirname(existing_go_buildfile)
         for address in self.context.address_mapper.addresses_in_spec_path(spec_path):
-          target = self.context.address_mapper.resolve(address)
+          target = self.context.build_graph.resolve_address(address)
           if isinstance(target, GoLocalSource):
-            os.unlink(existing_go_buildfile)
+            os.unlink(os.path.join(get_buildroot(), existing_go_buildfile))
             deleted.append(existing_go_buildfile)
       if deleted:
         self.context.log.info('Deleted the following obsolete BUILD files:\n\t{}'

src/python/pants/backend/jvm/tasks/bootstrap_jvm_tools.py

@@ -122,7 +122,7 @@ def _alternate_target_roots(cls, options, address_mapper, build_graph):
       if dep_address not in processed:
         processed.add(dep_address)
         try:
-          if build_graph.contains_address(dep_address) or address_mapper.resolve(dep_address):
+          if build_graph.resolve_address(dep_address):
             # The user has defined a tool classpath override - we let that stand.
             continue
         except AddressLookupError as e:

src/python/pants/bin/engine_initializer.py

@@ -89,7 +89,7 @@ def create_build_graph(self, target_roots, build_root=None):
         pass
 
     logger.debug('engine cache stats: %s', self.engine.cache_stats())
-    address_mapper = LegacyAddressMapper(graph, build_root or get_buildroot())
+    address_mapper = LegacyAddressMapper(self.scheduler, self.engine, build_root or get_buildroot())
     logger.debug('address_mapper is: %s', address_mapper)
     return graph, address_mapper, target_roots.as_string_specs()
 
@@ -99,13 +99,15 @@ class EngineInitializer(object):
 
   @staticmethod
   def setup_legacy_graph(pants_ignore_patterns,
+                         build_root=None,
                          symbol_table_cls=None,
                          build_ignore_patterns=None,
                          exclude_target_regexps=None):
     """Construct and return the components necessary for LegacyBuildGraph construction.
 
     :param list pants_ignore_patterns: A list of path ignore patterns for FileSystemProjectTree,
                                        usually taken from the '--pants-ignore' global option.
+    :param str build_root: A path to be used as the build root. If None, then default is used.
     :param SymbolTable symbol_table_cls: A SymbolTable class to use for build file parsing, or
                                          None to use the default.
     :param list build_ignore_patterns: A list of paths ignore patterns used when searching for BUILD
@@ -114,11 +116,12 @@ def setup_legacy_graph(pants_ignore_patterns,
     :returns: A tuple of (scheduler, engine, symbol_table_cls, build_graph_cls).
     """
 
-    build_root = get_buildroot()
+    build_root = build_root or get_buildroot()
     scm = get_scm()
-    project_tree = FileSystemProjectTree(build_root, pants_ignore_patterns)
     symbol_table_cls = symbol_table_cls or LegacySymbolTable
 
+    project_tree = FileSystemProjectTree(build_root, pants_ignore_patterns)
+
     # Register "literal" subjects required for these tasks.
     # TODO: Replace with `Subsystems`.
     address_mapper = AddressMapper(symbol_table_cls=symbol_table_cls,

src/python/pants/bin/goal_runner.py

@@ -16,7 +16,6 @@
 from pants.bin.engine_initializer import EngineInitializer
 from pants.bin.repro import Reproducer
 from pants.bin.target_roots import TargetRoots
-from pants.build_graph.address_lookup_error import AddressLookupError
 from pants.build_graph.build_file_address_mapper import BuildFileAddressMapper
 from pants.build_graph.build_file_parser import BuildFileParser
 from pants.build_graph.mutable_build_graph import MutableBuildGraph
@@ -135,13 +134,11 @@ def _init_graph(self, use_engine, pants_ignore_patterns, build_ignore_patterns,
 
   def _expand_goals(self, goals):
     """Check and populate the requested goals for a given run."""
+    spec_parser = CmdLineSpecParser(self._root_dir)
     for goal in goals:
-      try:
-        self._address_mapper.resolve_spec(goal)
+      if self._address_mapper.is_valid_single_address(spec_parser.parse_spec(goal)):
         logger.warning("Command-line argument '{0}' is ambiguous and was assumed to be "
                        "a goal. If this is incorrect, disambiguate it with ./{0}.".format(goal))
-      except AddressLookupError:
-        pass
 
     if self._help_request:
       help_printer = HelpPrinter(self._options)

src/python/pants/build_graph/BUILD

@@ -18,6 +18,7 @@ python_library(
     'src/python/pants/base:parse_context',
     'src/python/pants/base:payload',
     'src/python/pants/base:payload_field',
+    'src/python/pants/base:specs',
     'src/python/pants/base:validation',
     'src/python/pants/option',
     'src/python/pants/source',

src/python/pants/build_graph/address_mapper.py

@@ -6,10 +6,9 @@
                         unicode_literals, with_statement)
 
 import logging
-import os
 from abc import abstractmethod
 
-from pants.build_graph.address import Address
+from pants.base.specs import SingleAddress
 from pants.build_graph.address_lookup_error import AddressLookupError
 from pants.util.meta import AbstractClass
 
@@ -38,6 +37,14 @@ class BuildFileScanError(AddressLookupError):
   class InvalidRootError(BuildFileScanError):
     """Indicates an invalid scan root was supplied."""
 
+  @abstractmethod
+  def scan_build_files(self, base_path):
+    """Recursively gather all BUILD files under base_path.
+
+    :param base_path: The path to start scanning, relative to build_root.
+    :return: OrderedSet of BUILD file paths relative to build_root.
+    """
+
   @abstractmethod
   def addresses_in_spec_path(self, spec_path):
     """Returns the addresses of targets defined at spec_path.
@@ -49,31 +56,24 @@ def addresses_in_spec_path(self, spec_path):
 
   @abstractmethod
   def scan_specs(self, specs, fail_fast=True):
-    """Execute a collection of `specs.Spec` objects and return an ordered set of Addresses."""
+    """Execute a collection of `specs.Spec` objects and return a set of Addresses."""
 
-  @abstractmethod
-  def resolve(self, address):
-    """Maps an address in the virtual address space to an object.
+  def is_valid_single_address(self, single_address):
+    """Check if a potentially ambiguous single address spec really exists.
 
-    :param Address address: the address to lookup in a BUILD file
-    :raises AddressLookupError: if the path to the address is not found.
-    :returns: A tuple of the natively mapped BuildFileAddress and the Addressable it points to.
+    :param single_address: A SingleAddress spec.
+    :return: True if given spec exists, False otherwise.
     """
+    if not isinstance(single_address, SingleAddress):
+      raise TypeError(
+        'Parameter "{}" is of type {}, expecting type {}.'.format(
+          single_address, type(single_address), SingleAddress))
 
-  def resolve_spec(self, spec):
-    """Converts a spec to an address and maps it to an addressable using `resolve`.
-
-    :param spec: A string representing an address.
-    :raises InvalidAddressError: if there is a problem parsing the spec.
-    :raises AddressLookupError: if the address is not found.
-    :return: An addressable. Usually a target.
-    """
     try:
-      address = Address.parse(spec)
-    except ValueError as e:
-      raise self.InvalidAddressError(e)
-    _, addressable = self.resolve(address)
-    return addressable
+      self.scan_specs([single_address])
+      return True
+    except AddressLookupError:
+      return False
 
   @abstractmethod
   def scan_addresses(self, root=None):
@@ -85,42 +85,12 @@ def scan_addresses(self, root=None):
     :raises AddressLookupError: if there is a problem parsing a BUILD file
     """
 
-  @abstractmethod
-  def is_declaring_file(self, address, file_path):
+  @staticmethod
+  def is_declaring_file(address, file_path):
     """Returns True if the address could be declared in the file at file_path.
 
     :param Address address: The address to check for.
     :param string file_path: The path of the file that may contain a declaration for the address.
     """
-
-  def _raise_incorrect_address_error(self, spec_path, wrong_target_name, addresses):
-    """Search through the list of targets and return those which originate from the same folder
-    which wrong_target_name resides in.
-
-    :raises: A helpful error message listing possible correct target addresses.
-    """
-    was_not_found_message = '{target_name} was not found in BUILD files from {spec_path}'.format(
-      target_name=wrong_target_name, spec_path=spec_path)
-
-    if not addresses:
-      raise self.EmptyBuildFileError(
-        '{was_not_found_message}, because that directory contains no BUILD files defining addressable entities.'
-          .format(was_not_found_message=was_not_found_message))
-    # Print BUILD file extensions if there's more than one BUILD file with targets only.
-    if (any(not hasattr(address, 'build_file') for address in addresses) or
-        len(set([address.build_file for address in addresses])) == 1):
-      specs = [':{}'.format(address.target_name) for address in addresses]
-    else:
-      specs = [':{} (from {})'.format(address.target_name, os.path.basename(address.build_file.relpath))
-               for address in addresses]
-
-    # Might be neat to sort by edit distance or something, but for now alphabetical is fine.
-    specs = [''.join(pair) for pair in sorted(specs)]
-
-    # Give different error messages depending on whether BUILD file was empty.
-    one_of = ' one of' if len(specs) > 1 else ''  # Handle plurality, just for UX.
-    raise self.AddressNotInBuildFile(
-      '{was_not_found_message}. Perhaps you '
-      'meant{one_of}: \n  {specs}'.format(was_not_found_message=was_not_found_message,
-                                          one_of=one_of,
-                                          specs='\n  '.join(specs)))
+    # Subclass should implement this method.
+    raise NotImplementedError()

src/python/pants/build_graph/build_file_address_mapper.py

@@ -122,8 +122,9 @@ def spec_to_address(self, spec, relative_to=''):
                                            .format(message=e, spec=spec))
 
   def scan_build_files(self, base_path):
-    return BuildFile.scan_build_files(self._project_tree, base_path,
-                                      build_ignore_patterns=self._build_ignore_patterns)
+    build_files = BuildFile.scan_build_files(self._project_tree, base_path,
+                                             build_ignore_patterns=self._build_ignore_patterns)
+    return OrderedSet(bf.relpath for bf in build_files)
 
   def specs_to_addresses(self, specs, relative_to=''):
     """The equivalent of `spec_to_address` for a group of specs all relative to the same path.
@@ -165,7 +166,7 @@ def scan_addresses(self, root=None):
     return addresses
 
   def scan_specs(self, specs, fail_fast=True):
-    """Execute a collection of `specs.Spec` objects and return an ordered set of Addresses."""
+    """Execute a collection of `specs.Spec` objects and return a set of Addresses."""
     excluded_target_map = defaultdict(set)  # pattern -> targets (for debugging)
 
     def exclude_spec(spec):
@@ -179,9 +180,10 @@ def exclude_spec(spec):
     def exclude_address(address):
       return exclude_spec(address.spec)
 
+    #TODO: Investigate why using set will break ci. May help migration to v2 engine.
     addresses = OrderedSet()
     for spec in specs:
-      for address in self._scan_spec(spec, fail_fast, exclude_spec):
+      for address in self._scan_spec(spec, fail_fast):
         if not exclude_address(address):
           addresses.add(address)
 
@@ -207,7 +209,7 @@ def exclude_address(address):
   def is_declaring_file(address, file_path):
     return address.build_file.relpath == file_path
 
-  def _scan_spec(self, spec, fail_fast, exclude_spec):
+  def _scan_spec(self, spec, fail_fast):
     """Scans the given address spec."""
 
     errored_out = []
@@ -221,7 +223,7 @@ def _scan_spec(self, spec, fail_fast, exclude_spec):
 
       for build_file in build_files:
         try:
-          addresses.update(self.addresses_in_spec_path(build_file.spec_path))
+          addresses.update(self.addresses_in_spec_path(os.path.dirname(build_file)))
         except (BuildFile.BuildFileError, AddressLookupError) as e:
           if fail_fast:
             raise AddressLookupError(e)
@@ -238,3 +240,35 @@ def _scan_spec(self, spec, fail_fast, exclude_spec):
       return {self.spec_to_address(spec.to_spec_string())}
     else:
       raise ValueError('Unsupported Spec type: {}'.format(spec))
+
+  def _raise_incorrect_address_error(self, spec_path, wrong_target_name, addresses):
+    """Search through the list of targets and return those which originate from the same folder
+    which wrong_target_name resides in.
+
+    :raises: A helpful error message listing possible correct target addresses.
+    """
+    was_not_found_message = '{target_name} was not found in BUILD files from {spec_path}'.format(
+      target_name=wrong_target_name, spec_path=spec_path)
+
+    if not addresses:
+      raise self.EmptyBuildFileError(
+        '{was_not_found_message}, because that directory contains no BUILD files defining addressable entities.'
+          .format(was_not_found_message=was_not_found_message))
+    # Print BUILD file extensions if there's more than one BUILD file with targets only.
+    if (any(not hasattr(address, 'build_file') for address in addresses) or
+        len(set(address.build_file for address in addresses)) == 1):
+      specs = [':{}'.format(address.target_name) for address in addresses]
+    else:
+      specs = [':{} (from {})'.format(address.target_name, os.path.basename(address.build_file.relpath))
+               for address in addresses]
+
+    # Might be neat to sort by edit distance or something, but for now alphabetical is fine.
+    specs.sort()
+
+    # Give different error messages depending on whether BUILD file was empty.
+    one_of = ' one of' if len(specs) > 1 else ''  # Handle plurality, just for UX.
+    raise self.AddressNotInBuildFile(
+      '{was_not_found_message}. Perhaps you '
+      'meant{one_of}: \n  {specs}'.format(was_not_found_message=was_not_found_message,
+                                          one_of=one_of,
+                                          specs='\n  '.join(specs)))

src/python/pants/build_graph/build_graph.py

@@ -511,6 +511,15 @@ def resolve(self, spec):
     self.inject_address_closure(address)
     return self.transitive_subgraph_of_addresses([address])
 
+  @abstractmethod
+  def resolve_address(self, address):
+    """Maps an address in the virtual address space to an object.
+
+    :param Address address: the address to lookup in a BUILD file
+    :raises AddressLookupError: if the path to the address is not found.
+    :returns: The Addressable which address points to.
+    """
+
 
 class CycleException(Exception):
   """Thrown when a circular dependency is detected.

src/python/pants/build_graph/mutable_build_graph.py

@@ -123,3 +123,10 @@ def _target_addressable_to_target(self, address, addressable):
                                name=addressable.addressed_name,
                                address=address))
       raise
+
+  def resolve_address(self, address):
+    if self.contains_address(address):
+      return self.get_target(address)
+    else:
+      _, addressable = self._address_mapper.resolve(address)
+      return addressable

src/python/pants/engine/engine.py

@@ -129,6 +129,7 @@ def product_request(self, product, subjects):
       State.raise_unrecognized(unknown_state_types)
 
     # Throw handling.
+    #TODO: See https://github.com/pantsbuild/pants/issues/3912
     throw_roots = tuple(root for root, state in result_items if type(state) is Throw)
     if throw_roots:
       cumulative_trace = '\n'.join(

src/python/pants/engine/legacy/BUILD

@@ -5,9 +5,14 @@ python_library(
   name='address_mapper',
   sources=['address_mapper.py'],
   dependencies=[
-    'src/python/pants/base:exceptions',
+    'src/python/pants/base:build_file',
     'src/python/pants/base:specs',
     'src/python/pants/build_graph',
+    'src/python/pants/engine',
+    'src/python/pants/engine:fs',
+    'src/python/pants/engine:graph',
+    'src/python/pants/engine:selectors',
+    'src/python/pants/util:dirutil',
   ],
 )
 
@@ -18,8 +23,9 @@ python_library(
     ':structs',
     'src/python/pants/base:build_file_target_factory',
     'src/python/pants/base:parse_context',
-    'src/python/pants/engine:fs',
+    'src/python/pants/engine:objects',
     'src/python/pants/engine:parser',
+    'src/python/pants/util:memo',
   ],
 )
 
@@ -43,14 +49,13 @@ python_library(
   sources=['graph.py'],
   dependencies=[
     '3rdparty/python/twitter/commons:twitter.common.collections',
-    ':parser',
     ':structs',
     'src/python/pants/backend/jvm/targets:jvm',
-    'src/python/pants/base:parse_context',
     'src/python/pants/base:exceptions',
+    'src/python/pants/base:parse_context',
     'src/python/pants/build_graph',
-    'src/python/pants/engine:graph',
-    'src/python/pants/engine:parser',
+    'src/python/pants/engine:fs',
+    'src/python/pants/engine:nodes',
     'src/python/pants/engine:selectors',
     'src/python/pants/source',
     'src/python/pants/util:dirutil',