Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Starlark, run_shell: not passing arguments to script #6391

Closed
laszlocsomor opened this issue Oct 15, 2018 · 8 comments
Closed

Starlark, run_shell: not passing arguments to script #6391

laszlocsomor opened this issue Oct 15, 2018 · 8 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee)

Comments

@laszlocsomor
Copy link
Contributor

Description of the problem / feature request:

When using ctx.actions.run_shell with a shell script's path for command, the arguments are not passed to the script.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

I have a simple Starlark rule that write a script (script1), executes it using run_shell, and expects it to write another script (script2).

BUILD file:

load(":args_test.bzl", "args_test")
args_test(name = "y")

args_test.bzl:

def _impl(ctx):
    script1 = ctx.actions.declare_file(ctx.label.name + "-1.bash")
    script2 = ctx.actions.declare_file(ctx.label.name + "-2.bash")
    ctx.actions.write(
        script1,
        is_executable = True,
        content = "\n".join([
            "#!/bin/bash",
            "set -euo pipefail",
            "echo >&2 \"DEBUG: args=($@)\"",
            "echo -e '#!/bin/sh\\necho \"Failed!\"' > $1",
        ]),
    )

    ctx.actions.run_shell(
        tools = [script1],
        outputs = [script2],
        command = script1.path,
        arguments = [script2.path, "a", "b"],
    )

    return [DefaultInfo(executable = script2)]

args_test = rule(
    executable = True,
    test = True,
    implementation = _impl,
)

Bazel output:

  $ bazel test //:y --verbose_failures 
INFO: Analysed target //:y (0 packages loaded).
INFO: Found 1 test target...
ERROR: /tmp/foo/BUILD:2:1: error executing shell command: '/bin/bash -c bazel-out/k8-fastbuild/bin/y-1.bash  bazel-out/k8-fastbuild/bin/y-2.bash a b' failed (Exit 1): bash failed: error executing command 
  (cd /usr/local/google/home/laszlocsomor/.cache/bazel/_bazel_laszlocsomor/bc5487f033a0af65dff1ff0bd4000d75/execroot/__main__ && \
  exec env - \
  /bin/bash -c bazel-out/k8-fastbuild/bin/y-1.bash '' bazel-out/k8-fastbuild/bin/y-2.bash a b)

Use --sandbox_debug to see verbose messages from the sandbox
DEBUG: args=()
bazel-out/k8-fastbuild/bin/y-1.bash: line 4: $1: unbound variable
Target //:y failed to build
INFO: Elapsed time: 0.187s, Critical Path: 0.07s
INFO: 0 processes.
FAILED: Build did NOT complete successfully

FAILED: Build did NOT complete successfully

I expected to see "a" and "b" in the "DEBUG" line, but apparently no args are passed.

What operating system are you running Bazel on?

Debian GNU/Linux rodete

What's the output of bazel info release?

release 0.17.2

@laszlocsomor
Copy link
Contributor Author

In fact the Bash invocation seems wrong, because I can repro this outside of Bazel:

/tmp/args.sh:

#!/bin/bash
echo "Hello args=($@)"

The unwanted behavior:

  $ /bin/bash -c /tmp/args.sh '' bazel-out/k8-fastbuild/bin/y-2.bash a b
Hello args=()

Adding quotes solves the problem:

  $ /bin/bash -c "/tmp/args.sh '' bazel-out/k8-fastbuild/bin/y-2.bash a b"
Hello args=( bazel-out/k8-fastbuild/bin/y-2.bash a b)

And to explain what's the space after "args=(", let's change the '' argument:

  $ /bin/bash -c "/tmp/args.sh 'x' bazel-out/k8-fastbuild/bin/y-2.bash a b"
Hello args=(x bazel-out/k8-fastbuild/bin/y-2.bash a b)

@laszlocsomor
Copy link
Contributor Author

FYI: as a workaround, I can abstain from using arguments and instead pass everything as a single string. However, that puts the burden of correct quoting on me.

  $ cat args_test.bzl 
def _impl(ctx):
    script1 = ctx.actions.declare_file(ctx.label.name + "-1.bash")
    script2 = ctx.actions.declare_file(ctx.label.name + "-2.bash")
    ctx.actions.write(script1, is_executable = True,
                      content = "\n".join([
                          "#!/bin/bash",
                          "set -euo pipefail",
                          "echo >&2 \"DEBUG: args=($@)\"",
                          "echo -e '#!/bin/sh\\necho \"Failed!\"' > $1",
                      ]))

    ctx.actions.run_shell(
        tools = [script1],
        outputs = [script2],
        command = "%s %s %s %s" % (script1.path, script2.path, repr(""), "b"),
    )

    return [DefaultInfo(executable = script2)]

args_test = rule(
    executable = True,
    test = True,
    implementation = _impl,
)

Output:

...
DEBUG: args=(bazel-out/k8-fastbuild/bin/y-2.bash  b)

laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Oct 15, 2018
See bazelbuild#6391

Change-Id: Ia38669a9f3747e76853402775678c992723ccd72
@laurentlb
Copy link
Contributor

You're expected to use '$@' as part of the command:

$ /bin/bash -c 'echo $@' '' arg1 arg2 arg3                                                                                                    arg1 arg2 arg3

So I imagine this will fix the issue:

    ctx.actions.run_shell(
        tools = [script1],
        outputs = [script2],
        command = script1.path + " $@",
        arguments = [script2.path, "a", "b"],
    )

If your command is just script1.path, can you use ctx.actions.run instead?

@laszlocsomor
Copy link
Contributor Author

You're expected to use '$@' as part of the command:

By whom?

If your command is just script1.path, can you use ctx.actions.run instead?

No, that won't work on Windows.

@laurentlb
Copy link
Contributor

laurentlb commented Oct 17, 2018

Well, if you want to use the arguments, you should use $@ (or $1, $2...).

@jin
Copy link
Member

jin commented Jan 14, 2019

@laurentlb could you add a priority to this issue, please?

@mboes
Copy link
Contributor

mboes commented Jan 15, 2019

This issue I just filed is superficially related: #7122. But concerns differences in behaviour between Windows and Linux.

@laurentlb laurentlb added the P3 We're not considering working on this, but happy to review a PR. (No assignee) label Jan 17, 2019
@laurentlb
Copy link
Contributor

Closing this issue, because the behavior was intended.
We can follow up on #7122 for the problem on Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee)
Projects
None yet
Development

No branches or pull requests

4 participants