Support runfiles root_symlinks in images

Fixes symlink support and adds root_symlink support. Previously I was
seeing that symlinks (regardless of whether the target file existed in
runfiles.file), did not point to the correct location in the image.

For that reason, we use `_final_file_path` instead of `layer_file_path`.
This seems to work, but I am not sure if I'm missing a use-case here.

The overall method we take here is to create real files where the
[root_]symlink is declared if the target `File` is not seen in
`file_map`.

This PR also adds some extra test functionality to check that symlinks
point at the expected location.

Author: wolfd

lang/image.bzl

@@ -126,6 +126,12 @@ def _default_symlinks(dep):
     else:
         return dep[DefaultInfo].default_runfiles.symlinks
 
+def _default_root_symlinks(dep):
+    if FilterLayerInfo in dep:
+        return dep[FilterLayerInfo].runfiles.root_symlinks
+    else:
+        return dep[DefaultInfo].default_runfiles.root_symlinks
+
 def _app_layer_impl(ctx, runfiles = None, emptyfiles = None):
     """Appends a layer for a single dependency's runfiles.
 
@@ -186,11 +192,24 @@ def _app_layer_impl(ctx, runfiles = None, emptyfiles = None):
     # app layer, we can already create symlinks to the runfiles path.
     if ctx.attr.binary:
         # Include any symlinks from the runfiles of the target for which we are synthesizing the layer.
-        symlinks.update({
-            (_reference_dir(ctx) + "/" + s.path): layer_file_path(ctx, s.target_file)
-            for s in _default_symlinks(dep).to_list()
-            if hasattr(s, "path")  # "path" and "target_file" are exposed to starlark since bazel 0.21.0.
-        })
+        for s in _default_symlinks(dep).to_list():
+            symlink_path = _reference_dir(ctx) + "/" + s.path
+            if filepath(ctx, s.target_file) in file_map:  # If the target is a real file, link to it
+                symlinks.update({
+                    symlink_path: _final_file_path(ctx, s.target_file),
+                })
+            else:
+                file_map[symlink_path] = s.target_file  # Otherwise, synthesize the "symlink" as a real file
+
+        # Include root_symlinks
+        for s in _default_root_symlinks(dep).to_list():
+            symlink_path = _runfiles_dir(ctx) + "/" + s.path
+            if filepath(ctx, s.target_file) in file_map:
+                symlinks.update({
+                    symlink_path: _final_file_path(ctx, s.target_file),
+                })
+            else:
+                file_map[symlink_path] = s.target_file
 
         symlinks.update({
             _final_file_path(ctx, f): layer_file_path(ctx, f)

testdata/utils.bzl

@@ -28,9 +28,21 @@ def generate_deb(name, args = [], metadata_compression_type = "none"):
     )
 
 def _rule_with_symlinks_impl(ctx):
-    f = ctx.actions.declare_file("foo.txt")
-    ctx.actions.write(f, "test content")
-    runfiles = ctx.runfiles(files = [f], symlinks = {"foo-symlink.txt": f})
+    foo = ctx.actions.declare_file("foo.txt")
+    ctx.actions.write(foo, "test content")
+    bar = ctx.actions.declare_file("bar.txt")
+    ctx.actions.write(bar, "test content")
+    baz = ctx.actions.declare_file("baz.txt")
+    ctx.actions.write(baz, "test content")
+    runfiles = ctx.runfiles(
+        # bar and baz are specifically excluded from runfiles.files
+        # We want to test that we don't create broken symlinks for [root_]symlinks that don't
+        # have a corresponding file in runfiles.files.
+        # We expect that for those [root_]symlinks, a real file exists at the symlink path.
+        files = [foo],
+        symlinks = {"foo-symlink.txt": foo, "baz/dir/baz-symlink-real.txt": baz},
+        root_symlinks = {"foo-root-symlink.txt": foo, "bar-root-symlink-real.txt": bar},
+    )
     return DefaultInfo(runfiles = runfiles)
 
 rule_with_symlinks = rule(

tests/container/image_test.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 from io import BytesIO
+import contextlib
 import datetime
 import json
 import os
@@ -53,20 +54,33 @@ def assertTarballContains(self, tar, paths):
         self.maxDiff = None
         self.assertEqual(paths, tar.getnames())
 
-    def assertLayerNContains(self, img, n, paths):
+    def assertTarballSymlink(self, tar, path, target):
+        self.assertEqual(target, tar.getmember(path).linkname)
+
+    @contextlib.contextmanager
+    def _tarball_layer_n(self, img, n):
         buf = BytesIO(img.blob(img.fs_layers()[n]))
-        with tarfile.open(fileobj=buf, mode='r') as layer:
+        yield tarfile.open(fileobj=buf, mode='r')
+
+    def assertLayerNContains(self, img, n, paths):
+        with self._tarball_layer_n(img, n) as layer:
             self.assertTarballContains(layer, paths)
 
+    def assertLayerNSymlink(self, img, n, path, target):
+        with self._tarball_layer_n(img, n) as layer:
+            self.assertTarballSymlink(layer, path, target)
+
     def assertNonZeroMtimesInTopLayer(self, img):
-        buf = BytesIO(img.blob(img.fs_layers()[0]))
-        with tarfile.open(fileobj=buf, mode='r') as layer:
+        with self._tarball_layer_n(img, 0) as layer:
             for member in layer.getmembers():
                 self.assertNotEqual(member.mtime, 0)
 
     def assertTopLayerContains(self, img, paths):
         self.assertLayerNContains(img, 0, paths)
 
+    def assertTopLayerSymlink(self, img, path, target):
+        self.assertLayerNSymlink(img, 0, path, target)
+
     def assertConfigEqual(self, img, key, value):
         cfg = json.loads(img.config_file())
         self.assertEqual(value, cfg.get('config', {}).get(key))
@@ -562,6 +576,12 @@ def test_py_image_with_symlinks_in_data(self):
                 './app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker/testdata/py_image.py',
                 './app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker/testdata/py_image_with_symlinks_in_data.binary',
                 './app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker/testdata/foo.txt',
+                # baz-symlink-real.txt is a real file, since the File that it points to is not in runfiles.files
+                './app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker/baz',
+                './app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker/baz/dir',
+                './app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker/baz/dir/baz-symlink-real.txt',
+                # bar-root-symlink-real.txt is a real file, since the File that it points to is not in runfiles.files
+                './app/testdata/py_image_with_symlinks_in_data.binary.runfiles/bar-root-symlink-real.txt',
                 './app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker/testdata/__init__.py',
                 './app/io_bazel_rules_docker',
                 # TODO(mattmoor): The path normalization for symlinks should match
@@ -571,10 +591,18 @@ def test_py_image_with_symlinks_in_data(self):
                 '/app/testdata/py_image_with_symlinks_in_data.binary.runfiles',
                 '/app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker',
                 '/app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker/foo-symlink.txt',
+                '/app/testdata/py_image_with_symlinks_in_data.binary.runfiles/foo-root-symlink.txt',
                 '/app/testdata/py_image_with_symlinks_in_data.binary',
                 '/app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker/external',
             ])
 
+            # Test that root_symlinks that point to a file that is also in the runfiles tree is a symlink
+            self.assertTopLayerSymlink(
+                img,
+                '/app/testdata/py_image_with_symlinks_in_data.binary.runfiles/foo-root-symlink.txt',
+                '/app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker/testdata/foo.txt',
+            )
+
             # Below that, we have a layer that generates symlinks for the library layer.
             self.assertLayerNContains(img, 1, [
                 '.',
@@ -586,6 +614,14 @@ def test_py_image_with_symlinks_in_data(self):
                 '/app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker/testdata/py_image_library.py',
             ])
 
+            # Validate that library symlink is actually a symlink to the right path
+            self.assertLayerNSymlink(
+                img,
+                1,
+                '/app/testdata/py_image_with_symlinks_in_data.binary.runfiles/io_bazel_rules_docker/testdata/py_image_library.py',
+                '/app/io_bazel_rules_docker/testdata/py_image_library.py',
+            )
+
             # Check the library layer, which is two below our application layer.
             self.assertLayerNContains(img, 2, [
                 '.',