From ebe445b3284ff01a80787e7e30b76287d41d370a Mon Sep 17 00:00:00 2001 From: Brian Silverman Date: Thu, 29 Sep 2022 18:00:42 -0700 Subject: [PATCH 1/4] Add `out_dir` support in `cargo_dep_env` 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... --- cargo/cargo_build_script.bzl | 23 +++++++++++++++++++- test/dep_env/BUILD.bazel | 42 +++++++++++++++++++++++++++++++----- test/dep_env/dep_env.bzl | 12 +++++++++++ test/dep_env/empty_main.rs | 1 - test/dep_env/read_dep_dir.rs | 21 ++++++++++++++++++ 5 files changed, 92 insertions(+), 7 deletions(-) create mode 100644 test/dep_env/dep_env.bzl delete mode 100644 test/dep_env/empty_main.rs create mode 100644 test/dep_env/read_dep_dir.rs diff --git a/cargo/cargo_build_script.bzl b/cargo/cargo_build_script.bzl index 87ee88e513..1ce40c0114 100644 --- a/cargo/cargo_build_script.bzl +++ b/cargo/cargo_build_script.bzl @@ -450,6 +450,17 @@ def _cargo_dep_env_implementation(ctx): outputs = [empty_dir], executable = "true", ) + + build_infos = [] + if ctx.file.out_dir: + build_infos.append(BuildInfo( + dep_env = empty_file, + flags = empty_file, + link_flags = empty_file, + link_search_paths = empty_file, + out_dir = ctx.file.out_dir, + rustc_env = empty_file, + )) return [ DefaultInfo(files = depset(ctx.files.src)), BuildInfo( @@ -464,7 +475,7 @@ def _cargo_dep_env_implementation(ctx): 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(), @@ -480,6 +491,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. diff --git a/test/dep_env/BUILD.bazel b/test/dep_env/BUILD.bazel index 64addcded1..0296917ba3 100644 --- a/test/dep_env/BUILD.bazel +++ b/test/dep_env/BUILD.bazel @@ -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", @@ -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) > $@", +) diff --git a/test/dep_env/dep_env.bzl b/test/dep_env/dep_env.bzl new file mode 100644 index 0000000000..8ab953726b --- /dev/null +++ b/test/dep_env/dep_env.bzl @@ -0,0 +1,12 @@ +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, +) diff --git a/test/dep_env/empty_main.rs b/test/dep_env/empty_main.rs deleted file mode 100644 index f328e4d9d0..0000000000 --- a/test/dep_env/empty_main.rs +++ /dev/null @@ -1 +0,0 @@ -fn main() {} diff --git a/test/dep_env/read_dep_dir.rs b/test/dep_env/read_dep_dir.rs new file mode 100644 index 0000000000..c9a82edef1 --- /dev/null +++ b/test/dep_env/read_dep_dir.rs @@ -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::>>() + .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::>(); + assert_eq!(entries, vec!["a_file".to_string()]); +} From 7ded48d5da784c79f2f7c4ab42fcfe9e186aadc4 Mon Sep 17 00:00:00 2001 From: Brian Silverman Date: Fri, 30 Sep 2022 12:05:03 -0700 Subject: [PATCH 2/4] Fix buildifier and add directory checking --- cargo/cargo_build_script.bzl | 7 +++++-- test/dep_env/dep_env.bzl | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/cargo/cargo_build_script.bzl b/cargo/cargo_build_script.bzl index 1ce40c0114..3896791502 100644 --- a/cargo/cargo_build_script.bzl +++ b/cargo/cargo_build_script.bzl @@ -452,13 +452,16 @@ def _cargo_dep_env_implementation(ctx): ) build_infos = [] - if ctx.file.out_dir: + out_dir = ctx.file.out_dir + if out_dir: + if not out_dir.is_directory: + fail("out_dir must be a directory artifact") build_infos.append(BuildInfo( dep_env = empty_file, flags = empty_file, link_flags = empty_file, link_search_paths = empty_file, - out_dir = ctx.file.out_dir, + out_dir = out_dir, rustc_env = empty_file, )) return [ diff --git a/test/dep_env/dep_env.bzl b/test/dep_env/dep_env.bzl index 8ab953726b..f571fa6266 100644 --- a/test/dep_env/dep_env.bzl +++ b/test/dep_env/dep_env.bzl @@ -1,3 +1,5 @@ +"""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( From 627ebefc69a8d3b8ebb0c4e54752e4942c4313e8 Mon Sep 17 00:00:00 2001 From: Brian Silverman Date: Mon, 3 Oct 2022 11:27:28 -0700 Subject: [PATCH 3/4] Add some comments --- cargo/cargo_build_script.bzl | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/cargo/cargo_build_script.bzl b/cargo/cargo_build_script.bzl index 3896791502..f6a25c1f47 100644 --- a/cargo/cargo_build_script.bzl +++ b/cargo/cargo_build_script.bzl @@ -456,6 +456,11 @@ def _cargo_dep_env_implementation(ctx): 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( dep_env = empty_file, flags = empty_file, @@ -466,6 +471,14 @@ def _cargo_dep_env_implementation(ctx): )) 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, @@ -474,6 +487,9 @@ 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(), From 595572e2f1ea1db233c46c9f3471051540a4714a Mon Sep 17 00:00:00 2001 From: Brian Silverman Date: Tue, 4 Oct 2022 16:33:55 -0700 Subject: [PATCH 4/4] Fix buildifier --- cargo/cargo_build_script.bzl | 1 + 1 file changed, 1 insertion(+) diff --git a/cargo/cargo_build_script.bzl b/cargo/cargo_build_script.bzl index f6a25c1f47..2539b5534e 100644 --- a/cargo/cargo_build_script.bzl +++ b/cargo/cargo_build_script.bzl @@ -456,6 +456,7 @@ def _cargo_dep_env_implementation(ctx): 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`.