From 2580906d97bb329702fa4e9620e2a4c018679176 Mon Sep 17 00:00:00 2001 From: Adam Cozzette Date: Mon, 27 Apr 2026 20:16:25 -0700 Subject: [PATCH 1/2] feat: add `tools` parameter to `run_binary()` One missing feature in `run_binary()` is that it does not support a way to provide any additional dependencies for the execution platform. All inputs other than the `tool` binary itself are built for the target platform. This means that, for example, there is no reliable way to provide another binary for the tool to invoke during the build action. If the execution and target platforms are sufficiently different then this will fail. This change remedies the problem by introducing a new `tools` parameter analogous to the one on [genrule](https://bazel.build/reference/be/general#genrule.tools). This parameter works similarly to `srcs` except that it builds the dependencies in the `exec` configuration. Fixes #676. Co-Authored-By: Claude Sonnet 4.6 --- lib/private/run_binary.bzl | 19 ++- lib/tests/run_binary/BUILD.bazel | 215 +++++++++++++++++++++++++++++++ 2 files changed, 232 insertions(+), 2 deletions(-) diff --git a/lib/private/run_binary.bzl b/lib/private/run_binary.bzl index 7ce403824..5dbc141ad 100644 --- a/lib/private/run_binary.bzl +++ b/lib/private/run_binary.bzl @@ -50,13 +50,15 @@ Possible fixes: # Location and Make variable expansion can reference paths that aren't path mapped. can_path_map = True + targets = ctx.attr.tools + ctx.attr.srcs + inputs = ctx.files.tools + ctx.files.srcs for a in ctx.attr.args: - expanded = expand_variables(ctx, ctx.expand_location(a, targets = ctx.attr.srcs), inputs = ctx.files.srcs, outs = outputs) + expanded = expand_variables(ctx, ctx.expand_location(a, targets = targets), inputs = inputs, outs = outputs) can_path_map = can_path_map and expanded == a args.add_all(split_args(expanded)) envs = {} for k, v in ctx.attr.env.items(): - envs[k] = expand_variables(ctx, ctx.expand_location(v, targets = ctx.attr.srcs), inputs = ctx.files.srcs, outs = outputs, attribute_name = "env") + envs[k] = expand_variables(ctx, ctx.expand_location(v, targets = targets), inputs = inputs, outs = outputs, attribute_name = "env") can_path_map = can_path_map and envs[k] == v stamp = maybe_stamp(ctx) @@ -72,6 +74,7 @@ Possible fixes: outputs = outputs, inputs = inputs, executable = ctx.executable.tool, + tools = ctx.files.tools, arguments = [args], resource_set = resource_set(ctx.attr), mnemonic = ctx.attr.mnemonic if ctx.attr.mnemonic else None, @@ -95,6 +98,10 @@ _run_binary = rule( mandatory = True, cfg = "exec", ), + "tools": attr.label_list( + allow_files = True, + cfg = "exec", + ), "env": attr.string_dict(), "srcs": attr.label_list( allow_files = True, @@ -122,6 +129,7 @@ def run_binary( execution_requirements = None, use_default_shell_env = False, stamp = 0, + tools = [], **kwargs): """Runs a binary as a build action. @@ -215,12 +223,19 @@ def run_binary( These files can be read and parsed by the action, for example to pass some values to a linker. + tools: A list of tool dependencies for this action. + + These are treated similarly to srcs, except that they are built in + the exec configuration. Any code that will be executed as part of + the action should generally be listed here instead of in srcs. + **kwargs: Additional arguments """ _run_binary( name = name, tool = tool, srcs = srcs, + tools = tools, args = args, env = env, outs = outs, diff --git a/lib/tests/run_binary/BUILD.bazel b/lib/tests/run_binary/BUILD.bazel index 60fca4c5f..309a5983f 100644 --- a/lib/tests/run_binary/BUILD.bazel +++ b/lib/tests/run_binary/BUILD.bazel @@ -2,10 +2,12 @@ load("@bazel_skylib//rules:write_file.bzl", "write_file") load("@rules_shell//shell:sh_binary.bzl", "sh_binary") +load("@rules_shell//shell:sh_test.bzl", "sh_test") load("//lib:copy_to_directory.bzl", "copy_to_directory") load("//lib:diff_test.bzl", "diff_test") load("//lib:run_binary.bzl", "run_binary") load("//lib:testing.bzl", "assert_contains") +load("//lib:transitions.bzl", "platform_transition_filegroup") write_file( name = "make_dir_a_sh", @@ -80,3 +82,216 @@ assert_contains( actual = ":dir_a_rootpath", expected = "lib/tests/run_binary/dir_a_out", ) + +# Verify that items in `tools` are built in the exec configuration. +# In Bazel, exec-configured outputs live under a bazel-out subdirectory +# containing "-exec-", which distinguishes them from target-configured outputs. +write_file( + name = "gen_cfg_probe", + out = "cfg_probe.txt", + content = ["probe"], +) + +write_file( + name = "make_echo_arg_sh", + out = "echo_arg.sh", + content = [ + "#!/usr/bin/env bash", + 'echo "$1" > "$2"', + ], +) + +sh_binary( + name = "echo_arg", + srcs = [":echo_arg.sh"], +) + +run_binary( + name = "tools_cfg_location", + outs = ["tools_cfg_location.txt"], + args = [ + "$(execpath :cfg_probe.txt)", + "$@", + ], + tool = ":echo_arg", + tools = [":cfg_probe.txt"], +) + +assert_contains( + name = "tools_exec_cfg_test", + actual = ":tools_cfg_location.txt", + expected = "-exec-", +) + +# Below we set up a run_binary() target that writes the name of the execution +# platform OS to a file. Since we provide the OS name via the tools parameter, +# it should always be associated with the execution platform rather than the +# target platform. +# +# We do not know what the execution platform is going to be, since that is up +# to whoever is running the test. However, we can assert two things to +# establish some confidence that we are correctly making a distinction between +# the execution and target platforms: +# 1. Regardless of what platform we are targeting, the detected execution +# platform should remain the same. +# 2. The file paths produced for different target platforms should be different. +write_file( + name = "gen_os_name", + out = "os_name.txt", + content = select({ + "@platforms//os:linux": ["linux"], + "@platforms//os:macos": ["macos"], + "@platforms//os:windows": ["windows"], + "//conditions:default": ["other"], + }), + tags = ["manual"], +) + +write_file( + name = "make_cat_file_sh", + out = "cat_file.sh", + content = [ + "#!/usr/bin/env bash", + 'cat "$1" > "$2"', + ], +) + +sh_binary( + name = "cat_file", + srcs = [":cat_file.sh"], +) + +run_binary( + name = "gen_exec_os", + outs = ["exec_os.txt"], + args = [ + "$(execpath :os_name.txt)", + "$@", + ], + tags = ["manual"], + tool = ":cat_file", + tools = [":os_name.txt"], +) + +platform( + name = "linux", + constraint_values = [ + "@platforms//os:linux", + ], +) + +platform( + name = "macos", + constraint_values = [ + "@platforms//os:macos", + ], +) + +platform( + name = "windows", + constraint_values = [ + "@platforms//os:windows", + ], +) + +platform_transition_filegroup( + name = "exec_os_linux", + srcs = [":gen_exec_os"], + target_platform = ":linux", +) + +platform_transition_filegroup( + name = "exec_os_macos", + srcs = [":gen_exec_os"], + target_platform = ":macos", +) + +genrule( + name = "gen_platform_test", + srcs = [ + ":exec_os_linux", + ":exec_os_macos", + ], + outs = ["platform_test.sh"], + # The genrule runs in the execroot so execpaths resolve directly. It reads + # the two files and bakes their paths and contents into the test script as + # shell assignments; the assertions then run at test time via sh_test. + cmd = """ + exec_os_linux_path=$(execpath :exec_os_linux) + exec_os_macos_path=$(execpath :exec_os_macos) + { + echo '#!/usr/bin/env sh' + echo 'set -eu' + echo "linux_path='$$exec_os_linux_path'" + echo "macos_path='$$exec_os_macos_path'" + echo "linux_os='$$(cat $$exec_os_linux_path)'" + echo "macos_os='$$(cat $$exec_os_macos_path)'" + echo 'if [ "$$linux_path" = "$$macos_path" ]; then' + echo ' printf "ERROR: expected different file paths for linux and macos targets\\n" >&2' + echo ' exit 1' + echo 'fi' + echo 'if [ "$$linux_os" != "$$macos_os" ]; then' + echo ' printf "ERROR: exec OS should be the same regardless of target platform\\n" >&2' + echo ' printf " linux target produced: $$linux_os\\n" >&2' + echo ' printf " macos target produced: $$macos_os\\n" >&2' + echo ' exit 1' + echo 'fi' + } > $@ + """, +) + +sh_test( + name = "platform_test", + srcs = [":platform_test.sh"], +) + +# This target copies the OS name to another file, but this time takes in the +# name via the srcs parameter. We test below that the resulting output matches +# the platform we are targeting. +run_binary( + name = "gen_target_os", + srcs = [":os_name.txt"], + outs = ["target_os.txt"], + args = [ + "$(execpath :os_name.txt)", + "$@", + ], + tags = ["manual"], + tool = ":cat_file", +) + +platform_transition_filegroup( + name = "target_os_linux", + srcs = [":gen_target_os"], + target_platform = ":linux", +) + +platform_transition_filegroup( + name = "target_os_macos", + srcs = [":gen_target_os"], + target_platform = ":macos", +) + +platform_transition_filegroup( + name = "target_os_windows", + srcs = [":gen_target_os"], + target_platform = ":windows", +) + +assert_contains( + name = "linux_target_platform_test", + actual = ":target_os_linux", + expected = "linux", +) + +assert_contains( + name = "macos_target_platform_test", + actual = ":target_os_macos", + expected = "macos", +) + +assert_contains( + name = "windows_target_platform_test", + actual = ":target_os_windows", + expected = "windows", +) From 4ddcc88369a47cea173b5080ac9cf5746ca6e449 Mon Sep 17 00:00:00 2001 From: Adam Cozzette Date: Tue, 28 Apr 2026 15:17:21 -0700 Subject: [PATCH 2/2] Fixes --- lib/tests/run_binary/BUILD.bazel | 41 +++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/lib/tests/run_binary/BUILD.bazel b/lib/tests/run_binary/BUILD.bazel index 309a5983f..99e25f390 100644 --- a/lib/tests/run_binary/BUILD.bazel +++ b/lib/tests/run_binary/BUILD.bazel @@ -84,8 +84,8 @@ assert_contains( ) # Verify that items in `tools` are built in the exec configuration. -# In Bazel, exec-configured outputs live under a bazel-out subdirectory -# containing "-exec-", which distinguishes them from target-configured outputs. +# To do this we check that run_binary agrees with genrule on the location of a +# tool dependency. write_file( name = "gen_cfg_probe", out = "cfg_probe.txt", @@ -117,10 +117,17 @@ run_binary( tools = [":cfg_probe.txt"], ) -assert_contains( +genrule( + name = "expected_tools_cfg_location", + outs = ["expected_tools_cfg_location.txt"], + cmd = "echo $(execpath :cfg_probe.txt) > $@", + tools = [":cfg_probe.txt"], +) + +diff_test( name = "tools_exec_cfg_test", - actual = ":tools_cfg_location.txt", - expected = "-exec-", + file1 = ":tools_cfg_location.txt", + file2 = ":expected_tools_cfg_location.txt", ) # Below we set up a run_binary() target that writes the name of the execution @@ -206,34 +213,46 @@ platform_transition_filegroup( target_platform = ":macos", ) +platform_transition_filegroup( + name = "exec_os_windows", + srcs = [":gen_exec_os"], + target_platform = ":windows", +) + genrule( name = "gen_platform_test", srcs = [ ":exec_os_linux", ":exec_os_macos", + ":exec_os_windows", ], outs = ["platform_test.sh"], # The genrule runs in the execroot so execpaths resolve directly. It reads - # the two files and bakes their paths and contents into the test script as - # shell assignments; the assertions then run at test time via sh_test. + # the three files and bakes their paths and contents into the test script + # as shell assignments; the assertions then run at test time via sh_test. cmd = """ exec_os_linux_path=$(execpath :exec_os_linux) exec_os_macos_path=$(execpath :exec_os_macos) + exec_os_windows_path=$(execpath :exec_os_windows) { echo '#!/usr/bin/env sh' - echo 'set -eu' + echo 'set -o errexit -o nounset' echo "linux_path='$$exec_os_linux_path'" echo "macos_path='$$exec_os_macos_path'" + echo "windows_path='$$exec_os_windows_path'" echo "linux_os='$$(cat $$exec_os_linux_path)'" echo "macos_os='$$(cat $$exec_os_macos_path)'" - echo 'if [ "$$linux_path" = "$$macos_path" ]; then' - echo ' printf "ERROR: expected different file paths for linux and macos targets\\n" >&2' + echo "windows_os='$$(cat $$exec_os_windows_path)'" + echo 'n=$$(printf "%s\n" "$$linux_path" "$$macos_path" "$$windows_path" | sort -u | wc -l)' + echo 'if [ "$$n" -ne 3 ]; then' + echo ' printf "ERROR: expected different file paths for linux, macos, and windows targets\\n" >&2' echo ' exit 1' echo 'fi' - echo 'if [ "$$linux_os" != "$$macos_os" ]; then' + echo 'if [ "$$linux_os" != "$$macos_os" ] || [ "$$linux_os" != "$$windows_os" ]; then' echo ' printf "ERROR: exec OS should be the same regardless of target platform\\n" >&2' echo ' printf " linux target produced: $$linux_os\\n" >&2' echo ' printf " macos target produced: $$macos_os\\n" >&2' + echo ' printf " windows target produced: $$windows_os\\n" >&2' echo ' exit 1' echo 'fi' } > $@