If a header is in multiple targets, take it as part of the first target (lexicographically).
This is not Good Behavior. In particular, this means that adding a dependency can break Rust callers. Consider the following:
```cc
//h1.h
struct S {};
```
```cc
//h2.h
void Foo(S s);
```
with this BUILD file:
```py
cc_library(name="h1_a", hdrs=["h1.h"], ...)
cc_library(name="h1_b", hdrs=["h1.h"], ...)
cc_library(name="h2_a", hdrs=["h2.h"], deps=[":h1_a"])
cc_library(name="h2_b", hdrs=["h2.h"], deps=[":h1_b"])
cc_library(name="h2_ab", hdrs=["h2.h"], deps=[":h1_a", ":h1_b"])
```
Then:
* `h2_a` contains `fn Foo(s: h1_a::S);`
* `h2_b` contains `fn Foo(s: h2_a::S);`
* `h2_ab` contains `fn Foo(s: h1_a::S);`
Meaning that adding a single (TRANSITIVE) dependency can accidentally change the type of your parameters, breaking Rust callers -- a caller who was passing `h2_a::S` would get broken if a transitive dependency on `h1_a` were added.
But note: without this CL, we're even _worse_ off, because that same transitive dependency breaks the build entirely, and there is no way to resolve it. After this CL, there is a quick fix: use `h1_a::S`.
---
Alternatives considered:
* We could fix all headers everywhere to have a single canonical build target, but this is a big task. We do likely want to do it eventually, but it shouldn't block early adopters from trying Crubit.
---
It's worth noting that with the proposed[]
PiperOrigin-RevId: 501900537
diff --git a/rs_bindings_from_cc/BUILD b/rs_bindings_from_cc/BUILD
index 727d035..2389c72 100644
--- a/rs_bindings_from_cc/BUILD
+++ b/rs_bindings_from_cc/BUILD
@@ -172,6 +172,7 @@
"//common:status_macros",
"@absl//absl/container:flat_hash_map",
"@absl//absl/flags:flag",
+ "@absl//absl/log",
"@absl//absl/status:statusor",
"@absl//absl/strings",
"@llvm-project//llvm:Support",
diff --git a/rs_bindings_from_cc/cmdline.cc b/rs_bindings_from_cc/cmdline.cc
index aeabfce..fe6b7c7 100644
--- a/rs_bindings_from_cc/cmdline.cc
+++ b/rs_bindings_from_cc/cmdline.cc
@@ -11,6 +11,7 @@
#include <vector>
#include "absl/flags/flag.h"
+#include "absl/log/log.h"
#include "absl/strings/str_cat.h"
#include "absl/strings/substitute.h"
#include "common/status_macros.h"
@@ -187,13 +188,19 @@
"Expected `h` fields of `--targets_and_headers` to be an array of "
"non-empty strings");
}
- const auto [it, inserted] = cmdline.headers_to_targets_.insert(
- std::make_pair(HeaderName(header), BazelLabel(target)));
+ BazelLabel target_label(target);
+ auto [it, inserted] = cmdline.headers_to_targets_.try_emplace(
+ HeaderName(header), std::move(target_label));
if (!inserted) {
- return absl::InvalidArgumentError(absl::Substitute(
- "The `--targets_and_headers` cmdline argument assigns "
- "`$0` header to two conflicting targets: `$1` vs `$2`",
- header, target, it->second.value()));
+ LOG(WARNING) << "The `--targets_and_headers` cmdline argument assigns "
+ "`"
+ << header << "` header to two conflicting targets: `"
+ << target << "` vs `" << it->second.value() << "`";
+ // Assign the one that comes first alphabetically, to get a consistent
+ // result.
+ if (target_label.value() < it->second.value()) {
+ it->second = std::move(target_label);
+ }
}
}
}
diff --git a/rs_bindings_from_cc/cmdline.h b/rs_bindings_from_cc/cmdline.h
index 2994b29..2f1f7d6 100644
--- a/rs_bindings_from_cc/cmdline.h
+++ b/rs_bindings_from_cc/cmdline.h
@@ -77,8 +77,8 @@
const BazelLabel& current_target() const { return current_target_; }
- const absl::flat_hash_map<const HeaderName, const BazelLabel>&
- headers_to_targets() const {
+ const absl::flat_hash_map<HeaderName, BazelLabel>& headers_to_targets()
+ const {
return headers_to_targets_;
}
@@ -110,7 +110,7 @@
BazelLabel current_target_;
std::vector<HeaderName> public_headers_;
- absl::flat_hash_map<const HeaderName, const BazelLabel> headers_to_targets_;
+ absl::flat_hash_map<HeaderName, BazelLabel> headers_to_targets_;
std::vector<std::string> extra_rs_srcs_;
diff --git a/rs_bindings_from_cc/cmdline_test.cc b/rs_bindings_from_cc/cmdline_test.cc
index 0f59e9e..26e8406 100644
--- a/rs_bindings_from_cc/cmdline_test.cc
+++ b/rs_bindings_from_cc/cmdline_test.cc
@@ -146,14 +146,20 @@
}
TEST(CmdlineTest, TargetsAndHeadersDuplicateHeader) {
- constexpr absl::string_view kTestInput = R"([
+ constexpr absl::string_view kHeaders = R"([
{"t": "t1", "h": ["h1"]},
- {"t": "t2", "h": ["h1"]} ])";
- ASSERT_THAT(
- TestCmdline({"h1"}, std::string(kTestInput)),
- StatusIs(absl::StatusCode::kInvalidArgument,
- AllOf(HasSubstr("--targets_and_headers"), HasSubstr("conflict"),
- HasSubstr("h1"), HasSubstr("t1"), HasSubstr("t2"))));
+ {"t": "t2", "h": ["h1", "h2"]} ])";
+ ASSERT_OK_AND_ASSIGN(
+ Cmdline cmdline,
+ Cmdline::CreateForTesting(
+ "cc_out", "rs_out", "ir_out", "namespaces_out", "crubit_support_path",
+ "clang_format_exe_path", "rustfmt_exe_path", "rustfmt_config_path",
+ /* do_nothing= */ false, {"h1"}, std::string(kHeaders),
+ {"extra_file.rs"}, {"scan_for_instantiations.rs"},
+ "instantiations_out", "error_report_out"));
+ EXPECT_THAT(cmdline.headers_to_targets(),
+ UnorderedElementsAre(Pair(HeaderName("h1"), BazelLabel("t1")),
+ Pair(HeaderName("h2"), BazelLabel("t2"))));
}
TEST(CmdlineTest, PublicHeadersEmpty) {
diff --git a/rs_bindings_from_cc/decl_importer.h b/rs_bindings_from_cc/decl_importer.h
index b8b1277..94ab8de 100644
--- a/rs_bindings_from_cc/decl_importer.h
+++ b/rs_bindings_from_cc/decl_importer.h
@@ -20,8 +20,7 @@
class Invocation {
public:
Invocation(BazelLabel target, absl::Span<const HeaderName> public_headers,
- const absl::flat_hash_map<const HeaderName, const BazelLabel>&
- header_targets)
+ const absl::flat_hash_map<HeaderName, BazelLabel>& header_targets)
: target_(target),
public_headers_(public_headers),
lifetime_context_(std::make_shared<
@@ -57,8 +56,7 @@
IR ir_;
private:
- const absl::flat_hash_map<const HeaderName, const BazelLabel>&
- header_targets_;
+ const absl::flat_hash_map<HeaderName, BazelLabel>& header_targets_;
};
// Explicitly defined interface that defines how `DeclImporter`s are allowed to
diff --git a/rs_bindings_from_cc/ir_from_cc.cc b/rs_bindings_from_cc/ir_from_cc.cc
index 76660d4..4fff0ac 100644
--- a/rs_bindings_from_cc/ir_from_cc.cc
+++ b/rs_bindings_from_cc/ir_from_cc.cc
@@ -36,7 +36,7 @@
absl::Span<const HeaderName> public_headers,
absl::flat_hash_map<const HeaderName, const std::string>
virtual_headers_contents_for_testing,
- absl::flat_hash_map<const HeaderName, const BazelLabel> headers_to_targets,
+ absl::flat_hash_map<HeaderName, BazelLabel> headers_to_targets,
absl::Span<const std::string> extra_rs_srcs,
absl::Span<const absl::string_view> clang_args,
absl::Span<const std::string> extra_instantiations) {
diff --git a/rs_bindings_from_cc/ir_from_cc.h b/rs_bindings_from_cc/ir_from_cc.h
index 8ceff69..9dc7345 100644
--- a/rs_bindings_from_cc/ir_from_cc.h
+++ b/rs_bindings_from_cc/ir_from_cc.h
@@ -51,8 +51,7 @@
absl::Span<const HeaderName> public_headers = {},
absl::flat_hash_map<const HeaderName, const std::string>
virtual_headers_contents_for_testing = {},
- absl::flat_hash_map<const HeaderName, const BazelLabel> headers_to_targets =
- {},
+ absl::flat_hash_map<HeaderName, BazelLabel> headers_to_targets = {},
absl::Span<const std::string> extra_rs_srcs = {},
absl::Span<const absl::string_view> clang_args = {},
absl::Span<const std::string> extra_instantiations = {});
