From 9d2536c7683a800e496853e911e2eccd16ab9ddf Mon Sep 17 00:00:00 2001 From: Benjamin Schubert Date: Wed, 17 Jul 2019 12:03:44 +0100 Subject: node: Add 'as_enum' on ScalarNode and 'get_enum' helper on MappingNode This adds a value to ensure that a value is from a set of valid values and raises an error message accordingly. - Define Enum types for each of the relevant cases - Adapt all call places that were doing such things manually --- src/buildstream/_context.py | 35 ++-------------- src/buildstream/_frontend/cli.py | 5 ++- src/buildstream/_gitsourcebase.py | 13 +++--- src/buildstream/_options/optionpool.py | 14 +++---- src/buildstream/_project.py | 21 +++++----- src/buildstream/_types.pyx | 3 ++ src/buildstream/element.py | 9 +++-- src/buildstream/node.pxd | 2 + src/buildstream/node.pyx | 73 ++++++++++++++++++++++++++++++++++ src/buildstream/types.py | 33 +++++++++++++++ 10 files changed, 148 insertions(+), 60 deletions(-) diff --git a/src/buildstream/_context.py b/src/buildstream/_context.py index 3c20834b0..3b53654ed 100644 --- a/src/buildstream/_context.py +++ b/src/buildstream/_context.py @@ -29,6 +29,7 @@ from ._platform import Platform from ._artifactcache import ArtifactCache from ._sourcecache import SourceCache from ._cas import CASCache, CASQuota, CASCacheUsage +from .types import _CacheBuildTrees, _SchedulerErrorAction from ._workspaces import Workspaces, WorkspaceProjectCache from .node import Node from .sandbox import SandboxRemote @@ -297,8 +298,7 @@ class Context(): self.pull_buildtrees = cache.get_bool('pull-buildtrees') # Load cache build trees configuration - self.cache_buildtrees = _node_get_option_str( - cache, 'cache-buildtrees', ['always', 'auto', 'never']) + self.cache_buildtrees = cache.get_enum('cache-buildtrees', _CacheBuildTrees) # Load logging config logging = defaults.get_mapping('logging') @@ -322,8 +322,7 @@ class Context(): 'on-error', 'fetchers', 'builders', 'pushers', 'network-retries' ]) - self.sched_error_action = _node_get_option_str( - scheduler, 'on-error', ['continue', 'quit', 'terminate']) + self.sched_error_action = scheduler.get_enum('on-error', _SchedulerErrorAction) self.sched_fetchers = scheduler.get_int('fetchers') self.sched_builders = scheduler.get_int('builders') self.sched_pushers = scheduler.get_int('pushers') @@ -502,31 +501,3 @@ class Context(): if self._casquota is None: self._casquota = CASQuota(self) return self._casquota - - -# _node_get_option_str() -# -# Like Node.get_scalar().as_str(), but also checks value is one of the allowed option -# strings. Fetches a value from a dictionary node, and makes sure it's one of -# the pre-defined options. -# -# Args: -# node (dict): The dictionary node -# key (str): The key to get a value for in node -# allowed_options (iterable): Only accept these values -# -# Returns: -# The value, if found in 'node'. -# -# Raises: -# LoadError, when the value is not of the expected type, or is not found. -# -def _node_get_option_str(node, key, allowed_options): - result_node = node.get_scalar(key) - result = result_node.as_str() - if result not in allowed_options: - provenance = result_node.get_provenance() - raise LoadError(LoadErrorReason.INVALID_DATA, - "{}: {} should be one of: {}".format( - provenance, key, ", ".join(allowed_options))) - return result diff --git a/src/buildstream/_frontend/cli.py b/src/buildstream/_frontend/cli.py index bf92161bf..9c12fcb07 100644 --- a/src/buildstream/_frontend/cli.py +++ b/src/buildstream/_frontend/cli.py @@ -10,6 +10,7 @@ from .. import _yaml from .._exceptions import BstError, LoadError, AppError from .._versions import BST_FORMAT_VERSION from .complete import main_bashcomplete, complete_path, CompleteUnhandled +from ..types import _CacheBuildTrees, _SchedulerErrorAction from ..utils import _get_compression, UtilError @@ -234,7 +235,7 @@ def print_version(ctx, param, value): type=click.Path(file_okay=False, readable=True), help="Project directory (default: current directory)") @click.option('--on-error', default=None, - type=click.Choice(['continue', 'quit', 'terminate']), + type=click.Choice(_SchedulerErrorAction.values), help="What to do when an error is encountered") @click.option('--fetchers', type=click.INT, default=None, help="Maximum simultaneous download tasks") @@ -270,7 +271,7 @@ def print_version(ctx, param, value): @click.option('--pull-buildtrees', is_flag=True, default=None, help="Include an element's build tree when pulling remote element artifacts") @click.option('--cache-buildtrees', default=None, - type=click.Choice(['always', 'auto', 'never']), + type=click.Choice(_CacheBuildTrees.values), help="Cache artifact build tree content on creation") @click.pass_context def cli(context, **kwargs): diff --git a/src/buildstream/_gitsourcebase.py b/src/buildstream/_gitsourcebase.py index fb6010b1e..36e8a6ae7 100644 --- a/src/buildstream/_gitsourcebase.py +++ b/src/buildstream/_gitsourcebase.py @@ -32,6 +32,7 @@ from configparser import RawConfigParser from .source import Source, SourceError, SourceFetcher from .types import Consistency, CoreWarnings from . import utils +from .types import FastEnum from .utils import move_atomic, DirectoryExistsError GIT_MODULES = '.gitmodules' @@ -42,6 +43,11 @@ WARN_UNLISTED_SUBMODULE = "unlisted-submodule" WARN_INVALID_SUBMODULE = "invalid-submodule" +class _RefFormat(FastEnum): + SHA1 = "sha1" + GIT_DESCRIBE = "git-describe" + + # Because of handling of submodules, we maintain a _GitMirror # for the primary git source and also for each submodule it # might have at a given time @@ -156,7 +162,7 @@ class _GitMirror(SourceFetcher): cwd=self.mirror) ref = output.rstrip('\n') - if self.source.ref_format == 'git-describe': + if self.source.ref_format == _RefFormat.GIT_DESCRIBE: # Prefix the ref with the closest tag, if available, # to make the ref human readable exit_code, output = self.source.check_output( @@ -394,10 +400,7 @@ class _GitSourceBase(Source): self.mirror = self.BST_MIRROR_CLASS(self, '', self.original_url, ref, tags=tags, primary=True) self.tracking = node.get_str('track', None) - self.ref_format = node.get_str('ref-format', 'sha1') - if self.ref_format not in ['sha1', 'git-describe']: - provenance = node.get_scalar('ref-format').get_provenance() - raise SourceError("{}: Unexpected value for ref-format: {}".format(provenance, self.ref_format)) + self.ref_format = node.get_enum("ref-format", _RefFormat, default=_RefFormat.SHA1) # At this point we now know if the source has a ref and/or a track. # If it is missing both then we will be unable to track or build. diff --git a/src/buildstream/_options/optionpool.py b/src/buildstream/_options/optionpool.py index d7541530b..98e8393e6 100644 --- a/src/buildstream/_options/optionpool.py +++ b/src/buildstream/_options/optionpool.py @@ -18,6 +18,8 @@ # Tristan Van Berkom # +import enum + import jinja2 from .._exceptions import LoadError, LoadErrorReason @@ -40,6 +42,9 @@ _OPTION_TYPES = { } +OptionTypes = enum.Enum("OptionTypes", {key: key for key in _OPTION_TYPES}, type=str) + + class OptionPool(): def __init__(self, element_path): @@ -70,13 +75,8 @@ class OptionPool(): # Assert that the option name is a valid symbol _assert_symbol_name(option_name, "option name", ref_node=option_definition, allow_dashes=False) - opt_type_name = option_definition.get_str('type') - try: - opt_type = _OPTION_TYPES[opt_type_name] - except KeyError: - p = option_definition.get_scalar('type').get_provenance() - raise LoadError(LoadErrorReason.INVALID_DATA, - "{}: Invalid option type '{}'".format(p, opt_type_name)) + opt_type_name = option_definition.get_enum('type', OptionTypes) + opt_type = _OPTION_TYPES[opt_type_name.value] option = opt_type(option_name, option_definition, self) self._options[option_name] = option diff --git a/src/buildstream/_project.py b/src/buildstream/_project.py index fa02143e1..6fed36eca 100644 --- a/src/buildstream/_project.py +++ b/src/buildstream/_project.py @@ -34,6 +34,7 @@ from ._artifactcache import ArtifactCache from ._sourcecache import SourceCache from .node import ScalarNode, SequenceNode, _assert_symbol_name from .sandbox import SandboxRemote +from .types import FastEnum from ._elementfactory import ElementFactory from ._sourcefactory import SourceFactory from .types import CoreWarnings @@ -50,6 +51,12 @@ from ._workspaces import WORKSPACE_PROJECT_FILE _PROJECT_CONF_FILE = 'project.conf' +class PluginOrigins(FastEnum): + CORE = "core" + LOCAL = "local" + PIP = "pip" + + # HostMount() # # A simple object describing the behavior of @@ -869,16 +876,8 @@ class Project(): 'origin', 'sources', 'elements', 'package-name', 'path', ] - allowed_origins = ['core', 'local', 'pip'] origin.validate_keys(allowed_origin_fields) - origin_value = origin.get_str('origin') - if origin_value not in allowed_origins: - raise LoadError( - LoadErrorReason.INVALID_YAML, - "Origin '{}' is not one of the allowed types" - .format(origin_value)) - # Store source versions for checking later source_versions = origin.get_mapping('sources', default={}) for key in source_versions.keys(): @@ -899,7 +898,9 @@ class Project(): # Store the origins if they're not 'core'. # core elements are loaded by default, so storing is unnecessary. - if origin.get_str('origin') != 'core': + origin_value = origin.get_enum('origin', PluginOrigins) + + if origin_value != PluginOrigins.CORE: self._store_origin(origin, 'sources', plugin_source_origins) self._store_origin(origin, 'elements', plugin_element_origins) @@ -939,7 +940,7 @@ class Project(): if group in origin_node: del origin_node[group] - if origin_node.get_str('origin') == 'local': + if origin_node.get_enum('origin', PluginOrigins) == PluginOrigins.LOCAL: path = self.get_path_from_node(origin.get_scalar('path'), check_is_dir=True) # paths are passed in relative to the project, but must be absolute diff --git a/src/buildstream/_types.pyx b/src/buildstream/_types.pyx index 93649c8d8..1bcf2cc35 100644 --- a/src/buildstream/_types.pyx +++ b/src/buildstream/_types.pyx @@ -22,6 +22,7 @@ class MetaFastEnum(type): @classmethod def set_values(mcs, kls, data): value_to_entry = {} + values = [] assert len(set(data.values())) == len(data.values()), "Values for {} are not unique".format(kls) assert len(set(type(value) for value in data.values())) <= 1, \ @@ -35,8 +36,10 @@ class MetaFastEnum(type): type.__setattr__(kls, key, new_value) value_to_entry[value] = new_value + values.append(new_value) type.__setattr__(kls, "_value_to_entry", value_to_entry) + type.__setattr__(kls, "values", values) def __repr__(self): return "".format(self.__name__) diff --git a/src/buildstream/element.py b/src/buildstream/element.py index dffc33ba0..90937c21c 100644 --- a/src/buildstream/element.py +++ b/src/buildstream/element.py @@ -102,7 +102,7 @@ from .plugin import Plugin from .sandbox import SandboxFlags, SandboxCommandError from .sandbox._config import SandboxConfig from .sandbox._sandboxremote import SandboxRemote -from .types import Consistency, CoreWarnings, Scope, _KeyStrength +from .types import Consistency, CoreWarnings, Scope, _CacheBuildTrees, _KeyStrength from ._artifact import Artifact from .storage.directory import Directory @@ -1608,8 +1608,8 @@ class Element(Plugin): # download when it's not needed. buildroot = self.get_variable('build-root') cache_buildtrees = context.cache_buildtrees - if cache_buildtrees != 'never': - always_cache_buildtrees = cache_buildtrees == 'always' + if cache_buildtrees != _CacheBuildTrees.NEVER: + always_cache_buildtrees = cache_buildtrees == _CacheBuildTrees.ALWAYS sandbox._set_build_directory(buildroot, always=always_cache_buildtrees) if not self.BST_RUN_COMMANDS: @@ -1692,7 +1692,8 @@ class Element(Plugin): # result. Element types without a build-root dir will be cached # with an empty buildtreedir regardless of this configuration. - if cache_buildtrees == 'always' or (cache_buildtrees == 'auto' and not build_success): + if (cache_buildtrees == _CacheBuildTrees.ALWAYS or + (cache_buildtrees == _CacheBuildTrees.AUTO and not build_success)): try: sandbox_build_dir = sandbox_vroot.descend( *self.get_variable('build-root').lstrip(os.sep).split(os.sep)) diff --git a/src/buildstream/node.pxd b/src/buildstream/node.pxd index 18520146d..02e95d06f 100644 --- a/src/buildstream/node.pxd +++ b/src/buildstream/node.pxd @@ -46,6 +46,7 @@ cdef class MappingNode(Node): # Public Methods cpdef bint get_bool(self, str key, default=*) except * + cpdef object get_enum(self, str key, object constraint, object default=*) cpdef int get_int(self, str key, default=*) except * cpdef MappingNode get_mapping(self, str key, default=*) cpdef Node get_node(self, str key, list allowed_types=*, bint allow_none=*) @@ -78,6 +79,7 @@ cdef class ScalarNode(Node): # Public Methods cpdef bint as_bool(self) except * + cpdef object as_enum(self, object constraint) cpdef int as_int(self) except * cpdef str as_str(self) cpdef bint is_none(self) diff --git a/src/buildstream/node.pyx b/src/buildstream/node.pyx index fc17b8efa..5d9f98679 100644 --- a/src/buildstream/node.pyx +++ b/src/buildstream/node.pyx @@ -323,6 +323,43 @@ cdef class ScalarNode(Node): "{}: Value of '{}' is not of the expected type '{}'" .format(provenance, path, bool.__name__, self.value)) + cpdef object as_enum(self, object constraint): + """Get the value of the node as an enum member from `constraint` + + The constraint must be a python :class:`Enum` with one value matching the value of the node. + + For example you could do: + + .. code-block:: python + + from enum import Enum + + class SupportedCompressions(Enum): + NONE = "none" + GZIP = "gzip" + XZ = "xz" + + + x = config.get_scalar('compress').as_enum(SupportedCompressions) + + if x == SupportedCompressions.GZIP: + print("Using GZIP") + + Args: + constraint (:class:`Enum`): an enum from which to extract the value for the current node. + + Returns: + :class:`Enum`: the value contained in the node, as a member of `constraint` + """ + try: + return constraint(self.value) + except ValueError: + provenance = self.get_provenance() + path = provenance._toplevel._find(self)[-1] + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Value of '{}' should be one of {}".format( + provenance, path, ", ".join(constraint))) + cpdef int as_int(self) except *: """Get the value of the node as an integer. @@ -499,6 +536,42 @@ cdef class MappingNode(Node): cdef ScalarNode scalar = self.get_scalar(key, default) return scalar.as_bool() + cpdef object get_enum(self, str key, object constraint, object default=_sentinel): + """Get the value of the node as an enum member from `constraint` + + Args: + key (str): key for which to get the value + constraint (:class:`Enum`): an enum from which to extract the value for the current node. + default (object): default value to return if `key` is not in the mapping + + Raises: + :class:`buildstream._exceptions.LoadError`: if the value is not is not found or not part of the + provided enum. + + Returns: + :class:`Enum`: the value contained in the node, as a member of `constraint` + """ + cdef object value = self.value.get(key, _sentinel) + + if value is _sentinel: + if default is _sentinel: + provenance = self.get_provenance() + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Dictionary did not contain expected key '{}'".format(provenance, key)) + + if default is None: + return None + else: + return constraint(default) + + if type(value) is not ScalarNode: + provenance = value.get_provenance() + raise LoadError(LoadErrorReason.INVALID_DATA, + "{}: Value of '{}' is not of the expected type 'scalar'" + .format(provenance, key)) + + return ( value).as_enum(constraint) + cpdef int get_int(self, str key, object default=_sentinel) except *: """get_int(key, default=sentinel) diff --git a/src/buildstream/types.py b/src/buildstream/types.py index b711fd570..8babf9184 100644 --- a/src/buildstream/types.py +++ b/src/buildstream/types.py @@ -125,3 +125,36 @@ class _KeyStrength(FastEnum): # Includes names of direct build dependencies but does not include # cache keys of dependencies. WEAK = 2 + + +# _SchedulerErrorAction() +# +# Actions the scheduler can take on error +# +class _SchedulerErrorAction(FastEnum): + + # Continue building the rest of the tree + CONTINUE = "continue" + + # finish ongoing work and quit + QUIT = "quit" + + # Abort immediately + TERMINATE = "terminate" + + +# _CacheBuildTrees() +# +# When to cache build trees +# +class _CacheBuildTrees(FastEnum): + + # Always store build trees + ALWAYS = "always" + + # Store build trees when they might be useful for BuildStream + # (eg: on error, to allow for a shell to debug that) + AUTO = "auto" + + # Never cache build trees + NEVER = "never" -- cgit v1.2.1