Avoid double building cargo_build_script.data targets (#2826)

Before this change targets passed to `cargo_build_script.data` would get
built multiple times (once per configuration)
```
% bazel cquery 'filter(//test/cargo_build_script/location_expansion:target_data.txt, deps(//test/cargo_build_script/location_expansion:build_rs))'
//test/cargo_build_script/location_expansion:target_data.txt (be33641)
//test/cargo_build_script/location_expansion:target_data.txt (a1c326f)
```
We can see that this is the exec (`a1c326f`) and target (`be33641`)
configurations.
```
% bazel config a1c326f be33641
INFO: Displaying diff between configs a1c326f and be33641
Displaying diff between configs a1c326f and be33641
FragmentOptions com.google.devtools.build.lib.analysis.PlatformOptions {
  platforms: [@@internal_platforms_do_not_use//host:host], [@@bazel_tools//tools:host_platform]
}
FragmentOptions com.google.devtools.build.lib.analysis.config.CoreOptions {
  compilation_mode: opt, fastbuild
  is exec configuration: true, false
  platform_suffix: exec, null
}
FragmentOptions com.google.devtools.build.lib.rules.android.AndroidConfiguration$Options {
  fat_apk_cpu: [], [armeabi-v7a]
}
FragmentOptions com.google.devtools.build.lib.rules.cpp.CppOptions {
  copt: [-g0], []
  cxxopt: [-g0], []
  strip: always, sometimes
}
FragmentOptions com.google.devtools.build.lib.rules.java.JavaOptions {
  java_runtime_version: remotejdk_11, local_jdk
  jvmopt: [-XX:ErrorFile=/dev/stderr], []
}
```

This is caused by `data` being passed to the underlying rust binary in
`cargo_build_script_wrapper.bzl`.

The fix for this ends up requiring some gymnastics as many existing uses
of `cargo_build_script` rely on `data` files being available within the
runfiles of `cargo_build_script.script`. This change accounts for that
by creating a wrapper rule for the build script that symlinks the
`cfg=exec` Rust binary (which is the actual [Cargo build
script](https://doc.rust-lang.org/cargo/reference/build-scripts.html))
and updating the `cargo_build_script` rule to consume it as
`cfg=target`. Within the `cargo_build_script` rule this means the script
target and it's `data` attribute files will be in `cfg=target` but the
binary itself will have been built for `cfg=exec` and will be runnable
on the exec platform.

We see after this change that data targets only appear once now
```
% bazel cquery 'filter(//test/cargo_build_script/location_expansion:target_data.txt, deps(//test/cargo_build_script/location_expansion:build_rs))'
//test/cargo_build_script/location_expansion:target_data.txt (be33641)
```

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],
@@ -405,9 +496,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.",

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,21 @@ 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")
+    script_kwargs = {}
+    for arg in ("exec_compatible_with", "testonly"):
+        if arg in kwargs:
+            script_kwargs[arg] = kwargs[arg]
 
+    wrapper_kwargs = dict(script_kwargs)
+    for arg in ("compatible_with", "target_compatible_with"):
+        if arg in kwargs:
+            wrapper_kwargs[arg] = kwargs[arg]
+
+    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,26 +166,41 @@ 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,
         rustc_flags = rustc_flags,
         edition = edition,
         tags = binary_tags,
         aliases = aliases,
+        **script_kwargs
     )
+
+    # 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,
+        **wrapper_kwargs
+    )
+
+    # 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,

test/cargo_build_script/symlink_exec_root/test_exec_root_access.build.rs

@@ -32,10 +32,29 @@ fn main() {
         .collect::<HashSet<_>>();
 
     assert!(
-        root_files.take(build_script_name).is_some(),
+        root_files.remove(build_script_name),
         "Build script must be in the current directory"
     );
 
+    // An implementation detail of `cargo_build_script` is that is has two
+    // intermediate targets which represent the script runner. The script
+    // itself is suffixed with `_` while it's wrapper is suffixed with `-`.
+    // ensure the script is removed from consideration before continuing the
+    // test.
+    let alt_script_name = if cfg!(windows) {
+        format!(
+            "{}_.exe",
+            &build_script_name[0..(build_script_name.len() - ".exe".len() - 1)],
+        )
+    } else {
+        format!("{}_", &build_script_name[0..(build_script_name.len() - 1)],)
+    };
+    assert!(
+        root_files.remove(&alt_script_name),
+        "Failed to remove {}",
+        alt_script_name
+    );
+
     let cargo_manifest_dir_file = root_files.take("cargo_manifest_dir_file.txt");
     assert!(
         cargo_manifest_dir_file.is_some(),