Update node property support to match proto changes

9 files changed, 87 insertions, 59 deletions

diff --git a/src/buildstream/_artifact.py b/src/buildstream/_artifact.py
index 0a70d096f..cf82a1636 100644
--- a/src/buildstream/_artifact.py
+++ b/src/buildstream/_artifact.py
@@ -165,7 +165,7 @@ class Artifact:
         artifact.weak_key = self._weak_cache_key
 
         artifact.was_workspaced = bool(element._get_workspace())
-        properties = ["MTime"] if artifact.was_workspaced else []
+        properties = ["mtime"] if artifact.was_workspaced else []
 
         # Store files
         if collectvdir:
diff --git a/src/buildstream/_cas/cascache.py b/src/buildstream/_cas/cascache.py
index 74912c4e2..8df0c4640 100644
--- a/src/buildstream/_cas/cascache.py
+++ b/src/buildstream/_cas/cascache.py
@@ -231,18 +231,19 @@ class CASCache:
         for filenode in directory.files:
             # regular file, create hardlink
             fullpath = os.path.join(dest, filenode.name)
-            # generally, if the node holds properties we will fallback
-            # to copying instead of hardlinking
-            if can_link and not filenode.node_properties:
+
+            node_properties = filenode.node_properties
+            if node_properties.HasField("mtime"):
+                mtime = utils._parse_protobuf_timestamp(node_properties.mtime)
+            else:
+                mtime = None
+
+            if can_link and mtime is None:
                 utils.safe_link(self.objpath(filenode.digest), fullpath)
             else:
                 utils.safe_copy(self.objpath(filenode.digest), fullpath)
-                if filenode.node_properties:
-                    # see https://github.com/bazelbuild/remote-apis/blob/master/build/bazel/remote/execution/v2/nodeproperties.md
-                    # for supported node property specifications
-                    for prop in filenode.node_properties:
-                        if prop.name == "MTime" and prop.value:
-                            utils._set_file_mtime(fullpath, utils._parse_timestamp(prop.value))
+                if mtime is not None:
+                    utils._set_file_mtime(fullpath, mtime)
 
             if filenode.is_executable:
                 os.chmod(
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index 404cae5e7..e3d4ffc68 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -2454,7 +2454,7 @@ class Element(Plugin):
         platform = context.platform
 
         if self._get_workspace():
-            output_node_properties = ["MTime"]
+            output_node_properties = ["mtime"]
         else:
             output_node_properties = None
 
diff --git a/src/buildstream/plugins/sources/workspace.py b/src/buildstream/plugins/sources/workspace.py
index f1d965fa0..44d0889b3 100644
--- a/src/buildstream/plugins/sources/workspace.py
+++ b/src/buildstream/plugins/sources/workspace.py
@@ -96,7 +96,7 @@ class WorkspaceSource(Source):
     def stage(self, directory: Directory) -> None:
         assert isinstance(directory, Directory)
         with self.timed_activity("Staging local files"):
-            result = directory.import_files(self.path, properties=["MTime"])
+            result = directory.import_files(self.path, properties=["mtime"])
 
             if result.overwritten or result.ignored:
                 raise SourceError(
diff --git a/src/buildstream/storage/_casbaseddirectory.py b/src/buildstream/storage/_casbaseddirectory.py
index c83918e4d..72a6beeef 100644
--- a/src/buildstream/storage/_casbaseddirectory.py
+++ b/src/buildstream/storage/_casbaseddirectory.py
@@ -29,16 +29,16 @@ See also: :ref:`sandboxing`.
 
 import os
 import stat
-import copy
 import tarfile as tarfilelib
 from contextlib import contextmanager
 from io import StringIO
+from google.protobuf import timestamp_pb2
 
 from .. import utils
 from .._protos.build.bazel.remote.execution.v2 import remote_execution_pb2
 from .directory import Directory, VirtualDirectoryError, _FileType
 from ._filebaseddirectory import FileBasedDirectory
-from ..utils import FileListResult, BST_ARBITRARY_TIMESTAMP, _get_file_mtimestamp
+from ..utils import FileListResult, BST_ARBITRARY_TIMESTAMP
 
 
 class IndexEntry:
@@ -54,7 +54,7 @@ class IndexEntry:
         is_executable=False,
         buildstream_object=None,
         modified=False,
-        node_properties=None
+        mtime=None
     ):
         self.name = name
         self.type = entrytype
@@ -63,7 +63,7 @@ class IndexEntry:
         self.is_executable = is_executable
         self.buildstream_object = buildstream_object
         self.modified = modified
-        self.node_properties = copy.deepcopy(node_properties)
+        self.mtime = mtime
 
     def get_directory(self, parent):
         if not self.buildstream_object:
@@ -99,7 +99,7 @@ class IndexEntry:
             digest=self.get_digest(),
             target=self.target,
             is_executable=self.is_executable,
-            node_properties=self.node_properties,
+            mtime=self.mtime,
         )
 
     def __eq__(self, other: object) -> bool:
@@ -107,7 +107,7 @@ class IndexEntry:
             return NotImplemented
 
         def get_equivalency_properties(e: IndexEntry):
-            return (e.name, e.type, e.target, e.is_executable, e.node_properties, e.get_digest())
+            return (e.name, e.type, e.target, e.is_executable, e.mtime, e.get_digest())
 
         return get_equivalency_properties(self) == get_equivalency_properties(other)
 
@@ -142,7 +142,7 @@ class CasBasedDirectory(Directory):
         self.__digest = None
         self.index = {}
         self.parent = parent
-        self.__node_properties = []
+        self.__subtree_read_only = None
         self._reset(digest=digest)
 
     def _reset(self, *, digest=None):
@@ -159,17 +159,24 @@ class CasBasedDirectory(Directory):
         except FileNotFoundError as e:
             raise VirtualDirectoryError("Directory not found in local cache: {}".format(e)) from e
 
-        self.__node_properties = list(pb2_directory.node_properties)
+        for prop in pb2_directory.node_properties.properties:
+            if prop.name == "SubtreeReadOnly":
+                self.__subtree_read_only = prop.value == "true"
 
         for entry in pb2_directory.directories:
             self.index[entry.name] = IndexEntry(entry.name, _FileType.DIRECTORY, digest=entry.digest)
         for entry in pb2_directory.files:
+            if entry.node_properties.HasField("mtime"):
+                mtime = entry.node_properties.mtime
+            else:
+                mtime = None
+
             self.index[entry.name] = IndexEntry(
                 entry.name,
                 _FileType.REGULAR_FILE,
                 digest=entry.digest,
                 is_executable=entry.is_executable,
-                node_properties=list(entry.node_properties),
+                mtime=mtime,
             )
         for entry in pb2_directory.symlinks:
             self.index[entry.name] = IndexEntry(entry.name, _FileType.SYMLINK, target=entry.target)
@@ -196,14 +203,10 @@ class CasBasedDirectory(Directory):
     def _add_file(self, name, path, modified=False, can_link=False, properties=None):
         digest = self.cas_cache.add_object(path=path, link_directly=can_link)
         is_executable = os.access(path, os.X_OK)
-        node_properties = []
-        # see https://github.com/bazelbuild/remote-apis/blob/master/build/bazel/remote/execution/v2/nodeproperties.md
-        # for supported node property specifications
-        if properties and "MTime" in properties:
-            node_property = remote_execution_pb2.NodeProperty()
-            node_property.name = "MTime"
-            node_property.value = _get_file_mtimestamp(path)
-            node_properties.append(node_property)
+        mtime = None
+        if properties and "mtime" in properties:
+            mtime = timestamp_pb2.Timestamp()
+            utils._get_file_protobuf_mtimestamp(mtime, path)
 
         entry = IndexEntry(
             name,
@@ -211,7 +214,7 @@ class CasBasedDirectory(Directory):
             digest=digest,
             is_executable=is_executable,
             modified=modified or name in self.index,
-            node_properties=node_properties,
+            mtime=mtime,
         )
         self.index[name] = entry
 
@@ -817,9 +820,10 @@ class CasBasedDirectory(Directory):
             # Create updated Directory proto
             pb2_directory = remote_execution_pb2.Directory()
 
-            if self.__node_properties:
-                node_properties = sorted(self.__node_properties, key=lambda prop: prop.name)
-                pb2_directory.node_properties.extend(node_properties)
+            if self.__subtree_read_only is not None:
+                node_property = pb2_directory.node_properties.properties.add()
+                node_property.name = "SubtreeReadOnly"
+                node_property.value = "true" if self.__subtree_read_only else "false"
 
             for name, entry in sorted(self.index.items()):
                 if entry.type == _FileType.DIRECTORY:
@@ -839,9 +843,8 @@ class CasBasedDirectory(Directory):
                     filenode.name = name
                     filenode.digest.CopyFrom(entry.digest)
                     filenode.is_executable = entry.is_executable
-                    if entry.node_properties:
-                        node_properties = sorted(entry.node_properties, key=lambda prop: prop.name)
-                        filenode.node_properties.extend(node_properties)
+                    if entry.mtime is not None:
+                        filenode.node_properties.mtime.CopyFrom(entry.mtime)
                 elif entry.type == _FileType.SYMLINK:
                     symlinknode = pb2_directory.symlinks.add()
                     symlinknode.name = name
@@ -896,10 +899,8 @@ class CasBasedDirectory(Directory):
         if entry.type == _FileType.DIRECTORY or entry.is_executable:
             st_mode |= stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH
 
-        if entry.node_properties:
-            for prop in entry.node_properties:
-                if prop.name == "MTime" and prop.value:
-                    st_mtime = utils._parse_timestamp(prop.value)
+        if entry.mtime is not None:
+            st_mtime = utils._parse_protobuf_timestamp(entry.mtime)
 
         return os.stat_result((st_mode, 0, 0, st_nlink, 0, 0, st_size, st_mtime, st_mtime, st_mtime))
 
@@ -921,11 +922,7 @@ class CasBasedDirectory(Directory):
         yield from self.index.keys()
 
     def _set_subtree_read_only(self, read_only):
-        self.__node_properties = list(filter(lambda prop: prop.name != "SubtreeReadOnly", self.__node_properties))
-        node_property = remote_execution_pb2.NodeProperty()
-        node_property.name = "SubtreeReadOnly"
-        node_property.value = "true" if read_only else "false"
-        self.__node_properties.append(node_property)
+        self.__subtree_read_only = read_only
 
         self.__invalidate_digest()
 
diff --git a/src/buildstream/storage/_filebaseddirectory.py b/src/buildstream/storage/_filebaseddirectory.py
index 12312299d..3f1fc599a 100644
--- a/src/buildstream/storage/_filebaseddirectory.py
+++ b/src/buildstream/storage/_filebaseddirectory.py
@@ -36,7 +36,6 @@ from .. import utils
 from ..utils import link_files, copy_files, list_relative_paths, _get_link_mtime, BST_ARBITRARY_TIMESTAMP
 from ..utils import _set_deterministic_user, _set_deterministic_mtime
 from ..utils import FileListResult
-from .._exceptions import ImplError
 
 # FileBasedDirectory intentionally doesn't call its superclass constuctor,
 # which is meant to be unimplemented.
@@ -418,18 +417,12 @@ class FileBasedDirectory(Directory):
                     src_path = source_directory.cas_cache.objpath(entry.digest)
 
                     # fallback to copying if we require mtime support on this file
-                    if update_mtime or entry.node_properties:
+                    if update_mtime or entry.mtime is not None:
                         utils.safe_copy(src_path, dest_path, result=result)
                         mtime = update_mtime
                         # mtime property will override specified mtime
-                        # see https://github.com/bazelbuild/remote-apis/blob/master/build/bazel/remote/execution/v2/nodeproperties.md
-                        # for supported node property specifications
-                        if entry.node_properties:
-                            for prop in entry.node_properties:
-                                if prop.name == "MTime" and prop.value:
-                                    mtime = utils._parse_timestamp(prop.value)
-                                else:
-                                    raise ImplError("{} is not a supported node property.".format(prop.name))
+                        if entry.mtime is not None:
+                            mtime = utils._parse_protobuf_timestamp(entry.mtime)
                         if mtime:
                             utils._set_file_mtime(dest_path, mtime)
                     else:
diff --git a/src/buildstream/utils.py b/src/buildstream/utils.py
index 29b3bf484..99c22d169 100644
--- a/src/buildstream/utils.py
+++ b/src/buildstream/utils.py
@@ -24,6 +24,7 @@ Utilities
 import calendar
 import errno
 import hashlib
+import math
 import os
 import re
 import shutil
@@ -39,6 +40,7 @@ from contextlib import contextmanager
 from pathlib import Path
 from typing import Callable, IO, Iterable, Iterator, Optional, Tuple, Union
 from dateutil import parser as dateutil_parser
+from google.protobuf import timestamp_pb2
 
 import psutil
 
@@ -179,6 +181,41 @@ def _parse_timestamp(timestamp: str) -> float:
         raise UtilError(errmsg)
 
 
+def _make_protobuf_timestamp(timestamp: timestamp_pb2.Timestamp, timepoint: float):
+    """Obtain the Protobuf Timestamp represented by the time given in seconds.
+
+    Args:
+        timestamp: the Protobuf Timestamp to set
+        timepoint: the time since the epoch in seconds
+
+    """
+    timestamp.seconds = int(timepoint)
+    timestamp.nanos = int(math.modf(timepoint)[0] * 1e9)
+
+
+def _get_file_protobuf_mtimestamp(timestamp: timestamp_pb2.Timestamp, fullpath: str):
+    """Obtain the Protobuf Timestamp represented by the mtime of the
+    file at the given path."""
+    assert isinstance(fullpath, str), "Path to file must be a string: {}".format(str(fullpath))
+    try:
+        mtime = os.path.getmtime(fullpath)
+    except OSError:
+        raise UtilError("Failed to get mtime of file at {}".format(fullpath))
+    _make_protobuf_timestamp(timestamp, mtime)
+
+
+def _parse_protobuf_timestamp(timestamp: timestamp_pb2.Timestamp) -> float:
+    """Convert Protobuf Timestamp to seconds since epoch.
+
+    Args:
+        timestamp: the Protobuf Timestamp
+
+    Returns:
+        The time in seconds since epoch represented by the timestamp.
+    """
+    return timestamp.seconds + timestamp.nanos / 1e9
+
+
 def _set_file_mtime(fullpath: str, seconds: Union[int, float]) -> None:
     """Set the access and modification times of the file at the given path
     to the given time. The time of the file will be set with nanosecond
diff --git a/tests/internals/storage.py b/tests/internals/storage.py
index ea3f59b50..e9932e0a4 100644
--- a/tests/internals/storage.py
+++ b/tests/internals/storage.py
@@ -148,7 +148,7 @@ def test_merge_casdir_properties(tmpdir, datafiles, modification):
     elif modification == "time":
         os.utime(os.path.join(after, "root-file"), (200, 200))
 
-    _test_merge_dirs(before, after, buildtree, str(tmpdir), properties=["MTime"])
+    _test_merge_dirs(before, after, buildtree, str(tmpdir), properties=["mtime"])
 
 
 def _test_merge_dirs(
diff --git a/tests/internals/storage_vdir_import.py b/tests/internals/storage_vdir_import.py
index b302c630e..ffd727ffe 100644
--- a/tests/internals/storage_vdir_import.py
+++ b/tests/internals/storage_vdir_import.py
@@ -131,7 +131,7 @@ def file_contents_are(path, contents):
 
 def create_new_casdir(root_number, cas_cache, tmpdir):
     d = CasBasedDirectory(cas_cache)
-    d.import_files(os.path.join(tmpdir, "content", "root{}".format(root_number)), properties=["MTime"])
+    d.import_files(os.path.join(tmpdir, "content", "root{}".format(root_number)), properties=["mtime"])
     digest = d._get_digest()
     assert digest.hash != empty_hash_ref
     return d
@@ -199,7 +199,7 @@ def _import_test(tmpdir, original, overlay, generator_function, verify_contents=
         assert duplicate_cas._get_digest().hash == d._get_digest().hash
 
         d2 = create_new_casdir(overlay, cas_cache, tmpdir)
-        d.import_files(d2, properties=["MTime"])
+        d.import_files(d2, properties=["mtime"])
         export_dir = os.path.join(tmpdir, "output-{}-{}".format(original, overlay))
         roundtrip_dir = os.path.join(tmpdir, "roundtrip-{}-{}".format(original, overlay))
         d2.export_files(roundtrip_dir)
@@ -238,7 +238,7 @@ def _import_test(tmpdir, original, overlay, generator_function, verify_contents=
 
         # Now do the same thing with filebaseddirectories and check the contents match
 
-        duplicate_cas.import_files(roundtrip_dir, properties=["MTime"])
+        duplicate_cas.import_files(roundtrip_dir, properties=["mtime"])
 
         assert duplicate_cas._get_digest().hash == d._get_digest().hash
     finally: