Add out_dir support in cargo_dep_env

[bsilver8192]

This is handy for things like passing a PROTOC value to prost-build.

Also fix the other tests for cargo_dep_env, looks like I forgot to actually run them...

[illicitonion]

LGTM, thanks! Looks like you need to fix up one buildifier issue, then we can merge!

[UebelAndre]

Just a few questions from me 😅

[UebelAndre]

Why is this a separate BuildInfo provider? Is there a reason you couldn't just conditionally assign empty_dir or ctx.file.out_dir?

[bsilver8192]

Not sure which piece you're asking about, here's answers to two questions I can think of:

1. It seemed wasteful to create a BuildInfo even if there's no out_dir to manage.
2. A BuildInfo in DepInfo.transitive_build_infos is handled very differently than the main BuildInfo being returned, so they have to be separate. The former is is for build.rs scripts that depend on it transitively, and the latter is for direct dependencies of any kind.

[UebelAndre]

More directly, why does there need to be a new BuildInfo here vs updating the one created here:
https://github.com/bazelbuild/rules_rust/blob/0.10.0/cargo/cargo_build_script.bzl#L455-L462

[bsilver8192]

That's my #2: that BuildInfo is used in separate contexts from this new one, and I don't think cargo_dep_env should contribute settings there. Specifically, I don't see where dep_env in that BuildInfo is ever used, from what I see it just gets rolled up the dependency tree but nothing ever uses it. The out_dir from that BuildInfo does get used though, which I think would be pretty confusing to expose.

I think the out_dir from the main BuildInfo is intended for use in conjunction with rustc_env, which cargo_dep_env currently does not have the ability to set. I do not think adding that is necessary, because it's easy enough to write a cargo_build_script that passes values through. I'm not sure whether adding it would make sense, it gets kind of complicated to expose the dual meanings of out_dir, but at the same time I've got a cargo_build_script that just copies files from dep_env values to its out_dir and then sets rustc_env settings to point to them.

[UebelAndre]

Sorry for the delay here. I keep reading this and still don't think I've fully grokked the need for the transitive provider. The last two things I think I'll ask are:

1. Can you show me an example of something failing with out_dir being placed in the top level provider? Or is this more of a logical/correctness thing?
2. Can you add a code comment explaining the transitive nature here?

[bsilver8192]

1. Nothing is technically wrong, it just does something different. For example, my newly-added //test/dep_env:build_read_dep_dir fails because it can't find the file after changing that.

   The place this second (direct) BuildInfo would be useful is if a cargo_dep_env was directly depended on by a rust_library and set rustc_env.

2. Done

[UebelAndre]

More directly, why does there need to be a new BuildInfo here vs updating the one created here:
https://github.com/bazelbuild/rules_rust/blob/0.10.0/cargo/cargo_build_script.bzl#L455-L462

[bsilver8192]

Also, for context (and anybody else looking at the same problem), I'm using this for prost-build:

load("@rules_rust//cargo:cargo_build_script.bzl", "cargo_build_script", "cargo_dep_env")

genrule(
    name = "gen_set_protoc_env",
    srcs = [":build_script_out_dir"],
    outs = ["set_protoc_env.env"],
    cmd = 'echo "PROTOC=\\$${pwd}/$(execpath :build_script_out_dir)/protoc\nPROTOC_INCLUDE=\\$${pwd}/$(execpath :build_script_out_dir)/include" > $@',
)

cargo_dep_env(
    name = "set_protoc_env",
    src = "set_protoc_env.env",
    out_dir = ":build_script_out_dir",
    visibility = ["//visibility:public"],
)

filegroup(
    name = "build_script_out_dir",
    srcs = [":prost_build_build_script"],
    output_group = "out_dir",
)

cargo_build_script(
    name = "prost_build_build_script",
    srcs = [
        "@//third_party/cargo/prost:simple_prost_build.rs",
    ],
    build_script_env = {
        "PROTOC_PATH": "$(execpath @com_github_protocolbuffers_protobuf//:protoc)",
        "WELL_KNOWN_PATHS": "$(execpaths @com_github_protocolbuffers_protobuf//:well_known_type_protos)",
        "BUILT_IN_PATHS": "$(execpaths @com_github_protocolbuffers_protobuf//:built_in_runtime_protos)",
    },
    tools = [
        "@com_github_protocolbuffers_protobuf//:built_in_runtime_protos",
        "@com_github_protocolbuffers_protobuf//:protoc",
        "@com_github_protocolbuffers_protobuf//:well_known_type_protos",
    ],
)

with this in the .rs file:

//! A simple build script that copies inputs into OUT_DIR, and sets environment variables
//! for dependencies pointing there. Everything in the out_dir will be available when
//! compiling dependent crates, so we can point them to the files there.

use std::{env, path::{Path, PathBuf}, fs};

fn main() {
    let protoc_src = env::var("PROTOC_PATH").expect("PROTOC_PATH must be set");
    let well_known_srcs = env::var("WELL_KNOWN_PATHS").expect("WELL_KNOWN_PATHS must be set");
    let built_in_srcs = env::var("BUILT_IN_PATHS").expect("BUILT_IN_PATHS must be set");

    let out_dir: PathBuf = env::var("OUT_DIR").expect("OUT_DIR must be set").into();
    let protoc_dst = out_dir.join("protoc");
    let include_dir = out_dir.join("include");
    let well_known_dst = include_dir.join("google").join("protobuf");

    fs::copy(&protoc_src, &protoc_dst).unwrap_or_else(|e| panic!("Failed to copy protoc from {:?} to {:?}: {:?}", &protoc_src, &protoc_dst, e));
    fs::create_dir_all(&well_known_dst).expect("Failed to create well_known destination directory");
    for path in well_known_srcs.split(' ').chain(built_in_srcs.split(' ')) {
        let path = Path::new(&path);
        let dst = well_known_dst.join(path.file_name().unwrap());
        fs::copy(&path, &dst).unwrap_or_else(|e| panic!("Failed to copy a well_known protobuf from {:?} to {:?}: {:?}", &path, &dst, e));
    }

    println!("cargo:rustc-env=PROTOC={}", protoc_dst.to_string_lossy());
    println!("cargo:rustc-env=PROTOC_INCLUDE={}", include_dir.to_string_lossy());
}

I'm definitely abusing the fact that the @com_github_protocolbuffers_protobuf targets are a host tool and source files which are the same for any platform, this approach would not work if there were any files that needed to be built for the target.

[UebelAndre]

Sorry for the delay here. I keep reading this and still don't think I've fully grokked the need for the transitive provider. The last two things I think I'll ask are:

1. Can you show me an example of something failing with out_dir being placed in the top level provider? Or is this more of a logical/correctness thing?
2. Can you add a code comment explaining the transitive nature here?

[UebelAndre]

Thanks for being so patient with me!