context: Move 'CacheBuildTrees' to a FastEnum

This allows removing completely the '_node_get_option_str' on context
and ensures every method relying on a few set of keys set their errors
consistently

4 files changed, 26 insertions, 36 deletions

diff --git a/src/buildstream/_context.py b/src/buildstream/_context.py
index 4d68ef222..c1a3b0619 100644
--- a/src/buildstream/_context.py
+++ b/src/buildstream/_context.py
@@ -29,7 +29,7 @@ from ._platform import Platform
 from ._artifactcache import ArtifactCache
 from ._sourcecache import SourceCache
 from ._cas import CASCache, CASQuota, CASCacheUsage
-from .types import _SchedulerErrorAction
+from .types import _CacheBuildTrees, _SchedulerErrorAction
 from ._workspaces import Workspaces, WorkspaceProjectCache
 from .node import Node
 from .sandbox import SandboxRemote
@@ -299,8 +299,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')
@@ -503,30 +502,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("{}: {} should be one of: {}".format(provenance, key, ", ".join(allowed_options)),
-                        LoadErrorReason.INVALID_DATA)
-    return result
diff --git a/src/buildstream/_frontend/cli.py b/src/buildstream/_frontend/cli.py
index 5de02918f..276f81a6a 100644
--- a/src/buildstream/_frontend/cli.py
+++ b/src/buildstream/_frontend/cli.py
@@ -10,7 +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 _SchedulerErrorAction
+from ..types import _CacheBuildTrees, _SchedulerErrorAction
 from ..utils import _get_compression, UtilError
 
 
@@ -285,7 +285,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=FastEnumType(_CacheBuildTrees),
               help="Cache artifact build tree content on creation")
 @click.pass_context
 def cli(context, **kwargs):
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index ffdd1511e..df96d04ba 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
@@ -1617,8 +1617,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:
@@ -1701,7 +1701,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/types.py b/src/buildstream/types.py
index 08e2b0d08..0635f310e 100644
--- a/src/buildstream/types.py
+++ b/src/buildstream/types.py
@@ -208,3 +208,20 @@ class _SchedulerErrorAction(FastEnum):
 
     # 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"