Rename `--bindings-from-dependency` to `--crate-header`, with an alias to keep old calls working.
The problem with the old name is that it doesn't generalize: it's specifying the bindings that were generated from a given dependency, but we also want to have per-crate arguments that _aren't_ generated "from" them, and that aren't only for dependencies. In particular, we'll want a per-crate crubit feature flag argument, where one of the crates is the current crate. `feature-from-dependency` is consistent with the existing naming scheme, but misleading, I feel.
So the choices were:
1. introduce a _new_ naming scheme for the per-crate feature flags, such as `crate-feature`, which is inconsistent with the per-crate header flag.
2. be consistent (`feature-from-dependency`), but misleading.
3. be consistent (`feature-from-dependency`), and make it not-as-misleading by using a different flag for the current non-dependency crate.
One last thing is that I think `V_from_K` or `V_of_K` etc. names for K->V maps are somewhat rare. For example, rs_bindings_from_cc names the equivalent map just `V_K`: `target_args`. So, here, we can name it just `crate-header`. It's not entirely self-descriptive, but it doesn't need to be.
"When should we delete the old name"? This isn't use in GNI or anything, so we can remove it in the next toolchain release.
PiperOrigin-RevId: 663419411
Change-Id: I0bd9acb503352b9cbd6a8e7925186ade3b652365
diff --git a/cc_bindings_from_rs/bindings.rs b/cc_bindings_from_rs/bindings.rs
index d566e14..9423043 100644
--- a/cc_bindings_from_rs/bindings.rs
+++ b/cc_bindings_from_rs/bindings.rs
@@ -652,7 +652,7 @@
.ok_or_else(|| {
anyhow!(
"Type `{ty}` comes from the `{other_crate_name}` crate, \
- but no `--bindings-from-dependency` was specified for this crate"
+ but no `--crate-header` was specified for this crate"
)
})?;
prereqs.includes.extend(includes.iter().cloned());
@@ -7776,7 +7776,7 @@
(
"std::cmp::Ordering",
"Type `std::cmp::Ordering` comes from the `core` crate, \
- but no `--bindings-from-dependency` was specified for this crate",
+ but no `--crate-header` was specified for this crate",
),
("Option<i8>", "Generic types are not supported yet (b/259749095)"),
(
diff --git a/cc_bindings_from_rs/cc_bindings_from_rs.rs b/cc_bindings_from_rs/cc_bindings_from_rs.rs
index ffbc58b..4fe270b 100644
--- a/cc_bindings_from_rs/cc_bindings_from_rs.rs
+++ b/cc_bindings_from_rs/cc_bindings_from_rs.rs
@@ -37,7 +37,7 @@
let crubit_support_path_format = cmdline.crubit_support_path_format.as_str().into();
let mut crate_name_to_include_paths = <HashMap<Rc<str>, Vec<CcInclude>>>::new();
- for (crate_name, include_path) in &cmdline.bindings_from_dependencies {
+ for (crate_name, include_path) in &cmdline.crate_headers {
let paths = crate_name_to_include_paths.entry(crate_name.as_str().into()).or_default();
paths.push(CcInclude::user_header(include_path.as_str().into()));
}
diff --git a/cc_bindings_from_rs/cmdline.rs b/cc_bindings_from_rs/cmdline.rs
index d0ad460..692ece6 100644
--- a/cc_bindings_from_rs/cmdline.rs
+++ b/cc_bindings_from_rs/cmdline.rs
@@ -38,14 +38,15 @@
#[clap(long, value_parser, value_name = "FILE")]
pub clang_format_exe_path: PathBuf,
- /// Include paths of bindings for dependencies of the current crate
- /// (generated by previous invocations of the tool).
- /// Example: "--bindings-from-dependency=foo=some/path/foo_cc_api.h".
- #[clap(long = "bindings-from-dependency", value_parser = parse_bindings_from_dependency,
+ /// Include paths of bindings for dependency crates, generated by previous
+ /// invocations of Crubit. Keys are crate names, and values are include
+ /// paths. Example: "--crate-header=foo=some/path/foo_cc_api.h".
+ // TODO(b/262878759): Remove alias after next toolchain release.
+ #[clap(long = "crate-header", visible_alias = "bindings-from-dependency", value_parser = parse_crate_header,
value_name = "CRATE_NAME=INCLUDE_PATH")]
// TODO(b/271857814): A `CRATE_NAME` might not be globally unique - the key needs to also cover
// a "hash" of the crate version and compilation flags.
- pub bindings_from_dependencies: Vec<(String, String)>,
+ pub crate_headers: Vec<(String, String)>,
/// Path to a rustfmt executable that will be used to format the
/// Rust source files generated by the tool.
@@ -105,11 +106,11 @@
Ok(s.to_string())
}
-/// Parse cmdline arguments of the following form:`"crateName=includePath"`.
+/// Parse cmdline arguments of the following form:`"crate_name=include_path"`.
///
/// Adapted from
/// https://github.com/clap-rs/clap/blob/cc1474f97c78002f3d99261699114e61d70b0634/examples/typed-derive.rs#L47-L59
-fn parse_bindings_from_dependency(s: &str) -> Result<(String, String)> {
+fn parse_crate_header(s: &str) -> Result<(String, String)> {
let Some(pos) = s.find('=') else {
bail!("Expected KEY=VALUE syntax but no `=` found in `{s}`");
};
@@ -157,7 +158,7 @@
assert_eq!("<crubit/support/{header}>", cmdline.crubit_support_path_format.as_str());
assert_eq!(Path::new("clang-format.exe"), cmdline.clang_format_exe_path);
assert_eq!(Path::new("rustfmt.exe"), cmdline.rustfmt_exe_path);
- assert!(cmdline.bindings_from_dependencies.is_empty());
+ assert!(cmdline.crate_headers.is_empty());
assert!(cmdline.rustfmt_config_path.is_none());
// Ignoring `rustc_args` in this test - they are covered in a separate
// test below: `test_rustc_args_happy_path`.
@@ -227,8 +228,8 @@
This is the format to `#include` Crubit C++ support library headers, using `{header}` as the placeholder. Example: `<crubit/support/{header}>` will produce `#include <crubit/support/hdr.h>`
--clang-format-exe-path <FILE>
Path to a clang-format executable that will be used to format the C++ header files generated by the tool
- --bindings-from-dependency <CRATE_NAME=INCLUDE_PATH>
- Include paths of bindings for dependencies of the current crate (generated by previous invocations of the tool). Example: "--bindings-from-dependency=foo=some/path/foo_cc_api.h"
+ --crate-header <CRATE_NAME=INCLUDE_PATH>
+ Include paths of bindings for dependency crates, generated by previous invocations of Crubit. Keys are crate names, and values are include paths. Example: "--crate-header=foo=some/path/foo_cc_api.h" [aliases: bindings-from-dependency]
--rustfmt-exe-path <FILE>
Path to a rustfmt executable that will be used to format the Rust source files generated by the tool
--rustfmt-config-path <FILE>
@@ -282,45 +283,49 @@
}
#[test]
- fn test_bindings_from_dependencies_as_multiple_separate_cmdline_args() {
+ fn test_crate_headers_as_multiple_separate_cmdline_args() {
let cmdline = new_cmdline([
"--h-out=foo.h",
"--rs-out=foo_impl.rs",
"--crubit-support-path-format=<crubit/support/{header}>",
"--clang-format-exe-path=clang-format.exe",
"--rustfmt-exe-path=rustfmt.exe",
+ // The two names are interchangeable:
"--bindings-from-dependency=dep1=path1",
- "--bindings-from-dependency=dep2=path2",
+ "--crate-header=dep2=path2",
+ "--bindings-from-dependency=dep3=path3",
+ "--crate-header=dep4=path4",
])
.unwrap();
- assert_eq!(2, cmdline.bindings_from_dependencies.len());
- assert_eq!("dep1", cmdline.bindings_from_dependencies[0].0);
- assert_eq!("path1", cmdline.bindings_from_dependencies[0].1);
- assert_eq!("dep2", cmdline.bindings_from_dependencies[1].0);
- assert_eq!("path2", cmdline.bindings_from_dependencies[1].1);
+ assert_eq!(4, cmdline.crate_headers.len());
+ assert_eq!("dep1", cmdline.crate_headers[0].0);
+ assert_eq!("path1", cmdline.crate_headers[0].1);
+ assert_eq!("dep2", cmdline.crate_headers[1].0);
+ assert_eq!("path2", cmdline.crate_headers[1].1);
+ assert_eq!("dep3", cmdline.crate_headers[2].0);
+ assert_eq!("path3", cmdline.crate_headers[2].1);
+ assert_eq!("dep4", cmdline.crate_headers[3].0);
+ assert_eq!("path4", cmdline.crate_headers[3].1);
}
#[test]
- fn test_parse_bindings_from_dependency() {
+ fn test_parse_crate_header() {
+ assert_eq!(parse_crate_header("foo=bar").unwrap(), ("foo".into(), "bar".into()),);
assert_eq!(
- parse_bindings_from_dependency("foo=bar").unwrap(),
- ("foo".into(), "bar".into()),
- );
- assert_eq!(
- parse_bindings_from_dependency("").unwrap_err().to_string(),
+ parse_crate_header("").unwrap_err().to_string(),
"Expected KEY=VALUE syntax but no `=` found in ``",
);
assert_eq!(
- parse_bindings_from_dependency("no-equal-char").unwrap_err().to_string(),
+ parse_crate_header("no-equal-char").unwrap_err().to_string(),
"Expected KEY=VALUE syntax but no `=` found in `no-equal-char`",
);
assert_eq!(
- parse_bindings_from_dependency("=bar").unwrap_err().to_string(),
+ parse_crate_header("=bar").unwrap_err().to_string(),
"Empty crate names are invalid",
);
assert_eq!(
- parse_bindings_from_dependency("foo=").unwrap_err().to_string(),
+ parse_crate_header("foo=").unwrap_err().to_string(),
"Empty include paths are invalid",
);
}
@@ -333,7 +338,7 @@
"--crubit-support-path-format=<crubit/support/{header}>",
"--clang-format-exe-path=clang-format.exe",
"--rustfmt-exe-path=rustfmt.exe",
- "--bindings-from-dependency=dep1=path1",
+ "--crate-header=dep1=path1",
])
.unwrap();
@@ -348,7 +353,7 @@
"--crubit-support-path-format=\"crubit/support\"",
"--clang-format-exe-path=clang-format.exe",
"--rustfmt-exe-path=rustfmt.exe",
- "--bindings-from-dependency=dep1=path1",
+ "--crate-header=dep1=path1",
])
.expect_err("crubit-support-path without `{header}` should trigger an error");
let clap_err = anyhow_err.downcast::<clap::Error>().unwrap();
diff --git a/cc_bindings_from_rs/test/bazel/cross_crate/test_api.rs b/cc_bindings_from_rs/test/bazel/cross_crate/test_api.rs
index 4040595..6bddb89 100644
--- a/cc_bindings_from_rs/test/bazel/cross_crate/test_api.rs
+++ b/cc_bindings_from_rs/test/bazel/cross_crate/test_api.rs
@@ -18,7 +18,7 @@
// b/292231336: Crubit currently fails to generate bindings for this function.
// Error message: Type std::string::String comes from the `alloc` crate, but no
-// `--bindings-from-dependency` was specified for this crate.
+// `--crate-header` was specified for this crate.
pub fn return_a_type_from_a_rust_toolchain_crate() -> String {
"String".to_owned()
}
