Stop forwarding private hard_deps on Rust targets Previously, Rust targets (e.g. rust_library, rust_proc_macro) forwarded hard_deps from both public_deps and private deps to their dependents because Target::all_headers_public() defaults to true. However, Rust targets do not have C/C++ public headers, and downstream Rust targets already have direct dependencies on upstream .rlib/.rmeta files. Private dependencies of a Rust target (such as Cargo build script execution actions or internal rust_bindgen actions) are only needed to compile the Rust target itself, while any generated C++ headers exposed to dependents (e.g. cxxbridge or Crubit headers) are placed in public_deps. This change introduces ResolvedTargetData::OnlyForwardsPublicHardDeps() and applies it to Rust targets in addition to check_includes_strict targets, so that Rust targets only forward hard_deps from public_deps to dependents while still including private hard_deps in their own compilation input dependencies. In a Chromium Linux build, this eliminates 377 .harddeps phony rules and 2,587 .harddeps dependency edges (including std.harddeps and 3p Cargo crate build script outputs propagating into downstream C++ targets). Bug: 563098656 Change-Id: If74076cba64281db575afea5b84f61c25f29bb09 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27321 Reviewed-by: Matt Stark <msta@google.com> Commit-Queue: Takuto Ikuta <tikuta@google.com> Reviewed-by: David Turner <digit@google.com>
diff --git a/src/gn/ninja_target_writer.cc b/src/gn/ninja_target_writer.cc index d19a3aa..5e9c136 100644 --- a/src/gn/ninja_target_writer.cc +++ b/src/gn/ninja_target_writer.cc
@@ -340,12 +340,12 @@ // Keep in sync with ResolvedTargetData::ComputeHardDeps(). This produces // the same set of targets, but refers to the hard deps rules of deps // instead of flattening them. Because dependents refer to this rule, only - // public_deps are included when check_includes_strict is set. + // public_deps are included when OnlyForwardsPublicHardDeps() is true. const ResolvedTargetDeps& target_deps = resolved->GetTargetDeps(target); std::vector<OutputFile> deps; AppendHardDepsOutputsForDeps( target, - target->check_includes_strict() && !target->hard_dep() + ResolvedTargetData::OnlyForwardsPublicHardDeps(target) ? target_deps.public_deps() : target_deps.linked_deps(), *resolved, &deps); @@ -667,13 +667,13 @@ // depends on all hard deps that aren't BUNDLE_DATA, to not write // thousands of hard deps for every target. AppendHardDepsOutputs(target_, resolved(), &hard_deps_outputs); - if (target_->check_includes_strict() && !target_->hard_dep()) { - // When check_includes_strict is enabled, GetHardDeps(target_) and + if (ResolvedTargetData::OnlyForwardsPublicHardDeps(target_)) { + // When OnlyForwardsPublicHardDeps() is true, GetHardDeps(target_) and // <target>.harddeps only include hard deps from public_deps so that // private hard deps are not forwarded to dependents. However, compiling - // this target's own sources may still #include generated headers from - // its private_deps, so include their hard deps in this target's own - // input deps. + // this target's own sources may still depend on generated files from its + // private_deps, so include their hard deps in this target's own input + // deps. AppendHardDepsOutputsForDeps( target_, resolved().GetTargetDeps(target_).private_deps(), resolved(), &hard_deps_outputs);
diff --git a/src/gn/ninja_target_writer_unittest.cc b/src/gn/ninja_target_writer_unittest.cc index 2986903..1db594a 100644 --- a/src/gn/ninja_target_writer_unittest.cc +++ b/src/gn/ninja_target_writer_unittest.cc
@@ -919,4 +919,33 @@ EXPECT_EQ("phony/foo/action3", deps.order_only[1].value()); EXPECT_EQ("phony/foo/mid.harddeps", deps.order_only[2].value()); } + + // Rust targets also only include public_deps in their hard deps, while still + // including private deps in their own input deps. + Target rust_lib(setup.settings(), Label(SourceDir("//foo/"), "rust_lib")); + rust_lib.set_output_type(Target::RUST_LIBRARY); + rust_lib.visibility().SetPublic(); + SourceFile lib_rs("//foo/lib.rs"); + rust_lib.sources().push_back(lib_rs); + rust_lib.source_types_used().Set(SourceFile::SOURCE_RS); + rust_lib.rust_values().set_crate_root(lib_rs); + rust_lib.rust_values().crate_name() = "rust_lib"; + rust_lib.SetToolchain(setup.toolchain()); + rust_lib.public_deps().push_back(LabelTargetPair(&single)); + rust_lib.private_deps().push_back(LabelTargetPair(&mid)); + rust_lib.private_deps().push_back(LabelTargetPair(&action3)); + ASSERT_TRUE(rust_lib.OnResolved(&err)); + EXPECT_EQ("", hard_deps_rule(&rust_lib)); + EXPECT_EQ("phony/foo/action1 ", hard_deps_outputs(&rust_lib)); + { + std::ostringstream stream; + TestingNinjaTargetWriter writer(&rust_lib, setup.toolchain(), stream); + auto deps = writer.WriteInputDepsStampOrPhonyAndGetDep({}, 1u); + EXPECT_EQ("", stream.str()); + EXPECT_TRUE(deps.implicit.empty()); + ASSERT_EQ(3u, deps.order_only.size()); + EXPECT_EQ("phony/foo/action1", deps.order_only[0].value()); + EXPECT_EQ("phony/foo/action3", deps.order_only[1].value()); + EXPECT_EQ("phony/foo/mid.harddeps", deps.order_only[2].value()); + } }
diff --git a/src/gn/resolved_target_data.cc b/src/gn/resolved_target_data.cc index f492986..72758c3 100644 --- a/src/gn/resolved_target_data.cc +++ b/src/gn/resolved_target_data.cc
@@ -69,23 +69,29 @@ dep->public_headers().empty() && !dep->builds_swift_module()); } -// Keep in sync with NinjaTargetWriter::WriteHardDepsStampOrPhony(). -void ResolvedTargetData::ComputeHardDeps(TargetInfo* info) const { - TargetSet all_hard_deps; +// static +bool ResolvedTargetData::OnlyForwardsPublicHardDeps(const Target* target) { // Normally, both public_deps and private_deps (i.e. linked_deps()) must be // traversed because standard `gn check` allows a target's public headers to // #include headers from its private deps, meaning dependents might // transitively include generated headers from private deps. // - // However, when `check_includes_strict` is enabled, `gn check` guarantees - // that public headers do not #include anything from private deps. Thus, - // dependents only need hard deps from `public_deps()`. The target's own - // compilation still needs hard deps from its `private_deps()`, which are - // added directly in NinjaTargetWriter::WriteInputDepsStampOrPhonyAndGetDep(). - base::span<const Target*> deps = - info->target->check_includes_strict() && !info->target->hard_dep() - ? info->deps.public_deps() - : info->deps.linked_deps(); + // However, when `check_includes_strict` is enabled (`gn check` guarantees + // public headers do not #include anything from private deps) or for Rust + // targets (which have no C/C++ public headers and expose any generated C++ + // binding headers via `public_deps`), dependents only need hard deps from + // `public_deps()`. The target's own compilation still gets hard deps from its + // `private_deps()` in NinjaTargetWriter::WriteInputDepsStampOrPhonyAndGetDep. + return !target->hard_dep() && (target->check_includes_strict() || + target->source_types_used().RustSourceUsed()); +} + +// Keep in sync with NinjaTargetWriter::WriteHardDepsStampOrPhony(). +void ResolvedTargetData::ComputeHardDeps(TargetInfo* info) const { + TargetSet all_hard_deps; + base::span<const Target*> deps = OnlyForwardsPublicHardDeps(info->target) + ? info->deps.public_deps() + : info->deps.linked_deps(); for (const Target* dep : deps) { // Direct hard dependencies if (info->target->hard_dep() || dep->hard_dep()) {
diff --git a/src/gn/resolved_target_data.h b/src/gn/resolved_target_data.h index 5922afe..e498479 100644 --- a/src/gn/resolved_target_data.h +++ b/src/gn/resolved_target_data.h
@@ -109,8 +109,8 @@ // Retrieves a set of hard dependencies for this target that are propagated // to dependents (and used to construct this target's .harddeps rule). // Includes all hard deps from this target's dependencies, but not the target - // itself. When check_includes_strict is set on a non-hard_dep target, only - // hard deps from public_deps are included here. + // itself. When OnlyForwardsPublicHardDeps() is true, only hard deps from + // public_deps are included here. const TargetSet& GetHardDeps(const Target* target) const { return GetTargetHardDeps(target)->hard_deps; } @@ -154,6 +154,10 @@ // that computation. static bool ForwardsHardDeps(const Target* dep); + // Returns true if |target| only forwards hard deps from its public_deps + // (rather than both public_deps and private_deps) to dependents. + static bool OnlyForwardsPublicHardDeps(const Target* target); + const std::vector<OutputFile>& GetOrderOnlyDeps(const Target* target) const { return GetTargetOrderOnlyDeps(target)->order_only_deps; }
diff --git a/src/gn/resolved_target_data_unittest.cc b/src/gn/resolved_target_data_unittest.cc index 0361411..2607438 100644 --- a/src/gn/resolved_target_data_unittest.cc +++ b/src/gn/resolved_target_data_unittest.cc
@@ -554,3 +554,34 @@ EXPECT_TRUE(strict_hard_deps.contains(&pub_action)); EXPECT_FALSE(strict_hard_deps.contains(&priv_action)); } + +TEST(ResolvedTargetDataTest, HardDepsRustLibrary) { + TestWithScope setup; + Err err; + + TestTarget pub_action(setup, "//foo:pub_action", Target::ACTION); + TestTarget priv_action(setup, "//foo:priv_action", Target::ACTION); + + Target rust_lib(setup.settings(), Label(SourceDir("//foo/"), "rust_lib")); + rust_lib.set_output_type(Target::RUST_LIBRARY); + rust_lib.visibility().SetPublic(); + SourceFile lib_rs("//foo/lib.rs"); + rust_lib.sources().push_back(lib_rs); + rust_lib.source_types_used().Set(SourceFile::SOURCE_RS); + rust_lib.rust_values().set_crate_root(lib_rs); + rust_lib.rust_values().crate_name() = "rust_lib"; + rust_lib.SetToolchain(setup.toolchain()); + rust_lib.public_deps().push_back(LabelTargetPair(&pub_action)); + rust_lib.private_deps().push_back(LabelTargetPair(&priv_action)); + + ASSERT_TRUE(pub_action.OnResolved(&err)); + ASSERT_TRUE(priv_action.OnResolved(&err)); + ASSERT_TRUE(rust_lib.OnResolved(&err)); + + ResolvedTargetData resolved; + + const TargetSet& rust_hard_deps = resolved.GetHardDeps(&rust_lib); + EXPECT_EQ(1u, rust_hard_deps.size()); + EXPECT_TRUE(rust_hard_deps.contains(&pub_action)); + EXPECT_FALSE(rust_hard_deps.contains(&priv_action)); +}