[Docker] Take the package of the input as the data path

By default all input files path are taken relatively to their
declared package and no more relatively to the docker_build
package.

The old behavior can be restored by specifying `data_path = "."`
as an attribute of the docker_build target.

--
MOS_MIGRATED_REVID=103731161

7 files changed, 96 insertions, 10 deletions

diff --git a/tools/build_defs/docker/BUILD b/tools/build_defs/docker/BUILD
index dc3a8c158b..3d70aa3c43 100644
--- a/tools/build_defs/docker/BUILD
+++ b/tools/build_defs/docker/BUILD
@@ -32,6 +32,8 @@ TEST_DATA = [
     for t in TEST_TARGETS
 ] + [
     "//tools/build_defs/docker/testdata:gen_image",
+    "//tools/build_defs/docker/testdata:data_path_image",
+    "//tools/build_defs/docker/testdata:no_data_path_image",
 ]
 
 sh_test(
diff --git a/tools/build_defs/docker/README.md b/tools/build_defs/docker/README.md
index 9933e3bdf9..f8d147135b 100644
--- a/tools/build_defs/docker/README.md
+++ b/tools/build_defs/docker/README.md
@@ -218,7 +218,10 @@ symlinks, entrypoint, cmd, env, ports, volumes)`
           docker image but a prefix path determined by `data_path`
           is removed from the directory structure. This path can
           be absolute from the workspace root if starting with a `/` or
-          relative to the rule's directory. It is set to `.` by default.
+          relative to the rule's directory. A relative path may starts with "./"
+          (or be ".") but cannot use go up with "..". By default, the
+          `data_path` attribute is unused and all files are supposed to have no
+          prefix.
         </p>
       </td>
     </tr>
diff --git a/tools/build_defs/docker/build_test.sh b/tools/build_defs/docker/build_test.sh
index 04a002510f..f151025ff5 100755
--- a/tools/build_defs/docker/build_test.sh
+++ b/tools/build_defs/docker/build_test.sh
@@ -308,4 +308,41 @@ function test_with_double_env() {
     '["bar=blah blah blah", "baz=/asdf blah blah blah", "foo=/asdf"]'
 }
 
+function test_with_double_env() {
+  check_layers "with_double_env" \
+    "f86da639a9346bec6d3a821ad1f716a177a8ff8f71d66f8b70238ce7e7ba51b8" \
+    "80b94376a90de45256c3e94c82bc3812bc5cbd05b7d01947f29e6805e8cd7018" \
+    "548e1d847a1d051e3cb3af383b0ebe40d341c01c97e735ae5a78ee3e10353b93"
+
+  # Check both the aggregation and the expansion of embedded variables.
+  check_env "with_double_env" \
+    "548e1d847a1d051e3cb3af383b0ebe40d341c01c97e735ae5a78ee3e10353b93" \
+    '["bar=blah blah blah", "baz=/asdf blah blah blah", "foo=/asdf"]'
+}
+
+function get_layer_listing() {
+  local input=$1
+  local layer=$2
+  local test_data="${TEST_DATA_DIR}/${input}.tar"
+  tar xOf "${test_data}" \
+    "./${layer}/layer.tar" | tar tv | sed -e 's/^.*:00 //'
+}
+
+function test_data_path() {
+  local no_data_path_sha="29bab1d66ea34ebbb9604704322458e0f06a3a9a7b0ac08df8b95a6c9c3d9320"
+  local data_path_sha="9a5c6b96227af212d1300964aaeb0723e78e78de019de1ba2382603f3050558a"
+
+  check_layers_aux "no_data_path_image" "${no_data_path_sha}"
+  check_layers_aux "data_path_image" "${data_path_sha}"
+
+  # Without data_path = "." the file will be inserted as `./test`
+  # (since it is the path in the package) and with data_path = "."
+  # the file will be inserted relatively to the testdata package
+  # (so `./test/test`).
+  check_eq $(get_layer_listing "no_data_path_image" "${no_data_path_sha}") \
+    ./test
+  check_eq $(get_layer_listing "data_path_image" "${data_path_sha}") \
+    ./test/test
+}
+
 run_suite "build_test"
diff --git a/tools/build_defs/docker/docker.bzl b/tools/build_defs/docker/docker.bzl
index 1d803f4864..e5badabe99 100644
--- a/tools/build_defs/docker/docker.bzl
+++ b/tools/build_defs/docker/docker.bzl
@@ -57,20 +57,35 @@ def _short_path_dirname(path):
 
 def _dest_path(f, strip_prefix):
   """Returns the short path of f, stripped of strip_prefix."""
+  if not strip_prefix:
+    # If no strip_prefix was specified, use the package of the
+    # given input as the strip_prefix.
+    strip_prefix = _short_path_dirname(f)
   if f.short_path.startswith(strip_prefix):
     return f.short_path[len(strip_prefix):]
   return f.short_path
 
+def _compute_data_path(out, data_path):
+  """Compute the relative data path prefix from the data_path attribute."""
+  if data_path:
+    # Strip ./ from the beginning if specified.
+    # There is no way to handle .// correctly (no function that would make
+    # that possible and Skylark is not turing complete) so just consider it
+    # as an absolute path.
+    if data_path[0:2] == "./":
+      data_path = data_path[2:]
+    if data_path[0] == "/":  # Absolute path
+      return data_path[1:]
+    elif not data_path or data_path == ".":  # Relative to current package
+      return _short_path_dirname(out)
+    else:  # Relative to a sub-directory
+      return _short_path_dirname(out) + "/" + data_path
+  return data_path
+
 def _build_layer(ctx):
   """Build the current layer for appending it the base layer."""
   # Compute the relative path
-  data_path = ctx.attr.data_path
-  if not data_path:
-    data_path = _short_path_dirname(ctx.outputs.out)
-  elif data_path[0] == "/":
-    data_path = data_path[1:]
-  else:  # relative path
-    data_path = _short_path_dirname(ctx.outputs.out) + "/" + data_path
+  data_path = _compute_data_path(ctx.outputs.out, ctx.attr.data_path)
 
   layer = ctx.new_file(ctx.label.name + ".layer")
   build_layer = ctx.executable._build_layer
@@ -272,7 +287,8 @@ docker_build_ = rule(
 #      # equivalent to FROM.
 #      base="//another/build:rule",]
 #
-#      # The base directory of the files, defaulted to this package.
+#      # The base directory of the files, defaulted to
+#      # the package of the input.
 #      # All files structure relatively to that path will be preserved.
 #      # A leading '/' mean the workspace root and this path is relative
 #      # to the current package by default.
diff --git a/tools/build_defs/docker/testdata/BUILD b/tools/build_defs/docker/testdata/BUILD
index d337e92f3f..cce876cfb6 100644
--- a/tools/build_defs/docker/testdata/BUILD
+++ b/tools/build_defs/docker/testdata/BUILD
@@ -8,7 +8,9 @@ load("/tools/build_defs/docker/docker", "docker_build")
 
 filegroup(
     name = "srcs",
-    srcs = glob(["**"]),
+    srcs = glob(["**"]) + [
+        "//tools/build_defs/docker/testdata/test:srcs",
+    ],
 )
 
 filegroup(
@@ -23,6 +25,17 @@ genrule(
 )
 
 docker_build(
+    name = "no_data_path_image",
+    files = ["//tools/build_defs/docker/testdata/test:test-data"],
+)
+
+docker_build(
+    name = "data_path_image",
+    data_path = ".",
+    files = ["//tools/build_defs/docker/testdata/test:test-data"],
+)
+
+docker_build(
     name = "gen_image",
     files = [":gen"],
 )
diff --git a/tools/build_defs/docker/testdata/test/BUILD b/tools/build_defs/docker/testdata/test/BUILD
new file mode 100644
index 0000000000..f12ea6b16a
--- /dev/null
+++ b/tools/build_defs/docker/testdata/test/BUILD
@@ -0,0 +1,15 @@
+package(
+    default_visibility = [
+        "//tools/build_defs/docker:__subpackages__",
+    ],
+)
+
+filegroup(
+    name = "srcs",
+    srcs = glob(["**"]),
+)
+
+filegroup(
+    name = "test-data",
+    srcs = ["test"],
+)
diff --git a/tools/build_defs/docker/testdata/test/test b/tools/build_defs/docker/testdata/test/test
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/tools/build_defs/docker/testdata/test/test