sh_binary has multiple outputs in some contexts

[aherrmann]

Description of the problem:

A sh_binary target that wraps a shell script has multiple outputs in some contexts. This means that a sh_binary cannot be used (at least not straight-forwardly) in some contexts. E.g. it cannot be used in srcs of another sh_binary, or $(rootpath ) cannot be applied to it in some contexts (instead $(rootpaths ) is required).

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

A sh_binary cannot be srcs to another sh_binary

sh_binary(
    name = "script",
    srcs = ["script.sh"],
)
sh_binary(
    name = "wrapper",
    # ERROR: ... in srcs attribute of sh_binary rule //repro:wrapper: you must specify exactly one file in 'srcs'
    srcs = [":script"],
)

(A use-case would be a sh_test wrapping a sh_binary and extending the runfiles tree with additional data attributes.)

A sh_binary cannot be used with $(rootpath )

# ERROR: ... in cmd attribute of genrule rule //repro:genrule: label '//repro:script' in $(location) expression expands to more than one file, please use $(locations //repro:script) instead.  Files (at most 5 shown) are: [repro/script, repro/script.sh]
genrule(
    name = "genrule",
    srcs = [":script"],
    outs = ["genrule.txt"],
    cmd = "echo $(rootpath :script) > $(OUTS)",
)

(A use-case would be generating a script file that calls another tool at runtime.)

Instead one has to use the plural form $(execroots ).

# LINUX - NO ERROR
#   repro/script repro/script.sh
# WINDOWS - NO ERROR
#   repro/script repro/script.exe repro/script.sh
genrule(
    name = "genrule",
    srcs = [":script"],
    outs = ["genrule.txt"],
    cmd = "echo $(rootpaths :script) > $(OUTS)",
)

(On Linux this happens to place the symlink repro/script first which makes it directly executable. However, on Windows it places the .exe second, meaning that additional logic is required to find the .exe from the result of $(rootpaths ).)

However, one can use the singular $(execroot ) in the context of sh_test:

# LINUX - NO ERROR
#   ARG repro/script
#   LOC .../execroot/com_github_digital_asset_daml/bazel-out/k8-opt/bin/repro/test.runfiles/com_github_digital_asset_daml/repro/script
# WINDOWS - NO ERROR
#   ARG repro/script.exe
#   LOC .../execroot/com_github_digital_asset_daml/bazel-out/x64_windows-opt/bin/repro/script.exe
sh_test(
    name = "test",
    srcs = ["test.sh"],
    deps = ["@bazel_tools//tools/bash/runfiles"],
    data = [":script"],
    args = ["$(rootpath :script)"],
)

Where test.sh looks as follows:

# Copy-pasted from the Bazel Bash runfiles library v2.
set -uo pipefail; f=bazel_tools/tools/bash/runfiles/runfiles.bash
source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
  source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \
  source "$0.runfiles/$f" 2>/dev/null || \
  source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
  source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
  { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e
# --- end runfiles.bash initialization v2 ---

for arg in "$@"; do
    echo ARG "$arg"
    echo LOC "$(rlocation "$TEST_WORKSPACE/$arg")"
done

What operating system are you running Bazel on?

Ubuntu 19.10
Windows 10

What's the output of bazel info release?

Linux: release 3.3.1- (@non-git)
Windows: release 3.3.1-patched-1dac3221f72f5d22a0b79f0531af1f63

If bazel info release returns "development version" or "(@non-git)", tell us how you built Bazel.

On Linux, built using nixpkgs revision 1d8018068278a717771e9ec4054dff1ebd3252b0
On Windows, built with the following patch, which should be irrelevant to the issue.

Have you found anything relevant by searching the web?

No

Any other information, logs, or outputs that you want to share?

We encountered this issue when trying to work around the removal of --noincompatible_windows_native_test_wrapper on Windows. See digital-asset/daml@2248fcd and digital-asset/daml@23f4a59. We tried to replace instances of custom test rules that wrote an executable .sh file as the test executable, by a more generic sh_inline_test macro that combines a custom rule that generates a script file with a sh_test. With this change we had to replace instances of ctx.executable.some_tool by $(rootpath :some_tool), which didn't work as described above. On Windows we have to be careful to find the .exe wrapper because the tool is invoked indirectly in a way that doesn't support shell scripts but only Windows executables.

The following test target illustrates the issue

cc_binary(
    name = "runner",
    srcs = ["runner.c"],
)

sh_inline_test(
    name = "inline-test",
    # ERROR: ... in _sh_inline_script rule //repro:inline-test_script: label '//repro:script' in $(location) expression expands to more than one file, please use $(locations //repro:script) instead.  Files (at most 5 shown) are: [repro/script, repro/script.sh]
    # cmd = "$$(rlocation $$TEST_WORKSPACE/$(rootpath :script))",
    # LINUX - NO ERROR
    #   runner rootpath repro/runner
    #   script rootpaths repro/script repro/script.sh
    #   Hello from script.sh
    # WINDOWS - ERROR
    #   runner rootpath repro/runner.exe
    #   script rootpaths repro/script repro/script.exe repro/script.sh
    #   ERROR .../execroot/com_github_digital_asset_daml/bazel-out/x64_windows-opt/bin/repro/script: %1 is not a valid Win32 application.
    cmd = """\
echo runner rootpath $(rootpath :runner)
echo script rootpaths $(rootpaths :script)
$$(rlocation $$TEST_WORKSPACE/$(rootpath :runner)) $$(rlocation $$TEST_WORKSPACE/$(rootpaths :script))
""",
    data = [":runner", ":script"],
)

With the following building blocks:
sh.bzl:

def _sh_inline_script_impl(ctx):
    cmd = ctx.attr.cmd
    cmd = ctx.expand_location(cmd, ctx.attr.data)
    cmd = ctx.expand_make_variables("cmd", cmd, {})
    ctx.actions.expand_template(
        template = ctx.file._template,
        output = ctx.outputs.output,
        is_executable = True,
        substitutions = {
            "%cmd%": cmd,
        },
    )

    runfiles = ctx.runfiles(files = [ctx.outputs.output] + ctx.files.data)
    for data_dep in ctx.attr.data:
        runfiles = runfiles.merge(data_dep[DefaultInfo].default_runfiles)

    return DefaultInfo(
        files = depset([ctx.outputs.output]),
        runfiles = runfiles,
    )

_sh_inline_script = rule(
    _sh_inline_script_impl,
    attrs = {
        "cmd": attr.string(
            mandatory = True,
        ),
        "data": attr.label_list(
            allow_files = True,
        ),
        "output": attr.output(
            mandatory = True,
        ),
        "_template": attr.label(
            allow_single_file = True,
            default = "//repro:sh.tpl",
        ),
    },
)

def sh_inline_test(
        name,
        cmd,
        data = [],
        **kwargs):
    testonly = kwargs.pop("testonly", True)
    _sh_inline_script(
        name = name + "_script",
        cmd = cmd,
        output = name + ".sh",
        data = data,
        testonly = testonly,
    )
    native.sh_test(
        name = name,
        data = data,
        deps = ["@bazel_tools//tools/bash/runfiles"],
        srcs = [name + ".sh"],
        testonly = testonly,
        **kwargs
    )

sh.tpl:

#!/usr/bin/env bash
set +e
# Copy-pasted from the Bazel Bash runfiles library v2.
set -uo pipefail; f=bazel_tools/tools/bash/runfiles/runfiles.bash
source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
  source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \
  source "$0.runfiles/$f" 2>/dev/null || \
  source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
  source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
  { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e
# --- end runfiles.bash initialization v2 ---
set -e
%cmd%

runner.c:

#ifdef _WIN32
#include <stdio.h>
#include <windows.h>
#else
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#endif

#ifdef _WIN32
VOID exec(LPCTSTR lpApplicationName) {
  STARTUPINFO si;
  PROCESS_INFORMATION pi;

  ZeroMemory(&si, sizeof(si));
  si.cb = sizeof(si);
  ZeroMemory(&pi, sizeof(pi));

  BOOL r = CreateProcess(lpApplicationName, NULL, NULL, NULL, FALSE, 0, NULL,
                         NULL, &si, &pi);
  if (!r) {
    LPVOID lpMsgBuf;
    DWORD dw = GetLastError();

    FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM |
                      FORMAT_MESSAGE_IGNORE_INSERTS,
                  NULL, dw, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
                  (LPTSTR)&lpMsgBuf, 0, NULL);
    printf("ERROR %s: %s\n", lpApplicationName, lpMsgBuf);
    exit(EXIT_FAILURE);
  }
  WaitForSingleObject(pi.hProcess, INFINITE);
  CloseHandle(pi.hProcess);
  CloseHandle(pi.hThread);
}
#else
void exec(char *const prog) {
  char *const argv[] = {prog, NULL};
  execve(prog, argv, environ);
  perror("ERROR");
  exit(EXIT_FAILURE);
}
#endif

int main(int argc, char **argv) { exec(argv[1]); }

Full example: digital-asset/daml@a357bb4

[dws]

I've also noticed this. With a BUILD file as follows:

load(":expand_template.bzl", "expand_template")

expand_template(
    name = "file",
    template = "file.py.in",
    out = "file.py",
    substitutions = {
        "{{UTIL}}": "$(rootpath :util)",
    },
    data = [
        ":util",
    ],
)

py_binary(
    name = "prog",
    srcs = ["prog.py"],
    args = ["$(rootpath :util)"],
    data = [
        ":util",
    ],
)

sh_binary(
    name = "util",
    srcs = ["util.sh"],
)

With expand_template.bzl exactly as @laurentlb supplied it in bazelbuild/bazel-skylib#191

With file.py.in:

print("{{UTIL}}")

With prog,py:

import sys

print(f"argv: {sys.argv[1:]}")

With util.sh:

echo HELLO

and an empty WORKSPACE, I can

bazel run :prog

with no problems, but with

bazel build :file 

I get an error from bazel due to $(rootpath :util) expanding to two paths instead of one.

I verified that this behavior persists in bazel 3.7.0rc1 on Ubuntu 18.04.

[dws]

Even simpler: if we list the sh_binary label in the srcs of a genrule, then $(rootpath) breaks, but if we list it in the tools of a genrule, then $(rootpath) works:

# rootpath does not work here -- bazel wants to expand :util to two paths.
genrule(
    name = "file1",
    outs = ["file1.out"],
    srcs = [":util"],
    cmd = "echo $(rootpath :util) > $@",
)

# rootpath works here, where we list :util in tools instead of srcs.
genrule(
    name = "file2",
    outs = ["file2.out"],
    tools = [":util"],
    cmd = "echo $(rootpath :util) > $@",
)

sh_binary(
    name = "util",
    srcs = ["util.sh"],
)

[jheaff1]

This issue still exists in Bazel 5.3.1, is there a good workaround for it?

[bcsgh]

Or more generally: there needs to be a way to go from a *_binary label to "the command string that executes it" without having to write a custom rule. (I'm fine with it having to be done via a custom rule as long as that looks something like $(location) expansion so that the invocation of the rule can specify what gets expanded and how.)

[peakschris]

+1 I'm also facing issues due to this

[arrdem]

+1 also bitten by this

[fmeum]

I think that this may not be an issue with sh_binary, it's an issue in ctx.expand_locations that is triggered by sh_binary returning two files in DefaultInfo(files = ...). This line only considers files, not executable:
https://cs.opensource.google/bazel/bazel/+/98057becdd17b9e2079ed978e5deca6932c623c2:src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java;l=1243

CC @comius

[thesayyn]

Looks like this issue was fixed before https://github.com/bazelbuild/bazel/pull/16381/files/d8176177c95ff90c471db2c46085dfbc93fca4c1

and got reverted silently to "fix breakage in an internal use case" which sounds like the opposite of what should have happened. ab71a10

Something this trivial should not be reverted or at least be communicated clearly.

Cross-Ref: #20038

[thesayyn]

I'd like to fix this issue forever and have it not reverted due to "fix breakage in an internal use case". What needs to be done here?

[fmeum]

@brandjon Could you share the details of the use case that resulted in you rolling back the fix in ab71a10?

[fmeum]

@thesayyn A safe way to fix this would be to resubmit the reverted PR and have it use the executable only if the expansion would previously have failed (and explicitly not if files has size 1).

[thesayyn]

Oh great, Thank you! I'll put up a PR for this.