Fix workdir and expand arg location. #452
Conversation
Globegitter
changed the title
|
Thanks a lot for sending this PR! Looks good overall, will review in more detail in a bit, kicked off presubmits to make sure everything is passing. |
python3/image.bzl
Outdated
| @@ -74,6 +74,8 @@ def py3_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. | |||
|
|
|||
| args = kwargs.pop("args", None) | |||
There was a problem hiding this comment.
why is this necessary for py3 image but not for others?
There was a problem hiding this comment.
Sorry this is still a leftover, I also tried to fix the issue that args are being passed in twice.
| @@ -19,6 +21,9 @@ def main(): | |||
| print('Second: %d' % py3_image_library.fn(2)) | |||
| print('Third: %d' % py3_image_library.fn(3)) | |||
| print('Fourth: %d' % py3_image_library.fn(4)) | |||
| print(sys.argv) | |||
There was a problem hiding this comment.
can you add short doc line somewhere indicating these two tests (py3, py) expect the third argument to be a file location
lang/image.bzl
Outdated
| "directory": attr.string(default = "/app"), | ||
| "legacy_run_behavior": attr.bool(default = False), | ||
| "data": attr.label_list(cfg = "data", allow_files = True), |
There was a problem hiding this comment.
iiuc, this should have as value host/target? https://docs.bazel.build/versions/master/skylark/lib/attr.html#label_list
There was a problem hiding this comment.
I have seen some doc referring that for data it should still be data, there is also a ticket for it: bazelbuild/bazel#4316. But here: https://github.com/bazelbuild/examples/blob/5d1e5bd82e061e8e2e04bb54883fc4c79c935f76/rules/runfiles/execute.bzl#L32 there is nothing, so I will just remove it.
lang/image.bzl
Outdated
| }) | ||
|
|
||
| args = [ctx.expand_location(arg, ctx.attr.data) for arg in ctx.attr.args] |
There was a problem hiding this comment.
add doc comment above explaining you are expanding locations of the form $(location :some_target)
java/image.bzl
Outdated
| "legacy_run_behavior": attr.bool(default = False), | ||
| "data": attr.label_list(cfg = "data", allow_files = True), |
There was a problem hiding this comment.
should be host/target https://docs.bazel.build/versions/master/skylark/lib/attr.html#label_list
java/image.bzl
Outdated
| @@ -166,12 +168,13 @@ 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 = [ctx.expand_location(arg, ctx.attr.data) for arg in ctx.attr.args] | |||
There was a problem hiding this comment.
add doc comment about expanding location
container/layer_tools.bzl
Outdated
| @@ -158,6 +158,8 @@ def incremental_load( | |||
| ), | |||
| ] | |||
| if run: | |||
| # bazel automatically passes ctx.attr.args to the binary on run, so need to truncate | |||
| # them here to not pass twice, as they are already part of the docker entrypoint | |||
There was a problem hiding this comment.
Can you add a pointer to the Bazel issue you opened so we can track and remove once that is fixed?
There was a problem hiding this comment.
This is also still a leftover from when I tried to also fix that issue. Yes will clean up.
This supersedes #365. And should fix #355, #378, #451