From af191195423514b7cf34d13ae2fbb5f867a75d01 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 1 Mar 2019 22:50:49 -0800 Subject: [PATCH] enum cleanup (#7269) ### Problem Resolves #7232, resolves #7248, and addresses https://github.com/pantsbuild/pants/pull/7249#discussion_r257653836. ### Solution - Remove enum defaults and the `.create()` method with complicated argument handling. - Allow enums to be `==` compared as long as they're the same type. - Allow accessing enum instances with class attributes. ### Result enums are nicer! --- .../jvm/tasks/jvm_compile/rsc/rsc_compile.py | 4 +- .../pants/backend/jvm/tasks/nailgun_task.py | 4 +- .../backend/native/config/environment.py | 11 +- .../native/subsystems/native_build_step.py | 10 +- .../native/subsystems/native_toolchain.py | 16 +-- .../backend/native/targets/native_library.py | 2 +- .../pants/backend/native/tasks/conan_fetch.py | 2 +- .../native/tasks/link_shared_libraries.py | 2 +- src/python/pants/engine/fs.py | 10 +- src/python/pants/engine/legacy/structs.py | 10 +- src/python/pants/init/engine_initializer.py | 7 +- src/python/pants/option/custom_types.py | 10 +- src/python/pants/option/global_options.py | 2 +- src/python/pants/util/BUILD | 1 + src/python/pants/util/objects.py | 122 +++++++----------- src/rust/engine/src/nodes.rs | 4 +- .../native/subsystems/test_libc_resolution.py | 6 +- .../subsystems/test_native_toolchain.py | 2 +- .../backend/native/util/platform_utils.py | 6 +- .../python/targets/test_unpacked_whls.py | 8 +- .../tasks/native/test_ctypes_integration.py | 35 ++--- .../engine/legacy/test_graph_integration.py | 6 +- tests/python/pants_test/engine/test_fs.py | 9 +- tests/python/pants_test/util/test_objects.py | 55 ++++---- 24 files changed, 158 insertions(+), 186 deletions(-) diff --git a/src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py b/src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py index 91148b6a1e8..30f820be727 100644 --- a/src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py +++ b/src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py @@ -118,9 +118,9 @@ def classify_target(cls, target): if target.has_sources('.java') or \ isinstance(target, JUnitTests) or \ (isinstance(target, ScalaLibrary) and tuple(target.java_sources)): - target_type = _JvmCompileWorkflowType.create('zinc-only') + target_type = _JvmCompileWorkflowType('zinc-only') elif target.has_sources('.scala'): - target_type = _JvmCompileWorkflowType.create('rsc-then-zinc') + target_type = _JvmCompileWorkflowType('rsc-then-zinc') else: target_type = None return target_type diff --git a/src/python/pants/backend/jvm/tasks/nailgun_task.py b/src/python/pants/backend/jvm/tasks/nailgun_task.py index dbc3229a5e3..0a5bf7c64ee 100644 --- a/src/python/pants/backend/jvm/tasks/nailgun_task.py +++ b/src/python/pants/backend/jvm/tasks/nailgun_task.py @@ -31,7 +31,7 @@ class ExecutionStrategy(enum([NAILGUN, SUBPROCESS, HERMETIC])): pass def register_options(cls, register): super(NailgunTaskBase, cls).register_options(register) register_enum_option( - register, cls.ExecutionStrategy, '--execution-strategy', + register, cls.ExecutionStrategy, '--execution-strategy', 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. ' 'Hermetic execution is an experimental subprocess execution framework.') @@ -52,7 +52,7 @@ 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) + return self.ExecutionStrategy(self.get_options().execution_strategy) @classmethod def subsystem_dependencies(cls): diff --git a/src/python/pants/backend/native/config/environment.py b/src/python/pants/backend/native/config/environment.py index b433535668f..6d9e9b9f90f 100644 --- a/src/python/pants/backend/native/config/environment.py +++ b/src/python/pants/backend/native/config/environment.py @@ -15,9 +15,12 @@ from pants.util.strutil import create_path_env_var -class Platform(enum('normalized_os_name', all_normalized_os_names())): +class Platform(enum(all_normalized_os_names())): - default_value = get_normalized_os_name() + # TODO: try to turn all of these accesses into v2 dependency injections! + @memoized_classproperty + def current(cls): + return cls(get_normalized_os_name()) def _list_field(func): @@ -155,7 +158,7 @@ def extra_args(self): :rtype: list of str """ - _platform = Platform.create() + _platform = Platform.current @property def invocation_environment_dict(self): @@ -299,5 +302,5 @@ class HostLibcDev(datatype(['crti_object', 'fingerprint'])): pass def create_native_environment_rules(): return [ - SingletonRule(Platform, Platform.create()), + SingletonRule(Platform, Platform.current), ] diff --git a/src/python/pants/backend/native/subsystems/native_build_step.py b/src/python/pants/backend/native/subsystems/native_build_step.py index 55ffea10dd4..02c30d72953 100644 --- a/src/python/pants/backend/native/subsystems/native_build_step.py +++ b/src/python/pants/backend/native/subsystems/native_build_step.py @@ -36,7 +36,7 @@ def register_options(cls, register): 'for targets of this language.') register_enum_option( - register, ToolchainVariant, '--toolchain-variant', advanced=True, + register, ToolchainVariant, '--toolchain-variant', advanced=True, default='gnu', 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.") @@ -44,7 +44,13 @@ def get_compiler_option_sets_for_target(self, target): return self.get_target_mirrored_option('compiler_option_sets', target) def get_toolchain_variant_for_target(self, target): - return ToolchainVariant.create(self.get_target_mirrored_option('toolchain_variant', target)) + # TODO(#7233): convert this option into an enum instance using the `type` argument in option + # registration! + enum_or_value = self.get_target_mirrored_option('toolchain_variant', target) + if isinstance(enum_or_value, ToolchainVariant): + return enum_or_value + else: + return ToolchainVariant(enum_or_value) @classproperty def get_compiler_option_sets_enabled_default_value(cls): diff --git a/src/python/pants/backend/native/subsystems/native_toolchain.py b/src/python/pants/backend/native/subsystems/native_toolchain.py index 1885c68e0d9..aa0248adc34 100644 --- a/src/python/pants/backend/native/subsystems/native_toolchain.py +++ b/src/python/pants/backend/native/subsystems/native_toolchain.py @@ -110,7 +110,7 @@ def select_libc_objects(platform, native_toolchain): @rule(Assembler, [Select(Platform), Select(NativeToolchain)]) def select_assembler(platform, native_toolchain): - if platform.normalized_os_name == 'darwin': + if platform == Platform.darwin: assembler = yield Get(Assembler, XCodeCLITools, native_toolchain._xcode_cli_tools) else: assembler = yield Get(Assembler, Binutils, native_toolchain._binutils) @@ -127,7 +127,7 @@ class BaseLinker(datatype([('linker', Linker)])): # TODO: select the appropriate `Platform` in the `@rule` decl using variants! @rule(BaseLinker, [Select(Platform), Select(NativeToolchain)]) def select_base_linker(platform, native_toolchain): - if platform.normalized_os_name == 'darwin': + if platform == Platform.darwin: # TODO(#5663): turn this into LLVM when lld works. linker = yield Get(Linker, XCodeCLITools, native_toolchain._xcode_cli_tools) else: @@ -172,7 +172,7 @@ def select_gcc_install_location(gcc): def select_llvm_c_toolchain(platform, native_toolchain): provided_clang = yield Get(CCompiler, LLVM, native_toolchain._llvm) - if platform.normalized_os_name == 'darwin': + if platform == Platform.darwin: xcode_clang = yield Get(CCompiler, XCodeCLITools, native_toolchain._xcode_cli_tools) joined_c_compiler = provided_clang.sequence(xcode_clang) else: @@ -200,9 +200,9 @@ def select_llvm_cpp_toolchain(platform, native_toolchain): # On OSX, we use the libc++ (LLVM) C++ standard library implementation. This is feature-complete # for OSX, but not for Linux (see https://libcxx.llvm.org/ for more info). - if platform.normalized_os_name == 'darwin': - xcode_clang = yield Get(CppCompiler, XCodeCLITools, native_toolchain._xcode_cli_tools) - joined_cpp_compiler = provided_clangpp.sequence(xcode_clang) + if platform == Platform.darwin: + xcode_clangpp = yield Get(CppCompiler, XCodeCLITools, native_toolchain._xcode_cli_tools) + joined_cpp_compiler = provided_clangpp.sequence(xcode_clangpp) extra_llvm_linking_library_dirs = [] linker_extra_args = [] else: @@ -242,7 +242,7 @@ def select_llvm_cpp_toolchain(platform, native_toolchain): def select_gcc_c_toolchain(platform, native_toolchain): provided_gcc = yield Get(CCompiler, GCC, native_toolchain._gcc) - if platform.normalized_os_name == 'darwin': + if platform == Platform.darwin: # GCC needs access to some headers that are only provided by the XCode toolchain # currently (e.g. "_stdio.h"). These headers are unlikely to change across versions, so this is # probably safe. @@ -267,7 +267,7 @@ def select_gcc_c_toolchain(platform, native_toolchain): def select_gcc_cpp_toolchain(platform, native_toolchain): provided_gpp = yield Get(CppCompiler, GCC, native_toolchain._gcc) - if platform.normalized_os_name == 'darwin': + if platform == Platform.darwin: # GCC needs access to some headers that are only provided by the XCode toolchain # currently (e.g. "_stdio.h"). These headers are unlikely to change across versions, so this is # probably safe. diff --git a/src/python/pants/backend/native/targets/native_library.py b/src/python/pants/backend/native/targets/native_library.py index 547f1885844..20e331c0cdb 100644 --- a/src/python/pants/backend/native/targets/native_library.py +++ b/src/python/pants/backend/native/targets/native_library.py @@ -51,7 +51,7 @@ def toolchain_variant(self): if not self.payload.toolchain_variant: return None - return ToolchainVariant.create(self.payload.toolchain_variant) + return ToolchainVariant(self.payload.toolchain_variant) @property def strict_deps(self): diff --git a/src/python/pants/backend/native/tasks/conan_fetch.py b/src/python/pants/backend/native/tasks/conan_fetch.py index 5f9eb11a14f..1a602130b5b 100644 --- a/src/python/pants/backend/native/tasks/conan_fetch.py +++ b/src/python/pants/backend/native/tasks/conan_fetch.py @@ -124,7 +124,7 @@ def _conan_user_home(self, conan, in_workdir=False): @memoized_property def _conan_os_name(self): - return Platform.create().resolve_for_enum_variant({ + return Platform.current.resolve_for_enum_variant({ 'darwin': 'Macos', 'linux': 'Linux', }) diff --git a/src/python/pants/backend/native/tasks/link_shared_libraries.py b/src/python/pants/backend/native/tasks/link_shared_libraries.py index 913fa9f334a..e990411c864 100644 --- a/src/python/pants/backend/native/tasks/link_shared_libraries.py +++ b/src/python/pants/backend/native/tasks/link_shared_libraries.py @@ -64,7 +64,7 @@ def linker(self, native_library_target): @memoized_property def platform(self): # TODO: convert this to a v2 engine dependency injection. - return Platform.create() + return Platform.current def execute(self): targets_providing_artifacts = self.context.targets(NativeLibrary.produces_ctypes_native_library) diff --git a/src/python/pants/engine/fs.py b/src/python/pants/engine/fs.py index 8d009f01c06..c04cb412011 100644 --- a/src/python/pants/engine/fs.py +++ b/src/python/pants/engine/fs.py @@ -52,16 +52,18 @@ def __new__(cls, include, exclude=(), glob_match_error_behavior=None, conjunctio :param include: A list of filespecs to include. :param exclude: A list of filespecs to exclude. - :param glob_match_error_behavior: The value to pass to GlobMatchErrorBehavior.create() + :param GlobMatchErrorBehavior glob_match_error_behavior: How to respond to globs matching no + files. + :param GlobExpansionConjunction conjunction: Whether all globs are expected to match at least + one file, or if any glob matching is ok. :rtype: :class:`PathGlobs` """ return super(PathGlobs, cls).__new__( cls, include=tuple(include), exclude=tuple(exclude), - glob_match_error_behavior=GlobMatchErrorBehavior.create(glob_match_error_behavior, - none_is_default=True), - conjunction=GlobExpansionConjunction.create(conjunction, none_is_default=True)) + glob_match_error_behavior=(glob_match_error_behavior or GlobMatchErrorBehavior.ignore), + conjunction=(conjunction or GlobExpansionConjunction.any_match)) class Digest(datatype([('fingerprint', text_type), ('serialized_bytes_length', int)])): diff --git a/src/python/pants/engine/legacy/structs.py b/src/python/pants/engine/legacy/structs.py index ec56df5a922..fd456b1541d 100644 --- a/src/python/pants/engine/legacy/structs.py +++ b/src/python/pants/engine/legacy/structs.py @@ -66,13 +66,13 @@ def get_sources(self): globs = Globs(*self.default_sources_globs, spec_path=self.address.spec_path, exclude=self.default_sources_exclude_globs or []) - conjunction_globs = GlobsWithConjunction(globs, GlobExpansionConjunction.create('any_match')) + conjunction_globs = GlobsWithConjunction(globs, GlobExpansionConjunction.any_match) else: globs = None conjunction_globs = None else: globs = BaseGlobs.from_sources_field(sources, self.address.spec_path) - conjunction_globs = GlobsWithConjunction(globs, GlobExpansionConjunction.create('all_match')) + conjunction_globs = GlobsWithConjunction(globs, GlobExpansionConjunction.all_match) return conjunction_globs @@ -235,7 +235,7 @@ def _construct_bundles_field(self): # TODO: we want to have this field set from the global option --glob-expansion-failure, or # something set on the target. Should we move --glob-expansion-failure to be a bootstrap # option? See #5864. - path_globs = base_globs.to_path_globs(rel_root, GlobExpansionConjunction.create('all_match')) + path_globs = base_globs.to_path_globs(rel_root, GlobExpansionConjunction.all_match) filespecs_list.append(base_globs.filespecs) path_globs_list.append(path_globs) @@ -263,7 +263,7 @@ def field_adaptors(self): if getattr(self, 'resources', None) is None: return field_adaptors base_globs = BaseGlobs.from_sources_field(self.resources, self.address.spec_path) - path_globs = base_globs.to_path_globs(self.address.spec_path, GlobExpansionConjunction.create('all_match')) + path_globs = base_globs.to_path_globs(self.address.spec_path, GlobExpansionConjunction.all_match) sources_field = SourcesField(self.address, 'resources', base_globs.filespecs, @@ -425,4 +425,4 @@ class GlobsWithConjunction(datatype([ @classmethod def for_literal_files(cls, file_paths, spec_path): - return cls(Files(*file_paths, spec_path=spec_path), GlobExpansionConjunction.create('all_match')) + return cls(Files(*file_paths, spec_path=spec_path), GlobExpansionConjunction.all_match) diff --git a/src/python/pants/init/engine_initializer.py b/src/python/pants/init/engine_initializer.py index 41f042c8844..8a59e829e2c 100644 --- a/src/python/pants/init/engine_initializer.py +++ b/src/python/pants/init/engine_initializer.py @@ -262,7 +262,9 @@ def setup_legacy_graph(native, options_bootstrapper, build_configuration): options_bootstrapper, build_configuration, native=native, - glob_match_error_behavior=bootstrap_options.glob_expansion_failure, + # TODO(#7233): convert this into an enum instance using the `type` argument in option + # registration! + glob_match_error_behavior=GlobMatchErrorBehavior(bootstrap_options.glob_expansion_failure), build_ignore_patterns=bootstrap_options.build_ignore, exclude_target_regexps=bootstrap_options.exclude_target_regexp, subproject_roots=bootstrap_options.subproject_roots, @@ -346,8 +348,7 @@ def setup_legacy_graph_extended( rules = ( [ RootRule(Console), - SingletonRule.from_instance(GlobMatchErrorBehavior.create(glob_match_error_behavior, - none_is_default=True)), + SingletonRule.from_instance(glob_match_error_behavior or GlobMatchErrorBehavior.ignore), SingletonRule.from_instance(build_configuration), SingletonRule(SymbolTable, symbol_table), ] + diff --git a/src/python/pants/option/custom_types.py b/src/python/pants/option/custom_types.py index 7b5ee91a0df..79e78d98b24 100644 --- a/src/python/pants/option/custom_types.py +++ b/src/python/pants/option/custom_types.py @@ -308,9 +308,9 @@ def __repr__(self): return '{} {}'.format(self.action, self.val) -class GlobExpansionConjunction(enum('conjunction', ['any_match', 'all_match'])): +class GlobExpansionConjunction(enum(['any_match', 'all_match'])): + """Describe whether to require that only some or all glob strings match in a target's sources. - # NB: The `default_value` is automatically the first element of the value list, but can be - # overridden or made more explicit in the body of the class. We want to be explicit here about - # which behavior we have when merging globs, as that can affect performance. - default_value = 'any_match' + NB: this object is interpreted from within Snapshot::lift_path_globs() -- that method will need to + be aware of any changes to this object's definition. + """ diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index c346f85c4fd..13b5909b555 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -19,7 +19,7 @@ from pants.util.objects import datatype, enum, register_enum_option -class GlobMatchErrorBehavior(enum('failure_behavior', ['ignore', 'warn', 'error'])): +class GlobMatchErrorBehavior(enum(['ignore', 'warn', 'error'])): """Describe the action to perform when matching globs in BUILD files to source files. NB: this object is interpreted from within Snapshot::lift_path_globs() -- that method will need to diff --git a/src/python/pants/util/BUILD b/src/python/pants/util/BUILD index 5e2bd8f4a00..4b65960f42e 100644 --- a/src/python/pants/util/BUILD +++ b/src/python/pants/util/BUILD @@ -115,6 +115,7 @@ python_library( dependencies = [ '3rdparty/python/twitter/commons:twitter.common.collections', '3rdparty/python:future', + '3rdparty/python:six', ':collections_abc_backport', ':memo', ':meta', diff --git a/src/python/pants/util/objects.py b/src/python/pants/util/objects.py index f66ebc5d717..48c21358992 100644 --- a/src/python/pants/util/objects.py +++ b/src/python/pants/util/objects.py @@ -8,6 +8,7 @@ from builtins import zip from collections import namedtuple +import six from twitter.common.collections import OrderedSet from pants.util.collections_abc_backport import Iterable, OrderedDict @@ -205,50 +206,43 @@ class EnumVariantSelectionError(TypeCheckError): """Raised when an invalid variant for an enum() is constructed or matched against.""" -def enum(*args): +def enum(all_values): """A datatype which can take on a finite set of values. This method is experimental and unstable. Any enum subclass can be constructed with its create() classmethod. This method will use the first element of `all_values` as the default value, but enum classes can override this behavior by setting `default_value` in the class body. - NB: Relying on the `field_name` directly is discouraged in favor of using - resolve_for_enum_variant() in Python code. The `field_name` argument is exposed to make enum - instances more readable when printed, and to allow code in another language using an FFI to - reliably extract the value from an enum instance. + If `all_values` contains only strings, then each variant is made into an attribute on the + generated enum class object. This allows code such as the following: + + class MyResult(enum(['success', 'failure'])): + pass + + MyResult.success # The same as: MyResult('success') + MyResult.failure # The same as: MyResult('failure') - :param string field_name: A string used as the field for the datatype. This positional argument is - optional, and defaults to 'value'. Note that `enum()` does not yet - support type checking as with `datatype()`. :param Iterable all_values: A nonempty iterable of objects representing all possible values for the enum. This argument must be a finite, non-empty iterable with unique values. :raises: :class:`ValueError` """ - if len(args) == 1: - field_name = 'value' - all_values, = args - elif len(args) == 2: - field_name, all_values = args - else: - raise ValueError("enum() accepts only 1 or 2 args! args = {!r}".format(args)) + # namedtuple() raises a ValueError if you try to use a field with a leading underscore. + field_name = 'value' # This call to list() will eagerly evaluate any `all_values` which would otherwise be lazy, such # as a generator. all_values_realized = list(all_values) - # `OrderedSet` maintains the order of the input iterable, but is faster to check membership. - allowed_values_set = OrderedSet(all_values_realized) - if len(allowed_values_set) == 0: + unique_values = OrderedSet(all_values_realized) + if len(unique_values) == 0: raise ValueError("all_values must be a non-empty iterable!") - elif len(allowed_values_set) < len(all_values_realized): + elif len(unique_values) < len(all_values_realized): raise ValueError("When converting all_values ({}) to a set, at least one duplicate " "was detected. The unique elements of all_values were: {}." - .format(all_values_realized, list(allowed_values_set))) + .format(all_values_realized, list(unique_values))) class ChoiceDatatype(datatype([field_name])): - default_value = next(iter(allowed_values_set)) - # Overriden from datatype() so providing an invalid variant is catchable as a TypeCheckError, # but more specific. type_check_error_type = EnumVariantSelectionError @@ -260,7 +254,7 @@ def _singletons(cls): NB: The implementation of enum() should use this property as the source of truth for allowed values and enum instances from those values. """ - return OrderedDict((value, cls._make_singleton(value)) for value in allowed_values_set) + return OrderedDict((value, cls._make_singleton(value)) for value in all_values_realized) @classmethod def _make_singleton(cls, value): @@ -276,20 +270,27 @@ def _allowed_values(cls): return list(cls._singletons.keys()) def __new__(cls, value): - """Forward `value` to the .create() factory method. + """Create an instance of this enum. - The .create() factory method is preferred, but forwarding the constructor like this allows us - to use the generated enum type both as a type to check against with isinstance() as well as a - function to create instances with. This makes it easy to use as a pants option type. + :param value: Use this as the enum value. If `value` is an instance of this class, return it, + otherwise it is checked against the enum's allowed values. """ - return cls.create(value) + if value not in cls._singletons: + raise cls.make_type_error( + "Value {!r} must be one of: {!r}." + .format(value, cls._allowed_values)) + return cls._singletons[value] # TODO: figure out if this will always trigger on primitives like strings, and what situations # won't call this __eq__ (and therefore won't raise like we want). def __eq__(self, other): - """Redefine equality to raise to nudge people to use static pattern matching.""" - raise self.make_type_error( - "enum equality is defined to be an error -- use .resolve_for_enum_variant() instead!") + """Redefine equality to avoid accidentally comparing against a non-enum.""" + if type(self) != type(other): + raise self.make_type_error( + "when comparing {!r} against {!r} with type '{}': " + "enum equality is only defined for instances of the same enum class!" + .format(self, other, type(other).__name__)) + return super(ChoiceDatatype, self).__eq__(other) # Redefine the canary so datatype __new__ doesn't raise. __eq__._eq_override_canary = None @@ -298,43 +299,6 @@ def __eq__(self, other): def __hash__(self): return super(ChoiceDatatype, self).__hash__() - @classmethod - def create(cls, *args, **kwargs): - """Create an instance of this enum, using the default value if specified. - - :param value: Use this as the enum value. If `value` is an instance of this class, return it, - otherwise it is checked against the enum's allowed values. This positional - argument is optional, and if not specified, `cls.default_value` is used. - :param bool none_is_default: If this is True, a None `value` is converted into - `cls.default_value` before being checked against the enum's - allowed values. - """ - none_is_default = kwargs.pop('none_is_default', False) - if kwargs: - raise ValueError('unrecognized keyword arguments for {}.create(): {!r}' - .format(cls.__name__, kwargs)) - - if len(args) == 0: - value = cls.default_value - elif len(args) == 1: - value = args[0] - if none_is_default and value is None: - value = cls.default_value - else: - raise ValueError('{}.create() accepts 0 or 1 positional args! *args = {!r}' - .format(cls.__name__, args)) - - # If we get an instance of this enum class, just return it. This means you can call .create() - # on an allowed value for the enum, or an existing instance of the enum. - if isinstance(value, cls): - return value - - if value not in cls._singletons: - raise cls.make_type_error( - "Value {!r} for '{}' must be one of: {!r}." - .format(value, field_name, cls._allowed_values)) - return cls._singletons[value] - def resolve_for_enum_variant(self, mapping): """Return the object in `mapping` with the key corresponding to the enum value. @@ -349,9 +313,17 @@ def resolve_for_enum_variant(self, mapping): raise self.make_type_error( "pattern matching must have exactly the keys {} (was: {})" .format(self._allowed_values, list(keys))) - match_for_variant = mapping[getattr(self, field_name)] + match_for_variant = mapping[self.underlying()] return match_for_variant + def underlying(self): + """Get the element of `all_values` corresponding to this enum instance. + + This should be used only for generating option values in tests. In general, it is less + error-prone to deal with enum objects directly. + """ + return getattr(self, field_name) + @classmethod def iterate_enum_variants(cls): """Iterate over all instances of this enum, in the declared order. @@ -359,18 +331,22 @@ def iterate_enum_variants(cls): NB: This method is exposed for testing enum variants easily. resolve_for_enum_variant() should be used for performing conditional logic based on an enum instance's value. """ - # TODO(#7232): use this method to register attributes on the generated type object for each of - # the singletons! return cls._singletons.values() + # Python requires creating an explicit closure to save the value on each loop iteration. + accessor_generator = lambda case: lambda cls: cls(case) + for case in all_values_realized: + if isinstance(case, six.string_types): + accessor = classproperty(accessor_generator(case)) + setattr(ChoiceDatatype, case, accessor) + return ChoiceDatatype # TODO(#7233): allow usage of the normal register() by using an enum class as the `type` argument! def register_enum_option(register, enum_cls, *args, **kwargs): """A helper method for declaring a pants option from an `enum()`.""" - default_value = kwargs.pop('default', enum_cls.default_value) - register(*args, choices=enum_cls._allowed_values, default=default_value, **kwargs) + register(*args, choices=enum_cls._allowed_values, **kwargs) # TODO: make these members of the `TypeConstraint` class! diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index e1d5259d63d..e320b35ced9 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -504,11 +504,11 @@ impl Snapshot { let glob_match_error_behavior = externs::project_ignoring_type(item, "glob_match_error_behavior"); - let failure_behavior = externs::project_str(&glob_match_error_behavior, "failure_behavior"); + let failure_behavior = externs::project_str(&glob_match_error_behavior, "value"); let strict_glob_matching = StrictGlobMatching::create(failure_behavior.as_str())?; let conjunction_obj = externs::project_ignoring_type(item, "conjunction"); - let conjunction_string = externs::project_str(&conjunction_obj, "conjunction"); + let conjunction_string = externs::project_str(&conjunction_obj, "value"); let conjunction = GlobExpansionConjunction::create(&conjunction_string)?; PathGlobs::create(&include, &exclude, strict_glob_matching, conjunction).map_err(|e| { diff --git a/tests/python/pants_test/backend/native/subsystems/test_libc_resolution.py b/tests/python/pants_test/backend/native/subsystems/test_libc_resolution.py index 7d424aa60d7..770ca7c7659 100644 --- a/tests/python/pants_test/backend/native/subsystems/test_libc_resolution.py +++ b/tests/python/pants_test/backend/native/subsystems/test_libc_resolution.py @@ -23,7 +23,7 @@ def setUp(self): }) self.libc = global_subsystem_instance(LibcDev) - self.platform = Platform.create() + self.platform = Platform.current def test_libc_search_failure(self): with self.assertRaises(LibcDev.HostLibcDevResolutionError) as cm: @@ -44,7 +44,7 @@ def setUp(self): }) self.libc = global_subsystem_instance(LibcDev) - self.platform = Platform.create() + self.platform = Platform.current def test_libc_disabled_search(self): self.assertEqual([], self.libc.get_libc_objects()) @@ -61,7 +61,7 @@ def setUp(self): }) self.libc = global_subsystem_instance(LibcDev) - self.platform = Platform.create() + self.platform = Platform.current @platform_specific('linux') def test_libc_compiler_search_failure(self): diff --git a/tests/python/pants_test/backend/native/subsystems/test_native_toolchain.py b/tests/python/pants_test/backend/native/subsystems/test_native_toolchain.py index d3f822fb042..8e02463afc3 100644 --- a/tests/python/pants_test/backend/native/subsystems/test_native_toolchain.py +++ b/tests/python/pants_test/backend/native/subsystems/test_native_toolchain.py @@ -36,7 +36,7 @@ def setUp(self): }, }) - self.platform = Platform.create() + self.platform = Platform.current self.toolchain = global_subsystem_instance(NativeToolchain) self.rules = native_backend_rules() diff --git a/tests/python/pants_test/backend/native/util/platform_utils.py b/tests/python/pants_test/backend/native/util/platform_utils.py index 293770a488b..fd7e00b41ec 100644 --- a/tests/python/pants_test/backend/native/util/platform_utils.py +++ b/tests/python/pants_test/backend/native/util/platform_utils.py @@ -14,11 +14,7 @@ def platform_specific(normalized_os_name): def decorator(test_fn): def wrapper(self, *args, **kwargs): - # TODO: This should be drawn from the v2 engine somehow. - platform = Platform.create() - - if platform.normalized_os_name == normalized_os_name: + if Platform.current == Platform(normalized_os_name): test_fn(self, *args, **kwargs) - return wrapper return decorator diff --git a/tests/python/pants_test/backend/python/targets/test_unpacked_whls.py b/tests/python/pants_test/backend/python/targets/test_unpacked_whls.py index f0aaf4bc556..e090619e21c 100644 --- a/tests/python/pants_test/backend/python/targets/test_unpacked_whls.py +++ b/tests/python/pants_test/backend/python/targets/test_unpacked_whls.py @@ -4,6 +4,7 @@ from __future__ import absolute_import, division, print_function, unicode_literals +from builtins import str from textwrap import dedent from pants.backend.python.python_requirement import PythonRequirement @@ -56,6 +57,9 @@ def test_has_all_imported_req_libs(self): def assert_dep(reqA, reqB): self.assertEqual(reqA.requirement, reqB.requirement) + def sort_requirements(reqs): + return list(sorted(reqs, key=lambda r: str(r.requirement))) + self.add_to_build_file('BUILD', dedent(''' python_requirement_library(name='lib1', requirements=[ @@ -80,13 +84,13 @@ def assert_dep(reqA, reqB): lib2 = self.target('//:lib2') self.assertIsInstance(lib2, PythonRequirementLibrary) - lib2_reqs = list(lib2.requirements) + lib2_reqs = sort_requirements(lib2.requirements) self.assertEqual(2, len(lib2_reqs)) assert_dep(lib2_reqs[0], PythonRequirement('testName2==456')) assert_dep(lib2_reqs[1], PythonRequirement('testName3==789')) unpacked_lib = self.target('//:unpacked-lib') - unpacked_req_libs = unpacked_lib.all_imported_requirements + unpacked_req_libs = sort_requirements(unpacked_lib.all_imported_requirements) self.assertEqual(3, len(unpacked_req_libs)) assert_dep(unpacked_req_libs[0], PythonRequirement('testName1==123')) diff --git a/tests/python/pants_test/backend/python/tasks/native/test_ctypes_integration.py b/tests/python/pants_test/backend/python/tasks/native/test_ctypes_integration.py index 4dd86c367e2..6ada723c8a1 100644 --- a/tests/python/pants_test/backend/python/tasks/native/test_ctypes_integration.py +++ b/tests/python/pants_test/backend/python/tasks/native/test_ctypes_integration.py @@ -56,7 +56,7 @@ def test_ctypes_binary_creation(self, toolchain_variant): 'pants_distdir': tmp_dir, }, 'native-build-step': { - 'toolchain_variant': toolchain_variant.value, + 'toolchain_variant': toolchain_variant.underlying(), }, }) @@ -96,7 +96,7 @@ def test_ctypes_binary_creation(self, toolchain_variant): dist_name, dist_version, wheel_platform = name_and_platform(wheel_dist) self.assertEqual(dist_name, 'ctypes_test') - contains_current_platform = Platform.create().resolve_for_enum_variant({ + contains_current_platform = Platform.current.resolve_for_enum_variant({ 'darwin': wheel_platform.startswith('macosx'), 'linux': wheel_platform.startswith('linux'), }) @@ -134,7 +134,7 @@ def test_ctypes_native_language_interop(self, toolchain_variant): # Explicitly set to True (although this is the default). config={ 'native-build-step': { - 'toolchain_variant': toolchain_variant.value, + 'toolchain_variant': toolchain_variant.underlying(), }, # TODO(#6848): don't make it possible to forget to add the toolchain_variant option! 'native-build-settings': { @@ -152,17 +152,14 @@ def test_ctypes_native_language_interop(self, toolchain_variant): # TODO(#6848): we need to provide the libstdc++.so.6.dylib which comes with gcc on osx in the # DYLD_LIBRARY_PATH during the 'run' goal somehow. - attempt_pants_run = Platform.create().resolve_for_enum_variant({ - 'darwin': toolchain_variant.resolve_for_enum_variant({ - 'gnu': False, - 'llvm': True, - }), + attempt_pants_run = Platform.current.resolve_for_enum_variant({ + 'darwin': toolchain_variant == ToolchainVariant.llvm, 'linux': True, }) if attempt_pants_run: pants_run_interop = self.run_pants(['-q', 'run', self._binary_target_with_interop], config={ 'native-build-step': { - 'toolchain_variant': toolchain_variant.value, + 'toolchain_variant': toolchain_variant.underlying(), }, 'native-build-settings': { 'strict_deps': True, @@ -175,24 +172,21 @@ def test_ctypes_native_language_interop(self, toolchain_variant): def test_ctypes_third_party_integration(self, toolchain_variant): pants_binary = self.run_pants(['binary', self._binary_target_with_third_party], config={ 'native-build-step': { - 'toolchain_variant': toolchain_variant.value, + 'toolchain_variant': toolchain_variant.underlying(), }, }) self.assert_success(pants_binary) # TODO(#6848): this fails when run with gcc on osx as it requires gcc's libstdc++.so.6.dylib to # be available on the runtime library path. - attempt_pants_run = Platform.create().resolve_for_enum_variant({ - 'darwin': toolchain_variant.resolve_for_enum_variant({ - 'gnu': False, - 'llvm': True, - }), + attempt_pants_run = Platform.current.resolve_for_enum_variant({ + 'darwin': toolchain_variant == ToolchainVariant.llvm, 'linux': True, }) if attempt_pants_run: pants_run = self.run_pants(['-q', 'run', self._binary_target_with_third_party], config={ 'native-build-step': { - 'toolchain_variant': toolchain_variant.value, + 'toolchain_variant': toolchain_variant.underlying(), }, }) self.assert_success(pants_run) @@ -231,11 +225,8 @@ def test_native_compiler_option_sets_integration(self, toolchain_variant): """ # TODO(#6848): this fails when run with gcc on osx as it requires gcc's libstdc++.so.6.dylib to # be available on the runtime library path. - attempt_pants_run = Platform.create().resolve_for_enum_variant({ - 'darwin': toolchain_variant.resolve_for_enum_variant({ - 'gnu': False, - 'llvm': True, - }), + attempt_pants_run = Platform.current.resolve_for_enum_variant({ + 'darwin': toolchain_variant == ToolchainVariant.llvm, 'linux': True, }) if not attempt_pants_run: @@ -247,7 +238,7 @@ def test_native_compiler_option_sets_integration(self, toolchain_variant): ] pants_run = self.run_pants(command=command, config={ 'native-build-step': { - 'toolchain_variant': toolchain_variant.value, + 'toolchain_variant': toolchain_variant.underlying(), }, 'native-build-step.cpp-compile-settings': { 'compiler_option_sets_enabled_args': { diff --git a/tests/python/pants_test/engine/legacy/test_graph_integration.py b/tests/python/pants_test/engine/legacy/test_graph_integration.py index f517d3953c0..572c8815d2f 100644 --- a/tests/python/pants_test/engine/legacy/test_graph_integration.py +++ b/tests/python/pants_test/engine/legacy/test_graph_integration.py @@ -61,12 +61,12 @@ def _list_target_check_warnings_sources(self, target_name): _ERR_TARGETS = { 'testprojects/src/python/sources:some-missing-some-not': [ "globs('*.txt', '*.rs')", - "Snapshot(PathGlobs(include=({unicode_literal}\'testprojects/src/python/sources/*.txt\', {unicode_literal}\'testprojects/src/python/sources/*.rs\'), exclude=(), glob_match_error_behavior=GlobMatchErrorBehavior(failure_behavior=error), conjunction=GlobExpansionConjunction(conjunction=all_match)))".format(unicode_literal='u' if PY2 else ''), + "Snapshot(PathGlobs(include=({unicode_literal}\'testprojects/src/python/sources/*.txt\', {unicode_literal}\'testprojects/src/python/sources/*.rs\'), exclude=(), glob_match_error_behavior=GlobMatchErrorBehavior(value=error), conjunction=GlobExpansionConjunction(value=all_match)))".format(unicode_literal='u' if PY2 else ''), "Globs did not match. Excludes were: []. Unmatched globs were: [\"testprojects/src/python/sources/*.rs\"].", ], 'testprojects/src/python/sources:missing-sources': [ "*.scala", - "Snapshot(PathGlobs(include=({unicode_literal}\'testprojects/src/python/sources/*.scala\',), exclude=({unicode_literal}\'testprojects/src/python/sources/*Test.scala\', {unicode_literal}\'testprojects/src/python/sources/*Spec.scala\'), glob_match_error_behavior=GlobMatchErrorBehavior(failure_behavior=error), conjunction=GlobExpansionConjunction(conjunction=any_match)))".format(unicode_literal='u' if PY2 else ''), + "Snapshot(PathGlobs(include=({unicode_literal}\'testprojects/src/python/sources/*.scala\',), exclude=({unicode_literal}\'testprojects/src/python/sources/*Test.scala\', {unicode_literal}\'testprojects/src/python/sources/*Spec.scala\'), glob_match_error_behavior=GlobMatchErrorBehavior(value=error), conjunction=GlobExpansionConjunction(value=any_match)))".format(unicode_literal='u' if PY2 else ''), "Globs did not match. Excludes were: [\"testprojects/src/python/sources/*Test.scala\", \"testprojects/src/python/sources/*Spec.scala\"]. Unmatched globs were: [\"testprojects/src/python/sources/*.scala\"].", ], 'testprojects/src/java/org/pantsbuild/testproject/bundle:missing-bundle-fileset': [ @@ -75,7 +75,7 @@ def _list_target_check_warnings_sources(self, target_name): "Globs('*.aaaa')", "ZGlobs('**/*.abab')", "['file1.aaaa', 'file2.aaaa']", - "Snapshot(PathGlobs(include=({unicode_literal}\'testprojects/src/java/org/pantsbuild/testproject/bundle/*.aaaa\',), exclude=(), glob_match_error_behavior=GlobMatchErrorBehavior(failure_behavior=error), conjunction=GlobExpansionConjunction(conjunction=all_match)))".format(unicode_literal='u' if PY2 else ''), + "Snapshot(PathGlobs(include=({unicode_literal}\'testprojects/src/java/org/pantsbuild/testproject/bundle/*.aaaa\',), exclude=(), glob_match_error_behavior=GlobMatchErrorBehavior(value=error), conjunction=GlobExpansionConjunction(value=all_match)))".format(unicode_literal='u' if PY2 else ''), "Globs did not match. Excludes were: []. Unmatched globs were: [\"testprojects/src/java/org/pantsbuild/testproject/bundle/*.aaaa\"].", ] } diff --git a/tests/python/pants_test/engine/test_fs.py b/tests/python/pants_test/engine/test_fs.py index ecfdf29afc6..f4044d57605 100644 --- a/tests/python/pants_test/engine/test_fs.py +++ b/tests/python/pants_test/engine/test_fs.py @@ -17,6 +17,7 @@ MergedDirectories, PathGlobs, PathGlobsAndRoot, Snapshot, UrlToFetch, create_fs_rules) from pants.engine.scheduler import ExecutionError +from pants.option.global_options import GlobMatchErrorBehavior from pants.util.contextutil import http_server, temporary_dir from pants.util.dirutil import relative_symlink from pants.util.meta import AbstractClass @@ -375,7 +376,7 @@ def test_glob_match_error(self): self.assert_walk_files(PathGlobs( include=['not-a-file.txt'], exclude=[], - glob_match_error_behavior='error', + glob_match_error_behavior=GlobMatchErrorBehavior.error, ), []) expected_msg = ( "Globs did not match. Excludes were: []. Unmatched globs were: [\"not-a-file.txt\"].") @@ -386,7 +387,7 @@ def test_glob_match_exclude_error(self): self.assert_walk_files(PathGlobs( include=['*.txt'], exclude=['4.txt'], - glob_match_error_behavior='error', + glob_match_error_behavior=GlobMatchErrorBehavior.error, ), []) expected_msg = ( "Globs did not match. Excludes were: [\"4.txt\"]. Unmatched globs were: [\"*.txt\"].") @@ -397,7 +398,7 @@ def test_glob_match_ignore_logging(self): self.assert_walk_files(PathGlobs( include=['not-a-file.txt'], exclude=[''], - glob_match_error_behavior='ignore', + glob_match_error_behavior=GlobMatchErrorBehavior.ignore, ), []) self.assertEqual(0, len(captured.warnings())) @@ -407,7 +408,7 @@ def test_glob_match_warn_logging(self): self.assert_walk_files(PathGlobs( include=['not-a-file.txt'], exclude=[''], - glob_match_error_behavior='warn', + glob_match_error_behavior=GlobMatchErrorBehavior.warn, ), []) all_warnings = captured.warnings() self.assertEqual(1, len(all_warnings)) diff --git a/tests/python/pants_test/util/test_objects.py b/tests/python/pants_test/util/test_objects.py index 8cb1423c772..4d53b205c87 100644 --- a/tests/python/pants_test/util/test_objects.py +++ b/tests/python/pants_test/util/test_objects.py @@ -287,7 +287,7 @@ def __eq__(self, other): return NotImplemented -class SomeEnum(enum('x', [1, 2])): pass +class SomeEnum(enum([1, 2])): pass class DatatypeTest(TestBase): @@ -709,46 +709,33 @@ def test_enum_class_creation_errors(self): "When converting all_values ([1, 2, 3, 1]) to a set, at least one duplicate " "was detected. The unique elements of all_values were: [1, 2, 3].") with self.assertRaisesRegexp(ValueError, expected_rx): - class DuplicateAllowedValues(enum('x', [1, 2, 3, 1])): pass + class DuplicateAllowedValues(enum([1, 2, 3, 1])): pass def test_enum_instance_creation(self): - self.assertEqual(1, SomeEnum.create().x) - self.assertEqual(2, SomeEnum.create(2).x) - self.assertEqual(1, SomeEnum(1).x) + self.assertEqual(2, SomeEnum(2).value) + self.assertEqual(SomeEnum(2), SomeEnum(value=2)) - def test_enum_instance_creation_errors(self): expected_rx = re.escape( - "Value 3 for 'x' must be one of: [1, 2].") - with self.assertRaisesRegexp(EnumVariantSelectionError, expected_rx): - SomeEnum.create(3) + "Value 3 must be one of: [1, 2].") with self.assertRaisesRegexp(EnumVariantSelectionError, expected_rx): SomeEnum(3) - # Specifying the value by keyword argument is not allowed. - with self.assertRaisesRegexp(TypeError, re.escape("__new__() got an unexpected keyword argument 'x'")): - SomeEnum(x=3) - - # Test that None is not used as the default unless none_is_default=True. - with self.assertRaisesRegexp(EnumVariantSelectionError, re.escape( - "Value None for 'x' must be one of: [1, 2]." - )): - SomeEnum.create(None) - self.assertEqual(1, SomeEnum.create(None, none_is_default=True).x) - - expected_rx_falsy_value = re.escape( - "Value {}'' for 'x' must be one of: [1, 2]." - .format('u' if PY2 else '')) - with self.assertRaisesRegexp(EnumVariantSelectionError, expected_rx_falsy_value): - SomeEnum('') + def test_enum_generated_attrs(self): + class HasAttrs(enum(['a', 'b'])): pass + self.assertEqual(HasAttrs.a, HasAttrs('a')) + self.assertEqual(type(HasAttrs.a), HasAttrs) + self.assertEqual(HasAttrs.b, HasAttrs('b')) - def test_enum_comparison_fails(self): + def test_enum_comparison(self): enum_instance = SomeEnum(1) - rx_str = re.escape("enum equality is defined to be an error") - with self.assertRaisesRegexp(TypeCheckError, rx_str): - enum_instance == enum_instance - with self.assertRaisesRegexp(TypeCheckError, rx_str): - enum_instance != enum_instance - # Test that comparison also fails against another type. + another_enum_instance = SomeEnum(2) + self.assertEqual(enum_instance, enum_instance) + self.assertNotEqual(enum_instance, another_enum_instance) + + # Test that comparison fails against another type. + rx_str = re.escape( + "when comparing SomeEnum(value=1) against 1 with type 'int': " + "enum equality is only defined for instances of the same enum class!") with self.assertRaisesRegexp(TypeCheckError, rx_str): enum_instance == 1 with self.assertRaisesRegexp(TypeCheckError, rx_str): @@ -756,6 +743,10 @@ def test_enum_comparison_fails(self): class StrEnum(enum(['a'])): pass enum_instance = StrEnum('a') + rx_str = re.escape( + "when comparing StrEnum(value={u}'a') against {u}'a' with type '{string_type}': " + "enum equality is only defined for instances of the same enum class!" + .format(u='u' if PY2 else '', string_type='unicode' if PY2 else 'str')) with self.assertRaisesRegexp(TypeCheckError, rx_str): enum_instance == 'a' with self.assertRaisesRegexp(TypeCheckError, rx_str):