Merge branch 'juerg/vdirectory' into 'master'

Improve `Directory` API

See merge request BuildStream/buildstream!1827

7 files changed, 140 insertions, 52 deletions

diff --git a/src/buildstream/buildelement.py b/src/buildstream/buildelement.py
index aeafbcdda..95b085eba 100644
--- a/src/buildstream/buildelement.py
+++ b/src/buildstream/buildelement.py
@@ -280,7 +280,7 @@ class BuildElement(Element):
             buildroot = self.get_variable("build-root")
             buildroot_vdir = vdir.descend(*buildroot.lstrip(os.sep).split(os.sep))
 
-            if buildroot_vdir._exists(marker_filename, follow_symlinks=True):
+            if buildroot_vdir.exists(marker_filename):
                 # Already prepared
                 return
 
diff --git a/src/buildstream/sandbox/_sandboxreapi.py b/src/buildstream/sandbox/_sandboxreapi.py
index c2b6cac48..26fe5f2b2 100644
--- a/src/buildstream/sandbox/_sandboxreapi.py
+++ b/src/buildstream/sandbox/_sandboxreapi.py
@@ -76,7 +76,7 @@ class SandboxREAPI(Sandbox):
 
                 # Ensure mount point exists in sandbox
                 mount_point_components = mount_point.split(os.path.sep)
-                if not vdir._exists(*mount_point_components):
+                if not vdir.exists(*mount_point_components):
                     if os.path.isdir(mount_source):
                         # Mounting a directory, mount point must be a directory
                         vdir.descend(*mount_point_components, create=True)
diff --git a/src/buildstream/sandbox/sandbox.py b/src/buildstream/sandbox/sandbox.py
index b82e2da59..6f6acc946 100644
--- a/src/buildstream/sandbox/sandbox.py
+++ b/src/buildstream/sandbox/sandbox.py
@@ -568,14 +568,14 @@ class Sandbox:
         vroot = self.get_virtual_directory()
         command_as_parts = command.lstrip(os.sep).split(os.sep)
         if os.path.isabs(command):
-            return vroot._exists(*command_as_parts, follow_symlinks=True)
+            return vroot.exists(*command_as_parts, follow_symlinks=True)
 
         if len(command_as_parts) > 1:
             return False
 
         for path in env.get("PATH").split(":"):
             path_as_parts = path.lstrip(os.sep).split(os.sep)
-            if vroot._exists(*path_as_parts, command, follow_symlinks=True):
+            if vroot.exists(*path_as_parts, command, follow_symlinks=True):
                 return True
 
         return False
diff --git a/src/buildstream/storage/_casbaseddirectory.py b/src/buildstream/storage/_casbaseddirectory.py
index 51d9909fd..d6c4a7813 100644
--- a/src/buildstream/storage/_casbaseddirectory.py
+++ b/src/buildstream/storage/_casbaseddirectory.py
@@ -31,8 +31,10 @@ import os
 import stat
 import copy
 import tarfile as tarfilelib
+from contextlib import contextmanager
 from io import StringIO
 
+from .. import utils
 from .._protos.build.bazel.remote.execution.v2 import remote_execution_pb2
 from .directory import Directory, VirtualDirectoryError, _FileType
 from ._filebaseddirectory import FileBasedDirectory
@@ -191,8 +193,7 @@ class CasBasedDirectory(Directory):
 
         return newdir
 
-    def _add_file(self, basename, filename, modified=False, can_link=False, properties=None):
-        path = os.path.join(basename, filename)
+    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 = []
@@ -205,21 +206,13 @@ class CasBasedDirectory(Directory):
             node_properties.append(node_property)
 
         entry = IndexEntry(
-            filename,
+            name,
             _FileType.REGULAR_FILE,
             digest=digest,
             is_executable=is_executable,
-            modified=modified or filename in self.index,
+            modified=modified or name in self.index,
             node_properties=node_properties,
         )
-        self.index[filename] = entry
-
-        self.__invalidate_digest()
-
-    def _create_empty_file(self, name):
-        digest = self.cas_cache.add_object(buffer=b"")
-
-        entry = IndexEntry(name, _FileType.REGULAR_FILE, digest=digest)
         self.index[name] = entry
 
         self.__invalidate_digest()
@@ -314,14 +307,6 @@ class CasBasedDirectory(Directory):
 
         self.__invalidate_digest()
 
-    def find_root(self):
-        """ Finds the root of this directory tree by following 'parent' until there is
-        no parent. """
-        if self.parent:
-            return self.parent.find_root()
-        else:
-            return self
-
     def descend(self, *paths, create=False, follow_symlinks=False):
         """Descend one or more levels of directory hierarchy and return a new
         Directory object for that directory.
@@ -356,7 +341,7 @@ class CasBasedDirectory(Directory):
                     linklocation = entry.target
                     newpaths = linklocation.split(os.path.sep)
                     if os.path.isabs(linklocation):
-                        current_dir = current_dir.find_root().descend(*newpaths, follow_symlinks=True)
+                        current_dir = current_dir._find_root().descend(*newpaths, follow_symlinks=True)
                     else:
                         current_dir = current_dir.descend(*newpaths, follow_symlinks=True)
                 else:
@@ -517,8 +502,8 @@ class CasBasedDirectory(Directory):
         result = FileListResult()
         if self._check_replacement(os.path.basename(external_pathspec), os.path.dirname(external_pathspec), result):
             self._add_file(
-                os.path.dirname(external_pathspec),
                 os.path.basename(external_pathspec),
+                external_pathspec,
                 modified=os.path.basename(external_pathspec) in result.overwritten,
                 properties=properties,
             )
@@ -740,6 +725,34 @@ class CasBasedDirectory(Directory):
             path += "/" + self.common_name
         return path
 
+    @contextmanager
+    def open_file(self, *path: str, mode: str = "r"):
+        subdir = self.descend(*path[:-1])
+        entry = subdir.index.get(path[-1])
+
+        if entry and entry.type != _FileType.REGULAR_FILE:
+            raise VirtualDirectoryError("{} in {} is not a file".format(path[-1], str(subdir)))
+
+        if mode not in ["r", "rb", "w", "wb", "x", "xb"]:
+            raise ValueError("Unsupported mode: `{}`".format(mode))
+
+        if "r" in mode:
+            if not entry:
+                raise FileNotFoundError("{} not found in {}".format(path[-1], str(subdir)))
+
+            # Read-only access, allow direct access to CAS object
+            with open(self.cas_cache.objpath(entry.digest), mode, encoding="utf-8") as f:
+                yield f
+        else:
+            if "x" in mode and entry:
+                raise FileExistsError("{} already exists in {}".format(path[-1], str(subdir)))
+
+            with utils._tempnamedfile(mode, encoding="utf-8", dir=self.cas_cache.tmpdir) as f:
+                yield f
+                # Import written temporary file into CAS
+                f.flush()
+                subdir._add_file(path[-1], f.name, modified=True)
+
     def __str__(self):
         return "[CAS:{}]".format(self._get_identifier())
 
@@ -750,6 +763,14 @@ class CasBasedDirectory(Directory):
             "_get_underlying_directory was called on a CAS-backed directory," + " which has no underlying directory."
         )
 
+    def _find_root(self):
+        """ Finds the root of this directory tree by following 'parent' until there is
+        no parent. """
+        if self.parent:
+            return self.parent._find_root()
+        else:
+            return self
+
     # _get_digest():
     #
     # Return the Digest for this directory.
@@ -796,7 +817,7 @@ class CasBasedDirectory(Directory):
 
         return self.__digest
 
-    def _exists(self, *path, follow_symlinks=False):
+    def exists(self, *path, follow_symlinks=False):
         try:
             subdir = self.descend(*path[:-1], follow_symlinks=follow_symlinks)
             target = subdir.index.get(path[-1])
@@ -805,8 +826,8 @@ class CasBasedDirectory(Directory):
                     linklocation = target.target
                     newpath = linklocation.split(os.path.sep)
                     if os.path.isabs(linklocation):
-                        return subdir.find_root()._exists(*newpath, follow_symlinks=True)
-                    return subdir._exists(*newpath, follow_symlinks=True)
+                        return subdir._find_root().exists(*newpath, follow_symlinks=True)
+                    return subdir.exists(*newpath, follow_symlinks=True)
                 else:
                     return True
             return False
diff --git a/src/buildstream/storage/_filebaseddirectory.py b/src/buildstream/storage/_filebaseddirectory.py
index 07efa4598..53a87b64c 100644
--- a/src/buildstream/storage/_filebaseddirectory.py
+++ b/src/buildstream/storage/_filebaseddirectory.py
@@ -44,15 +44,13 @@ from .._exceptions import ImplError
 
 
 class FileBasedDirectory(Directory):
-    def __init__(self, external_directory=None):
+    def __init__(self, external_directory=None, *, parent=None):
         self.external_directory = external_directory
+        self.parent = parent
 
     def descend(self, *paths, create=False, follow_symlinks=False):
         """ See superclass Directory for arguments """
 
-        if follow_symlinks:
-            ImplError("FileBasedDirectory.Decend dose not implement follow_symlinks=True")
-
         current_dir = self
 
         for path in paths:
@@ -60,21 +58,38 @@ class FileBasedDirectory(Directory):
             if not path:
                 continue
 
+            if path == ".":
+                continue
+            if path == "..":
+                if current_dir.parent is not None:
+                    current_dir = current_dir.parent
+                # In POSIX /.. == / so just stay at the root dir
+                continue
+
             new_path = os.path.join(current_dir.external_directory, path)
             try:
                 st = os.lstat(new_path)
-                if not stat.S_ISDIR(st.st_mode):
+                if stat.S_ISDIR(st.st_mode):
+                    current_dir = FileBasedDirectory(new_path, parent=current_dir)
+                elif follow_symlinks and stat.S_ISLNK(st.st_mode):
+                    linklocation = os.readlink(new_path)
+                    newpaths = linklocation.split(os.path.sep)
+                    if os.path.isabs(linklocation):
+                        current_dir = current_dir._find_root().descend(*newpaths, follow_symlinks=True)
+                    else:
+                        current_dir = current_dir.descend(*newpaths, follow_symlinks=True)
+                else:
                     raise VirtualDirectoryError(
-                        "Cannot descend into '{}': '{}' is not a directory".format(path, new_path)
+                        "Cannot descend into '{}': '{}' is not a directory".format(path, new_path),
+                        reason="not-a-directory",
                     )
             except FileNotFoundError:
                 if create:
                     os.mkdir(new_path)
+                    current_dir = FileBasedDirectory(new_path, parent=current_dir)
                 else:
                     raise VirtualDirectoryError("Cannot descend into '{}': '{}' does not exist".format(path, new_path))
 
-            current_dir = FileBasedDirectory(new_path)
-
         return current_dir
 
     def import_files(
@@ -217,6 +232,35 @@ class FileBasedDirectory(Directory):
     def get_size(self):
         return utils._get_dir_size(self.external_directory)
 
+    def exists(self, *path, follow_symlinks=False):
+        try:
+            subdir = self.descend(*path[:-1], follow_symlinks=follow_symlinks)
+            newpath = os.path.join(subdir.external_directory, path[-1])
+            st = os.lstat(newpath)
+            if follow_symlinks and stat.S_ISLNK(st.st_mode):
+                linklocation = os.readlink(newpath)
+                newpath = linklocation.split(os.path.sep)
+                if os.path.isabs(linklocation):
+                    return subdir._find_root().exists(*newpath, follow_symlinks=True)
+                return subdir.exists(*newpath, follow_symlinks=True)
+            else:
+                return True
+        except (VirtualDirectoryError, FileNotFoundError):
+            return False
+
+    def open_file(self, *path: str, mode: str = "r"):
+        # Use descend() to avoid following symlinks (potentially escaping the sandbox)
+        subdir = self.descend(*path[:-1])
+        newpath = os.path.join(subdir.external_directory, path[-1])
+
+        if mode not in ["r", "rb", "w", "wb", "x", "xb"]:
+            raise ValueError("Unsupported mode: `{}`".format(mode))
+
+        if "r" in mode:
+            return open(newpath, mode=mode, encoding="utf-8")
+        else:
+            return utils.save_file_atomic(newpath, mode=mode, encoding="utf-8")
+
     def __str__(self):
         # This returns the whole path (since we don't know where the directory started)
         # which exposes the sandbox directory; we will have to assume for the time being
@@ -228,6 +272,14 @@ class FileBasedDirectory(Directory):
         object refers to. """
         return self.external_directory
 
+    def _find_root(self):
+        """ Finds the root of this directory tree by following 'parent' until there is
+        no parent. """
+        if self.parent:
+            return self.parent._find_root()
+        else:
+            return self
+
     def _get_filetype(self, name=None):
         path = self.external_directory
 
@@ -334,16 +386,3 @@ class FileBasedDirectory(Directory):
                     assert entry.type == _FileType.SYMLINK
                     os.symlink(entry.target, dest_path)
                 result.files_written.append(relative_pathname)
-
-    def _exists(self, *path, follow_symlinks=False):
-        """This is very simple but mirrors the cas based storage were it is less trivial"""
-        if follow_symlinks:
-            # The lexists is not ideal as it cant spot broken symlinks but this is a long
-            # standing bug in buildstream as exists follow absolute syslinks to real root
-            # and incorrectly thinks they are broken the new casbaseddirectory dose not have this bug.
-            return os.path.lexists(os.path.join(self.external_directory, *path))
-        raise ImplError("_exists can only follow symlinks in filebaseddirectory")
-
-    def _create_empty_file(self, name):
-        with open(os.path.join(self.external_directory, name), "w"):
-            pass
diff --git a/src/buildstream/storage/directory.py b/src/buildstream/storage/directory.py
index 55cc717f2..bb9d78f7e 100644
--- a/src/buildstream/storage/directory.py
+++ b/src/buildstream/storage/directory.py
@@ -196,6 +196,32 @@ class Directory:
         and effective space used may be lower than this number due to deduplication. """
         raise NotImplementedError()
 
+    def exists(self, *paths: str, follow_symlinks: bool = False) -> bool:
+        """ Check whether the specified path exists.
+
+        Args:
+          *paths: A list of strings which are all path components.
+          follow_symlinks: True to follow symlinks.
+
+        Returns:
+          True if the path exists, False otherwise.
+        """
+        raise NotImplementedError()
+
+    def open_file(self, *paths: str, mode: str = "r"):
+        """ Open file and return a corresponding file object. In text mode,
+        UTF-8 is used as encoding.
+
+        Args:
+          *paths: A list of strings which are all path components.
+          mode (str): An optional string that specifies the mode in which the file is opened.
+        """
+        raise NotImplementedError()
+
+    def _create_empty_file(self, *paths):
+        with self.open_file(*paths, mode="w"):
+            pass
+
 
 # FileType:
 #
diff --git a/src/buildstream/utils.py b/src/buildstream/utils.py
index b588566df..997c073b9 100644
--- a/src/buildstream/utils.py
+++ b/src/buildstream/utils.py
@@ -1168,6 +1168,8 @@ def _tempdir(*, suffix="", prefix="tmp", dir):  # pylint: disable=redefined-buil
 # which is guaranteed to be named and have an entry in the filesystem.
 #
 # Args:
+#    mode (str): The mode in which the file is opened
+#    encoding (str): The name of the encoding used to decode or encode the file
 #    dir (str): A path to a parent directory for the temporary file
 #    suffix (str): A suffix for the temproary file name
 #    prefix (str): A prefix for the temporary file name
@@ -1180,7 +1182,7 @@ def _tempdir(*, suffix="", prefix="tmp", dir):  # pylint: disable=redefined-buil
 # on SIGTERM.
 #
 @contextmanager
-def _tempnamedfile(suffix="", prefix="tmp", dir=None):  # pylint: disable=redefined-builtin
+def _tempnamedfile(mode="w+b", encoding=None, suffix="", prefix="tmp", dir=None):  # pylint: disable=redefined-builtin
     temp = None
 
     def close_tempfile():
@@ -1188,7 +1190,7 @@ def _tempnamedfile(suffix="", prefix="tmp", dir=None):  # pylint: disable=redefi
             temp.close()
 
     with _signals.terminator(close_tempfile), tempfile.NamedTemporaryFile(
-        suffix=suffix, prefix=prefix, dir=dir
+        mode=mode, encoding=encoding, suffix=suffix, prefix=prefix, dir=dir
     ) as temp:
         yield temp