unify precedence logic for options which may be overridden on targets (…

…pantsbuild#7594)

### Problem

Addresses the first item of pantsbuild#7183. `MirroredTargetOptionMixin` was created in `backend/native/` to make it easier to support pants options with equivalents on a target payload entry, for example. This generalizes that and uses it to simplify some options in tasks in the jvm backend.

### Solution

*commits are independently reviewable*
- move `MirroredTargetOptionMixin` to `build_graph/` and substantially refactor it.
- make `ZincLanguageMixin` a `MirroredTargetOptionMixin` (this allows some of the `DependencyContext` logic to be simplified).
- remove the `--default-workflow` option from `RscCompile`, renaming it to `--workflow` and using the shared logic from `MirroredTargetOptionMixin`.

### Result

The path to create options which can be overridden by targets and to access them from a task is more clear. The RscCompile task now uses the shared option precedence logic of MirroredTargetOptionMixin instead of reinventing it.

src/python/pants/backend/jvm/subsystems/BUILD

@@ -149,6 +149,7 @@ python_library(
   sources = ['zinc_language_mixin.py'],
   dependencies = [
     '3rdparty/python:future',
+    'src/python/pants/build_graph',
     'src/python/pants/subsystem',
   ]
 )

src/python/pants/backend/jvm/subsystems/dependency_context.py

@@ -50,7 +50,7 @@ def all_dependencies(self, target):
   def create_fingerprint_strategy(self, classpath_products):
     return ResolvedJarAwareFingerprintStrategy(classpath_products, self)
 
-  def defaulted_property(self, target, selector):
+  def defaulted_property(self, target, option_name):
     """Computes a language property setting for the given JvmTarget.
 
     :param selector A function that takes a target or platform and returns the boolean value of the
@@ -60,19 +60,17 @@ def defaulted_property(self, target, selector):
     If the target does not override the language property, returns true iff the property
     is true for any of the matched languages for the target.
     """
-    target_property_selected = selector(target)
-    if target_property_selected is not None:
-      return target_property_selected
-
-    prop = None
     if target.has_sources('.java'):
-      prop = prop or selector(Java.global_instance())
-    if target.has_sources('.scala'):
-      prop = prop or selector(ScalaPlatform.global_instance())
-    return prop
+      matching_subsystem = Java.global_instance()
+    elif target.has_sources('.scala'):
+      matching_subsystem = ScalaPlatform.global_instance()
+    else:
+      return getattr(target, option_name)
+
+    return matching_subsystem.get_scalar_mirrored_target_option(option_name, target)
 
   def dependencies_respecting_strict_deps(self, target):
-    if self.defaulted_property(target, lambda x: x.strict_deps):
+    if self.defaulted_property(target, 'strict_deps'):
       dependencies = target.strict_dependencies(self)
     else:
       dependencies = self.all_dependencies(target)
@@ -107,7 +105,7 @@ def compute_fingerprint(self, target):
     return hasher.hexdigest() if PY3 else hasher.hexdigest().decode('utf-8')
 
   def direct(self, target):
-    return self._dep_context.defaulted_property(target, lambda x: x.strict_deps)
+    return self._dep_context.defaulted_property(target, 'strict_deps')
 
   def dependencies(self, target):
     if self.direct(target):

src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py

@@ -4,12 +4,18 @@
 
 from __future__ import absolute_import, division, print_function, unicode_literals
 
-from builtins import object
+from pants.build_graph.mirrored_target_option_mixin import MirroredTargetOptionMixin
 
 
-class ZincLanguageMixin(object):
+class ZincLanguageMixin(MirroredTargetOptionMixin):
   """A mixin for subsystems for languages compiled with Zinc."""
 
+  mirrored_target_option_actions = {
+    'strict_deps': lambda tgt: tgt.strict_deps,
+    'compiler_option_sets': lambda tgt: tgt.compiler_option_sets,
+    'zinc_file_manager': lambda tgt: tgt.zinc_file_manager,
+  }
+
   @classmethod
   def register_options(cls, register):
     super(ZincLanguageMixin, cls).register_options(register)

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

@@ -908,8 +908,8 @@ def _default_work_for_vts(self, vts, ctx, input_classpath_product_key, counter,
 
       dep_context = DependencyContext.global_instance()
       tgt, = vts.targets
-      compiler_option_sets = dep_context.defaulted_property(tgt, lambda x: x.compiler_option_sets)
-      zinc_file_manager = dep_context.defaulted_property(tgt, lambda x: x.zinc_file_manager)
+      compiler_option_sets = dep_context.defaulted_property(tgt, 'compiler_option_sets')
+      zinc_file_manager = dep_context.defaulted_property(tgt, 'zinc_file_manager')
       with Timer() as timer:
         directory_digest = self._compile_vts(vts,
                           ctx,

src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py

@@ -14,21 +14,21 @@
 
 from pants.backend.jvm.subsystems.dependency_context import DependencyContext  # noqa
 from pants.backend.jvm.subsystems.shader import Shader
-from pants.backend.jvm.targets.junit_tests import JUnitTests
 from pants.backend.jvm.targets.jvm_target import JvmTarget
-from pants.backend.jvm.targets.scala_library import ScalaLibrary
 from pants.backend.jvm.tasks.classpath_entry import ClasspathEntry
 from pants.backend.jvm.tasks.jvm_compile.compile_context import CompileContext
 from pants.backend.jvm.tasks.jvm_compile.execution_graph import Job
 from pants.backend.jvm.tasks.jvm_compile.zinc.zinc_compile import ZincCompile
 from pants.base.build_environment import get_buildroot
 from pants.base.exceptions import TaskError
 from pants.base.workunit import WorkUnitLabel
+from pants.build_graph.mirrored_target_option_mixin import MirroredTargetOptionMixin
 from pants.engine.fs import (EMPTY_DIRECTORY_DIGEST, Digest, DirectoryToMaterialize, PathGlobs,
                              PathGlobsAndRoot)
 from pants.engine.isolated_process import ExecuteProcessRequest, FallibleExecuteProcessResult
 from pants.java.jar.jar_dependency import JarDependency
 from pants.reporting.reporting_utils import items_to_report_element
+from pants.util.collections import assert_single_element
 from pants.util.contextutil import Timer
 from pants.util.dirutil import fast_relpath, fast_relpath_optional, safe_mkdir
 from pants.util.memo import memoized_method, memoized_property
@@ -89,50 +89,6 @@ def add_for_target(self, *args, **kwargs):
       product.add_for_target(*args, **kwargs)
 
 
-class _CompileWorkflowChoice(enum(['zinc-only', 'guess'])):
-  """Enum covering the values for the default workflow option.
-
-  guess - Try to classify compile targets based on whether they are Scala/Java or test targets.
-  zinc-only - Always use zinc."""
-
-
-class _JvmCompileWorkflowType(enum(['zinc-only', 'rsc-then-zinc'])):
-  """Target classifications used to correctly schedule Zinc and Rsc jobs.
-
-  There are some limitations we have to work around before we can compile everything through Rsc
-  and followed by Zinc.
-  - rsc is not able to outline all scala code just yet (this is also being addressed through
-    automated rewrites).
-  - javac is unable to consume rsc's jars just yet.
-  - rsc is not able to outline all java code just yet (this is likely to *not* require rewrites,
-    just some more work on rsc).
-
-  As we work on improving our Rsc integration, we'll need to create more workflows to more closely
-  map the supported features of Rsc. This enum class allows us to do that.
-
-    - zinc-only: compiles targets just with Zinc and uses the Zinc products of their dependencies.
-    - rsc-then-zinc: compiles targets with Rsc to create "header" jars, then runs Zinc against the
-      Rsc products of their dependencies. The Rsc compile uses the Rsc products of Rsc compatible
-      targets and the Zinc products of zinc-only targets.
-  """
-
-  @classmethod
-  def classify_target(cls, target):
-    # NB: Java targets, Scala+Java targets, and some test targets are not currently supported for
-    # compile with Rsc.
-    # TODO: Rsc's produced header jars don't yet work with javac. Once this is resolved, we may add
-    #       additional workflow types.
-    if target.has_sources('.java') or \
-      isinstance(target, JUnitTests) or \
-      (isinstance(target, ScalaLibrary) and tuple(target.java_sources)):
-      target_type = _JvmCompileWorkflowType.zinc_only
-    elif target.has_sources('.scala'):
-      target_type = _JvmCompileWorkflowType.rsc_then_zinc
-    else:
-      target_type = None
-    return target_type
-
-
 class RscCompileContext(CompileContext):
   def __init__(self,
                target,
@@ -153,36 +109,60 @@ def ensure_output_dirs_exist(self):
     safe_mkdir(os.path.dirname(self.rsc_jar_file))
 
 
-class RscCompile(ZincCompile):
+class RscCompile(ZincCompile, MirroredTargetOptionMixin):
   """Compile Scala and Java code to classfiles using Rsc."""
 
   _name = 'mixed' # noqa
   compiler_name = 'rsc'
 
+  @memoized_property
+  def mirrored_target_option_actions(self):
+    return {
+      'workflow': self._identify_workflow_tags,
+    }
+
   @classmethod
   def implementation_version(cls):
     return super(RscCompile, cls).implementation_version() + [('RscCompile', 172)]
 
-  def __init__(self, *args, **kwargs):
-    super(RscCompile, self).__init__(*args, **kwargs)
+  class JvmCompileWorkflowType(enum(['zinc-only', 'rsc-then-zinc'])):
+    """Target classifications used to correctly schedule Zinc and Rsc jobs.
+
+    There are some limitations we have to work around before we can compile everything through Rsc
+    and followed by Zinc.
+    - rsc is not able to outline all scala code just yet (this is also being addressed through
+      automated rewrites).
+    - javac is unable to consume rsc's jars just yet.
+    - rsc is not able to outline all java code just yet (this is likely to *not* require rewrites,
+      just some more work on rsc).
+
+    As we work on improving our Rsc integration, we'll need to create more workflows to more closely
+    map the supported features of Rsc. This enum class allows us to do that.
 
-    self._rsc_compat_tag = self.get_options().force_compiler_tag_prefix
-    self._compiler_tags = {'{}:{}'.format(self._rsc_compat_tag, workflow.value): workflow
-      for workflow in _JvmCompileWorkflowType.all_variants}
-    self._default_workflow = self.get_options().default_workflow
+      - zinc-only: compiles targets just with Zinc and uses the Zinc products of their dependencies.
+      - rsc-then-zinc: compiles targets with Rsc to create "header" jars, then runs Zinc against the
+        Rsc products of their dependencies. The Rsc compile uses the Rsc products of Rsc compatible
+        targets and the Zinc products of zinc-only targets.
+    """
 
-    # If the default workflow is conveyed through a flag, override tag values.
-    self._ignore_tags_when_calculating_workflow = self.get_options().is_flagged('default_workflow')
+  @memoized_property
+  def _compiler_tags(self):
+    return {
+      '{prefix}:{workflow_name}'.format(
+        prefix=self.get_options().force_compiler_tag_prefix,
+        workflow_name=workflow.value): workflow
+      for workflow in self.JvmCompileWorkflowType.all_variants
+    }
 
   @classmethod
   def register_options(cls, register):
     super(RscCompile, cls).register_options(register)
     register('--force-compiler-tag-prefix', default='use-compiler', metavar='<tag>',
       help='Always compile targets marked with this tag with rsc, unless the workflow is '
            'specified on the cli.')
-    register('--default-workflow', type=_CompileWorkflowChoice,
-      default=_CompileWorkflowChoice.guess, metavar='<workflow>',
-      help='Defines how a compile workflow is chosen when a tag is not present.')
+    register('--workflow', type=cls.JvmCompileWorkflowType,
+      default=cls.JvmCompileWorkflowType.zinc_only, metavar='<workflow>',
+      help='The workflow to use to compile JVM targets.')
 
     cls.register_jvm_tool(
       register,
@@ -278,22 +258,26 @@ def create_empty_extra_products(self):
   def select(self, target):
     if not isinstance(target, JvmTarget):
       return False
-    return self._classify_compile_target(target) is not None
+    return self._classify_target_compile_workflow(target) is not None
 
-  def _classify_compile_target(self, target):
-    if not target.has_sources('.java') and not target.has_sources('.scala'):
+  @memoized_method
+  def _identify_workflow_tags(self, target):
+    try:
+      all_tags = [self._compiler_tags.get(tag) for tag in target.tags]
+      filtered_tags = filter(None, all_tags)
+      return assert_single_element(list(filtered_tags))
+    except StopIteration:
       return None
+    except ValueError as e:
+      raise ValueError('Multiple compile workflow tags specified for target {}: {}'
+                       .format(target, e))
 
-    if not self._ignore_tags_when_calculating_workflow:
-      for t in target.tags:
-        flow = self._compiler_tags.get(t)
-        if flow:
-          return _JvmCompileWorkflowType(flow)
-
-    return self._default_workflow.resolve_for_enum_variant({
-      'zinc-only': _JvmCompileWorkflowType.zinc_only,
-      'guess': _JvmCompileWorkflowType.classify_target(target)
-    })
+  @memoized_method
+  def _classify_target_compile_workflow(self, target):
+    """Return the compile workflow to use for this target."""
+    if target.has_sources('.java') or target.has_sources('.scala'):
+      return self.get_scalar_mirrored_target_option('workflow', target)
+    return None
 
   def _key_for_target_as_dep(self, target, workflow):
     # used for jobs that are either rsc jobs or zinc jobs run against rsc
@@ -489,6 +473,7 @@ def make_zinc_job(target, input_product_key, output_products, dep_keys):
           output_products=[
             runtime_classpath_product,
           ],
+          # TODO: remove this dep and fix tests!!!
           dep_keys=[
             # TODO we could remove the dependency on the rsc target in favor of bumping
             # the cache separately. We would need to bring that dependency back for
@@ -526,7 +511,7 @@ def create_compile_context(self, target, target_workdir):
         rsc_jar_file=os.path.join(rsc_dir, 'm.jar'),
         log_dir=os.path.join(rsc_dir, 'logs'),
         sources=sources,
-        workflow=self._classify_compile_target(target),
+        workflow=self._classify_target_compile_workflow(target),
       ),
       CompileContext(
         target=target,
@@ -659,6 +644,6 @@ def _on_invalid_compile_dependency(self, dep, compile_target, contexts):
     S1/2 are rsc-then-zinc targets and S2 must be on the classpath to compile J successfully.
     """
     return contexts[compile_target][0].workflow.resolve_for_enum_variant({
-      'zinc-only': lambda : contexts[dep][0].workflow == _JvmCompileWorkflowType.rsc_then_zinc,
+      'zinc-only': lambda : contexts[dep][0].workflow == self.JvmCompileWorkflowType.rsc_then_zinc,
       'rsc-then-zinc': lambda : False
     })()

src/python/pants/backend/native/subsystems/BUILD

@@ -8,6 +8,7 @@ python_library(
     'src/python/pants/backend/native/subsystems/binaries',
     'src/python/pants/backend/native/subsystems/utils',
     'src/python/pants/backend/python/subsystems',
+    'src/python/pants/build_graph',
     'src/python/pants/engine:rules',
     'src/python/pants/engine:selectors',
     'src/python/pants/subsystem',

src/python/pants/backend/native/subsystems/native_build_settings.py

@@ -4,17 +4,16 @@
 
 from __future__ import absolute_import, division, print_function, unicode_literals
 
-from pants.backend.native.subsystems.utils.mirrored_target_option_mixin import \
-  MirroredTargetOptionMixin
+from pants.build_graph.mirrored_target_option_mixin import MirroredTargetOptionMixin
 from pants.subsystem.subsystem import Subsystem
 
 
 class NativeBuildSettings(Subsystem, MirroredTargetOptionMixin):
   """Settings which affect both the compile and link phases."""
   options_scope = 'native-build-settings'
 
-  mirrored_option_to_kwarg_map = {
-    'strict_deps': 'strict_deps',
+  mirrored_target_option_actions = {
+    'strict_deps': lambda tgt: tgt.strict_deps,
   }
 
   @classmethod
@@ -29,4 +28,4 @@ def register_options(cls, register):
                   "this behavior with the strict_deps keyword argument as well.")
 
   def get_strict_deps_value_for_target(self, target):
-    return self.get_target_mirrored_option('strict_deps', target)
+    return self.get_scalar_mirrored_target_option('strict_deps', target)

src/python/pants/backend/native/subsystems/native_build_step.py

@@ -6,8 +6,7 @@
 
 from abc import abstractmethod
 
-from pants.backend.native.subsystems.utils.mirrored_target_option_mixin import \
-  MirroredTargetOptionMixin
+from pants.build_graph.mirrored_target_option_mixin import MirroredTargetOptionMixin
 from pants.option.compiler_option_sets_mixin import CompilerOptionSetsMixin
 from pants.subsystem.subsystem import Subsystem
 from pants.util.memo import memoized_property
@@ -23,9 +22,9 @@ class NativeBuildStep(CompilerOptionSetsMixin, MirroredTargetOptionMixin, Subsys
 
   options_scope = 'native-build-step'
 
-  mirrored_option_to_kwarg_map = {
-    'compiler_option_sets': 'compiler_option_sets',
-    'toolchain_variant': 'toolchain_variant'
+  mirrored_target_option_actions = {
+    'compiler_option_sets': lambda tgt: tgt.compiler_option_sets,
+    'toolchain_variant': lambda tgt: tgt.toolchain_variant,
   }
 
   @classmethod
@@ -43,10 +42,10 @@ def register_options(cls, register):
                   "linking is done with binutils ld on Linux, and the XCode CLI Tools on MacOS.")
 
   def get_compiler_option_sets_for_target(self, target):
-    return self.get_target_mirrored_option('compiler_option_sets', target)
+    return self.get_scalar_mirrored_target_option('compiler_option_sets', target)
 
   def get_toolchain_variant_for_target(self, target):
-    return self.get_target_mirrored_option('toolchain_variant', target)
+    return self.get_scalar_mirrored_target_option('toolchain_variant', target)
 
   @classproperty
   def get_compiler_option_sets_enabled_default_value(cls):