Stop forwarding private hard_deps on check_includes_strict targets Previously, ResolvedTargetData::ComputeHardDeps() and NinjaTargetWriter::WriteHardDepsStampOrPhony() forwarded hard_deps from both public_deps and private deps of a target T (unless T was a binary target with public = []), because standard 'gn check' allows T's public headers to #include headers from T's private deps. However, when T sets check_includes_strict = true, 'gn check' enforces that T's public headers cannot #include headers from T's private deps. Therefore, targets depending on T cannot transitively reach generated headers from T's private dependencies. This change restricts ComputeHardDeps() and WriteHardDepsStampOrPhony() to only traverse public_deps when check_includes_strict = true (for non-hard targets), while still including hard_deps of private_deps in T's own input dependencies in WriteInputDepsStampOrPhonyAndGetDep(). Suggested in: https://chromium-review.googlesource.com/c/chromium/src/+/8497030/comment/9c2d1aca_83609b40/ Bug: 563098656 Change-Id: If9d5ef1e18475f0bcae1a65553256ff84543d871 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27100 Reviewed-by: Matt Stark <msta@google.com> Reviewed-by: David Turner <digit@google.com> Commit-Queue: Takuto Ikuta <tikuta@google.com>
diff --git a/docs/reference.md b/docs/reference.md index 6a03917..2796a0b 100644 --- a/docs/reference.md +++ b/docs/reference.md
@@ -5776,7 +5776,8 @@ compile this target's public headers), but they are not transitively forwarded to dependents. * "deps" act as private implementation dependencies (dependencies required to - compile this target's sources). + compile this target's sources). This also allows dependent targets to + compile without waiting for this target's private "deps". When false (the default), the default loose include checking rules apply. ```
diff --git a/src/gn/ninja_target_writer.cc b/src/gn/ninja_target_writer.cc index 7e1ba0e..604f2f5 100644 --- a/src/gn/ninja_target_writer.cc +++ b/src/gn/ninja_target_writer.cc
@@ -290,6 +290,31 @@ outputs->push_back(single_hard_dep->dependency_output()); } +namespace { + +// Appends the ninja outputs representing hard dependencies for the given list +// of direct |deps| of |target|, deduplicating the resulting |outputs|. +// For each dep, if it (or |target|) is a direct hard_dep, its dependency output +// is added directly; otherwise, if it forwards hard deps, its aggregated hard +// deps output (at most one file/phony per dep) is appended. +void AppendHardDepsOutputsForDeps(const Target* target, + base::span<const Target*> deps, + const ResolvedTargetData& resolved, + std::vector<OutputFile>* outputs) { + for (const Target* dep : deps) { + if (target->hard_dep() || dep->hard_dep()) { + if (IsListedHardDep(dep)) + outputs->push_back(dep->dependency_output()); + } else if (ResolvedTargetData::ForwardsHardDeps(dep)) { + NinjaTargetWriter::AppendHardDepsOutputs(dep, resolved, outputs); + } + } + std::sort(outputs->begin(), outputs->end()); + outputs->erase(std::unique(outputs->begin(), outputs->end()), outputs->end()); +} + +} // namespace + // static void NinjaTargetWriter::WriteHardDepsStampOrPhony(const Target* target, ResolvedTargetData* resolved, @@ -309,18 +334,16 @@ // 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. + // instead of flattening them. Because dependents refer to this rule, only + // public_deps are included when check_includes_strict is set. + const ResolvedTargetDeps& target_deps = resolved->GetTargetDeps(target); std::vector<OutputFile> deps; - for (const Target* dep : resolved->GetTargetDeps(target).linked_deps()) { - if (target->hard_dep() || dep->hard_dep()) { - if (IsListedHardDep(dep)) - deps.push_back(dep->dependency_output()); - } else if (ResolvedTargetData::ForwardsHardDeps(dep)) { - AppendHardDepsOutputs(dep, *resolved, &deps); - } - } - std::sort(deps.begin(), deps.end()); - deps.erase(std::unique(deps.begin(), deps.end()), deps.end()); + AppendHardDepsOutputsForDeps( + target, + target->check_includes_strict() && !target->hard_dep() + ? target_deps.public_deps() + : target_deps.linked_deps(), + *resolved, &deps); PathOutput path_output(build_settings->build_dir(), build_settings->root_path_utf8(), ESCAPE_NINJA); @@ -639,6 +662,17 @@ // 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 + // <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. + AppendHardDepsOutputsForDeps( + target_, resolved().GetTargetDeps(target_).private_deps(), resolved(), + &hard_deps_outputs); + } } // Additional hard dependencies passed in. These are usually empty or small,
diff --git a/src/gn/ninja_target_writer_unittest.cc b/src/gn/ninja_target_writer_unittest.cc index fb00320..a8662d7 100644 --- a/src/gn/ninja_target_writer_unittest.cc +++ b/src/gn/ninja_target_writer_unittest.cc
@@ -894,4 +894,28 @@ "build phony/foo/action4.harddeps: phony phony/foo/action3 " "phony/foo/mid\n", hard_deps_rule(&action4)); + + // Targets with check_includes_strict = true only include public_deps in + // their hard deps, while still including private deps in their own input + // deps. + Target strict(setup.settings(), Label(SourceDir("//foo/"), "strict")); + make_source_set(&strict); + strict.set_check_includes_strict(true); + strict.public_deps().push_back(LabelTargetPair(&single)); + strict.private_deps().push_back(LabelTargetPair(&mid)); + strict.private_deps().push_back(LabelTargetPair(&action3)); + ASSERT_TRUE(strict.OnResolved(&err)); + EXPECT_EQ("", hard_deps_rule(&strict)); + EXPECT_EQ("phony/foo/action1 ", hard_deps_outputs(&strict)); + { + std::ostringstream stream; + TestingNinjaTargetWriter writer(&strict, 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 b0ef9a6..f492986 100644 --- a/src/gn/resolved_target_data.cc +++ b/src/gn/resolved_target_data.cc
@@ -72,7 +72,21 @@ // Keep in sync with NinjaTargetWriter::WriteHardDepsStampOrPhony(). void ResolvedTargetData::ComputeHardDeps(TargetInfo* info) const { TargetSet all_hard_deps; - for (const Target* dep : info->deps.linked_deps()) { + // 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(); + for (const Target* dep : deps) { // Direct hard dependencies if (info->target->hard_dep() || dep->hard_dep()) { all_hard_deps.insert(dep);
diff --git a/src/gn/resolved_target_data.h b/src/gn/resolved_target_data.h index 544872d..5922afe 100644 --- a/src/gn/resolved_target_data.h +++ b/src/gn/resolved_target_data.h
@@ -106,9 +106,11 @@ return GetTargetFrameworkInfo(target)->weak_libraries; } - // Retrieves a set of hard dependencies for this target. - // All hard deps from this target and all dependencies, but not the - // target itself. + // 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. const TargetSet& GetHardDeps(const Target* target) const { return GetTargetHardDeps(target)->hard_deps; }
diff --git a/src/gn/resolved_target_data_unittest.cc b/src/gn/resolved_target_data_unittest.cc index 49dc623..0361411 100644 --- a/src/gn/resolved_target_data_unittest.cc +++ b/src/gn/resolved_target_data_unittest.cc
@@ -530,3 +530,27 @@ // A2 has G2 as private dep, so A2 does not export public_inputs. EXPECT_FALSE(resolved.ExportsPublicInputs(&a2)); } + +TEST(ResolvedTargetDataTest, HardDepsStrictIncludes) { + TestWithScope setup; + Err err; + + TestTarget pub_action(setup, "//foo:pub_action", Target::ACTION); + TestTarget priv_action(setup, "//foo:priv_action", Target::ACTION); + + TestTarget strict(setup, "//foo:strict", Target::SOURCE_SET); + strict.set_check_includes_strict(true); + strict.public_deps().push_back(LabelTargetPair(&pub_action)); + strict.private_deps().push_back(LabelTargetPair(&priv_action)); + + ASSERT_TRUE(pub_action.OnResolved(&err)); + ASSERT_TRUE(priv_action.OnResolved(&err)); + ASSERT_TRUE(strict.OnResolved(&err)); + + ResolvedTargetData resolved; + + const TargetSet& strict_hard_deps = resolved.GetHardDeps(&strict); + EXPECT_EQ(1u, strict_hard_deps.size()); + EXPECT_TRUE(strict_hard_deps.contains(&pub_action)); + EXPECT_FALSE(strict_hard_deps.contains(&priv_action)); +}
diff --git a/src/gn/variables.cc b/src/gn/variables.cc index 994322d..15b60d2 100644 --- a/src/gn/variables.cc +++ b/src/gn/variables.cc
@@ -936,7 +936,8 @@ compile this target's public headers), but they are not transitively forwarded to dependents. * "deps" act as private implementation dependencies (dependencies required to - compile this target's sources). + compile this target's sources). This also allows dependent targets to + compile without waiting for this target's private "deps". When false (the default), the default loose include checking rules apply.