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) {