Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Canonicalize enum pattern matching for execution strategy, platform, and elsewhere #7226

Original file line number Diff line number Diff line change
Expand Up @@ -201,11 +201,11 @@ def _nailgunnable_combined_classpath(self):

# Overrides the normal zinc compiler classpath, which only contains zinc.
def get_zinc_compiler_classpath(self):
return self.do_for_execution_strategy_variant({
return self.execution_strategy_enum.resolve_for_enum_variant({
self.HERMETIC: lambda: super(RscCompile, self).get_zinc_compiler_classpath(),
self.SUBPROCESS: lambda: super(RscCompile, self).get_zinc_compiler_classpath(),
self.NAILGUN: lambda: self._nailgunnable_combined_classpath,
})
})()

def register_extra_products_from_contexts(self, targets, compile_contexts):
super(RscCompile, self).register_extra_products_from_contexts(targets, compile_contexts)
Expand Down Expand Up @@ -861,15 +861,15 @@ def _runtool_nonhermetic(self, parent_workunit, classpath, main, tool_name, args
def _runtool(self, main, tool_name, args, distribution,
tgt=None, input_files=tuple(), input_digest=None, output_dir=None):
with self.context.new_workunit(tool_name) as wu:
return self.do_for_execution_strategy_variant({
return self.execution_strategy_enum.resolve_for_enum_variant({
self.HERMETIC: lambda: self._runtool_hermetic(
main, tool_name, args, distribution,
tgt=tgt, input_files=input_files, input_digest=input_digest, output_dir=output_dir),
self.SUBPROCESS: lambda: self._runtool_nonhermetic(
wu, self.tool_classpath(tool_name), main, tool_name, args, distribution),
self.NAILGUN: lambda: self._runtool_nonhermetic(
wu, self._nailgunnable_combined_classpath, main, tool_name, args, distribution),
})
})()

def _run_metai_tool(self,
distribution,
Expand Down
39 changes: 17 additions & 22 deletions src/python/pants/backend/jvm/tasks/nailgun_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from pants.process.subprocess import Subprocess
from pants.task.task import Task, TaskBase
from pants.util.memo import memoized_property
from pants.util.objects import enum, register_enum_option


class NailgunTaskBase(JvmToolTaskMixin, TaskBase):
Expand All @@ -24,30 +25,16 @@ class NailgunTaskBase(JvmToolTaskMixin, TaskBase):
SUBPROCESS = 'subprocess'
HERMETIC = 'hermetic'

class InvalidExecutionStrategyMapping(Exception): pass

_all_execution_strategies = frozenset([NAILGUN, SUBPROCESS, HERMETIC])

def do_for_execution_strategy_variant(self, mapping):
"""Invoke the method in `mapping` with the key corresponding to the execution strategy.

`mapping` is a dict mapping execution strategy -> zero-argument lambda.
"""
variants = frozenset(mapping.keys())
if variants != self._all_execution_strategies:
raise self.InvalidExecutionStrategyMapping(
'Must specify a mapping with exactly the keys {} (was: {})'
.format(self._all_execution_strategies, variants))
method_for_variant = mapping[self.execution_strategy]
# The methods need not return a value, but we pass it along if they do.
return method_for_variant()
class ExecutionStrategy(enum([NAILGUN, SUBPROCESS, HERMETIC])): pass

@classmethod
def register_options(cls, register):
super(NailgunTaskBase, cls).register_options(register)
register('--execution-strategy', choices=[cls.NAILGUN, cls.SUBPROCESS, cls.HERMETIC], default=cls.NAILGUN,
help='If set to nailgun, nailgun will be enabled and repeated invocations of this '
'task will be quicker. If set to subprocess, then the task will be run without nailgun.')
register_enum_option(
register, cls.ExecutionStrategy, '--execution-strategy',
help='If set to nailgun, nailgun will be enabled and repeated invocations of this '
'task will be quicker. If set to subprocess, then the task will be run without nailgun. '
'Hermetic execution is an experimental subprocess execution framework.')
register('--nailgun-timeout-seconds', advanced=True, default=10, type=float,
help='Timeout (secs) for nailgun startup.')
register('--nailgun-connect-attempts', advanced=True, default=5, type=int,
Expand All @@ -60,6 +47,13 @@ def register_options(cls, register):
rev='0.9.1'),
])

@memoized_property
def execution_strategy_enum(self):
# TODO: This .create() call can be removed when the enum interface is more stable as the option
# is converted into an instance of self.ExecutionStrategy via the `type` argument through
# register_enum_option().
return self.ExecutionStrategy.create(self.get_options().execution_strategy)

@classmethod
def subsystem_dependencies(cls):
return super(NailgunTaskBase, cls).subsystem_dependencies() + (Subprocess.Factory,)
Expand All @@ -76,9 +70,10 @@ def __init__(self, *args, **kwargs):
self._executor_workdir = os.path.join(self.context.options.for_global_scope().pants_workdir,
*id_tuple)

@memoized_property
# TODO: eventually deprecate this when we can move all subclasses to use the enum!
@property
def execution_strategy(self):
return self.get_options().execution_strategy
return self.execution_strategy_enum.value

def create_java_executor(self, dist=None):
"""Create java executor that uses this task's ng daemon, if allowed.
Expand Down
35 changes: 6 additions & 29 deletions src/python/pants/backend/native/config/environment.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,37 +9,14 @@

from pants.engine.rules import SingletonRule
from pants.util.meta import AbstractClass
from pants.util.objects import datatype
from pants.util.objects import datatype, enum
from pants.util.osutil import all_normalized_os_names, get_normalized_os_name
from pants.util.strutil import create_path_env_var


class Platform(datatype(['normalized_os_name'])):
class Platform(enum('normalized_os_name', all_normalized_os_names())):

class UnsupportedPlatformError(Exception):
"""Thrown if pants is running on an unrecognized platform."""

@classmethod
def create(cls):
return Platform(get_normalized_os_name())

_NORMALIZED_OS_NAMES = frozenset(all_normalized_os_names())

def resolve_platform_specific(self, platform_specific_funs):
arg_keys = frozenset(platform_specific_funs.keys())
unknown_plats = self._NORMALIZED_OS_NAMES - arg_keys
if unknown_plats:
raise self.UnsupportedPlatformError(
"platform_specific_funs {} must support platforms {}"
.format(platform_specific_funs, list(unknown_plats)))
extra_plats = arg_keys - self._NORMALIZED_OS_NAMES
if extra_plats:
raise self.UnsupportedPlatformError(
"platform_specific_funs {} has unrecognized platforms {}"
.format(platform_specific_funs, list(extra_plats)))

fun_for_platform = platform_specific_funs[self.normalized_os_name]
return fun_for_platform()
default_value = get_normalized_os_name()


class Executable(AbstractClass):
Expand Down Expand Up @@ -89,9 +66,9 @@ def as_invocation_environment_dict(self):

:rtype: dict of string -> string
"""
lib_env_var = self._platform.resolve_platform_specific({
'darwin': lambda: 'DYLD_LIBRARY_PATH',
'linux': lambda: 'LD_LIBRARY_PATH',
lib_env_var = self._platform.resolve_for_enum_variant({
'darwin': 'DYLD_LIBRARY_PATH',
'linux': 'LD_LIBRARY_PATH',
})
return {
'PATH': create_path_env_var(self.path_entries),
Expand Down
6 changes: 3 additions & 3 deletions src/python/pants/backend/native/subsystems/binaries/gcc.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ def path_entries(self):

@memoized_method
def _common_lib_dirs(self, platform):
lib64_tuples = platform.resolve_platform_specific({
'darwin': lambda: [],
'linux': lambda: [('lib64',)],
lib64_tuples = platform.resolve_for_enum_variant({
'darwin': [],
'linux': [('lib64',)],
})
return self._filemap(lib64_tuples + [
('lib',),
Expand Down
11 changes: 4 additions & 7 deletions src/python/pants/backend/native/subsystems/binaries/llvm.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,16 +80,13 @@ def _filemap(self, all_components_list):
def path_entries(self):
return self._filemap([('bin',)])

_PLATFORM_SPECIFIC_LINKER_NAME = {
'darwin': lambda: 'ld64.lld',
'linux': lambda: 'lld',
}

def linker(self, platform):
return Linker(
path_entries=self.path_entries,
exe_filename=platform.resolve_platform_specific(
self._PLATFORM_SPECIFIC_LINKER_NAME),
exe_filename=platform.resolve_for_enum_variant({
'darwin': 'ld64.lld',
'linux': 'lld',
}),
library_dirs=[],
linking_library_dirs=[],
extra_args=[],
Expand Down
17 changes: 6 additions & 11 deletions src/python/pants/backend/native/subsystems/native_build_step.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,10 @@
from pants.subsystem.subsystem import Subsystem
from pants.util.memo import memoized_property
from pants.util.meta import classproperty
from pants.util.objects import enum
from pants.util.objects import enum, register_enum_option


class ToolchainVariant(enum('descriptor', ['gnu', 'llvm'])):

@property
def is_gnu(self):
return self.descriptor == 'gnu'
class ToolchainVariant(enum(['gnu', 'llvm'])): pass


class NativeBuildStep(CompilerOptionSetsMixin, MirroredTargetOptionMixin, Subsystem):
Expand All @@ -39,11 +35,10 @@ def register_options(cls, register):
help='The default for the "compiler_option_sets" argument '
'for targets of this language.')

register('--toolchain-variant', type=str, fingerprint=True, advanced=True,
choices=ToolchainVariant.allowed_values,
default=ToolchainVariant.default_value,
help="Whether to use gcc (gnu) or clang (llvm) to compile C and C++. Currently all "
"linking is done with binutils ld on Linux, and the XCode CLI Tools on MacOS.")
register_enum_option(
register, ToolchainVariant, '--toolchain-variant', advanced=True,
help="Whether to use gcc (gnu) or clang (llvm) to compile C and C++. Currently all "
"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)
Expand Down
20 changes: 15 additions & 5 deletions src/python/pants/backend/native/subsystems/native_toolchain.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,11 @@ class LLVMCppToolchain(datatype([('cpp_toolchain', CppToolchain)])): pass

@rule(LibcObjects, [Select(Platform), Select(NativeToolchain)])
def select_libc_objects(platform, native_toolchain):
paths = platform.resolve_platform_specific({
# We use lambdas here to avoid searching for libc on osx, where it will fail.
paths = platform.resolve_for_enum_variant({
'darwin': lambda: [],
'linux': lambda: native_toolchain._libc_dev.get_libc_objects(),
})
})()
yield LibcObjects(paths)


Expand Down Expand Up @@ -343,8 +344,12 @@ class ToolchainVariantRequest(datatype([
@rule(CToolchain, [Select(ToolchainVariantRequest)])
def select_c_toolchain(toolchain_variant_request):
native_toolchain = toolchain_variant_request.toolchain
# TODO: make an enum exhaustiveness checking method that works with `yield Get(...)` statements!
if toolchain_variant_request.variant.is_gnu:
# TODO(#5933): make an enum exhaustiveness checking method that works with `yield Get(...)`!
use_gcc = toolchain_variant_request.variant.resolve_for_enum_variant({
'gnu': True,
'llvm': False,
})
if use_gcc:
toolchain_resolved = yield Get(GCCCToolchain, NativeToolchain, native_toolchain)
else:
toolchain_resolved = yield Get(LLVMCToolchain, NativeToolchain, native_toolchain)
Expand All @@ -354,7 +359,12 @@ def select_c_toolchain(toolchain_variant_request):
@rule(CppToolchain, [Select(ToolchainVariantRequest)])
def select_cpp_toolchain(toolchain_variant_request):
native_toolchain = toolchain_variant_request.toolchain
if toolchain_variant_request.variant.is_gnu:
# TODO(#5933): make an enum exhaustiveness checking method that works with `yield Get(...)`!
use_gcc = toolchain_variant_request.variant.resolve_for_enum_variant({
'gnu': True,
'llvm': False,
})
if use_gcc:
toolchain_resolved = yield Get(GCCCppToolchain, NativeToolchain, native_toolchain)
else:
toolchain_resolved = yield Get(LLVMCppToolchain, NativeToolchain, native_toolchain)
Expand Down
6 changes: 3 additions & 3 deletions src/python/pants/backend/native/targets/native_artifact.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ def alias(cls):

def as_shared_lib(self, platform):
# TODO: check that the name conforms to some format in the constructor (e.g. no dots?).
return platform.resolve_platform_specific({
'darwin': lambda: 'lib{}.dylib'.format(self.lib_name),
'linux': lambda: 'lib{}.so'.format(self.lib_name),
return platform.resolve_for_enum_variant({
'darwin': 'lib{}.dylib'.format(self.lib_name),
'linux': 'lib{}.so'.format(self.lib_name),
})

def _compute_fingerprint(self):
Expand Down
6 changes: 3 additions & 3 deletions src/python/pants/backend/native/tasks/conan_fetch.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,9 @@ def _conan_user_home(self, conan, in_workdir=False):

@memoized_property
def _conan_os_name(self):
return Platform.create().resolve_platform_specific({
'darwin': lambda: 'Macos',
'linux': lambda: 'Linux',
return Platform.create().resolve_for_enum_variant({
'darwin': 'Macos',
'linux': 'Linux',
})

@property
Expand Down
10 changes: 4 additions & 6 deletions src/python/pants/backend/native/tasks/link_shared_libraries.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,6 @@ def _make_link_request(self, vt, compiled_objects_product):

return link_request

_SHARED_CMDLINE_ARGS = {
'darwin': lambda: ['-Wl,-dylib'],
'linux': lambda: ['-shared'],
}

def _execute_link_request(self, link_request):
object_files = link_request.object_files

Expand All @@ -163,7 +158,10 @@ def _execute_link_request(self, link_request):
self.context.log.debug("resulting_shared_lib_path: {}".format(resulting_shared_lib_path))
# We are executing in the results_dir, so get absolute paths for everything.
cmd = ([linker.exe_filename] +
self.platform.resolve_platform_specific(self._SHARED_CMDLINE_ARGS) +
self.platform.resolve_for_enum_variant({
'darwin': ['-Wl,-dylib'],
'linux': ['-shared'],
}) +
linker.extra_args +
['-o', os.path.abspath(resulting_shared_lib_path)] +
['-L{}'.format(lib_dir) for lib_dir in link_request.external_lib_dirs] +
Expand Down
6 changes: 3 additions & 3 deletions src/python/pants/backend/python/tasks/unpack_wheels.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,9 @@ def _name_and_platform(whl):

@memoized_classproperty
def _current_platform_abbreviation(cls):
return NativeBackendPlatform.create().resolve_platform_specific({
'darwin': lambda: 'macosx',
'linux': lambda: 'linux',
return NativeBackendPlatform.create().resolve_for_enum_variant({
'darwin': 'macosx',
'linux': 'linux',
})

@classmethod
Expand Down
5 changes: 3 additions & 2 deletions src/python/pants/engine/fs.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,9 @@ def __new__(cls, include, exclude=(), glob_match_error_behavior=None, conjunctio
cls,
include=tuple(include),
exclude=tuple(exclude),
glob_match_error_behavior=GlobMatchErrorBehavior.create(glob_match_error_behavior),
conjunction=GlobExpansionConjunction.create(conjunction))
glob_match_error_behavior=GlobMatchErrorBehavior.create(glob_match_error_behavior,
none_is_default=True),
conjunction=GlobExpansionConjunction.create(conjunction, none_is_default=True))


class PathGlobsAndRoot(datatype([('path_globs', PathGlobs), ('root', text_type)])):
Expand Down
3 changes: 2 additions & 1 deletion src/python/pants/init/engine_initializer.py
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,8 @@ def setup_legacy_graph_extended(
rules = (
[
RootRule(Console),
SingletonRule.from_instance(GlobMatchErrorBehavior.create(glob_match_error_behavior)),
SingletonRule.from_instance(GlobMatchErrorBehavior.create(glob_match_error_behavior,
none_is_default=True)),
SingletonRule.from_instance(build_configuration),
SingletonRule(SymbolTable, symbol_table),
] +
Expand Down
16 changes: 7 additions & 9 deletions src/python/pants/option/global_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from pants.option.optionable import Optionable
from pants.option.scope import ScopeInfo
from pants.subsystem.subsystem_client_mixin import SubsystemClientMixin
from pants.util.objects import datatype, enum
from pants.util.objects import datatype, enum, register_enum_option


class GlobMatchErrorBehavior(enum('failure_behavior', ['ignore', 'warn', 'error'])):
Expand All @@ -26,8 +26,6 @@ class GlobMatchErrorBehavior(enum('failure_behavior', ['ignore', 'warn', 'error'
be aware of any changes to this object's definition.
"""

default_option_value = 'warn'


class ExecutionOptions(datatype([
'remote_store_server',
Expand Down Expand Up @@ -197,12 +195,12 @@ def register_bootstrap_options(cls, register):
help='Paths to ignore for all filesystem operations performed by pants '
'(e.g. BUILD file scanning, glob matching, etc). '
'Patterns use the gitignore syntax (https://git-scm.com/docs/gitignore).')
register('--glob-expansion-failure', type=str,
choices=GlobMatchErrorBehavior.allowed_values,
default=GlobMatchErrorBehavior.default_option_value,
advanced=True,
help="Raise an exception if any targets declaring source files "
"fail to match any glob provided in the 'sources' argument.")
register_enum_option(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to suggest exposing this on Optionable (like we do with register), but I definitely prefer @baroquebobcat's suggestion :)

# TODO: allow using the attribute `GlobMatchErrorBehavior.warn` for more safety!
register, GlobMatchErrorBehavior, '--glob-expansion-failure', default='warn',
advanced=True,
help="Raise an exception if any targets declaring source files "
"fail to match any glob provided in the 'sources' argument.")

register('--exclude-target-regexp', advanced=True, type=list, default=[], daemon=False,
metavar='<regexp>', help='Exclude target roots that match these regexes.')
Expand Down
Loading