Skip to content

Add out_dir support in cargo_dep_env #1571

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion cargo/cargo_build_script.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -450,8 +450,36 @@ def _cargo_dep_env_implementation(ctx):
outputs = [empty_dir],
executable = "true",
)

build_infos = []
out_dir = ctx.file.out_dir
if out_dir:
if not out_dir.is_directory:
fail("out_dir must be a directory artifact")

# BuildInfos in this list are collected up for all transitive cargo_build_script
# dependencies. This is important for any flags set in `dep_env` which reference this
# `out_dir`.
#
# TLDR: This BuildInfo propagates up build script dependencies.
build_infos.append(BuildInfo(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@bsilver8192 bsilver8192 Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@bsilver8192 bsilver8192 Oct 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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

dep_env = empty_file,
flags = empty_file,
link_flags = empty_file,
link_search_paths = empty_file,
out_dir = out_dir,
rustc_env = empty_file,
))
return [
DefaultInfo(files = depset(ctx.files.src)),
# Parts of this BuildInfo is used when building all transitive dependencies
# (cargo_build_script and otherwise), alongside the DepInfo. This is how other rules
# identify this one as a valid dependency, but we don't otherwise have a use for it.
#
# TLDR: This BuildInfo propagates up normal (non build script) depenencies.
#
# In the future, we could consider setting rustc_env here, and also propagating dep_dir
# so files in it can be referenced there.
BuildInfo(
dep_env = empty_file,
flags = empty_file,
Expand All @@ -460,11 +488,14 @@ def _cargo_dep_env_implementation(ctx):
out_dir = empty_dir,
rustc_env = empty_file,
),
# Information here is used directly by dependencies, and it is an error to have more than
# one dependency which sets this. This is the main way to specify information from build
# scripts, which is what we're looking to do.
_DepInfo(
dep_env = ctx.file.src,
direct_crates = depset(),
link_search_path_files = depset(),
transitive_build_infos = depset(),
transitive_build_infos = depset(direct = build_infos),
transitive_crate_outputs = depset(),
transitive_crates = depset(),
transitive_noncrates = depset(),
Expand All @@ -480,6 +511,16 @@ cargo_dep_env = rule(
"for build scripts which depend on this crate."
),
attrs = {
"out_dir": attr.label(
doc = dedent("""\
Folder containing additional inputs when building all direct dependencies.

This has the same effect as a `cargo_build_script` which prints
puts files into `$OUT_DIR`, but without requiring a build script.
"""),
allow_single_file = True,
mandatory = False,
),
"src": attr.label(
doc = dedent("""\
File containing additional environment variables to set for build scripts of direct dependencies.
Expand Down
42 changes: 37 additions & 5 deletions test/dep_env/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
"""Tests for passing configuration to cargo_build_script rules"""

load("//cargo:cargo_build_script.bzl", "cargo_build_script", "cargo_dep_env")
load("//rust:defs.bzl", "rust_binary", "rust_library")
load("//rust:defs.bzl", "rust_library", "rust_test")
load(":dep_env.bzl", "create_dep_dir")

cargo_build_script(
name = "set_a_build",
Expand Down Expand Up @@ -36,16 +37,47 @@ cargo_build_script(
deps = [":set_b"],
)

rust_binary(
cargo_build_script(
name = "read_dep_dir",
srcs = ["read_dep_dir.rs"],
edition = "2018",
deps = [":set_dep_dir"],
)

rust_test(
name = "build_read_a",
srcs = ["empty_main.rs"],
srcs = ["read_a.rs"],
edition = "2018",
deps = [":read_a"],
)

rust_binary(
rust_test(
name = "build_read_b",
srcs = ["empty_main.rs"],
srcs = ["read_b.rs"],
edition = "2018",
deps = [":read_b"],
)

rust_test(
name = "build_read_dep_dir",
srcs = ["read_dep_dir.rs"],
edition = "2018",
deps = [":read_dep_dir"],
)

create_dep_dir(
name = "dep_dir",
)

cargo_dep_env(
name = "set_dep_dir",
src = "set_dep_dir.env",
out_dir = "dep_dir",
)

genrule(
name = "gen_set_dep_dir",
srcs = ["dep_dir"],
outs = ["set_dep_dir.env"],
cmd = "echo DEP_DIR=\\$${pwd}/$(execpath dep_dir) > $@",
)
14 changes: 14 additions & 0 deletions test/dep_env/dep_env.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
"""Tests for passing configuration to cargo_build_script rules"""

def _create_dep_dir(ctx):
out = ctx.actions.declare_directory("dep_dir")
ctx.actions.run_shell(
outputs = [out],
arguments = [out.path],
command = 'echo contents > "$@/a_file"',
)
return [DefaultInfo(files = depset(direct = [out]))]

create_dep_dir = rule(
implementation = _create_dep_dir,
)
1 change: 0 additions & 1 deletion test/dep_env/empty_main.rs

This file was deleted.

21 changes: 21 additions & 0 deletions test/dep_env/read_dep_dir.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
use std::{env::var, fs, io::Result};

fn main() {
let dep_dir = &var("DEP_DIR").expect("DEP_DIR should be set");
let entries = fs::read_dir(dep_dir)
.expect("Failed to open DEP_DIR directory")
.collect::<Result<Vec<_>>>()
.expect("Failed to read DEP_DIR directory entries");
let entries = entries
.iter()
.map(|entry| {
entry
.path()
.file_name()
.unwrap()
.to_string_lossy()
.to_string()
})
.collect::<Vec<_>>();
assert_eq!(entries, vec!["a_file".to_string()]);
}