Merge branch 'juerg/symlinks' into 'master'

Symlink fixes

See merge request BuildStream/buildstream!1138

5 files changed, 43 insertions, 38 deletions

diff --git a/buildstream/plugins/sources/local.py b/buildstream/plugins/sources/local.py
index 55cdc14d3..d4965cc9e 100644
--- a/buildstream/plugins/sources/local.py
+++ b/buildstream/plugins/sources/local.py
@@ -97,7 +97,7 @@ class LocalSource(Source):
         with self.timed_activity("Staging local files at {}".format(self.path)):
 
             if os.path.isdir(self.fullpath):
-                files = list(utils.list_relative_paths(self.fullpath, list_dirs=True))
+                files = list(utils.list_relative_paths(self.fullpath))
                 utils.copy_files(self.fullpath, directory, files=files)
             else:
                 destfile = os.path.join(directory, os.path.basename(self.path))
@@ -133,11 +133,11 @@ def unique_key(filename):
 
     # Return some hard coded things for files which
     # have no content to calculate a key for
-    if os.path.isdir(filename):
-        return "0"
-    elif os.path.islink(filename):
+    if os.path.islink(filename):
         # For a symbolic link, use the link target as its unique identifier
         return os.readlink(filename)
+    elif os.path.isdir(filename):
+        return "0"
 
     return utils.sha256sum(filename)
 
diff --git a/buildstream/sandbox/sandbox.py b/buildstream/sandbox/sandbox.py
index cb6f43314..2159c0fef 100644
--- a/buildstream/sandbox/sandbox.py
+++ b/buildstream/sandbox/sandbox.py
@@ -525,11 +525,11 @@ class Sandbox():
     #         (bool): Whether a command exists inside the sandbox.
     def _has_command(self, command, env=None):
         if os.path.isabs(command):
-            return os.path.exists(os.path.join(
+            return os.path.lexists(os.path.join(
                 self._root, command.lstrip(os.sep)))
 
         for path in env.get('PATH').split(':'):
-            if os.path.exists(os.path.join(
+            if os.path.lexists(os.path.join(
                     self._root, path.lstrip(os.sep), command)):
                 return True
 
diff --git a/buildstream/storage/_casbaseddirectory.py b/buildstream/storage/_casbaseddirectory.py
index d88222273..09c8c9875 100644
--- a/buildstream/storage/_casbaseddirectory.py
+++ b/buildstream/storage/_casbaseddirectory.py
@@ -795,24 +795,11 @@ class CasBasedDirectory(Directory):
         Return value: List(str) - list of all paths
         """
 
-        symlink_list = filter(lambda i: isinstance(i[1].pb_object, remote_execution_pb2.SymlinkNode),
-                              self.index.items())
-        file_list = list(filter(lambda i: isinstance(i[1].pb_object, remote_execution_pb2.FileNode),
+        file_list = list(filter(lambda i: not isinstance(i[1].buildstream_object, CasBasedDirectory),
                                 self.index.items()))
         directory_list = filter(lambda i: isinstance(i[1].buildstream_object, CasBasedDirectory),
                                 self.index.items())
 
-        # We need to mimic the behaviour of os.walk, in which symlinks
-        # to directories count as directories and symlinks to file or
-        # broken symlinks count as files. os.walk doesn't follow
-        # symlinks, so we don't recurse.
-        for (k, v) in sorted(symlink_list):
-            target = self._resolve(k, absolute_symlinks_resolve=True)
-            if isinstance(target, CasBasedDirectory):
-                yield os.path.join(relpath, k)
-            else:
-                file_list.append((k, v))
-
         if file_list == [] and relpath != "":
             yield relpath
         else:
diff --git a/buildstream/utils.py b/buildstream/utils.py
index c8d79c95a..76f95637e 100644
--- a/buildstream/utils.py
+++ b/buildstream/utils.py
@@ -111,7 +111,7 @@ class FileListResult():
         return ret
 
 
-def list_relative_paths(directory, *, list_dirs=True):
+def list_relative_paths(directory):
     """A generator for walking directory relative paths
 
     This generator is useful for checking the full manifest of
@@ -125,13 +125,26 @@ def list_relative_paths(directory, *, list_dirs=True):
 
     Args:
        directory (str): The directory to list files in
-       list_dirs (bool): Whether to list directories
 
     Yields:
        Relative filenames in `directory`
     """
     for (dirpath, dirnames, filenames) in os.walk(directory):
 
+        # os.walk does not decend into symlink directories, which
+        # makes sense because otherwise we might have redundant
+        # directories, or end up descending into directories outside
+        # of the walk() directory.
+        #
+        # But symlinks to directories are still identified as
+        # subdirectories in the walked `dirpath`, so we extract
+        # these symlinks from `dirnames` and add them to `filenames`.
+        #
+        for d in dirnames:
+            fullpath = os.path.join(dirpath, d)
+            if os.path.islink(fullpath):
+                filenames.append(d)
+
         # Modifying the dirnames directly ensures that the os.walk() generator
         # allows us to specify the order in which they will be iterated.
         dirnames.sort()
@@ -143,25 +156,10 @@ def list_relative_paths(directory, *, list_dirs=True):
         # `directory`, prefer to have no prefix in that case.
         basepath = relpath if relpath != '.' and dirpath != directory else ''
 
-        # os.walk does not decend into symlink directories, which
-        # makes sense because otherwise we might have redundant
-        # directories, or end up descending into directories outside
-        # of the walk() directory.
-        #
-        # But symlinks to directories are still identified as
-        # subdirectories in the walked `dirpath`, so we extract
-        # these symlinks from `dirnames`
-        #
-        if list_dirs:
-            for d in dirnames:
-                fullpath = os.path.join(dirpath, d)
-                if os.path.islink(fullpath):
-                    yield os.path.join(basepath, d)
-
         # We've decended into an empty directory, in this case we
         # want to include the directory itself, but not in any other
         # case.
-        if list_dirs and not filenames:
+        if not filenames:
             yield relpath
 
         # List the filenames in the walked directory
diff --git a/tests/sources/local.py b/tests/sources/local.py
index f7c1f4bd2..0da9e8cc1 100644
--- a/tests/sources/local.py
+++ b/tests/sources/local.py
@@ -136,3 +136,23 @@ def test_stage_file_exists(cli, tmpdir, datafiles):
     result = cli.run(project=project, args=['build', 'target.bst'])
     result.assert_main_error(ErrorDomain.STREAM, None)
     result.assert_task_error(ErrorDomain.SOURCE, 'ensure-stage-dir-fail')
+
+
+@pytest.mark.datafiles(os.path.join(DATA_DIR, 'directory'))
+def test_stage_directory_symlink(cli, tmpdir, datafiles):
+    project = os.path.join(datafiles.dirname, datafiles.basename)
+    checkoutdir = os.path.join(str(tmpdir), "checkout")
+
+    symlink = os.path.join(project, 'files', 'symlink-to-subdir')
+    os.symlink('subdir', symlink)
+
+    # Build, checkout
+    result = cli.run(project=project, args=['build', 'target.bst'])
+    result.assert_success()
+    result = cli.run(project=project, args=['artifact', 'checkout', 'target.bst', '--directory', checkoutdir])
+    result.assert_success()
+
+    # Check that the checkout contains the expected directory and directory symlink
+    assert(os.path.exists(os.path.join(checkoutdir, 'subdir', 'anotherfile.txt')))
+    assert(os.path.exists(os.path.join(checkoutdir, 'symlink-to-subdir', 'anotherfile.txt')))
+    assert(os.path.islink(os.path.join(checkoutdir, 'symlink-to-subdir')))