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.
