Fix docker_build file path when file is generated

If the file is generated, by a genrule for instance, then
the stripping of the prefix was incorrect because the used path
was containing 'bazel-out/...' prefix. This change moved the prefix
stripping to the Skylark rule so we can use "short_path".

Also fix the test paths.
Tested by bazel test //tools/build_defs/docker:build_test.

--
Change-Id: Ia3eb98268505002bade1f6b0dd916e1a9767a530
Reviewed-on: https://bazel-review.googlesource.com/1770
MOS_MIGRATED_REVID=99710676

5 files changed, 61 insertions, 37 deletions

diff --git a/tools/build_defs/docker/BUILD b/tools/build_defs/docker/BUILD
index 3b6ebd937a..f53e1fa264 100644
--- a/tools/build_defs/docker/BUILD
+++ b/tools/build_defs/docker/BUILD
@@ -1,21 +1,31 @@
 licenses(["notice"])  # Apache 2.0
 
+TEST_TARGETS = [
+    "base_with_entrypoint",
+    "base_with_volume",
+    "derivative_with_cmd",
+    "derivative_with_shadowed_cmd",
+    "derivative_with_volume",
+    "files_base",
+    "files_with_files_base",
+    "files_with_tar_base",
+    "tar_base",
+    "tar_with_files_base",
+    "tar_with_tar_base",
+    "generated_tarball",
+    "with_env",
+    "with_double_env",
+    "link_with_files_base",
+]
+
 TEST_DATA = [
-    "//tools/build_defs/docker/testdata:base_with_entrypoint",
-    "//tools/build_defs/docker/testdata:base_with_volume",
-    "//tools/build_defs/docker/testdata:derivative_with_cmd",
-    "//tools/build_defs/docker/testdata:derivative_with_shadowed_cmd",
-    "//tools/build_defs/docker/testdata:derivative_with_volume",
-    "//tools/build_defs/docker/testdata:files_base",
-    "//tools/build_defs/docker/testdata:files_with_files_base",
-    "//tools/build_defs/docker/testdata:files_with_tar_base",
-    "//tools/build_defs/docker/testdata:tar_base",
-    "//tools/build_defs/docker/testdata:tar_with_files_base",
-    "//tools/build_defs/docker/testdata:tar_with_tar_base",
-    "//tools/build_defs/docker/testdata:generated_tarball",
-    "//tools/build_defs/docker/testdata:with_env",
-    "//tools/build_defs/docker/testdata:with_double_env",
-    "//tools/build_defs/docker/testdata:link_with_files_base",
+    "//tools/build_defs/docker/testdata:" + t
+    for t in TEST_TARGETS
+] + [
+    "//tools/build_defs/docker/testdata:notop_" + t
+    for t in TEST_TARGETS
+] + [
+    "//tools/build_defs/docker/testdata:gen_image",
 ]
 
 sh_test(
diff --git a/tools/build_defs/docker/build_layer.py b/tools/build_defs/docker/build_layer.py
index b9aa4316c7..fcf67a4bb8 100644
--- a/tools/build_defs/docker/build_layer.py
+++ b/tools/build_defs/docker/build_layer.py
@@ -48,10 +48,6 @@ gflags.RegisterValidator(
     message='--link value should contains a : separator')
 
 gflags.DEFINE_string(
-    'file_path', '',
-    'The path to strip from the files passed using the --file option')
-
-gflags.DEFINE_string(
     'directory', None,
     'Directory in which to store the file inside the layer')
 
@@ -64,8 +60,7 @@ class DockerLayer(object):
   class DebError(Exception):
     pass
 
-  def __init__(self, output, file_path, directory):
-    self.file_path = file_path
+  def __init__(self, output, directory):
     self.directory = directory
     self.output = output
 
@@ -76,22 +71,18 @@ class DockerLayer(object):
   def __exit__(self, t, v, traceback):
     self.tarfile.close()
 
-  def add_file(self, f):
+  def add_file(self, f, destfile):
     """Add a file to the layer.
 
     Args:
        f: the file to add to the layer
+       destfile: the name of the file in the layer
 
-    `f` will be copied into the `self.directory` sub-directory of the layer.
-    Directory structure beyond `self.file_path` will be preserved (so, if
-    `f`'s path is `a/b/c/file`, `directory` is `d/e` and `file_path` is `a/b`,
-    the path of the file in the layer will be `d/e/c/file`).
+    `f` will be copied to `self.directory/destfile` in the layer.
     """
-    dest = f
+    dest = destfile
     # TODO(mattmoor): Consider applying the working directory to all four
     # options, not just files...
-    if dest.startswith(self.file_path):
-      dest = dest[len(self.file_path):]
     if self.directory:
       dest = self.directory + '/' + dest
     dest = dest.lstrip('/')  # Remove leading slashes
@@ -152,9 +143,10 @@ class DockerLayer(object):
 
 
 def main(unused_argv):
-  with DockerLayer(FLAGS.output, FLAGS.file_path, FLAGS.directory) as layer:
+  with DockerLayer(FLAGS.output, FLAGS.directory) as layer:
     for f in FLAGS.file:
-      layer.add_file(f)
+      (inf, tof) = f.split('=', 1)
+      layer.add_file(inf, tof)
     for tar in FLAGS.tar:
       layer.add_tar(tar)
     for deb in FLAGS.deb:
diff --git a/tools/build_defs/docker/build_test.sh b/tools/build_defs/docker/build_test.sh
index cae75d7069..04a002510f 100755
--- a/tools/build_defs/docker/build_test.sh
+++ b/tools/build_defs/docker/build_test.sh
@@ -174,6 +174,11 @@ function check_layers() {
   check_layers_aux "notop_$input" "$@"
 }
 
+function test_gen_image() {
+  grep -Fsq "./gen.out" "$TEST_DATA_DIR/gen_image.tar" \
+    || fail "'./gen.out' not found in '$TEST_DATA_DIR/gen_image.tar'"
+}
+
 function test_files_base() {
   check_layers "files_base" \
     "240dd12c02aee796394ce18eee3108475f7d544294b17fc90ec54e983601fe1b"
diff --git a/tools/build_defs/docker/docker.bzl b/tools/build_defs/docker/docker.bzl
index 6ad6d4f4e6..1d803f4864 100644
--- a/tools/build_defs/docker/docker.bzl
+++ b/tools/build_defs/docker/docker.bzl
@@ -55,6 +55,12 @@ def _short_path_dirname(path):
   sp = path.short_path
   return sp[:sp.rfind("/")]
 
+def _dest_path(f, strip_prefix):
+  """Returns the short path of f, stripped of strip_prefix."""
+  if f.short_path.startswith(strip_prefix):
+    return f.short_path[len(strip_prefix):]
+  return f.short_path
+
 def _build_layer(ctx):
   """Build the current layer for appending it the base layer."""
   # Compute the relative path
@@ -69,11 +75,11 @@ def _build_layer(ctx):
   layer = ctx.new_file(ctx.label.name + ".layer")
   build_layer = ctx.executable._build_layer
   args = [
-      "--file_path=" + data_path,
       "--output=" + layer.path,
       "--directory=" + ctx.attr.directory
       ]
-  args += ["--file=" + f.path for f in ctx.files.files]
+  args += ["--file=%s=%s" % (f.path, _dest_path(f, data_path))
+           for f in ctx.files.files]
   args += ["--tar=" + f.path for f in ctx.files.tars]
   args += ["--deb=" + f.path for f in ctx.files.debs]
   args += ["--link=%s:%s" % (k, ctx.attr.symlinks[k])
diff --git a/tools/build_defs/docker/testdata/BUILD b/tools/build_defs/docker/testdata/BUILD
index a8d5e6faa4..e365847cff 100644
--- a/tools/build_defs/docker/testdata/BUILD
+++ b/tools/build_defs/docker/testdata/BUILD
@@ -1,10 +1,21 @@
 package(
     default_visibility = [
-        "//tools/build_rules/docker:__subpackages__",
+        "//tools/build_defs/docker:__subpackages__",
     ],
 )
 
-load("/tools/build_rules/docker/docker", "docker_build")
+load("/tools/build_defs/docker/docker", "docker_build")
+
+genrule(
+    name = "gen",
+    outs = ["gen.out"],
+    cmd = "echo generated > $@",
+)
+
+docker_build(
+    name = "gen_image",
+    files = [":gen"],
+)
 
 docker_build(
     name = "files_base",
@@ -89,7 +100,7 @@ docker_build(
 py_binary(
     name = "extras_gen",
     srcs = ["extras_gen.py"],
-    deps = ["//tools/build_rules/docker:archive"],
+    deps = ["//tools/build_defs/docker:archive"],
 )
 
 genrule(
@@ -135,7 +146,7 @@ docker_build(
 py_binary(
     name = "strip_top",
     srcs = ["strip_top.py"],
-    deps = ["//third_party/bazel/tools/build_defs/docker:archive"],
+    deps = ["//tools/build_defs/docker:archive"],
 )
 
 [genrule(