Fix gn meta to handle non-string metadata Before this patch, 'gn meta' will fail if the meta data is not a string. This patch fixes this problem. This patch also fixes the problem that rebase is invoked even if '-rebase-files ' flag is not used. Bug: crbug.com/gn/67 Change-Id: I0890b5dec5e7331c69c055fc8f13fe81dedbce83 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/4660 Reviewed-by: Julie Hockett <juliehockett@google.com> Commit-Queue: Haowei Wu <haowei@google.com>
diff --git a/tools/gn/command_meta.cc b/tools/gn/command_meta.cc index 0603731..bb689e9 100644 --- a/tools/gn/command_meta.cc +++ b/tools/gn/command_meta.cc
@@ -114,8 +114,15 @@ walk_keys_str, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY); Err err; std::set<const Target*> targets_walked; + SourceDir rebase_source_dir(rebase_dir); + // When SourceDir constructor is supplied with an empty string, + // a trailing slash will be added. This prevent SourceDir::is_null() + // from returning true. Explicitly remove this traling slash here. + if (rebase_dir.empty()) { + rebase_source_dir = SourceDir(); + } std::vector<Value> result = - WalkMetadata(targets, data_keys, walk_keys, SourceDir(rebase_dir), + WalkMetadata(targets, data_keys, walk_keys, rebase_source_dir, &targets_walked, &err); if (err.has_error()) { err.PrintToStdout();
diff --git a/tools/gn/metadata.cc b/tools/gn/metadata.cc index 3250b03..5091645 100644 --- a/tools/gn/metadata.cc +++ b/tools/gn/metadata.cc
@@ -28,16 +28,11 @@ if (!rebase_dir.is_null()) { for (const auto& val : iter->second.list_value()) { - if (!val.VerifyTypeIs(Value::STRING, err)) + std::pair<Value, bool> pair = + this->RebaseValue(settings, rebase_dir, val, err); + if (!pair.second) return false; - std::string filename = source_dir_.ResolveRelativeAs( - /*as_file = */ true, val, err, settings->root_path_utf8()); - if (err->has_error()) - return false; - - result->emplace_back( - val.origin(), - RebasePath(filename, rebase_dir, settings->root_path_utf8())); + result->push_back(pair.first); } } else { result->insert(result->end(), iter->second.list_value().begin(), @@ -67,3 +62,73 @@ return true; } + +std::pair<Value, bool> Metadata::RebaseValue(const BuildSettings* settings, + const SourceDir& rebase_dir, + const Value& value, + Err* err) const { + switch (value.type()) { + case Value::STRING: + return this->RebaseStringValue(settings, rebase_dir, value, err); + case Value::LIST: + return this->RebaseListValue(settings, rebase_dir, value, err); + case Value::SCOPE: + return this->RebaseScopeValue(settings, rebase_dir, value, err); + default: + return std::make_pair(value, true); + } +} + +std::pair<Value, bool> Metadata::RebaseStringValue( + const BuildSettings* settings, + const SourceDir& rebase_dir, + const Value& value, + Err* err) const { + if (!value.VerifyTypeIs(Value::STRING, err)) + return std::make_pair(value, false); + std::string filename = source_dir_.ResolveRelativeAs( + /*as_file = */ true, value, err, settings->root_path_utf8()); + if (err->has_error()) + return std::make_pair(value, false); + Value rebased_value(value.origin(), RebasePath(filename, rebase_dir, + settings->root_path_utf8())); + return std::make_pair(rebased_value, true); +} + +std::pair<Value, bool> Metadata::RebaseListValue(const BuildSettings* settings, + const SourceDir& rebase_dir, + const Value& value, + Err* err) const { + if (!value.VerifyTypeIs(Value::LIST, err)) + return std::make_pair(value, false); + + Value rebased_list_value(value.origin(), Value::LIST); + for (auto& val : value.list_value()) { + std::pair<Value, bool> pair = RebaseValue(settings, rebase_dir, val, err); + if (!pair.second) + return std::make_pair(value, false); + rebased_list_value.list_value().push_back(pair.first); + } + return std::make_pair(rebased_list_value, true); +} + +std::pair<Value, bool> Metadata::RebaseScopeValue(const BuildSettings* settings, + const SourceDir& rebase_dir, + const Value& value, + Err* err) const { + if (!value.VerifyTypeIs(Value::SCOPE, err)) + return std::make_pair(value, false); + Value rebased_scope_value(value); + Scope::KeyValueMap scope_values; + value.scope_value()->GetCurrentScopeValues(&scope_values); + for (auto& value_pair : scope_values) { + std::pair<Value, bool> pair = + RebaseValue(settings, rebase_dir, value_pair.second, err); + if (!pair.second) + return std::make_pair(value, false); + + rebased_scope_value.scope_value()->SetValue(value_pair.first, pair.first, + value.origin()); + } + return std::make_pair(rebased_scope_value, true); +} \ No newline at end of file
diff --git a/tools/gn/metadata.h b/tools/gn/metadata.h index da79fdf..3c16d27 100644 --- a/tools/gn/metadata.h +++ b/tools/gn/metadata.h
@@ -63,6 +63,26 @@ Contents contents_; SourceDir source_dir_; + std::pair<Value, bool> RebaseValue(const BuildSettings* settings, + const SourceDir& rebase_dir, + const Value& value, + Err* err) const; + + std::pair<Value, bool> RebaseStringValue(const BuildSettings* settings, + const SourceDir& rebase_dir, + const Value& value, + Err* err) const; + + std::pair<Value, bool> RebaseListValue(const BuildSettings* settings, + const SourceDir& rebase_dir, + const Value& value, + Err* err) const; + + std::pair<Value, bool> RebaseScopeValue(const BuildSettings* settings, + const SourceDir& rebase_dir, + const Value& value, + Err* err) const; + DISALLOW_COPY_AND_ASSIGN(Metadata); };
diff --git a/tools/gn/metadata_unittest.cc b/tools/gn/metadata_unittest.cc index b159372..901e539 100644 --- a/tools/gn/metadata_unittest.cc +++ b/tools/gn/metadata_unittest.cc
@@ -99,29 +99,58 @@ EXPECT_EQ(results, expected); } -TEST(MetadataTest, WalkWithRebaseError) { +TEST(MetadataTest, WalkWithRebaseNonString) { TestWithScope setup; Metadata metadata; metadata.set_source_dir(SourceDir("/usr/home/files/")); - Value a_expected(nullptr, Value::LIST); - a_expected.list_value().push_back(Value(nullptr, "foo.cpp")); - a_expected.list_value().push_back(Value(nullptr, true)); + Value a(nullptr, Value::LIST); + Value inner_list(nullptr, Value::LIST); + Value inner_scope(nullptr, Value::SCOPE); + inner_list.list_value().push_back(Value(nullptr, "foo.cpp")); + inner_list.list_value().push_back(Value(nullptr, "foo/bar.h")); + a.list_value().push_back(inner_list); - metadata.contents().insert( - std::pair<base::StringPiece, Value>("a", a_expected)); + std::unique_ptr<Scope> scope(new Scope(setup.settings())); + scope->SetValue("a1", Value(nullptr, "foo2.cpp"), nullptr); + scope->SetValue("a2", Value(nullptr, "foo/bar2.h"), nullptr); + inner_scope.SetScopeValue(std::move(scope)); + a.list_value().push_back(inner_scope); + metadata.contents().insert(std::pair<base::StringPiece, Value>("a", a)); std::vector<std::string> data_keys; data_keys.emplace_back("a"); std::vector<std::string> walk_keys; std::vector<Value> next_walk_keys; std::vector<Value> results; + std::vector<Value> expected; + Value inner_list_expected(nullptr, Value::LIST); + Value inner_scope_expected(nullptr, Value::SCOPE); + inner_list_expected.list_value().push_back( + Value(nullptr, "../home/files/foo.cpp")); + inner_list_expected.list_value().push_back( + Value(nullptr, "../home/files/foo/bar.h")); + expected.push_back(inner_list_expected); + + std::unique_ptr<Scope> scope_expected(new Scope(setup.settings())); + scope_expected->SetValue("a1", Value(nullptr, "../home/files/foo2.cpp"), + nullptr); + scope_expected->SetValue("a2", Value(nullptr, "../home/files/foo/bar2.h"), + nullptr); + inner_scope_expected.SetScopeValue(std::move(scope_expected)); + expected.push_back(inner_scope_expected); + + std::vector<Value> expected_walk_keys; + expected_walk_keys.emplace_back(nullptr, ""); + Err err; - EXPECT_FALSE(metadata.WalkStep(setup.settings()->build_settings(), data_keys, - walk_keys, SourceDir("/foo_dir/"), - &next_walk_keys, &results, &err)); - EXPECT_TRUE(err.has_error()); + EXPECT_TRUE(metadata.WalkStep(setup.settings()->build_settings(), data_keys, + walk_keys, SourceDir("/usr/foo_dir/"), + &next_walk_keys, &results, &err)); + EXPECT_FALSE(err.has_error()); + EXPECT_EQ(next_walk_keys, expected_walk_keys); + EXPECT_EQ(results, expected); } TEST(MetadataTest, WalkKeysToWalk) {