GN: Make file/dir resolving return errors. Previously GN would silently eat some types of errors when resolving files or directory names, leading to unexpected behavior. This adds error checking to the resolution functions. Review URL: https://codereview.chromium.org/1155713006 Cr-Original-Commit-Position: refs/heads/master@{#332697} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: ff0986feacdae709a944b7cc6ee990b6cc304b76
diff --git a/tools/gn/action_target_generator.cc b/tools/gn/action_target_generator.cc index a1ab3a3..3cd4e47 100644 --- a/tools/gn/action_target_generator.cc +++ b/tools/gn/action_target_generator.cc
@@ -76,12 +76,11 @@ return false; SourceFile script_file = - scope_->GetSourceDir().ResolveRelativeFile(value->string_value(), + scope_->GetSourceDir().ResolveRelativeFile( + *value, err_, scope_->settings()->build_settings()->root_path_utf8()); - if (script_file.value().empty()) { - *err_ = Err(*value, "script name is empty"); + if (err_->has_error()) return false; - } target_->action_values().set_script(script_file); return true; }
diff --git a/tools/gn/command_format.cc b/tools/gn/command_format.cc index 7aa8749..a8143ff 100644 --- a/tools/gn/command_format.cc +++ b/tools/gn/command_format.cc
@@ -1012,7 +1012,14 @@ Setup setup; SourceDir source_dir = SourceDirForCurrentDirectory(setup.build_settings().root_path()); - SourceFile file = source_dir.ResolveRelativeFile(args[0]); + + Err err; + SourceFile file = source_dir.ResolveRelativeFile(Value(nullptr, args[0]), + &err); + if (err.has_error()) { + err.PrintToStdout(); + return 1; + } std::string output_string; if (FormatFileToString(&setup, file, dump_tree, &output_string)) {
diff --git a/tools/gn/commands.cc b/tools/gn/commands.cc index 556f75f..ab22ebe 100644 --- a/tools/gn/commands.cc +++ b/tools/gn/commands.cc
@@ -90,8 +90,13 @@ Value(nullptr, input), &err); if (err.has_error()) { // Not a valid label, assume this must be a file. + err = Err(); file_matches->push_back(current_dir.ResolveRelativeFile( - input, setup->build_settings().root_path_utf8())); + Value(nullptr, input), &err, setup->build_settings().root_path_utf8())); + if (err.has_error()) { + err.PrintToStdout(); + return false; + } return true; } @@ -106,7 +111,11 @@ } else { // Not an item, assume this must be a file. file_matches->push_back(current_dir.ResolveRelativeFile( - input, setup->build_settings().root_path_utf8())); + Value(nullptr, input), &err, setup->build_settings().root_path_utf8())); + if (err.has_error()) { + err.PrintToStdout(); + return false; + } } return true;
diff --git a/tools/gn/filesystem_utils_unittest.cc b/tools/gn/filesystem_utils_unittest.cc index 9f874eb..14bc951 100644 --- a/tools/gn/filesystem_utils_unittest.cc +++ b/tools/gn/filesystem_utils_unittest.cc
@@ -236,6 +236,11 @@ input = "foo\\..\\..\\bar"; NormalizePath(&input); EXPECT_EQ("../bar", input); + + // Trailing slashes should get preserved. + input = "//foo/bar/"; + NormalizePath(&input); + EXPECT_EQ("//foo/bar/", input); } TEST(FilesystemUtils, RebasePath) {
diff --git a/tools/gn/function_exec_script.cc b/tools/gn/function_exec_script.cc index 6bc39a6..d5e66ae 100644 --- a/tools/gn/function_exec_script.cc +++ b/tools/gn/function_exec_script.cc
@@ -133,11 +133,11 @@ return Value(); // Find the python script to run. - if (!args[0].VerifyTypeIs(Value::STRING, err)) - return Value(); SourceFile script_source = - cur_dir.ResolveRelativeFile(args[0].string_value(), + cur_dir.ResolveRelativeFile(args[0], err, scope->settings()->build_settings()->root_path_utf8()); + if (err->has_error()) + return Value(); base::FilePath script_path = build_settings->GetFullPath(script_source); if (!build_settings->secondary_source_path().empty() && !base::PathExists(script_path)) { @@ -161,8 +161,10 @@ return Value(); g_scheduler->AddGenDependency( build_settings->GetFullPath(cur_dir.ResolveRelativeFile( - dep.string_value(), + dep, err, scope->settings()->build_settings()->root_path_utf8()))); + if (err->has_error()) + return Value(); } }
diff --git a/tools/gn/function_get_path_info.cc b/tools/gn/function_get_path_info.cc index a3f2c10..e79afca 100644 --- a/tools/gn/function_get_path_info.cc +++ b/tools/gn/function_get_path_info.cc
@@ -28,15 +28,19 @@ // |current_dir|), regardless of whether the input is a directory or a file. SourceDir DirForInput(const Settings* settings, const SourceDir& current_dir, - const std::string& input_string) { + const Value& input, + Err* err) { + // Input should already have been validated as a string. + const std::string& input_string = input.string_value(); + if (!input_string.empty() && input_string[input_string.size() - 1] == '/') { // Input is a directory. - return current_dir.ResolveRelativeDir(input_string, + return current_dir.ResolveRelativeDir(input, err, settings->build_settings()->root_path_utf8()); } // Input is a directory. - return current_dir.ResolveRelativeFile(input_string, + return current_dir.ResolveRelativeFile(input, err, settings->build_settings()->root_path_utf8()).GetDir(); } @@ -85,21 +89,21 @@ return DirectoryWithNoLastSlash( GetGenDirForSourceDir(settings, DirForInput(settings, current_dir, - input_string))); + input, err))); } case WHAT_OUT_DIR: { return DirectoryWithNoLastSlash( GetOutputDirForSourceDir(settings, DirForInput(settings, current_dir, - input_string))); + input, err))); } case WHAT_ABSPATH: { if (!input_string.empty() && input_string[input_string.size() - 1] == '/') { - return current_dir.ResolveRelativeDir(input_string, + return current_dir.ResolveRelativeDir(input, err, settings->build_settings()->root_path_utf8()).value(); } else { - return current_dir.ResolveRelativeFile(input_string, + return current_dir.ResolveRelativeFile(input, err, settings->build_settings()->root_path_utf8()).value(); } }
diff --git a/tools/gn/function_read_file.cc b/tools/gn/function_read_file.cc index 5a7662a..11585d8 100644 --- a/tools/gn/function_read_file.cc +++ b/tools/gn/function_read_file.cc
@@ -53,8 +53,10 @@ // Compute the file name. const SourceDir& cur_dir = scope->GetSourceDir(); - SourceFile source_file = cur_dir.ResolveRelativeFile(args[0].string_value(), + SourceFile source_file = cur_dir.ResolveRelativeFile(args[0], err, scope->settings()->build_settings()->root_path_utf8()); + if (err->has_error()) + return Value(); base::FilePath file_path = scope->settings()->build_settings()->GetFullPath(source_file);
diff --git a/tools/gn/function_rebase_path.cc b/tools/gn/function_rebase_path.cc index 0cc3252..ffe4eae 100644 --- a/tools/gn/function_rebase_path.cc +++ b/tools/gn/function_rebase_path.cc
@@ -71,13 +71,16 @@ base::FilePath system_path; if (looks_like_dir) { system_path = scope->settings()->build_settings()->GetFullPath( - from_dir.ResolveRelativeDir(string_value, + from_dir.ResolveRelativeDir(value, err, scope->settings()->build_settings()->root_path_utf8())); } else { system_path = scope->settings()->build_settings()->GetFullPath( - from_dir.ResolveRelativeFile(string_value, + from_dir.ResolveRelativeFile(value, err, scope->settings()->build_settings()->root_path_utf8())); } + if (err->has_error()) + return Value(); + result = Value(function, FilePathToUTF8(system_path)); if (looks_like_dir) MakeSlashEndingMatchInput(string_value, &result.string_value()); @@ -87,17 +90,19 @@ result = Value(function, Value::STRING); if (looks_like_dir) { result.string_value() = RebasePath( - from_dir.ResolveRelativeDir(string_value, + from_dir.ResolveRelativeDir(value, err, scope->settings()->build_settings()->root_path_utf8()).value(), to_dir, scope->settings()->build_settings()->root_path_utf8()); MakeSlashEndingMatchInput(string_value, &result.string_value()); } else { result.string_value() = RebasePath( - from_dir.ResolveRelativeFile(string_value, + from_dir.ResolveRelativeFile(value, err, scope->settings()->build_settings()->root_path_utf8()).value(), to_dir, scope->settings()->build_settings()->root_path_utf8()); + if (err->has_error()) + return Value(); } return result; @@ -224,8 +229,10 @@ return result; if (!args[kArgIndexDest].string_value().empty()) { to_dir = current_dir.ResolveRelativeDir( - args[kArgIndexDest].string_value(), + args[kArgIndexDest], err, scope->settings()->build_settings()->root_path_utf8()); + if (err->has_error()) + return Value(); convert_to_system_absolute = false; } } @@ -236,8 +243,10 @@ if (!args[kArgIndexFrom].VerifyTypeIs(Value::STRING, err)) return result; from_dir = current_dir.ResolveRelativeDir( - args[kArgIndexFrom].string_value(), + args[kArgIndexFrom], err, scope->settings()->build_settings()->root_path_utf8()); + if (err->has_error()) + return Value(); } else { // Default to current directory if unspecified. from_dir = current_dir;
diff --git a/tools/gn/function_write_file.cc b/tools/gn/function_write_file.cc index 9f853f8..8a6fee3 100644 --- a/tools/gn/function_write_file.cc +++ b/tools/gn/function_write_file.cc
@@ -100,11 +100,11 @@ } // Compute the file name and make sure it's in the output dir. - if (!args[0].VerifyTypeIs(Value::STRING, err)) - return Value(); const SourceDir& cur_dir = scope->GetSourceDir(); - SourceFile source_file = cur_dir.ResolveRelativeFile(args[0].string_value(), + SourceFile source_file = cur_dir.ResolveRelativeFile(args[0], err, scope->settings()->build_settings()->root_path_utf8()); + if (err->has_error()) + return Value(); if (!EnsureStringIsInOutputDir( scope->settings()->build_settings()->build_dir(), source_file.value(), args[0].origin(), err))
diff --git a/tools/gn/functions.cc b/tools/gn/functions.cc index b7e1335..c99075a 100644 --- a/tools/gn/functions.cc +++ b/tools/gn/functions.cc
@@ -491,10 +491,12 @@ const SourceDir& input_dir = scope->GetSourceDir(); SourceFile import_file = - input_dir.ResolveRelativeFile(args[0].string_value(), + input_dir.ResolveRelativeFile(args[0], err, scope->settings()->build_settings()->root_path_utf8()); - scope->settings()->import_manager().DoImport(import_file, function, - scope, err); + if (!err->has_error()) { + scope->settings()->import_manager().DoImport(import_file, function, + scope, err); + } return Value(); }
diff --git a/tools/gn/label.cc b/tools/gn/label.cc index 89cd335..60ebf31 100644 --- a/tools/gn/label.cc +++ b/tools/gn/label.cc
@@ -37,7 +37,7 @@ return true; } - *result = current_dir.ResolveRelativeDir(input); + *result = current_dir.ResolveRelativeDir(input_value, input, err); return true; }
diff --git a/tools/gn/label_pattern.cc b/tools/gn/label_pattern.cc index 712826f..e5ea5cf 100644 --- a/tools/gn/label_pattern.cc +++ b/tools/gn/label_pattern.cc
@@ -183,13 +183,9 @@ } // Resolve the non-wildcard stuff. - dir = current_dir.ResolveRelativeDir(path); - if (dir.is_null()) { - *err = Err(value, "Label pattern didn't resolve to a dir.", - "The directory name \"" + path.as_string() + "\" didn't\n" - "resolve to a directory."); + dir = current_dir.ResolveRelativeDir(value, path, err); + if (err->has_error()) return LabelPattern(); - } } // Resolve the name. At this point, we're doing wildcard matches so the
diff --git a/tools/gn/setup.cc b/tools/gn/setup.cc index d8d525a..79c46e7 100644 --- a/tools/gn/setup.cc +++ b/tools/gn/setup.cc
@@ -447,13 +447,13 @@ } bool Setup::FillBuildDir(const std::string& build_dir, bool require_exists) { + Err err; SourceDir resolved = SourceDirForCurrentDirectory(build_settings_.root_path()). - ResolveRelativeDir(build_dir, build_settings_.root_path_utf8()); - if (resolved.is_null()) { - Err(Location(), "Couldn't resolve build directory.", - "The build directory supplied (\"" + build_dir + "\") was not valid."). - PrintToStdout(); + ResolveRelativeDir(Value(nullptr, build_dir), &err, + build_settings_.root_path_utf8()); + if (err.has_error()) { + err.PrintToStdout(); return false; } @@ -622,7 +622,11 @@ err.PrintToStdout(); return false; } - whitelist->insert(current_dir.ResolveRelativeFile(item.string_value())); + whitelist->insert(current_dir.ResolveRelativeFile(item, &err)); + if (err.has_error()) { + err.PrintToStdout(); + return false; + } } build_settings_.set_exec_script_whitelist(whitelist.Pass()); }
diff --git a/tools/gn/source_dir.cc b/tools/gn/source_dir.cc index 8798b4d..0966cc6 100644 --- a/tools/gn/source_dir.cc +++ b/tools/gn/source_dir.cc
@@ -18,7 +18,7 @@ #else DCHECK(s[0] == '/'); #endif - DCHECK(EndsWithSlash(s)); + DCHECK(EndsWithSlash(s)) << s; } } @@ -45,32 +45,44 @@ } SourceFile SourceDir::ResolveRelativeFile( - const base::StringPiece& p, + const Value& p, + Err* err, const base::StringPiece& source_root) const { SourceFile ret; - - DCHECK(source_root.empty() || !source_root.ends_with("/")); + if (!p.VerifyTypeIs(Value::STRING, err)) + return ret; // It's an error to resolve an empty string or one that is a directory // (indicated by a trailing slash) because this is the function that expects // to return a file. - if (p.empty() || (p.size() > 0 && p[p.size() - 1] == '/')) - return SourceFile(); - if (p.size() >= 2 && p[0] == '/' && p[1] == '/') { + const std::string& str = p.string_value(); + if (str.empty()) { + *err = Err(p, "Empty file path.", + "You can't use empty strings as file paths. That's just wrong."); + return ret; + } else if (str[str.size() - 1] == '/') { + *err = Err(p, "File path ends in a slash.", + "You specified the path\n " + str + "\n" + "and it ends in a slash, indicating you think it's a directory." + "\nBut here you're supposed to be listing a file."); + return ret; + } + + if (str.size() >= 2 && str[0] == '/' && str[1] == '/') { // Source-relative. - ret.value_.assign(p.data(), p.size()); + ret.value_.assign(str.data(), str.size()); NormalizePath(&ret.value_); return ret; - } else if (IsPathAbsolute(p)) { + } else if (IsPathAbsolute(str)) { if (source_root.empty() || - !MakeAbsolutePathRelativeIfPossible(source_root, p, &ret.value_)) { + !MakeAbsolutePathRelativeIfPossible(source_root, str, &ret.value_)) { #if defined(OS_WIN) // On Windows we'll accept "C:\foo" as an absolute path, which we want // to convert to "/C:..." here. - if (p[0] != '/') + if (str[0] != '/') ret.value_ = "/"; #endif - ret.value_.append(p.data(), p.size()); + ret.value_.append(str.data(), str.size()); } NormalizePath(&ret.value_); return ret; @@ -79,7 +91,7 @@ if (!source_root.empty()) { std::string absolute = FilePathToUTF8(Resolve(UTF8ToFilePath(source_root)).AppendASCII( - p.as_string()).value()); + str).value()); NormalizePath(&absolute); if (!MakeAbsolutePathRelativeIfPossible(source_root, absolute, &ret.value_)) { @@ -97,38 +109,52 @@ // With no source_root_, there's nothing we can do about // e.g. p=../../../path/to/file and value_=//source and we'll // errornously return //file. - ret.value_.reserve(value_.size() + p.size()); + ret.value_.reserve(value_.size() + str.size()); ret.value_.assign(value_); - ret.value_.append(p.data(), p.size()); + ret.value_.append(str.data(), str.size()); NormalizePath(&ret.value_); return ret; } SourceDir SourceDir::ResolveRelativeDir( - const base::StringPiece& p, + const Value& p, + Err* err, + const base::StringPiece& source_root) const { + if (!p.VerifyTypeIs(Value::STRING, err)) + return SourceDir(); + return ResolveRelativeDir(p, p.string_value(), err, source_root); +} + +SourceDir SourceDir::ResolveRelativeDir( + const Value& blame_but_dont_use, + const base::StringPiece& str, + Err* err, const base::StringPiece& source_root) const { SourceDir ret; - DCHECK(source_root.empty() || !source_root.ends_with("/")); - - if (p.empty()) + if (str.empty()) { + *err = Err(blame_but_dont_use, "Empty directory path.", + "You can't use empty strings as directories. " + "That's just wrong."); return ret; - if (p.size() >= 2 && p[0] == '/' && p[1] == '/') { + } + + if (str.size() >= 2 && str[0] == '/' && str[1] == '/') { // Source-relative. - ret.value_.assign(p.data(), p.size()); + ret.value_.assign(str.data(), str.size()); if (!EndsWithSlash(ret.value_)) ret.value_.push_back('/'); NormalizePath(&ret.value_); return ret; - } else if (IsPathAbsolute(p)) { + } else if (IsPathAbsolute(str)) { if (source_root.empty() || - !MakeAbsolutePathRelativeIfPossible(source_root, p, &ret.value_)) { + !MakeAbsolutePathRelativeIfPossible(source_root, str, &ret.value_)) { #if defined(OS_WIN) - if (p[0] != '/') // See the file case for why we do this check. + if (str[0] != '/') // See the file case for why we do this check. ret.value_ = "/"; #endif - ret.value_.append(p.data(), p.size()); + ret.value_.append(str.data(), str.size()); } NormalizePath(&ret.value_); if (!EndsWithSlash(ret.value_)) @@ -139,7 +165,7 @@ if (!source_root.empty()) { std::string absolute = FilePathToUTF8(Resolve(UTF8ToFilePath(source_root)).AppendASCII( - p.as_string()).value()); + str.as_string()).value()); NormalizePath(&absolute); if (!MakeAbsolutePathRelativeIfPossible(source_root, absolute, &ret.value_)) { @@ -154,9 +180,9 @@ return ret; } - ret.value_.reserve(value_.size() + p.size()); + ret.value_.reserve(value_.size() + str.size()); ret.value_.assign(value_); - ret.value_.append(p.data(), p.size()); + ret.value_.append(str.data(), str.size()); NormalizePath(&ret.value_); if (!EndsWithSlash(ret.value_))
diff --git a/tools/gn/source_dir.h b/tools/gn/source_dir.h index 755e092..43f7e53 100644 --- a/tools/gn/source_dir.h +++ b/tools/gn/source_dir.h
@@ -13,7 +13,9 @@ #include "base/logging.h" #include "base/strings/string_piece.h" +class Err; class SourceFile; +class Value; // Represents a directory within the source tree. Source dirs begin and end in // slashes. @@ -34,18 +36,28 @@ ~SourceDir(); // Resolves a file or dir name relative to this source directory. Will return - // an empty SourceDir/File on error. Empty input is always an error (it's - // possible we should say ResolveRelativeDir vs. an empty string should be - // the source dir, but we require "." instead). + // an empty SourceDir/File on error and set the give *err pointer (required). + // Empty input is always an error. // // If source_root is supplied, these functions will additionally handle the // case where the input is a system-absolute but still inside the source // tree. This is the case for some external tools. SourceFile ResolveRelativeFile( - const base::StringPiece& p, + const Value& p, + Err* err, const base::StringPiece& source_root = base::StringPiece()) const; SourceDir ResolveRelativeDir( + const Value& p, + Err* err, + const base::StringPiece& source_root = base::StringPiece()) const; + + // Like ResolveRelativeDir but takes a separate value (which gets blamed) + // and string to use (in cases where a substring has been extracted from the + // value, as with label resolution). + SourceDir ResolveRelativeDir( + const Value& blame_but_dont_use, const base::StringPiece& p, + Err* err, const base::StringPiece& source_root = base::StringPiece()) const; // Resolves this source file relative to some given source root. Returns
diff --git a/tools/gn/source_dir_unittest.cc b/tools/gn/source_dir_unittest.cc index 5034335..04684e6 100644 --- a/tools/gn/source_dir_unittest.cc +++ b/tools/gn/source_dir_unittest.cc
@@ -3,10 +3,13 @@ // found in the LICENSE file. #include "testing/gtest/include/gtest/gtest.h" +#include "tools/gn/err.h" #include "tools/gn/source_dir.h" #include "tools/gn/source_file.h" +#include "tools/gn/value.h" TEST(SourceDir, ResolveRelativeFile) { + Err err; SourceDir base("//base/"); #if defined(OS_WIN) base::StringPiece source_root("C:/source/root"); @@ -15,48 +18,69 @@ #endif // Empty input is an error. - EXPECT_TRUE(base.ResolveRelativeFile("", source_root) == SourceFile()); + EXPECT_TRUE(base.ResolveRelativeFile( + Value(nullptr, std::string()), &err, source_root) == SourceFile()); + EXPECT_TRUE(err.has_error()); // These things are directories, so should be an error. - EXPECT_TRUE(base.ResolveRelativeFile("//foo/bar/", source_root) == - SourceFile()); - EXPECT_TRUE(base.ResolveRelativeFile("bar/", source_root) == - SourceFile()); + err = Err(); + EXPECT_TRUE(base.ResolveRelativeFile( + Value(nullptr, "//foo/bar/"), &err, source_root) == SourceFile()); + EXPECT_TRUE(err.has_error()); + + err = Err(); + EXPECT_TRUE(base.ResolveRelativeFile( + Value(nullptr, "bar/"), &err, source_root) == SourceFile()); + EXPECT_TRUE(err.has_error()); // Absolute paths should be passed unchanged. - EXPECT_TRUE(base.ResolveRelativeFile("//foo",source_root) == - SourceFile("//foo")); - EXPECT_TRUE(base.ResolveRelativeFile("/foo", source_root) == - SourceFile("/foo")); + err = Err(); + EXPECT_TRUE(base.ResolveRelativeFile( + Value(nullptr, "//foo"), &err, source_root) == SourceFile("//foo")); + EXPECT_FALSE(err.has_error()); + + EXPECT_TRUE(base.ResolveRelativeFile( + Value(nullptr, "/foo"), &err, source_root) == SourceFile("/foo")); + EXPECT_FALSE(err.has_error()); // Basic relative stuff. - EXPECT_TRUE(base.ResolveRelativeFile("foo", source_root) == - SourceFile("//base/foo")); - EXPECT_TRUE(base.ResolveRelativeFile("./foo", source_root) == - SourceFile("//base/foo")); - EXPECT_TRUE(base.ResolveRelativeFile("../foo", source_root) == - SourceFile("//foo")); + EXPECT_TRUE(base.ResolveRelativeFile( + Value(nullptr, "foo"), &err, source_root) == SourceFile("//base/foo")); + EXPECT_FALSE(err.has_error()); + EXPECT_TRUE(base.ResolveRelativeFile( + Value(nullptr, "./foo"), &err, source_root) == SourceFile("//base/foo")); + EXPECT_FALSE(err.has_error()); + EXPECT_TRUE(base.ResolveRelativeFile( + Value(nullptr, "../foo"), &err, source_root) == SourceFile("//foo")); + EXPECT_FALSE(err.has_error()); // If the given relative path points outside the source root, we // expect an absolute path. #if defined(OS_WIN) - EXPECT_TRUE(base.ResolveRelativeFile("../../foo", source_root) == - SourceFile("/C:/source/foo")); + EXPECT_TRUE(base.ResolveRelativeFile( + Value(nullptr, "../../foo"), &err, source_root) == + SourceFile("/C:/source/foo")); + EXPECT_FALSE(err.has_error()); #else - EXPECT_TRUE(base.ResolveRelativeFile("../../foo", source_root) == - SourceFile("/source/foo")); + EXPECT_TRUE(base.ResolveRelativeFile( + Value(nullptr, "../../foo"), &err, source_root) == + SourceFile("/source/foo")); + EXPECT_FALSE(err.has_error()); #endif #if defined(OS_WIN) // Note that we don't canonicalize the backslashes to forward slashes. // This could potentially be changed in the future which would mean we should // just change the expected result. - EXPECT_TRUE(base.ResolveRelativeFile("C:\\foo\\bar.txt", source_root) == - SourceFile("/C:/foo/bar.txt")); + EXPECT_TRUE(base.ResolveRelativeFile( + Value(nullptr, "C:\\foo\\bar.txt"), &err, source_root) == + SourceFile("/C:/foo/bar.txt")); + EXPECT_FALSE(err.has_error()); #endif } TEST(SourceDir, ResolveRelativeDir) { + Err err; SourceDir base("//base/"); #if defined(OS_WIN) base::StringPiece source_root("C:/source/root"); @@ -65,36 +89,52 @@ #endif // Empty input is an error. - EXPECT_TRUE(base.ResolveRelativeDir("", source_root) == SourceDir()); + EXPECT_TRUE(base.ResolveRelativeDir( + Value(nullptr, std::string()), &err, source_root) == SourceDir()); + EXPECT_TRUE(err.has_error()); // Absolute paths should be passed unchanged. - EXPECT_TRUE(base.ResolveRelativeDir("//foo", source_root) == - SourceDir("//foo/")); - EXPECT_TRUE(base.ResolveRelativeDir("/foo", source_root) == - SourceDir("/foo/")); + err = Err(); + EXPECT_TRUE(base.ResolveRelativeDir( + Value(nullptr, "//foo"), &err, source_root) == SourceDir("//foo/")); + EXPECT_FALSE(err.has_error()); + EXPECT_TRUE(base.ResolveRelativeDir( + Value(nullptr, "/foo"), &err, source_root) == SourceDir("/foo/")); + EXPECT_FALSE(err.has_error()); // Basic relative stuff. - EXPECT_TRUE(base.ResolveRelativeDir("foo", source_root) == - SourceDir("//base/foo/")); - EXPECT_TRUE(base.ResolveRelativeDir("./foo", source_root) == - SourceDir("//base/foo/")); - EXPECT_TRUE(base.ResolveRelativeDir("../foo", source_root) == - SourceDir("//foo/")); + EXPECT_TRUE(base.ResolveRelativeDir( + Value(nullptr, "foo"), &err, source_root) == SourceDir("//base/foo/")); + EXPECT_FALSE(err.has_error()); + EXPECT_TRUE(base.ResolveRelativeDir( + Value(nullptr, "./foo"), &err, source_root) == SourceDir("//base/foo/")); + EXPECT_FALSE(err.has_error()); + EXPECT_TRUE(base.ResolveRelativeDir( + Value(nullptr, "../foo"), &err, source_root) == SourceDir("//foo/")); + EXPECT_FALSE(err.has_error()); // If the given relative path points outside the source root, we // expect an absolute path. #if defined(OS_WIN) - EXPECT_TRUE(base.ResolveRelativeDir("../../foo", source_root) == - SourceDir("/C:/source/foo/")); + EXPECT_TRUE(base.ResolveRelativeDir( + Value(nullptr, "../../foo"), &err, source_root) == + SourceDir("/C:/source/foo/")); + EXPECT_FALSE(err.has_error()); #else - EXPECT_TRUE(base.ResolveRelativeDir("../../foo", source_root) == - SourceDir("/source/foo/")); + EXPECT_TRUE(base.ResolveRelativeDir( + Value(nullptr, "../../foo"), &err, source_root) == + SourceDir("/source/foo/")); + EXPECT_FALSE(err.has_error()); #endif #if defined(OS_WIN) // Canonicalize the existing backslashes to forward slashes and add a // leading slash if necessary. - EXPECT_TRUE(base.ResolveRelativeDir("\\C:\\foo") == SourceDir("/C:/foo/")); - EXPECT_TRUE(base.ResolveRelativeDir("C:\\foo") == SourceDir("/C:/foo/")); + EXPECT_TRUE(base.ResolveRelativeDir( + Value(nullptr, "\\C:\\foo"), &err) == SourceDir("/C:/foo/")); + EXPECT_FALSE(err.has_error()); + EXPECT_TRUE(base.ResolveRelativeDir( + Value(nullptr, "C:\\foo"), &err) == SourceDir("/C:/foo/")); + EXPECT_FALSE(err.has_error()); #endif }
diff --git a/tools/gn/source_file.cc b/tools/gn/source_file.cc index 71d8592..8204295 100644 --- a/tools/gn/source_file.cc +++ b/tools/gn/source_file.cc
@@ -18,7 +18,7 @@ #else DCHECK(s[0] == '/'); #endif - DCHECK(!EndsWithSlash(s)); + DCHECK(!EndsWithSlash(s)) << s; } } // namespace
diff --git a/tools/gn/value_extractors.cc b/tools/gn/value_extractors.cc index 6dfaa2f..365c534 100644 --- a/tools/gn/value_extractors.cc +++ b/tools/gn/value_extractors.cc
@@ -65,11 +65,9 @@ current_dir(current_dir_in) { } bool operator()(const Value& v, SourceFile* out, Err* err) const { - if (!v.VerifyTypeIs(Value::STRING, err)) - return false; - *out = current_dir.ResolveRelativeFile(v.string_value(), + *out = current_dir.ResolveRelativeFile(v, err, build_settings->root_path_utf8()); - return true; + return !err->has_error(); } const BuildSettings* build_settings; const SourceDir& current_dir; @@ -82,9 +80,7 @@ current_dir(current_dir_in) { } bool operator()(const Value& v, SourceDir* out, Err* err) const { - if (!v.VerifyTypeIs(Value::STRING, err)) - return false; - *out = current_dir.ResolveRelativeDir(v.string_value(), + *out = current_dir.ResolveRelativeDir(v, err, build_settings->root_path_utf8()); return true; }