docker_build: Properly handle / as data_path

The handling in _compute_data_path would incorrectly result in an empty
strip_prefix being passed to _dest_path. The outcome was all files would
end up at the top of directory, instead of nested according to the
repository structure.

This change brings the behavior in line with the documentation, which
states:

> This path can be absolute from the workspace root if starting with a
> `/` or relative to the rule's directory.

Fixes https://github.com/bazelbuild/bazel/issues/677

--
Change-Id: Ifdab97ed0e851cf6cabc7bd3206343766861b725
Reviewed-on: https://bazel-review.googlesource.com/#/c/2480
MOS_MIGRATED_REVID=109418486

4 files changed, 47 insertions, 1 deletions

diff --git a/tools/build_defs/docker/BUILD b/tools/build_defs/docker/BUILD
index 01fe75c3b4..d5d6f7b1f7 100644
--- a/tools/build_defs/docker/BUILD
+++ b/tools/build_defs/docker/BUILD
@@ -36,6 +36,8 @@ TEST_DATA = [
     "//tools/build_defs/docker/testdata:gen_image.tar",
     "//tools/build_defs/docker/testdata:data_path_image.tar",
     "//tools/build_defs/docker/testdata:no_data_path_image.tar",
+    "//tools/build_defs/docker/testdata:absolute_data_path_image.tar",
+    "//tools/build_defs/docker/testdata:root_data_path_image.tar",
     "//tools/build_defs/docker/testdata:dummy_repository.tar",
     "//tools/build_defs/docker/testdata:extras_with_deb.tar",
 ]
diff --git a/tools/build_defs/docker/build_test.sh b/tools/build_defs/docker/build_test.sh
index ec4c00f38c..7aea3fd4f7 100755
--- a/tools/build_defs/docker/build_test.sh
+++ b/tools/build_defs/docker/build_test.sh
@@ -352,9 +352,13 @@ function get_layer_listing() {
 function test_data_path() {
   local no_data_path_sha="451d182e5c71840f00ba9726dc0239db73a21b7e89e79c77f677e3f7c5c23d44"
   local data_path_sha="9a41c9e1709558f7ef06f28f66e9056feafa7e0f83990801e1b27c987278d8e8"
+  local absolute_data_path_sha="f196c42ab4f3eb850d9655b950b824db2c99c01527703ac486a7b48bb2a34f44"
+  local root_data_path_sha="19d7fd26d67bfaeedd6232dcd441f14ee163bc81c56ed565cc20e73311c418b6"
 
   check_layers_aux "no_data_path_image" "${no_data_path_sha}"
   check_layers_aux "data_path_image" "${data_path_sha}"
+  check_layers_aux "absolute_data_path_image" "${absolute_data_path_sha}"
+  check_layers_aux "root_data_path_image" "${root_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 = "."
@@ -367,6 +371,29 @@ function test_data_path() {
     './
 ./test/
 ./test/test'
+
+  # With an absolute path for data_path, we should strip that prefix
+  # from the files' paths. Since the testdata images are in
+  # //tools/build_defs/docker/testdata and data_path is set to
+  # "/tools/build_defs", we should have `docker` as the top-level
+  # directory.
+  check_eq "$(get_layer_listing "absolute_data_path_image" "${absolute_data_path_sha}")" \
+    './
+./docker/
+./docker/testdata/
+./docker/testdata/test/
+./docker/testdata/test/test'
+
+  # With data_path = "/", we expect the entire path from the repository
+  # root.
+  check_eq "$(get_layer_listing "root_data_path_image" "${root_data_path_sha}")" \
+    "./
+./tools/
+./tools/build_defs/
+./tools/build_defs/docker/
+./tools/build_defs/docker/testdata/
+./tools/build_defs/docker/testdata/test/
+./tools/build_defs/docker/testdata/test/test"
 }
 
 function test_extras_with_deb() {
diff --git a/tools/build_defs/docker/docker.bzl b/tools/build_defs/docker/docker.bzl
index 9b1cc31da9..395f0277d0 100644
--- a/tools/build_defs/docker/docker.bzl
+++ b/tools/build_defs/docker/docker.bzl
@@ -71,7 +71,10 @@ def _compute_data_path(out, 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.
+    # as an absolute path. A data_path of / should preserve the entire
+    # path up to the repository root.
+    if data_path == "/":
+      return data_path
     if len(data_path) >= 2 and data_path[0:2] == "./":
       data_path = data_path[2:]
     if not data_path or data_path == ".":  # Relative to current package
diff --git a/tools/build_defs/docker/testdata/BUILD b/tools/build_defs/docker/testdata/BUILD
index 88f248f1b2..c1d1896e50 100644
--- a/tools/build_defs/docker/testdata/BUILD
+++ b/tools/build_defs/docker/testdata/BUILD
@@ -39,6 +39,20 @@ docker_build(
 )
 
 docker_build(
+    name = "absolute_data_path_image",
+    data_path = "/tools/build_defs",
+    files = ["//tools/build_defs/docker/testdata/test:test-data"],
+    mode = "0644",
+)
+
+docker_build(
+    name = "root_data_path_image",
+    data_path = "/",
+    files = ["//tools/build_defs/docker/testdata/test:test-data"],
+    mode = "0644",
+)
+
+docker_build(
     name = "gen_image",
     files = [":gen"],
     mode = "0644",