sandbox/_config.py, element.py: Refactor SandboxConfig

This commit changes SandboxConfig such that it now has a simple constructor
and a new SandboxConfig.new_from_node() classmethod to load it from a YAML
configuration node. The new version of SandboxConfig now uses type annotations.

SandboxConfig also now sports a to_dict() method to help in serialization in
artifacts, this replaces SandboxConfig.get_unique_key() since it does exactly
the same thing, but uses the same names as expected in the YAML configuration
to achieve it.

The element.py code has been updated to use the classmethod, and to
use the to_dict() method when constructing cache keys.

This refactor is meant to allow instantiating a SandboxConfig without
any MappingNode, such that we can later load a SandboxConfig from an
Artifact instead of from an parsed Element.

This commit also updates the cache keys in the cache key test, as
the cache key format is slightly changed by the to_dict() method.

2 files changed, 103 insertions, 32 deletions

diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index 6c4e45d03..595017052 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -347,7 +347,7 @@ class Element(Plugin):
         # Extract Sandbox config
         sandbox_config = self.__extract_sandbox_config(project, load_element)
         self.__variables.expand(sandbox_config)
-        self.__sandbox_config = SandboxConfig(sandbox_config, context.platform)
+        self.__sandbox_config = SandboxConfig.new_from_node(sandbox_config, platform=context.platform)
 
     def __lt__(self, other):
         return self.name < other.name
@@ -2249,7 +2249,7 @@ class Element(Plugin):
                 "element-plugin-key": self.get_unique_key(),
                 "element-plugin-name": self.get_kind(),
                 "element-plugin-version": self.BST_ARTIFACT_VERSION,
-                "sandbox": self.__sandbox_config.get_unique_key(),
+                "sandbox": self.__sandbox_config.to_dict(),
                 "environment": cache_env,
                 "public": self.__public.strip_node_info(),
             }
diff --git a/src/buildstream/sandbox/_config.py b/src/buildstream/sandbox/_config.py
index 114274190..6f8323453 100644
--- a/src/buildstream/sandbox/_config.py
+++ b/src/buildstream/sandbox/_config.py
@@ -1,5 +1,5 @@
 #
-#  Copyright (C) 2018 Codethink Limited
+#  Copyright (C) 2020 Codethink Limited
 #
 #  This program is free software; you can redistribute it and/or
 #  modify it under the terms of the GNU Lesser General Public
@@ -16,52 +16,123 @@
 #
 #  Authors:
 #        Jim MacArthur <jim.macarthur@codethink.co.uk>
+#        Tristan Van Berkom <tristan.vanberkom@codethink.co.uk>
+#
 
+from typing import TYPE_CHECKING, Dict, Optional, Union
 from .._platform import Platform
 
+if TYPE_CHECKING:
+    from ..node import Node, MappingNode
+
 
 # SandboxConfig
 #
-# A container for sandbox configuration data. We want the internals
-# of this to be opaque, hence putting it in its own private file.
+# The Sandbox configuration parameters, this object carries configuration
+# required to instantiate the correct type of sandbox, and assert that
+# the local or remote worker sandbox has the capabilities required.
+#
+# Args:
+#    build_os: The build OS name
+#    build_arch: A canonical machine architecture name, as defined by Platform.canonicalize_arch()
+#    build_uid: The UID for the sandbox process
+#    build_gid: The GID for the sandbox process
+#
+# If the build_uid or build_gid is unspecified, then the underlying sandbox implementation
+# does not guarantee what UID/GID will be used, but generally UID/GID 0 will be used in a
+# sandbox implementation which supports UID/GID control.
+#
+# If the build_uid or build_gid is specified, then the UID/GID is guaranteed to match
+# the specified UID/GID, if the underlying sandbox implementation does not support UID/GID
+# control, then an error will be raised when attempting to configure the sandbox.
+#
 class SandboxConfig:
-    def __init__(self, sandbox_config, platform):
-        host_arch = platform.get_host_arch()
-        host_os = platform.get_host_os()
+    def __init__(
+        self, *, build_os: str, build_arch: str, build_uid: Optional[int] = None, build_gid: Optional[int] = None
+    ):
+        self.build_os = build_os
+        self.build_arch = build_arch
+        self.build_uid = build_uid
+        self.build_gid = build_gid
 
-        sandbox_config.validate_keys(["build-uid", "build-gid", "build-os", "build-arch"])
+    # to_dict():
+    #
+    # Represent the SandboxConfig as a dictionary.
+    #
+    # This dictionary will be stored in the corresponding artifact
+    # whenever an artifact is cached. When loading an element from
+    # an artifact, then this dict will be loaded as a MappingNode
+    # and interpreted by SandboxConfig.new_from_node().
+    #
+    # This function is also used to contribute to the owning element's cache key.
+    #
+    # Returns:
+    #    A dictionary representation of this SandboxConfig
+    #
+    def to_dict(self) -> Dict[str, Union[str, int]]:
 
-        build_os = sandbox_config.get_str("build-os", default=None)
-        if build_os:
-            self.build_os = build_os.lower()
-        else:
-            self.build_os = host_os
+        # Assign mandatory portions of the sandbox configuration
+        #
+        # /!\ No additional mandatory members can ever be added to
+        #     the sandbox configuration, as that would result in
+        #     breaking cache key stability.
+        #
+        sandbox_dict: Dict[str, Union[str, int]] = {"build-os": self.build_os, "build-arch": self.build_arch}
 
-        build_arch = sandbox_config.get_str("build-arch", default=None)
-        if build_arch:
-            self.build_arch = Platform.canonicalize_arch(build_arch)
-        else:
-            self.build_arch = host_arch
+        # Assign optional portions of the sandbox configuration
+        #
+        # /!\ In order to preserve cache key stability, these attributes
+        #     are only ever added to the dictionary if they have been
+        #     explicitly set, unset values must not affect the dictionary.
+        #
+        if self.build_uid is not None:
+            sandbox_dict["build-uid"] = self.build_uid
+        if self.build_gid is not None:
+            sandbox_dict["build-gid"] = self.build_gid
 
-        self.build_uid = sandbox_config.get_int("build-uid", None)
-        self.build_gid = sandbox_config.get_int("build-gid", None)
+        return sandbox_dict
 
-    # get_unique_key():
+    # new_from_node():
+    #
+    # Instantiate a new SandboxConfig from YAML configuration.
     #
-    # This returns the SandboxConfig's contribution
-    # to an element's cache key.
+    # If the Platform is specified, then we expect to be loading
+    # from project definitions, and some defaults will be derived
+    # from the Platform. Otherwise, we expect to be loading from
+    # a cached artifact, and values are expected to exist on the
+    # given node.
+    #
+    # Args:
+    #    config: The YAML configuration node
+    #    platform: The host Platform instance, or None
     #
     # Returns:
-    #    (dict): A dictionary to add to an element's cache key
+    #    A new SandboxConfig instance
     #
-    def get_unique_key(self):
+    @classmethod
+    def new_from_node(cls, config: "MappingNode[Node]", *, platform: Optional[Platform] = None) -> "SandboxConfig":
+        config.validate_keys(["build-uid", "build-gid", "build-os", "build-arch"])
 
-        unique_key = {"os": self.build_os, "arch": self.build_arch}
+        build_os: str
+        build_arch: str
 
-        if self.build_uid is not None:
-            unique_key["build-uid"] = self.build_uid
+        if platform:
+            tmp = config.get_str("build-os", None)
+            if tmp:
+                build_os = tmp.lower()
+            else:
+                build_os = platform.get_host_os()
 
-        if self.build_gid is not None:
-            unique_key["build-gid"] = self.build_gid
+            tmp = config.get_str("build-arch", None)
+            if tmp:
+                build_arch = Platform.canonicalize_arch(tmp)
+            else:
+                build_arch = platform.get_host_arch()
+        else:
+            build_os = config.get_str("build-os")
+            build_arch = config.get_str("build-arch")
+
+        build_uid = config.get_int("build-uid", None)
+        build_gid = config.get_int("build-gid", None)
 
-        return unique_key
+        return cls(build_os=build_os, build_arch=build_arch, build_uid=build_uid, build_gid=build_gid)