Skip to content

Commit

Permalink
Remove default value checking in GN, adds getenv function, reorders p…
Browse files Browse the repository at this point in the history 
…arameters to rebase_path.

This allows us to have the default value vary according to other parameters. This is what we need in many cases, so having this restriction just made things more complicated.

The getenv function will allow us to replace some Python scripts with a fast native function call.

The use of rebase path is pretty annoying. 99% of all callers want the "from" directory to be the current one. I reordered the parameters to move the from directory later so we can have a default value for it.

This will require a patch to change all current callers of rebase_path when new deps are pulled.

BUG=342937
R=viettrungluu@chromium.org

Review URL: https://codereview.chromium.org/161783002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@251821 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
brettw@chromium.org committed Feb 18, 2014
1 parent a509963 commit 55a3dd7
Show file tree
Hide file tree
Showing 7 changed files with 145 additions and 98 deletions.
14 changes: 1 addition & 13 deletions tools/gn/args.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,7 @@ bool Args::DeclareArgs(const Scope::KeyValueMap& args,
//
// The tricky part is that a buildfile can be interpreted multiple times
// when used from different toolchains, so we can't just check that we've
// seen it before. Instead, we check that the location matches. We
// additionally check that the value matches to prevent people from
// declaring defaults based on other parameters that may change. The
// rationale is that you should have exactly one default value for each
// argument that we can display in the help.
// seen it before. Instead, we check that the location matches.
Scope::KeyValueMap::iterator previously_declared =
declared_arguments_.find(i->first);
if (previously_declared != declared_arguments_.end()) {
Expand All @@ -142,14 +138,6 @@ bool Args::DeclareArgs(const Scope::KeyValueMap& args,
"See also \"gn help buildargs\" for more on how "
"build arguments work."));
return false;
} else if (previously_declared->second != i->second) {
// Default value mismatch.
*err = Err(i->second.origin(),
"Non-constant default value for build argument.",
"Each build argument should have one default value so we report "
"it nicely in the\n\"gn args\" command. Please make this value "
"constant.");
return false;
}
} else {
declared_arguments_.insert(*i);
Expand Down
26 changes: 15 additions & 11 deletions tools/gn/command_args.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,14 @@ size_t BackUpToLineBegin(const std::string& data, size_t offset) {
return 0;
}

// Assumes DoesLineBeginWithComment().
std::string StripCommentFromLine(const base::StringPiece& line) {
std::string ret = line.as_string();
for (size_t i = 0; i < ret.size(); i++) {
if (ret[i] == '#') {
ret[i] = ' ';
break;
}
}
return ret;
// Assumes DoesLineBeginWithComment(), this strips the # character from the
// beginning and normalizes preceeding whitespace.
std::string StripHashFromLine(const base::StringPiece& line) {
// Replace the # sign and everything before it with 3 spaces, so that a
// normal comment that has a space after the # will be indented 4 spaces
// (which makes our formatting come out nicely). If the comment is indented
// from there, we want to preserve that indenting.
return " " + line.substr(line.find('#') + 1).as_string();
}

// Tries to find the comment before the setting of the given value.
Expand Down Expand Up @@ -79,7 +77,7 @@ void GetContextForValue(const Value& value,
if (!DoesLineBeginWithComment(line))
break;

comment->insert(0, StripCommentFromLine(line) + "\n");
comment->insert(0, StripHashFromLine(line) + "\n");
line_off = previous_line_offset;
}
}
Expand Down Expand Up @@ -150,6 +148,12 @@ int RunArgs(const std::vector<std::string>& args) {
i != build_args.end(); ++i)
sorted_args.insert(*i);

OutputString(
"Available build arguments. Note that the which arguments are declared\n"
"and their default values may depend on other arguments or the current\n"
"platform and architecture. So setting some values may add, remove, or\n"
"change the default value of other values.\n\n");

for (std::map<base::StringPiece, Value>::iterator i = sorted_args.begin();
i != sorted_args.end(); ++i) {
PrintArgHelp(i->first, i->second);
Expand Down
96 changes: 58 additions & 38 deletions tools/gn/function_rebase_path.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,10 @@ const char kRebasePath[] = "rebase_path";
const char kRebasePath_Help[] =
"rebase_path: Rebase a file or directory to another location.\n"
"\n"
" converted = rebase_path(input, current_base, new_base,\n"
" [path_separators])\n"
" converted = rebase_path(input,\n"
" new_base = \"\",\n"
" current_base = \".\",\n"
" path_separators = \"none\")\n"
"\n"
" Takes a string argument representing a file name, or a list of such\n"
" strings and converts it/them to be relative to a different base\n"
Expand All @@ -175,22 +177,22 @@ const char kRebasePath_Help[] =
" paths (\"/foo/bar.txt\"), or source absolute paths\n"
" (\"//foo/bar.txt\").\n"
"\n"
" current_base\n"
" Directory representing the base for relative paths in the input.\n"
" If this is not an absolute path, it will be treated as being\n"
" relative to the current build file. Use \".\" to convert paths\n"
" from the current BUILD-file's directory.\n"
"\n"
" new_base\n"
" The directory to convert the paths to be relative to. As with the\n"
" current_base, this can be a relative path, which will be treated\n"
" as being relative to the current BUILD-file's directory.\n"
" The directory to convert the paths to be relative to. This can be\n"
" an absolute path or a relative path (which will be treated\n"
" as being relative to the current BUILD-file's directory).\n"
"\n"
" As a special case, if new_base is the empty string, all paths\n"
" will be converted to system-absolute native style paths with\n"
" system path separators. This is useful for invoking external\n"
" As a special case, if new_base is the empty string (the default),\n"
" all paths will be converted to system-absolute native style paths\n"
" with system path separators. This is useful for invoking external\n"
" programs.\n"
"\n"
" current_base\n"
" Directory representing the base for relative paths in the input.\n"
" If this is not an absolute path, it will be treated as being\n"
" relative to the current build file. Use \".\" (the default) to\n"
" convert paths from the current BUILD-file's directory.\n"
"\n"
" path_separators\n"
" On Windows systems, indicates whether and how path separators\n"
" should be converted as part of the transformation. It can be one\n"
Expand Down Expand Up @@ -218,11 +220,11 @@ const char kRebasePath_Help[] =
"\n"
" # Convert a file in the current directory to be relative to the build\n"
" # directory (the current dir when executing compilers and scripts).\n"
" foo = rebase_path(\"myfile.txt\", \".\", root_build_dir)\n"
" foo = rebase_path(\"myfile.txt\", root_build_dir)\n"
" # might produce \"../../project/myfile.txt\".\n"
"\n"
" # Convert a file to be system absolute:\n"
" foo = rebase_path(\"myfile.txt\", \".\", \"\")\n"
" foo = rebase_path(\"myfile.txt\")\n"
" # Might produce \"D:\\source\\project\\myfile.txt\" on Windows or\n"
" # \"/home/you/source/project/myfile.txt\" on Linux.\n"
"\n"
Expand All @@ -241,9 +243,9 @@ const char kRebasePath_Help[] =
" # to be relative to the build directory:\n"
" args = [\n"
" \"--data\",\n"
" rebase_path(\"//mything/data/input.dat\", \".\", root_build_dir),\n"
" rebase_path(\"//mything/data/input.dat\", root_build_dir),\n"
" \"--rel\",\n"
" rebase_path(\"relative_path.txt\", \".\", root_build_dir)\n"
" rebase_path(\"relative_path.txt\", root_build_dir)\n"
" ]\n"
" }\n";

Expand All @@ -253,33 +255,48 @@ Value RunRebasePath(Scope* scope,
Err* err) {
Value result;

// Argument indices.
static const size_t kArgIndexInputs = 0;
static const size_t kArgIndexDest = 1;
static const size_t kArgIndexFrom = 2;
static const size_t kArgIndexPathConversion = 3;

// Inputs.
if (args.size() != 3 && args.size() != 4) {
*err = Err(function->function(), "rebase_path takes 3 or 4 args.");
if (args.size() < 1 || args.size() > 4) {
*err = Err(function->function(), "Wrong # of arguments for rebase_path.");
return result;
}
const Value& inputs = args[0];

// From path.
if (!args[1].VerifyTypeIs(Value::STRING, err))
return result;
const SourceDir& current_dir = scope->GetSourceDir();
SourceDir from_dir = current_dir.ResolveRelativeDir(args[1].string_value());
const Value& inputs = args[kArgIndexInputs];

// To path.
if (!args[2].VerifyTypeIs(Value::STRING, err))
return result;
bool convert_to_system_absolute = false;
bool convert_to_system_absolute = true;
SourceDir to_dir;
if (args[2].string_value().empty()) {
convert_to_system_absolute = true;
const SourceDir& current_dir = scope->GetSourceDir();
if (args.size() > kArgIndexDest) {
if (!args[kArgIndexDest].VerifyTypeIs(Value::STRING, err))
return result;
if (!args[kArgIndexDest].string_value().empty()) {
to_dir =
current_dir.ResolveRelativeDir(args[kArgIndexDest].string_value());
convert_to_system_absolute = false;
}
}

// From path.
SourceDir from_dir;
if (args.size() > kArgIndexFrom) {
if (!args[kArgIndexFrom].VerifyTypeIs(Value::STRING, err))
return result;
from_dir =
current_dir.ResolveRelativeDir(args[kArgIndexFrom].string_value());
} else {
to_dir = current_dir.ResolveRelativeDir(args[2].string_value());
// Default to current directory if unspecified.
from_dir = current_dir;
}

// Path conversion.
SeparatorConversion sep_conversion = SEP_NO_CHANGE;
if (args.size() == 4) {
if (args.size() > kArgIndexPathConversion) {
if (convert_to_system_absolute) {
*err = Err(function, "Can't specify slash conversion.",
"You specified absolute system path output by using an empty string "
Expand All @@ -288,17 +305,20 @@ Value RunRebasePath(Scope* scope,
return result;
}

if (!args[3].VerifyTypeIs(Value::STRING, err))
if (!args[kArgIndexPathConversion].VerifyTypeIs(Value::STRING, err))
return result;
const std::string& sep_string = args[3].string_value();
const std::string& sep_string =
args[kArgIndexPathConversion].string_value();
if (sep_string == "to_system") {
sep_conversion = SEP_TO_SYSTEM;
} else if (sep_string == "from_system") {
sep_conversion = SEP_FROM_SYSTEM;
} else if (sep_string != "none") {
*err = Err(args[3], "Invalid path separator conversion mode.",
*err = Err(args[kArgIndexPathConversion],
"Invalid path separator conversion mode.",
"I was expecting \"none\", \"to_system\", or \"from_system\" and\n"
"you gave me \"" + args[3].string_value() + "\".");
"you gave me \"" + args[kArgIndexPathConversion].string_value() +
"\".");
return result;
}
}
Expand Down
58 changes: 22 additions & 36 deletions tools/gn/function_rebase_path_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ namespace {

std::string RebaseOne(Scope* scope,
const char* input,
const char* from_dir,
const char* to_dir,
const char* from_dir,
const char* sep = NULL) {
std::vector<Value> args;
args.push_back(Value(NULL, input));
args.push_back(Value(NULL, from_dir));
args.push_back(Value(NULL, to_dir));
args.push_back(Value(NULL, from_dir));
if (sep)
args.push_back(Value(NULL, sep));

Expand All @@ -40,24 +40,24 @@ TEST(RebasePath, Strings) {
scope->set_source_dir(SourceDir("//tools/gn/"));

// Build-file relative paths.
EXPECT_EQ("../../tools/gn", RebaseOne(scope, ".", ".", "//out/Debug"));
EXPECT_EQ("../../tools/gn/", RebaseOne(scope, "./", ".", "//out/Debug"));
EXPECT_EQ("../../tools/gn/foo", RebaseOne(scope, "foo", ".", "//out/Debug"));
EXPECT_EQ("../..", RebaseOne(scope, "../..", ".", "//out/Debug"));
EXPECT_EQ("../../", RebaseOne(scope, "../../", ".", "//out/Debug"));
EXPECT_EQ("../../tools/gn", RebaseOne(scope, ".", "//out/Debug", "."));
EXPECT_EQ("../../tools/gn/", RebaseOne(scope, "./", "//out/Debug", "."));
EXPECT_EQ("../../tools/gn/foo", RebaseOne(scope, "foo", "//out/Debug", "."));
EXPECT_EQ("../..", RebaseOne(scope, "../..", "//out/Debug", "."));
EXPECT_EQ("../../", RebaseOne(scope, "../../", "//out/Debug", "."));

// We don't allow going above the root source dir.
EXPECT_EQ("../..", RebaseOne(scope, "../../..", ".", "//out/Debug"));
EXPECT_EQ("../..", RebaseOne(scope, "../../..", "//out/Debug", "."));

// Source-absolute input paths.
EXPECT_EQ("./", RebaseOne(scope, "//", "//", "//"));
EXPECT_EQ("foo", RebaseOne(scope, "//foo", "//", "//"));
EXPECT_EQ("foo/", RebaseOne(scope, "//foo/", "//", "//"));
EXPECT_EQ("../../foo/bar", RebaseOne(scope, "//foo/bar", ".", "//out/Debug"));
EXPECT_EQ("./", RebaseOne(scope, "//foo/", "//", "//foo/"));
EXPECT_EQ("../../foo/bar", RebaseOne(scope, "//foo/bar", "//out/Debug", "."));
EXPECT_EQ("./", RebaseOne(scope, "//foo/", "//foo/", "//"));
// Thie one is technically correct but could be simplified to "." if
// necessary.
EXPECT_EQ("../foo", RebaseOne(scope, "//foo", "//", "//foo"));
EXPECT_EQ("../foo", RebaseOne(scope, "//foo", "//foo", "//"));

// Test slash conversion.
#if defined(OS_WIN)
Expand All @@ -77,18 +77,18 @@ TEST(RebasePath, Strings) {
// Test system path output.
#if defined(OS_WIN)
setup.build_settings()->SetRootPath(base::FilePath(L"C:\\source"));
EXPECT_EQ("C:\\source", RebaseOne(scope, ".", "//", ""));
EXPECT_EQ("C:\\source\\", RebaseOne(scope, "//", "//", ""));
EXPECT_EQ("C:\\source\\foo", RebaseOne(scope, "foo", "//", ""));
EXPECT_EQ("C:\\source\\foo\\", RebaseOne(scope, "foo/", "//", ""));
EXPECT_EQ("C:\\source\\tools\\gn\\foo", RebaseOne(scope, "foo", ".", ""));
EXPECT_EQ("C:\\source", RebaseOne(scope, ".", "", "//"));
EXPECT_EQ("C:\\source\\", RebaseOne(scope, "//", "", "//"));
EXPECT_EQ("C:\\source\\foo", RebaseOne(scope, "foo", "", "//"));
EXPECT_EQ("C:\\source\\foo\\", RebaseOne(scope, "foo/", "", "//"));
EXPECT_EQ("C:\\source\\tools\\gn\\foo", RebaseOne(scope, "foo", "", "."));
#else
setup.build_settings()->SetRootPath(base::FilePath("/source"));
EXPECT_EQ("/source", RebaseOne(scope, ".", "//", ""));
EXPECT_EQ("/source/", RebaseOne(scope, "//", "//", ""));
EXPECT_EQ("/source/foo", RebaseOne(scope, "foo", "//", ""));
EXPECT_EQ("/source/foo/", RebaseOne(scope, "foo/", "//", ""));
EXPECT_EQ("/source/tools/gn/foo", RebaseOne(scope, "foo", ".", ""));
EXPECT_EQ("/source", RebaseOne(scope, ".", "", "//"));
EXPECT_EQ("/source/", RebaseOne(scope, "//", "", "//"));
EXPECT_EQ("/source/foo", RebaseOne(scope, "foo", "", "//"));
EXPECT_EQ("/source/foo/", RebaseOne(scope, "foo/", "", "//"));
EXPECT_EQ("/source/tools/gn/foo", RebaseOne(scope, "foo", "", "."));
#endif
}

Expand All @@ -102,8 +102,8 @@ TEST(RebasePath, List) {
args.push_back(Value(NULL, Value::LIST));
args[0].list_value().push_back(Value(NULL, "foo.txt"));
args[0].list_value().push_back(Value(NULL, "bar.txt"));
args.push_back(Value(NULL, "."));
args.push_back(Value(NULL, "//out/Debug/"));
args.push_back(Value(NULL, "."));

Err err;
FunctionCallNode function;
Expand All @@ -127,18 +127,4 @@ TEST(RebasePath, Errors) {
FunctionCallNode function;
Value ret = functions::RunRebasePath(setup.scope(), &function, args, &err);
EXPECT_TRUE(err.has_error());

// One arg int input.
args.push_back(Value(NULL, static_cast<int64>(5)));
err = Err();
ret = functions::RunRebasePath(setup.scope(), &function, args, &err);
EXPECT_TRUE(err.has_error());

// Two arg string input.
args.clear();
args.push_back(Value(NULL, "hello"));
args.push_back(Value(NULL, "world"));
err = Err();
ret = functions::RunRebasePath(setup.scope(), &function, args, &err);
EXPECT_TRUE(err.has_error());
}
Loading

0 comments on commit 55a3dd7

Please sign in to comment.