[rust] rs_bindings_from_cc.gni: Headers depending on another target.
This CL adds support for generating Rust bindings for C++ headers
that depend on another, separate target.
Bug: 1329611
Change-Id: I1451310b8a1c2ac3d1b89199ea5236089cd9928d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3773113
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1025850}
diff --git a/build/rust/rs_bindings_from_cc.gni b/build/rust/rs_bindings_from_cc.gni
index f98c79f..a4e052b 100644
--- a/build/rust/rs_bindings_from_cc.gni
+++ b/build/rust/rs_bindings_from_cc.gni
@@ -26,13 +26,25 @@
# Implementation note: This doesn't just take *all* the headers of the C++
# target, because typically only a *subset* of headers provides the
# *public* API that bindings are needed for.
+# TODO(crbug.com/1329611): Internal headers still need to be included in
+# the targets_and_headers metadata...
#
-# TODO(crbug.com/1297592): Support dependencies in the generated bindings.
-# Tentative plan:
-# 1) introduce `bindings_target` and use it A) in regular `deps` of
-# mixed_static_library and B) to build the crate name by appending
-# "_rs_api" suffix.
-# 2) introduce `bindings_deps` and use it when gathering GN metadata.
+# deps:
+# Other `rs_bindings_from_cc` targets that the bindings need to depend on
+# (e.g. because APIs in the `public_headers` refer to `struct`s declared in
+# those other targets. Note how in the usage example below bindings for
+# `struct Goat` are provided by `goat_rs_api`, and that therefore the
+# bindings for the `TeleportGoat` provided by `teleport_rs_api` depend on
+# `goat_rs_api`).
+#
+# Oftentimes `deps` can be a copy of the `public_deps` of the bindings
+# target, but depending on targets with the suffix "_rs_api". Still, there
+# are scenarios where `deps` don't parallel *all* entries from `public_deps`:
+# * `public_deps` that don't expose Rust APIs (i.e. there are no
+# "..._rs_api" targets to depend on).
+# * `public_deps` that Crubit bindings don't depend on (dependencies that
+# don't provide re-exportable C++ APIs, or that only provide items
+# that are ignored by Crubit - e.g. `#define`s).
#
# Usage example:
#
@@ -43,27 +55,49 @@
# rust_executable("my_target") {
# crate_root = "main.rs"
# sources = [ "main.rs" ]
-# deps = [ ":cpp_lib_rs_api" ]
+# deps = [ ":teleport_rs_api" ]
# ]
#
-# # Crubit's convention is to append "_rs_api" suffix to the target name
-# # when naming the generated Rust crate.
-# rs_bindings_from_cc("cpp_lib") {
-# public_headers = ["cpp_lib.h"]
+# # This will generate "teleport_rs_api" target that provides Rust
+# # bindings for the "teleport.h" header from the ":teleport" source
+# # set.
+# rs_bindings_from_cc("teleport") {
+# public_headers = ["teleport.h"]
+# deps = [ ":goat_rs_api" ] # Parallel's `public_deps` of ":teleport".
# }
#
-# source_set("cpp_lib") {
-# sources = [ "cpp_lib.h", ... ]
+# source_set("teleport") {
+# sources = [ "teleport.h", ... ]
+# public_deps = [ ":goat" ]
# }
#
-# cpp_lib.h:
-# void TeleportGoats(int number_of_goats);
+# rs_bindings_from_cc("goat") {
+# public_headers = ["goat.h"]
+# }
+# source_set("goat") {
+# sources = [ "goat.h", ... ]
+# }
+#
+# teleport.h:
+# #include "goat.h"
+# void TeleportGoat(const Goat& goat_to_teleport);
+#
+# goat.h:
+# struct Goat { ... };
#
# main.rs:
# fn main() {
-# cpp_lib_rs_api::TeleportGoats(42);
+# let g: goat_rs_api::Goat = ...;
+# teleport_rs_api::TeleportGoat(&g);
# }
#
+# Debugging and implementation notes:
+#
+# - Consider running the build while CRUBIT_DEBUG environment variable is set.
+# This will generate additional `.ir` file and log extra information from
+# the `run_rs_bindings_from_cc.py` script (e.g. full cmdlines for invoking
+# `rs_bindings_from_cc`).
+#
template("rs_bindings_from_cc") {
# Mandatory parameter: public_headers.
assert(defined(invoker.public_headers),
@@ -84,9 +118,20 @@
_visibility = invoker.visibility
}
- # Future parameters:
+ # Optional parameter: deps.
+ #
+ # TODO(crbug.com/1329611): Can we somehow assert that `_deps` only contains
+ # some "..._rs_api" targets crated via
+ # `mixed_static_library($_lib_target_name)` below? foreach(dep, _deps) {
+ # assert something }
+ _deps = []
+ if (defined(invoker.deps)) {
+ _deps = invoker.deps
+ }
+
+ # TODO(crbug.com/1329611): Should `_bindings_target` or `_lib_target_name` be
+ # provided explicitly?
_bindings_target = ":${target_name}"
- _bindings_deps = []
# Various names and paths that are shared across multiple targets defined
# in the template here.
@@ -103,7 +148,10 @@
# and putting it into GN's `metadata`.
group(_metadata_target_name) {
testonly = _testonly
- visibility = [ ":${_gen_metadata_target_name}" ]
+ visibility = [
+ ":${_gen_metadata_target_name}",
+ ":${_lib_target_name}",
+ ]
deps = []
metadata = {
@@ -111,7 +159,11 @@
# `--targets_and_headers` cmdline argument of `rs_bindings_from_cc`.
crubit_target_and_headers = [
{
- t = _base_target_name
+ # The `get_label_info` call below expands ":foo_rs_api" into
+ # something like "//dir/bar/baz:foo_rs_api". Crubit assumes that
+ # there is a colon + uses the after-colon-suffix as the name of the
+ # crate.
+ t = get_label_info(":${_lib_target_name}", "label_no_toolchain")
h = _rebased_public_headers
},
]
@@ -125,20 +177,18 @@
visibility = [ ":${_gen_bindings_target_name}" ]
deps = [ ":${_metadata_target_name}" ]
- deps += _bindings_deps
+ deps += _deps
testonly = _testonly
outputs = [ _metadata_path ]
output_conversion = "json"
data_keys = [ "crubit_target_and_headers" ]
- # TODO(https://crbug.com/1329611): Use `walk_keys` to limit how deep the
- # transitive dependency goes. Crubit doesn't care about all the `public_deps`
- # - for example if target1.h re-exports a function from target2.h that uses
- # only built-in C types as parameter types and return type, then Crubit doesn't
- # need to know about target2.h when generating bindings for target1.h. Crubit
- # only needs to know about target2.h if bindings for target1.h need to expose
- # types that get their bindings from target2.h.
+ # `walk_keys` are used to limit how deep the transitive dependency goes.
+ # This is important, because Crubit doesn't care about all the `deps` or
+ # `public_deps` of the `_bindings_target`. (See also the doc comment about
+ # `rs_bindings_from_cc.deps` parameter at the top of this file.)
+ walk_keys = [ "crubit_metadata_deps" ]
}
# Exposing the generated Rust bindings.
@@ -149,21 +199,32 @@
}
sources = [ _cc_out_path ]
- deps = _bindings_deps
+ deps = _deps
deps += [
":${_gen_bindings_target_name}",
+ ":${_metadata_target_name}",
"//third_party/crubit:deps_of_rs_api_impl",
_bindings_target,
]
+ # Chromium already covers `chromium/src/` and `out/Release/gen` in the
+ # include path, but we need to explicitly add `out/Release` below. This
+ # is needed, because `--public_headers` passed to Crubit use paths relative
+ # to the `out/Release` directory. See also b/239238801.
+ include_dirs = [ root_build_dir ]
+
rs_sources = [ _rs_out_path ]
rs_crate_name = _lib_target_name
rs_crate_root = _rs_out_path
- rs_deps = _bindings_deps
+ rs_deps = _deps
rs_deps += [
":${_gen_bindings_target_name}",
"//third_party/crubit:deps_of_rs_api",
]
+
+ metadata = {
+ crubit_metadata_deps = _deps + [ ":${_metadata_target_name}" ]
+ }
}
# Invoking Crubit's `rs_bindings_from_cc` tool to generate Rust bindings.
diff --git a/build/rust/run_rs_bindings_from_cc.py b/build/rust/run_rs_bindings_from_cc.py
index 24e38091..6190856 100755
--- a/build/rust/run_rs_bindings_from_cc.py
+++ b/build/rust/run_rs_bindings_from_cc.py
@@ -5,6 +5,7 @@
# found in the LICENSE file.
import argparse
+import json
import os
import subprocess
import sys
@@ -24,7 +25,7 @@
RS_BINDINGS_FROM_CC_EXE_PATH = os.path.join(RUST_TOOLCHAIN_DIR, "bin",
"rs_bindings_from_cc")
-# TODO(crbug.com/1297592): The instructions below can be moved (once
+# TODO(crbug.com/1329611): The instructions below can be moved (once
# tools/rust/package_rust.py includes Crubit binaries) into a (new)
# doc with a title like: "Developing Crubit in Chromium".
@@ -43,12 +44,21 @@
#
# 3. Run:
# $ cp \
-# third_party/crubit/bazel-bin/rs_bindings_from_cc/rs_bindings_from_cc_impl \
+# third_party/crubit/src/bazel-bin/rs_bindings_from_cc/rs_bindings_from_cc_impl \
# third_party/rust-toolchain/rs_bindings_from_cc
#
# (note that the `_impl` suffix has been dropped from the binary name).
+def format_cmdline(args):
+ def quote_arg(x):
+ if ' ' not in x: return x
+ x = x.replace('"', '\\"')
+ return f"\"{x}\""
+
+ return " ".join([quote_arg(x) for x in args])
+
+
def main():
parser = argparse.ArgumentParser()
parser.add_argument("--targets_and_headers_from_gn",
@@ -73,30 +83,44 @@
nargs=argparse.REMAINDER)
args = parser.parse_args()
- # Fixed config.
- genargs = []
+ # Start building the cmdline args to pass to the `rs_bindings_from_cc` tool.
+ genargs = [RS_BINDINGS_FROM_CC_EXE_PATH]
+
+ # Output paths
+ genargs.extend(["--rs_out", args.rs_out])
+ genargs.extend(["--cc_out", args.cc_out])
+ if "CRUBIT_DEBUG" in os.environ:
+ genargs.extend(["--ir_out", args.rs_out.replace(".rs", ".ir")])
+
+ # Public headers.
+ genargs.extend([
+ "--public_headers",
+ ",".join([os.path.relpath(hdr) for hdr in args.public_headers.split(",")])
+ ])
+
+ # Targets to headers map.
+ with open(args.targets_and_headers_from_gn, "r") as f:
+ targets_and_headers = json.load(f)
+ for entry in targets_and_headers:
+ hdrs = entry["h"]
+ for i in range(len(hdrs)):
+ hdrs[i] = os.path.relpath(hdrs[i])
+ genargs.extend(["--targets_and_headers", json.dumps(targets_and_headers)])
+
+ # All Crubit invocations in Chromium share the following cmdline args.
+ genargs.extend(["--rustfmt_exe_path", RUSTFMT_EXE_PATH])
+ genargs.extend(["--rustfmt_config_path", RUSTFMT_CONFIG_PATH])
genargs.extend([
"--crubit_support_path",
"third_party/crubit/src/rs_bindings_from_cc/support"
])
- genargs.extend(["--rustfmt_exe_path", RUSTFMT_EXE_PATH])
- genargs.extend(["--rustfmt_config_path", RUSTFMT_CONFIG_PATH])
- # Morph contents of the GN-generated JSON file into a single-line string.
- with open(args.targets_and_headers_from_gn, "r") as f:
- targets_and_headers = f.read().replace('\n', ' ').replace('\r', ' ')
- genargs.extend(["--targets_and_headers", targets_and_headers])
-
- # Other inputs and outputs.
- genargs.extend(["--public_headers", args.public_headers])
- genargs.extend(["--rs_out", args.rs_out])
- genargs.extend(["--cc_out", args.cc_out])
-
+ # Clang arguments.
+ #
# The call to `filter_clang_args` is needed to avoid the following error:
# error: unable to find plugin 'find-bad-constructs'
genargs.extend(filter_clang_args(args.clang_args))
-
- # TODO(crbug.com/1297592): This warning needs to be suppressed, because
+ # TODO(crbug.com/1329611): This warning needs to be suppressed, because
# otherwise Crubit/Clang complains as follows:
# error: .../third_party/rust-toolchain/bin/rs_bindings_from_cc:
# 'linker' input unused [-Werror,-Wunused-command-line-argument]
@@ -104,10 +128,15 @@
# then `{{cflags}}` seems perfectly reasonable...
genargs += ["-Wno-unused-command-line-argument"]
- # TODO(crbug.com/1297592): run_bindgen.py removes the outputs when the tool
+ # Print a copy&pastable final cmdline when asked for debugging help.
+ if "CRUBIT_DEBUG" in os.environ:
+ pretty_cmdline = format_cmdline(genargs)
+ print(f"CRUBIT_DEBUG: CMDLINE: {pretty_cmdline}", file=sys.stderr)
+
+ # TODO(crbug.com/1329611): run_bindgen.py removes the outputs when the tool
# fails. Maybe we need to do something similar here? OTOH in most failure
# modes Crubit will fail *before* generating its outputs...
- return subprocess.run([RS_BINDINGS_FROM_CC_EXE_PATH, *genargs]).returncode
+ return subprocess.run(genargs).returncode
if __name__ == '__main__':
diff --git a/build/rust/tests/test_rs_bindings_from_cc/BUILD.gn b/build/rust/tests/test_rs_bindings_from_cc/BUILD.gn
index e4cdc3e..527578a 100644
--- a/build/rust/tests/test_rs_bindings_from_cc/BUILD.gn
+++ b/build/rust/tests/test_rs_bindings_from_cc/BUILD.gn
@@ -8,16 +8,16 @@
rust_executable("test_rs_bindings_from_cc") {
crate_root = "main.rs"
sources = [ "main.rs" ]
- deps = [ ":self_contained_target_rs_api" ]
+ deps = [
+ ":self_contained_target_rs_api",
+ ":target_depending_on_another_rs_api",
+ "//third_party/crubit:ctor",
+ ]
build_native_rust_unit_tests = true
}
rs_bindings_from_cc("self_contained_target") {
- # TODO(crbug.com/1297592): Is there something we can do (a convention?) to
- # avoid duplication/repetition across
- # 1a) `public_headers` here and 1b) `sources` in ":self_contained_target"
- # 2) mentioning "self_contained_target" in `deps` and in the name of the
- # target (with "_rs_api" suffix added).
+ # Lists public headers from `sources` of `self_contained_target`.
public_headers = [
"self_contained_target_header1.h",
"self_contained_target_header2.h",
@@ -31,3 +31,22 @@
"self_contained_target_header2.h",
]
}
+
+rs_bindings_from_cc("target_depending_on_another") {
+ # Lists public headers from `sources` of `target_depending_on_another`.
+ #
+ # TODO(crbug.com/1297592): Is there something we can do (a convention?) to
+ # avoid this duplication/repetition?
+ public_headers = [ "target_depending_on_another.h" ]
+
+ # Parallels `public_deps` of `target_depending_on_another`
+ #
+ # TODO(crbug.com/1297592): Is there something we can do (a convention?) to
+ # avoid this duplication/repetition?
+ deps = [ ":self_contained_target_rs_api" ]
+}
+
+source_set("target_depending_on_another") {
+ sources = [ "target_depending_on_another.h" ]
+ public_deps = [ ":self_contained_target" ]
+}
diff --git a/build/rust/tests/test_rs_bindings_from_cc/main.rs b/build/rust/tests/test_rs_bindings_from_cc/main.rs
index b8f6546..6aa484b 100644
--- a/build/rust/tests/test_rs_bindings_from_cc/main.rs
+++ b/build/rust/tests/test_rs_bindings_from_cc/main.rs
@@ -21,4 +21,12 @@
let x = ::self_contained_target_rs_api::CcPodStruct { value: 123 };
assert_eq!(x.value, 123);
}
+
+ #[test]
+ fn test_target_depending_on_another() {
+ ctor::emplace! {
+ let x = ::target_depending_on_another_rs_api::CreateCcPodStructFromValue(456);
+ }
+ assert_eq!(x.value, 456);
+ }
}
diff --git a/third_party/crubit/BUILD.gn b/third_party/crubit/BUILD.gn
index 4cb4fc7..68e334b 100644
--- a/third_party/crubit/BUILD.gn
+++ b/third_party/crubit/BUILD.gn
@@ -32,7 +32,6 @@
}
rust_static_library("ctor") {
- visibility = [ ":*" ]
crate_root = "${_support_dir}/ctor.rs"
sources = [ "${_support_dir}/ctor.rs" ]
build_native_rust_unit_tests = true
@@ -51,7 +50,6 @@
}
rust_static_library("forward_declare") {
- visibility = [ ":*" ]
crate_root = "${_support_dir}/forward_declare.rs"
sources = [ "${_support_dir}/forward_declare.rs" ]
build_native_rust_unit_tests = true
@@ -70,7 +68,6 @@
}
rust_static_library("oops") {
- visibility = [ ":*" ]
crate_root = "${_support_dir}/oops.rs"
sources = [ "${_support_dir}/oops.rs" ]
build_native_rust_unit_tests = true
