From 1df4f85888c828e3d21ae145e6d48c1306ee33a2 Mon Sep 17 00:00:00 2001 From: UebelAndre Date: Sun, 1 Sep 2024 07:20:11 -0700 Subject: [PATCH] Avoid double building `cargo_build_script.data` targets --- cargo/private/cargo_build_script.bzl | 115 +++++++++++++++++-- cargo/private/cargo_build_script_wrapper.bzl | 29 +++-- 2 files changed, 125 insertions(+), 19 deletions(-) diff --git a/cargo/private/cargo_build_script.bzl b/cargo/private/cargo_build_script.bzl index 244c9a70c3..25d02c7eee 100644 --- a/cargo/private/cargo_build_script.bzl +++ b/cargo/private/cargo_build_script.bzl @@ -27,6 +27,78 @@ load( # Reexport for cargo_build_script_wrapper.bzl name_to_crate_name = _name_to_crate_name +CargoBuildScriptRunfilesInfo = provider( + doc = "Info about a `cargo_build_script.script` target.", + fields = { + "data": "List[Target]: The raw `cargo_build_script_runfiles.data` attribute.", + "tools": "List[Target]: The raw `cargo_build_script_runfiles.tools` attribute.", + }, +) + +def _cargo_build_script_runfiles_impl(ctx): + script = ctx.executable.script + + is_windows = script.extension == "exe" + exe = ctx.actions.declare_file("{}{}".format(ctx.label.name, ".exe" if is_windows else "")) + ctx.actions.symlink( + output = exe, + target_file = script, + is_executable = True, + ) + + # Tools are ommitted here because they should be within the `script` + # attribute's runfiles. + runfiles = ctx.runfiles(files = ctx.files.data) + + return [ + DefaultInfo( + files = depset([exe]), + runfiles = runfiles.merge(ctx.attr.script[DefaultInfo].default_runfiles), + executable = exe, + ), + CargoBuildScriptRunfilesInfo( + data = ctx.attr.data, + tools = ctx.attr.tools, + ), + ] + +cargo_build_script_runfiles = rule( + doc = """\ +A rule for producing `cargo_build_script.script` with proper runfiles. + +This rule ensure's the executable for `cargo_build_script` has properly formed runfiles with `cfg=target` and +`cfg=exec` files. This is a challenge becuase had the script binary been directly consumed, it would have been +in either configuration which would have been incorrect for either the `tools` (exec) or `data` (target) attributes. +This is solved here by consuming the script as exec and creating a symlink to consumers of this rule can consume +with `cfg=target` and still get an exec compatible binary. + +This rule may not be necessary if it becomes possible to construct runfiles trees within a rule for an action as +we'd be able to build the correct runfiles tree and configure the script runner to run the script in the new runfiles +directory: +https://github.com/bazelbuild/bazel/issues/15486 +""", + implementation = _cargo_build_script_runfiles_impl, + attrs = { + "data": attr.label_list( + doc = "Data required by the build script.", + allow_files = True, + ), + "script": attr.label( + doc = "The binary script to run, generally a `rust_binary` target.", + executable = True, + mandatory = True, + providers = [rust_common.crate_info], + cfg = "exec", + ), + "tools": attr.label_list( + doc = "Tools required by the build script.", + allow_files = True, + cfg = "exec", + ), + }, + executable = True, +) + def get_cc_compile_args_and_env(cc_toolchain, feature_configuration): """Gather cc environment variables from the given `cc_toolchain` @@ -259,20 +331,33 @@ def _cargo_build_script_impl(ctx): variables = getattr(target[platform_common.TemplateVariableInfo], "variables", depset([])) env.update(variables) + script_info = ctx.attr.script[CargoBuildScriptRunfilesInfo] + _merge_env_dict(env, expand_dict_value_locations( ctx, ctx.attr.build_script_env, getattr(ctx.attr, "data", []) + getattr(ctx.attr, "compile_data", []) + - getattr(ctx.attr, "tools", []), + getattr(ctx.attr, "tools", []) + + script_info.data + + script_info.tools, )) + script_tools = [] + script_data = [] + for target in script_info.data: + script_data.append(target[DefaultInfo].files) + script_data.append(target[DefaultInfo].default_runfiles.files) + for target in script_info.tools: + script_tools.append(target[DefaultInfo].files) + script_tools.append(target[DefaultInfo].default_runfiles.files) + tools = depset( direct = [ script, ctx.executable._cargo_build_script_runner, - ] + ctx.files.data + ctx.files.tools + ([toolchain.target_json] if toolchain.target_json else []), - transitive = toolchain_tools, + ] + ([toolchain.target_json] if toolchain.target_json else []), + transitive = script_data + script_tools + toolchain_tools, ) # dep_env_file contains additional environment variables coming from @@ -315,14 +400,24 @@ def _cargo_build_script_impl(ctx): ctx.actions.run( executable = ctx.executable._cargo_build_script_runner, arguments = [args], - outputs = [out_dir, env_out, flags_out, link_flags, link_search_paths, dep_env_out, streams.stdout, streams.stderr], + outputs = [ + out_dir, + env_out, + flags_out, + link_flags, + link_search_paths, + dep_env_out, + streams.stdout, + streams.stderr, + ], tools = tools, inputs = build_script_inputs, mnemonic = "CargoBuildScriptRun", progress_message = "Running Cargo build script {}".format(pkg_name), env = env, toolchain = None, - # Set use_default_shell_env so that $PATH is set, as tools like cmake may want to probe $PATH for helper tools. + # Set use_default_shell_env so that $PATH is set, as tools like Cmake + # may want to probe $PATH for helper tools. use_default_shell_env = True, ) @@ -338,7 +433,7 @@ def _cargo_build_script_impl(ctx): flags = flags_out, linker_flags = link_flags, link_search_paths = link_search_paths, - compile_data = depset([]), + compile_data = depset(), ), OutputGroupInfo( streams = depset([streams.stdout, streams.stderr]), @@ -359,10 +454,6 @@ cargo_build_script = rule( "crate_features": attr.string_list( doc = "The list of rust features that the build script should consider activated.", ), - "data": attr.label_list( - doc = "Data required by the build script.", - allow_files = True, - ), "deps": attr.label_list( doc = "The Rust build-dependencies of the crate", providers = [rust_common.dep_info], @@ -407,9 +498,9 @@ cargo_build_script = rule( "script": attr.label( doc = "The binary script to run, generally a `rust_binary` target.", executable = True, - allow_files = True, mandatory = True, - cfg = "exec", + cfg = "target", + providers = [CargoBuildScriptRunfilesInfo], ), "tools": attr.label_list( doc = "Tools required by the build script.", diff --git a/cargo/private/cargo_build_script_wrapper.bzl b/cargo/private/cargo_build_script_wrapper.bzl index 1860e6127c..c7f5131e57 100644 --- a/cargo/private/cargo_build_script_wrapper.bzl +++ b/cargo/private/cargo_build_script_wrapper.bzl @@ -2,6 +2,7 @@ load( "//cargo/private:cargo_build_script.bzl", + "cargo_build_script_runfiles", "name_to_crate_name", "name_to_pkg_name", _build_script_run = "cargo_build_script", @@ -142,10 +143,11 @@ def cargo_build_script( if "CARGO_CRATE_NAME" not in rustc_env: rustc_env["CARGO_CRATE_NAME"] = name_to_crate_name(name_to_pkg_name(name)) - binary_tags = [tag for tag in tags or []] - if "manual" not in binary_tags: - binary_tags.append("manual") + binary_tags = depset( + (tags if tags else []) + ["manual"], + ).to_list() + # This target exists as the actual build script. rust_binary( name = name + "_", crate_name = crate_name, @@ -154,7 +156,7 @@ def cargo_build_script( crate_features = crate_features, deps = deps, proc_macro_deps = proc_macro_deps, - data = data, + data = tools, compile_data = compile_data, rustc_env = rustc_env, rustc_env_files = rustc_env_files, @@ -163,17 +165,30 @@ def cargo_build_script( tags = binary_tags, aliases = aliases, ) + + # Because the build script is expected to be run on the exec host, the + # script above needs to be in the exec configuration but the script may + # need data files that are in the target configuration. This rule wraps + # the script above so the `cfg=exec` target can be run without issue in + # a `cfg=target` environment. More details can be found on the rule. + cargo_build_script_runfiles( + name = name + "-", + script = ":{}_".format(name), + data = data, + tools = tools, + tags = binary_tags, + ) + + # This target executes the build script. _build_script_run( name = name, - script = ":{}_".format(name), + script = ":{}-".format(name), crate_features = crate_features, version = version, build_script_env = build_script_env, links = links, deps = deps, link_deps = link_deps, - data = data, - tools = tools, rundir = rundir, rustc_flags = rustc_flags, visibility = visibility,