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

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 <tristan.vanberkom@codethink.co.uk>
 #
 
+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 "<fastenum '{}'>".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 (<ScalarNode> 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"