Fix analyze not noticing when BUILD.gn files change The dependency tracking for template scopes was not taking into account invoker scopes. So, if a BUILD.gn file changed, targets defined in it through templates were not being considered "stale" by analyze. Bug: chromium/999002 Change-Id: Ibd0a82308e5ffc99e9856e628ee0c242b2890350 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/7660 Reviewed-by: Brett Wilson <brettw@chromium.org> Commit-Queue: Brett Wilson <brettw@chromium.org>
diff --git a/src/gn/input_file_manager.cc b/src/gn/input_file_manager.cc index f529990..1c1fe01 100644 --- a/src/gn/input_file_manager.cc +++ b/src/gn/input_file_manager.cc
@@ -36,6 +36,7 @@ bool DoLoadFile(const LocationRange& origin, const BuildSettings* build_settings, const SourceFile& name, + InputFileManager::SyncLoadFileCallback load_file_callback, InputFile* file, std::vector<Token>* tokens, std::unique_ptr<ParseNode>* root, @@ -52,7 +53,13 @@ // Read. base::FilePath primary_path = build_settings->GetFullPath(name); ScopedTrace load_trace(TraceItem::TRACE_FILE_LOAD, name.value()); - if (!file->Load(primary_path)) { + if (load_file_callback) { + if (!load_file_callback(name, file)) { + *err = Err(origin, "Can't load input file.", + "File not mocked by load_file_callback:\n " + name.value()); + return false; + } + } else if (!file->Load(primary_path)) { if (!build_settings->secondary_source_path().empty()) { // Fall back to secondary source tree. base::FilePath secondary_path = @@ -283,7 +290,7 @@ std::vector<Token> tokens; std::unique_ptr<ParseNode> root; bool success = - DoLoadFile(origin, build_settings, name, file, &tokens, &root, err); + DoLoadFile(origin, build_settings, name, load_file_callback_, file, &tokens, &root, err); // Can't return early. We have to ensure that the completion event is // signaled in all cases bacause another thread could be blocked on this one.
diff --git a/src/gn/input_file_manager.h b/src/gn/input_file_manager.h index c3633e8..f5ac565 100644 --- a/src/gn/input_file_manager.h +++ b/src/gn/input_file_manager.h
@@ -40,6 +40,10 @@ // refer to the root block of the file. On failure, this will be NULL. using FileLoadCallback = std::function<void(const ParseNode*)>; + // Callback to emulate SyncLoadFile in tests. + using SyncLoadFileCallback = + std::function<bool(const SourceFile& file_name, InputFile* file)>; + InputFileManager(); // Loads the given file and executes the callback on the worker pool. @@ -90,6 +94,10 @@ // Fills the vector with all input files. void GetAllPhysicalInputFileNames(std::vector<base::FilePath>* result) const; + void set_load_file_callback(SyncLoadFileCallback load_file_callback) { + load_file_callback_ = load_file_callback; + } + private: friend class base::RefCountedThreadSafe<InputFileManager>; @@ -149,6 +157,9 @@ // See AddDynamicInput(). std::vector<std::unique_ptr<InputFileData>> dynamic_inputs_; + // Used by unit tests to mock out SyncLoadFile(). + SyncLoadFileCallback load_file_callback_; + DISALLOW_COPY_AND_ASSIGN(InputFileManager); };
diff --git a/src/gn/loader_unittest.cc b/src/gn/loader_unittest.cc index 3bd598b..3896b59 100644 --- a/src/gn/loader_unittest.cc +++ b/src/gn/loader_unittest.cc
@@ -54,9 +54,10 @@ public: using Callback = std::function<void(const ParseNode*)>; - MockInputFileManager() = default; + MockInputFileManager(); + ~MockInputFileManager(); - LoaderImpl::AsyncLoadFileCallback GetCallback(); + LoaderImpl::AsyncLoadFileCallback GetAsyncCallback(); // Sets a given response for a given source file. void AddCannedResponse(const SourceFile& source_file, @@ -75,6 +76,8 @@ std::unique_ptr<ParseNode> root; }; + InputFileManager::SyncLoadFileCallback GetSyncCallback(); + bool AsyncLoadFile(const LocationRange& origin, const BuildSettings* build_settings, const SourceFile& file_name, @@ -90,7 +93,15 @@ std::vector<std::pair<SourceFile, Callback>> pending_; }; -LoaderImpl::AsyncLoadFileCallback MockInputFileManager::GetCallback() { +MockInputFileManager::MockInputFileManager() { + g_scheduler->input_file_manager()->set_load_file_callback(GetSyncCallback()); +} + +MockInputFileManager::~MockInputFileManager() { + g_scheduler->input_file_manager()->set_load_file_callback(nullptr); +} + +LoaderImpl::AsyncLoadFileCallback MockInputFileManager::GetAsyncCallback() { return [this](const LocationRange& origin, const BuildSettings* build_settings, const SourceFile& file_name, const Callback& callback, Err* err) { @@ -98,6 +109,17 @@ }; } +InputFileManager::SyncLoadFileCallback MockInputFileManager::GetSyncCallback() { + return + [this](const SourceFile& file_name, InputFile* file) { + CannedResponseMap::const_iterator found = canned_responses_.find(file_name); + if (found == canned_responses_.end()) + return false; + file->SetContents(found->second->input_file->contents()); + return true; + }; +} + // Sets a given response for a given source file. void MockInputFileManager::AddCannedResponse(const SourceFile& source_file, const std::string& source) { @@ -112,6 +134,8 @@ // Parse. canned->root = Parser::Parse(canned->tokens, &err); + if (err.has_error()) + err.PrintToStdout(); EXPECT_FALSE(err.has_error()); canned_responses_[source_file] = std::move(canned); @@ -167,7 +191,7 @@ mock_ifm_.AddCannedResponse(build_config, "set_default_toolchain(\"//tc:tc\")"); - loader->set_async_load_file(mock_ifm_.GetCallback()); + loader->set_async_load_file(mock_ifm_.GetAsyncCallback()); // Request the root build file be loaded. This should kick off the default // build config loading. @@ -228,7 +252,6 @@ scoped_refptr<LoaderImpl> loader(new LoaderImpl(&build_settings_)); mock_ifm_.AddCannedResponse(build_config, "set_default_toolchain(\"//tc:tc\")"); - mock_ifm_.AddCannedResponse(SourceFile("//test.gni"), "concurrent_jobs = 1"); std::string root_build_content = "executable(\"a\") { sources = [ \"a.cc\" ] }\n" "config(\"b\") { configs = [\"//t:t\"] }\n" @@ -236,7 +259,7 @@ "pool(\"d\") { depth = 1 }"; mock_ifm_.AddCannedResponse(root_build, root_build_content); - loader->set_async_load_file(mock_ifm_.GetCallback()); + loader->set_async_load_file(mock_ifm_.GetAsyncCallback()); // Request the root build file be loaded. This should kick off the default // build config loading. @@ -265,3 +288,50 @@ EXPECT_FALSE(scheduler().is_failed()); } + +TEST_F(LoaderTest, TemplateBuildDependencyFilesAreCollected) { + SourceFile build_config("//build/config/BUILDCONFIG.gn"); + SourceFile root_build("//BUILD.gn"); + build_settings_.set_build_config_file(build_config); + build_settings_.set_item_defined_callback( + [builder = &mock_builder_](std::unique_ptr<Item> item) { + builder->OnItemDefined(std::move(item)); + }); + + scoped_refptr<LoaderImpl> loader(new LoaderImpl(&build_settings_)); + mock_ifm_.AddCannedResponse(build_config, + "set_default_toolchain(\"//tc:tc\")"); + mock_ifm_.AddCannedResponse( + SourceFile("//test.gni"), + "template(\"tmpl\") {\n" + " executable(target_name) { sources = invoker.sources }\n" + "}\n"); + mock_ifm_.AddCannedResponse(root_build, + "import(\"//test.gni\")\n" + "tmpl(\"a\") {sources = [ \"a.cc\" ]}\n"); + + loader->set_async_load_file(mock_ifm_.GetAsyncCallback()); + + // Request the root build file be loaded. This should kick off the default + // build config loading. + loader->Load(root_build, LocationRange(), Label()); + EXPECT_TRUE(mock_ifm_.HasOnePending(build_config)); + + // Completing the build config load should kick off the root build file load. + mock_ifm_.IssueAllPending(); + MsgLoop::Current()->RunUntilIdleForTesting(); + EXPECT_TRUE(mock_ifm_.HasOnePending(root_build)); + + // Completing the root build file should define a target which must have + // set of source files hashes. + mock_ifm_.IssueAllPending(); + MsgLoop::Current()->RunUntilIdleForTesting(); + + std::vector<const Item*> items = mock_builder_.GetAllItems(); + EXPECT_TRUE(items[0]->AsTarget()); + // Ensure the target as a dep on BUILD.gn even though it was defined via + // a template. + EXPECT_TRUE(ItemContainsBuildDependencyFile(items[0], root_build)); + + EXPECT_FALSE(scheduler().is_failed()); +}
diff --git a/src/gn/scope.cc b/src/gn/scope.cc index adbfa9f..a220c8e 100644 --- a/src/gn/scope.cc +++ b/src/gn/scope.cc
@@ -435,8 +435,7 @@ } // Propogate build dependency files, - dest->build_dependency_files_.insert(build_dependency_files_.begin(), - build_dependency_files_.end()); + dest->AddBuildDependencyFiles(build_dependency_files_); return true; } @@ -540,6 +539,12 @@ build_dependency_files_.insert(build_dependency_file); } +void Scope::AddBuildDependencyFiles( + const SourceFileSet& build_dependency_files) { + build_dependency_files_.insert(build_dependency_files.begin(), + build_dependency_files.end()); +} + Scope::ItemVector* Scope::GetItemCollector() { if (item_collector_) return item_collector_;
diff --git a/src/gn/scope.h b/src/gn/scope.h index c5dcd38..e800621 100644 --- a/src/gn/scope.h +++ b/src/gn/scope.h
@@ -293,6 +293,7 @@ return build_dependency_files_; } void AddBuildDependencyFile(const SourceFile& build_dependency_file); + void AddBuildDependencyFiles(const SourceFileSet& build_dependency_files); // The item collector is where Items (Targets, Configs, etc.) go that have // been defined. If a scope can generate items, this non-owning pointer will
diff --git a/src/gn/template.cc b/src/gn/template.cc index 3a65122..d5b2ecd 100644 --- a/src/gn/template.cc +++ b/src/gn/template.cc
@@ -64,6 +64,11 @@ Scope template_scope(closure_.get()); template_scope.set_source_dir(scope->GetSourceDir()); + // Propogate build dependency files from invoker scope (template scope already + // propagated via parent scope). + template_scope.AddBuildDependencyFiles( + invocation_scope->build_dependency_files()); + ScopePerFileProvider per_file_provider(&template_scope, true); // Targets defined in the template go in the collector for the invoking file.