Fix workdir and expand arg location. (bazelbuild#452)

* Fix workdir so data / files can be accessible in the same path as expected for corresponding rules

* Expand arg location: args of form ($location ...) are now properly expanded 

* Fix bazel run arg duplication

cc/image.bzl

@@ -92,4 +92,5 @@ def cc_image(name, base = None, deps = [], layers = [], binary = None, **kwargs)
         visibility = visibility,
         tags = tags,
         args = kwargs.get("args"),
+        data = kwargs.get("data"),
     )

container/image.bzl

@@ -99,7 +99,8 @@ def _image_config(
         creation_time = None,
         env = None,
         base_config = None,
-        layer_name = None):
+        layer_name = None,
+        workdir = None):
     """Create the configuration for a new container image."""
     config = ctx.new_file(name + "." + layer_name + ".config")
 
@@ -147,8 +148,8 @@ def _image_config(
 
     if ctx.attr.user:
         args += ["--user=" + ctx.attr.user]
-    if ctx.attr.workdir:
-        args += ["--workdir=" + ctx.attr.workdir]
+    if workdir:
+        args += ["--workdir=" + workdir]
 
     inputs = layer_names
     for layer_name in layer_names:
@@ -206,7 +207,8 @@ def _impl(
         tars = None,
         output_executable = None,
         output_tarball = None,
-        output_layer = None):
+        output_layer = None,
+        workdir = None):
     """Implementation for the container_image rule.
 
   Args:
@@ -229,6 +231,7 @@ def _impl(
     output_executable: File to use as output for script to load docker image
     output_tarball: File, overrides ctx.outputs.out
     output_layer: File, overrides ctx.outputs.layer
+    workdir: str, overrides ctx.attr.workdir
   """
     name = name or ctx.label.name
     entrypoint = entrypoint or ctx.attr.entrypoint
@@ -282,6 +285,7 @@ def _impl(
             env = layer.env,
             base_config = config_file,
             layer_name = str(i),
+            workdir = workdir or ctx.attr.workdir,
         )
 
     # Construct a temporary name based on the build target.

container/image_test.py

@@ -462,6 +462,7 @@ def test_py_image(self):
         './app/testdata/py_image.binary.runfiles/io_bazel_rules_docker/testdata',
         './app/testdata/py_image.binary.runfiles/io_bazel_rules_docker/testdata/py_image.py',
         './app/testdata/py_image.binary.runfiles/io_bazel_rules_docker/testdata/py_image.binary',
+        './app/testdata/py_image.binary.runfiles/io_bazel_rules_docker/testdata/BUILD',
         './app/testdata/py_image.binary.runfiles/io_bazel_rules_docker/testdata/__init__.py',
         # TODO(mattmoor): The path normalization for symlinks should match
         # files to avoid this redundancy.
@@ -495,6 +496,7 @@ def test_cc_image(self):
         './app/testdata/cc_image.binary.runfiles/io_bazel_rules_docker',
         './app/testdata/cc_image.binary.runfiles/io_bazel_rules_docker/testdata',
         './app/testdata/cc_image.binary.runfiles/io_bazel_rules_docker/testdata/cc_image.binary',
+        './app/testdata/cc_image.binary.runfiles/io_bazel_rules_docker/testdata/BUILD',
         # TODO(mattmoor): The path normalization for symlinks should match
         # files to avoid this redundancy.
         '/app',
@@ -541,7 +543,7 @@ def test_java_image(self):
           '/app/io_bazel_rules_docker/testdata/java_image.binary.jar',
           '/app/io_bazel_rules_docker/testdata/java_image.binary'
         ]),
-        '-XX:MaxPermSize=128M', 'examples.images.Binary', 'arg0', 'arg1'])
+        '-XX:MaxPermSize=128M', 'examples.images.Binary', 'arg0', 'arg1', 'testdata/BUILD'])
 
   def test_war_image(self):
     with TestImage('war_image') as img:
@@ -572,7 +574,9 @@ def test_cc_image_args(self):
       self.assertConfigEqual(img, 'Entrypoint', [
         '/app/testdata/cc_image.binary',
         'arg0',
-        'arg1'])
+        'arg1',
+        'testdata/BUILD',
+      ])
 
   # Re-enable once https://github.com/bazelbuild/rules_d/issues/14 is fixed.
   # def test_d_image_args(self):
@@ -588,15 +592,19 @@ def test_py_image_args(self):
         '/usr/bin/python',
         '/app/testdata/py_image.binary',
         'arg0',
-        'arg1'])
+        'arg1',
+        'testdata/BUILD',
+      ])
 
   def test_py3_image_args(self):
     with TestImage('py3_image') as img:
       self.assertConfigEqual(img, 'Entrypoint', [
         '/usr/bin/python',
         '/app/testdata/py3_image.binary',
         'arg0',
-        'arg1'])
+        'arg1',
+        'testdata/BUILD',
+      ])
 
   def test_java_image_args(self):
     with TestImage('java_image') as img:
@@ -610,21 +618,27 @@ def test_java_image_args(self):
         '-XX:MaxPermSize=128M',
         'examples.images.Binary',
         'arg0',
-        'arg1'])
+        'arg1',
+        'testdata/BUILD',
+      ])
 
   def test_go_image_args(self):
     with TestImage('go_image') as img:
       self.assertConfigEqual(img, 'Entrypoint', [
         '/app/testdata/go_image.binary',
         'arg0',
-        'arg1'])
+        'arg1',
+        'testdata/BUILD',
+      ])
 
   def test_rust_image_args(self):
     with TestImage('rust_image') as img:
       self.assertConfigEqual(img, 'Entrypoint', [
         '/app/testdata/rust_image_binary',
         'arg0',
-        'arg1'])
+        'arg1',
+        'testdata/BUILD',
+      ])
 
   def test_scala_image_args(self):
     with TestImage('scala_image') as img:
@@ -639,7 +653,9 @@ def test_scala_image_args(self):
         '/app/io_bazel_rules_docker/testdata/scala_image.binary',
         'examples.images.Binary',
         'arg0',
-        'arg1'])
+        'arg1',
+        'testdata/BUILD',
+      ])
 
   def test_groovy_image_args(self):
     with TestImage('groovy_image') as img:
@@ -654,7 +670,9 @@ def test_groovy_image_args(self):
         '/app/io_bazel_rules_docker/testdata/groovy_image.binary',
         'examples.images.Binary',
         'arg0',
-        'arg1'])
+        'arg1',
+        'testdata/BUILD',
+      ])
 
   def test_nodejs_image_args(self):
     with TestImage('nodejs_image') as img:

container/layer_tools.bzl

@@ -158,6 +158,8 @@ def incremental_load(
             ),
         ]
         if run:
+            # bazel automatically passes ctx.attr.args to the binary on run, so args get passed in
+            # twice. See https://github.com/bazelbuild/rules_docker/issues/374
             run_statements += [
                 "docker run %s %s \"$@\"" % (run_flags, tag_reference),
             ]

d/image.bzl

@@ -65,4 +65,5 @@ def d_image(name, base = None, deps = [], layers = [], binary = None, **kwargs):
         visibility = visibility,
         tags = tags,
         args = kwargs.get("args"),
+        data = kwargs.get("data"),
     )

go/image.bzl

@@ -96,4 +96,5 @@ def go_image(name, base = None, deps = [], layers = [], binary = None, **kwargs)
         visibility = visibility,
         tags = tags,
         args = kwargs.get("args"),
+        data = kwargs.get("data"),
     )

groovy/image.bzl

@@ -85,6 +85,7 @@ def groovy_image(
         visibility = visibility,
         tags = tags,
         args = kwargs.get("args"),
+        data = kwargs.get("data"),
     )
 
 def repositories():

java/image.bzl

@@ -106,6 +106,7 @@ load(
     "//lang:image.bzl",
     "dep_layer_impl",
     "layer_file_path",
+    "runfiles_dir",
 )
 
 def _jar_dep_layer_impl(ctx):
@@ -138,6 +139,7 @@ jar_dep_layer = rule(
 def _jar_app_layer_impl(ctx):
     """Appends the app layer with all remaining runfiles."""
 
+    workdir = ctx.attr.workdir or "/".join([runfiles_dir(ctx), ctx.workspace_name])
     available = depset()
     for jar in ctx.attr.jar_layers:
         available += java_files(jar)
@@ -166,12 +168,15 @@ def _jar_app_layer_impl(ctx):
 
     binary_path = layer_file_path(ctx, ctx.files.binary[0])
     classpath_path = layer_file_path(ctx, classpath_file)
+
+    # args of the form $(location :some_target) are expanded to the path of the underlying file
+    args = [ctx.expand_location(arg, ctx.attr.data) for arg in ctx.attr.args]
     entrypoint = [
         "/usr/bin/java",
         "-cp",
         # Support optionally passing the classpath as a file.
         "@" + classpath_path if ctx.attr._classpath_as_file else classpath,
-    ] + ctx.attr.jvm_flags + [ctx.attr.main_class] + ctx.attr.args
+    ] + ctx.attr.jvm_flags + [ctx.attr.main_class] + args
 
     file_map = {
         layer_file_path(ctx, f): f
@@ -184,6 +189,7 @@ def _jar_app_layer_impl(ctx):
         directory = "/",
         file_map = file_map,
         entrypoint = entrypoint,
+        workdir = workdir,
     )
 
 jar_app_layer = rule(
@@ -215,7 +221,9 @@ jar_app_layer = rule(
         "directory": attr.string(default = "/app"),
         # https://github.com/bazelbuild/bazel/issues/2176
         "data_path": attr.string(default = "."),
+        "workdir": attr.string(default = ""),
         "legacy_run_behavior": attr.bool(default = False),
+        "data": attr.label_list(allow_files = True),
     }.items()),
     executable = True,
     outputs = _container.image.outputs,
@@ -271,6 +279,7 @@ def java_image(
         jar_layers = layers,
         visibility = visibility,
         args = kwargs.get("args"),
+        data = kwargs.get("data"),
     )
 
 def _war_dep_layer_impl(ctx):

lang/image.bzl

@@ -28,7 +28,7 @@ def _binary_name(ctx):
         ctx.attr.binary.label.name,
     ])
 
-def _runfiles_dir(ctx):
+def runfiles_dir(ctx):
     # For @foo//bar/baz:blah this would translate to
     # /app/bar/baz/blah.runfiles
     return _binary_name(ctx) + ".runfiles"
@@ -38,7 +38,7 @@ def _runfiles_dir(ctx):
 def _reference_dir(ctx):
     # For @foo//bar/baz:blah this would translate to
     # /app/bar/baz/blah.runfiles/foo
-    return "/".join([_runfiles_dir(ctx), ctx.workspace_name])
+    return "/".join([runfiles_dir(ctx), ctx.workspace_name])
 
 # The special "external" directory which is an alternate way of accessing
 # other repositories.
@@ -65,7 +65,7 @@ def _final_emptyfile_path(ctx, name):
     # so we "fix" the empty files' paths by removing "external/" and basing them
     # directly on the runfiles path.
 
-    return "/".join([_runfiles_dir(ctx), name[len("external/"):]])
+    return "/".join([runfiles_dir(ctx), name[len("external/"):]])
 
 # The final location that this file needs to exist for the foo_binary target to
 # properly execute.
@@ -166,6 +166,7 @@ def _app_layer_impl(ctx, runfiles = None, emptyfiles = None):
 
     runfiles = runfiles or _default_runfiles
     emptyfiles = emptyfiles or _default_emptyfiles
+    workdir = ctx.attr.workdir or "/".join([runfiles_dir(ctx), ctx.workspace_name])
 
     # Compute the set of runfiles that have been made available
     # in our base image, tracking absolute paths.
@@ -211,21 +212,25 @@ def _app_layer_impl(ctx, runfiles = None, emptyfiles = None):
         _binary_name(ctx): _final_file_path(ctx, ctx.executable.binary),
         # Create a directory symlink from <workspace>/external to the runfiles
         # root, since they may be accessed via either path.
-        _external_dir(ctx): _runfiles_dir(ctx),
+        _external_dir(ctx): runfiles_dir(ctx),
     })
 
+    # args of the form $(location :some_target) are expanded to the path of the underlying file
+    args = [ctx.expand_location(arg, ctx.attr.data) for arg in ctx.attr.args]
+
     return _container.image.implementation(
         ctx,
         # We use all absolute paths.
         directory = "/",
         file_map = file_map,
         empty_files = empty_files,
         symlinks = symlinks,
+        workdir = workdir,
         # Use entrypoint so we can easily add arguments when the resulting
         # image is `docker run ...`.
         # Per: https://docs.docker.com/engine/reference/builder/#entrypoint
         # we should use the "exec" (list) form of entrypoint.
-        entrypoint = ctx.attr.entrypoint + [_binary_name(ctx)] + ctx.attr.args,
+        entrypoint = ctx.attr.entrypoint + [_binary_name(ctx)] + args,
     )
 
 app_layer = rule(
@@ -251,9 +256,10 @@ app_layer = rule(
 
         # Override the defaults.
         "data_path": attr.string(default = "."),
-        "workdir": attr.string(default = "/app"),
+        "workdir": attr.string(default = ""),
         "directory": attr.string(default = "/app"),
         "legacy_run_behavior": attr.bool(default = False),
+        "data": attr.label_list(allow_files = True),
     }.items()),
     executable = True,
     outputs = _container.image.outputs,

nodejs/image.bzl

@@ -151,4 +151,5 @@ def nodejs_image(
         visibility = visibility,
         tags = tags,
         args = kwargs.get("args"),
+        data = kwargs.get("data"),
     )

python/image.bzl

@@ -73,7 +73,6 @@ def py_image(name, base = None, deps = [], layers = [], **kwargs):
 
     # TODO(mattmoor): Consider using par_binary instead, so that
     # a single target can be used for all three.
-
     native.py_binary(name = binary_name, deps = deps + layers, **kwargs)
 
     # TODO(mattmoor): Consider making the directory into which the app
@@ -95,4 +94,5 @@ def py_image(name, base = None, deps = [], layers = [], **kwargs):
         visibility = visibility,
         tags = tags,
         args = kwargs.get("args"),
+        data = kwargs.get("data"),
     )

python3/image.bzl

@@ -95,4 +95,5 @@ def py3_image(name, base = None, deps = [], layers = [], **kwargs):
         visibility = visibility,
         tags = tags,
         args = kwargs.get("args"),
+        data = kwargs.get("data"),
     )

rust/image.bzl

@@ -65,4 +65,5 @@ def rust_image(name, base = None, deps = [], layers = [], binary = None, **kwarg
         visibility = visibility,
         tags = tags,
         args = kwargs.get("args"),
+        data = kwargs.get("data"),
     )

scala/image.bzl

@@ -77,6 +77,7 @@ def scala_image(
         visibility = visibility,
         tags = tags,
         args = kwargs.get("args"),
+        data = kwargs.get("data"),
     )
 
 def repositories():