Fix target being written twice under certain circumstances The Fuchsia build triggered a situation where a target was written twice when it changed from ShouldGenerate false to true. A target can start out with `ShouldGenerate = false` if it is not using the default toolchain or there are root patterns that don't match. `RecursiveSetShouldGenerate` triggers writes for this target if another target is discovered that has `ShouldGenerate = true` and depends on it. The logic in `RecursiveShouldGenerateWithValidations` did not include the new `CanWrite` check introduced with validations. This CL colocates the logic into CheckAndTriggerWrite so the logic lives in one place. Bug: 478798763 Change-Id: I82413b8292f05fe38a7454446f01a42e7165f7ed Reviewed-on: https://gn-review.googlesource.com/c/gn/+/20980 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Neri Marschik <nerima@google.com>
diff --git a/src/gn/builder.cc b/src/gn/builder.cc index 51f9a02..90e3f5b 100644 --- a/src/gn/builder.cc +++ b/src/gn/builder.cc
@@ -500,8 +500,7 @@ record->set_should_generate(true); // This may have caused the item to go into "resolved and generated" state. - if (record->resolved() && resolved_and_generated_callback_) - resolved_and_generated_callback_(record); + CheckAndTriggerWrite(record); } else if (!force) { return; // Already set and we're not required to iterate dependencies. } @@ -586,10 +585,7 @@ bool Builder::CompleteItemResolution(BuilderRecord* record, Err* err) { record->set_resolved(true); - if (record->should_generate() && record->can_write() && - resolved_and_generated_callback_) { - resolved_and_generated_callback_(record); - } + CheckAndTriggerWrite(record); // Recursively update everybody waiting on this item to be resolved. const BuilderRecordSet waiting_deps = record->waiting_on_resolution(); @@ -608,10 +604,7 @@ for (auto it = waiting_for_writing.begin(); it.valid(); ++it) { BuilderRecord* waiting = *it; if (waiting->OnResolvedValidationDep(record)) { - if (waiting->can_write() && waiting->should_generate() && - resolved_and_generated_callback_) { - resolved_and_generated_callback_(waiting); - } + CheckAndTriggerWrite(waiting); } } record->waiting_on_resolution_for_writing().clear(); @@ -726,6 +719,13 @@ return true; } +void Builder::CheckAndTriggerWrite(BuilderRecord* record) { + if (record->resolved() && record->should_generate() && record->can_write() && + resolved_and_generated_callback_) { + resolved_and_generated_callback_(record); + } +} + std::string Builder::CheckForCircularDependencies( const std::vector<const BuilderRecord*>& bad_records) const { std::vector<const BuilderRecord*> cycle;
diff --git a/src/gn/builder.h b/src/gn/builder.h index d6c42eb..927de0a 100644 --- a/src/gn/builder.h +++ b/src/gn/builder.h
@@ -138,6 +138,10 @@ bool ResolveToolchain(Target* target, Err* err); bool ResolvePools(Toolchain* toolchain, Err* err); + // Checks if the given record is ready to be written (resolved, generated, + // and writable) and calls the callback if so. + void CheckAndTriggerWrite(BuilderRecord* record); + // Given a list of unresolved records, tries to find any circular // dependencies and returns the string describing the problem. If no circular // deps were found, returns the empty string.
diff --git a/src/gn/builder_unittest.cc b/src/gn/builder_unittest.cc index e53bf29..8279061 100644 --- a/src/gn/builder_unittest.cc +++ b/src/gn/builder_unittest.cc
@@ -577,4 +577,88 @@ EXPECT_EQ(a_record, written[2]); } +// Tests that RecursiveSetShouldGenerate does not trigger a write callback +// if the target is waiting on validations (can_write() is false). +TEST_F(BuilderTest, RecursiveShouldGenerateWithValidations) { + DefineToolchain(); + SourceDir toolchain_dir = settings_.toolchain_label().dir(); + std::string toolchain_name = settings_.toolchain_label().name(); + + // Set root patterns to something that doesn't match A, B, etc. + // This ensures they are not generated by default. + std::vector<LabelPattern> dummy_patterns; + dummy_patterns.push_back( + LabelPattern(LabelPattern::MATCH, SourceDir("//c/"), "c", Label())); + build_settings_.SetRootPatterns(std::move(dummy_patterns)); + + 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); + Label e_label(SourceDir("//e/"), "e", toolchain_dir, toolchain_name); + + // Define A with validation B. + auto a = std::make_unique<Target>(&settings_, a_label); + a->set_output_type(Target::GROUP); + a->validations().push_back(LabelTargetPair(b_label)); + a->visibility().SetPublic(); + builder_.ItemDefined(std::move(a)); + BuilderRecord* a_record = builder_.GetRecord(a_label); + + // Define B with dependency E delay resolution. + auto b = std::make_unique<Target>(&settings_, b_label); + b->set_output_type(Target::ACTION); + b->private_deps().push_back(LabelTargetPair(e_label)); + b->visibility().SetPublic(); + builder_.ItemDefined(std::move(b)); + BuilderRecord* b_record = builder_.GetRecord(b_label); + + // Track written targets. + std::vector<const BuilderRecord*> written; + builder_.set_resolved_and_generated_callback( + [&written](const BuilderRecord* record) { written.push_back(record); }); + + // A resolves (validations don't block resolution). + // B waits for E. + scheduler().Run(); + + EXPECT_TRUE(a_record->resolved()); + EXPECT_FALSE(b_record->resolved()); + EXPECT_FALSE(a_record->can_write()); + EXPECT_FALSE(a_record->should_generate()); + EXPECT_TRUE(written.empty()); + + // Define C depending on A. + // C will generate because root pattern matches. + auto c = std::make_unique<Target>(&settings_, c_label); + c->set_output_type(Target::GROUP); + c->public_deps().push_back(LabelTargetPair(a_label)); + c->visibility().SetPublic(); + builder_.ItemDefined(std::move(c)); + BuilderRecord* c_record = builder_.GetRecord(c_label); + + scheduler().Run(); + + EXPECT_TRUE(c_record->should_generate()); + EXPECT_TRUE(a_record->should_generate()); + + // Only C should be written now. + std::vector<const BuilderRecord*> expected_c = {c_record}; + EXPECT_EQ(expected_c, written); + + // Define E to cause B to resolve and write A. + auto e = std::make_unique<Target>(&settings_, e_label); + e->set_output_type(Target::ACTION); + e->visibility().SetPublic(); + builder_.ItemDefined(std::move(e)); + BuilderRecord* e_record = builder_.GetRecord(e_label); + + scheduler().Run(); + + // B writes (resolved), then A writes (unblocked). + // C was already written. + std::vector<const BuilderRecord*> expected_final = {c_record, e_record, + b_record, a_record}; + EXPECT_EQ(expected_final, written); +} + } // namespace gn_builder_unittest