Propagate should_generate to sub-configs Previously, Builder::ConfigDefined() did not propagate should_generate to sub-configs (only calling ScheduleItemLoadIfNecessary()), assuming that the flag was irrelevant for configs since they are not written directly to Ninja files. However, Builder::CheckForBadItems() only populates bad_records with unresolved items that have should_generate() set, and only checks direct unresolved dependencies of bad_records for missing definitions. When a config chain had an undefined sub-config (or cycle) and an intermediate config was defined after the referencing target or parent config, the intermediate config did not have should_generate() set. It was omitted from bad_records, and CheckForBadItems() fell through to the unhelpful "I have no idea what went wrong, but these are unresolved, possibly due to an internal error:" message. Update Builder::ConfigDefined() to call RecursiveSetShouldGenerate() when record->should_generate() is true, and update Builder::CheckForCircularDependencies() to check all bad_records rather than only bad_records[0]. Produced with Gemini assistance; reviewed and verified by the author. Test: gn_unittests Ai-Assisted: true Change-Id: I16e1120d476ddc8b9ba82c7198c90ea1272c56a4 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27880 Commit-Queue: Roland McGrath <mcgrathr@google.com> Reviewed-by: David Turner <digit@google.com>
diff --git a/src/gn/builder.cc b/src/gn/builder.cc index cdcf422..d2413f1 100644 --- a/src/gn/builder.cc +++ b/src/gn/builder.cc
@@ -300,15 +300,16 @@ if (!AddDeps(record, config->configs(), err)) return false; - // Make sure all deps of this config are scheduled to be loaded. For other - // item types like targets, the "should generate" flag is propagated around - // to mark whether this should happen. We could call - // RecursiveSetShouldGenerate to do this step here, but since configs nor - // anything they depend on is actually written, the "generate" flag isn't - // relevant and means extra book keeping. Just force load any deps of this - // config. - for (auto it = record->all_deps().begin(); it.valid(); ++it) { - ScheduleItemLoadIfNecessary(*it); + // If this config is required by a generated target or config, propagate the + // "should generate" flag to its dependencies. Otherwise, schedule them to be + // loaded in case this config is referenced later (see + // https://crbug.com/41204844). + if (record->should_generate()) { + RecursiveSetShouldGenerate(record, true); + } else { + for (auto it = record->all_deps().begin(); it.valid(); ++it) { + ScheduleItemLoadIfNecessary(*it); + } } return true; @@ -721,7 +722,11 @@ std::string Builder::CheckForCircularDependencies( const std::vector<const BuilderRecord*>& bad_records) const { std::vector<const BuilderRecord*> cycle; - if (!RecursiveFindCycle(bad_records[0], &cycle)) + for (const auto* record : bad_records) { + if (RecursiveFindCycle(record, &cycle)) + break; + } + if (cycle.empty()) return std::string(); // Didn't find a cycle, something else is wrong. std::string ret;
diff --git a/src/gn/builder_unittest.cc b/src/gn/builder_unittest.cc index 1b38d0a..e4fd5cc 100644 --- a/src/gn/builder_unittest.cc +++ b/src/gn/builder_unittest.cc
@@ -363,7 +363,8 @@ EXPECT_TRUE(b_record->should_generate()); } -// Tests that configs applied to a config get loaded (bug 536844). +// Tests that configs applied to a config get loaded +// (https://crbug.com/41204844). TEST_F(BuilderTest, ConfigLoad) { SourceDir toolchain_dir = settings_.toolchain_label().dir(); std::string toolchain_name = settings_.toolchain_label().name(); @@ -384,6 +385,149 @@ EXPECT_TRUE(loader_->HasLoadedOne(SourceFile("//b/BUILD.gn"))); } +// Tests that when a target depends on a config chain with an undefined +// sub-config, the error reports the unresolved dependency instead of an +// internal error (due to missing should_generate propagation). +TEST_F(BuilderTest, SubConfigUndefinedError) { + DefineToolchain(); + SourceDir toolchain_dir = settings_.toolchain_label().dir(); + std::string toolchain_name = settings_.toolchain_label().name(); + + // Dependency chain: Target T -> Config A -> Config B -> Config C (undefined). + Label t_label(SourceDir("//t/"), "t", toolchain_dir, toolchain_name); + Label a_label(SourceDir("//a/"), "a", toolchain_dir, toolchain_name); + Label b_label(SourceDir("//b/"), "b", toolchain_dir, toolchain_name); + Label c_label(SourceDir("//c/"), "c", toolchain_dir, toolchain_name); + + // 1. Define target T first, referencing Config A. + auto t = std::make_unique<Target>(&settings_, t_label); + t->set_output_type(Target::EXECUTABLE); + t->configs().push_back(LabelConfigPair(a_label)); + builder_.ItemDefined(std::move(t)); + + // 2. Define Config A, referencing Config B. + auto a = std::make_unique<Config>(&settings_, a_label); + a->configs().push_back(LabelConfigPair(b_label)); + builder_.ItemDefined(std::move(a)); + + // 3. Define Config B, referencing Config C. + auto b = std::make_unique<Config>(&settings_, b_label); + b->configs().push_back(LabelConfigPair(c_label)); + builder_.ItemDefined(std::move(b)); + + // Config C is never defined. + scheduler().Run(); + + BuilderRecord* a_record = builder_.GetRecord(a_label); + ASSERT_TRUE(a_record); + EXPECT_TRUE(a_record->should_generate()); + + BuilderRecord* b_record = builder_.GetRecord(b_label); + ASSERT_TRUE(b_record); + EXPECT_TRUE(b_record->should_generate()); + + BuilderRecord* c_record = builder_.GetRecord(c_label); + ASSERT_TRUE(c_record); + EXPECT_TRUE(c_record->should_generate()); + EXPECT_FALSE(c_record->item()); + + Err err; + EXPECT_FALSE(builder_.CheckForBadItems(&err)); + EXPECT_TRUE(err.has_error()); + EXPECT_EQ("Unresolved dependencies.", err.message()); + EXPECT_EQ(b_label.GetUserVisibleName(true) + "\n needs " + + c_label.GetUserVisibleName(true) + "\n", + err.help_text()); +} + +// Tests the same dependency chain when configs are defined before the target. +TEST_F(BuilderTest, SubConfigUndefinedErrorReverseOrder) { + DefineToolchain(); + SourceDir toolchain_dir = settings_.toolchain_label().dir(); + std::string toolchain_name = settings_.toolchain_label().name(); + + Label t_label(SourceDir("//t/"), "t", toolchain_dir, toolchain_name); + Label a_label(SourceDir("//a/"), "a", toolchain_dir, toolchain_name); + Label b_label(SourceDir("//b/"), "b", toolchain_dir, toolchain_name); + Label c_label(SourceDir("//c/"), "c", toolchain_dir, toolchain_name); + + // 1. Define Config B, referencing Config C (undefined). + auto b = std::make_unique<Config>(&settings_, b_label); + b->configs().push_back(LabelConfigPair(c_label)); + builder_.ItemDefined(std::move(b)); + + // 2. Define Config A, referencing Config B. + auto a = std::make_unique<Config>(&settings_, a_label); + a->configs().push_back(LabelConfigPair(b_label)); + builder_.ItemDefined(std::move(a)); + + // 3. Define Target T, referencing Config A. + auto t = std::make_unique<Target>(&settings_, t_label); + t->set_output_type(Target::EXECUTABLE); + t->configs().push_back(LabelConfigPair(a_label)); + builder_.ItemDefined(std::move(t)); + + scheduler().Run(); + + BuilderRecord* a_record = builder_.GetRecord(a_label); + ASSERT_TRUE(a_record); + EXPECT_TRUE(a_record->should_generate()); + + BuilderRecord* b_record = builder_.GetRecord(b_label); + ASSERT_TRUE(b_record); + EXPECT_TRUE(b_record->should_generate()); + + BuilderRecord* c_record = builder_.GetRecord(c_label); + ASSERT_TRUE(c_record); + EXPECT_TRUE(c_record->should_generate()); + EXPECT_FALSE(c_record->item()); + + Err err; + EXPECT_FALSE(builder_.CheckForBadItems(&err)); + EXPECT_TRUE(err.has_error()); + EXPECT_EQ("Unresolved dependencies.", err.message()); + EXPECT_EQ(b_label.GetUserVisibleName(true) + "\n needs " + + c_label.GetUserVisibleName(true) + "\n", + err.help_text()); +} + +// Tests that a cycle among sub-configs is detected and reported as a cycle. +TEST_F(BuilderTest, SubConfigCycle) { + DefineToolchain(); + SourceDir toolchain_dir = settings_.toolchain_label().dir(); + std::string toolchain_name = settings_.toolchain_label().name(); + + Label t_label(SourceDir("//t/"), "t", toolchain_dir, toolchain_name); + Label a_label(SourceDir("//a/"), "a", toolchain_dir, toolchain_name); + Label b_label(SourceDir("//b/"), "b", toolchain_dir, toolchain_name); + Label c_label(SourceDir("//c/"), "c", toolchain_dir, toolchain_name); + + // Target T -> Config A -> Config B -> Config C -> Config B. + auto t = std::make_unique<Target>(&settings_, t_label); + t->set_output_type(Target::EXECUTABLE); + t->configs().push_back(LabelConfigPair(a_label)); + builder_.ItemDefined(std::move(t)); + + auto a = std::make_unique<Config>(&settings_, a_label); + a->configs().push_back(LabelConfigPair(b_label)); + builder_.ItemDefined(std::move(a)); + + auto b = std::make_unique<Config>(&settings_, b_label); + b->configs().push_back(LabelConfigPair(c_label)); + builder_.ItemDefined(std::move(b)); + + auto c = std::make_unique<Config>(&settings_, c_label); + c->configs().push_back(LabelConfigPair(b_label)); + builder_.ItemDefined(std::move(c)); + + scheduler().Run(); + + Err err; + EXPECT_FALSE(builder_.CheckForBadItems(&err)); + EXPECT_TRUE(err.has_error()); + EXPECT_EQ("Dependency cycle:", err.message()); +} + // Tests that "validations" dependencies behave correctly: // 1. They trigger the loading of the validated target's build file (simulating // cross-directory deps).