From 13dd08c1a03e5a8f4299816fbd3af1b6cb6d9642 Mon Sep 17 00:00:00 2001 From: Evan Martin Date: Sat, 27 Oct 2012 13:09:40 -0700 Subject: [PATCH] rearrange handling of builtin bindings to make rules simpler Now, a 'build' block can override any special binding like 'command' or 'description' if it needs to. --- src/build.cc | 38 +++++++++++---------- src/clean.cc | 6 ++-- src/eval_env.cc | 16 +++++++++ src/eval_env.h | 12 +++++++ src/graph.cc | 81 +++++++++++++++++++++++++-------------------- src/graph.h | 43 ++++++------------------ src/graph_test.cc | 35 ++++++++++++++++++++ src/manifest_parser.cc | 76 ++++++++++++++---------------------------- src/manifest_parser_test.cc | 20 ++++++----- src/state.cc | 4 +-- src/state.h | 2 +- src/state_test.cc | 4 +-- 12 files changed, 183 insertions(+), 154 deletions(-) diff --git a/src/build.cc b/src/build.cc index b4229c4..701fa92 100644 --- a/src/build.cc +++ b/src/build.cc @@ -256,9 +256,9 @@ void BuildStatus::PrintStatus(Edge* edge) { bool force_full_command = config_.verbosity == BuildConfig::VERBOSE; - string to_print = edge->GetDescription(); + string to_print = edge->GetBinding("description"); if (to_print.empty() || force_full_command) - to_print = edge->EvaluateCommand(); + to_print = edge->GetBinding("command"); #ifdef _WIN32 CONSOLE_SCREEN_BUFFER_INFO csbi; @@ -612,7 +612,7 @@ void Builder::Cleanup() { for (vector::iterator i = active_edges.begin(); i != active_edges.end(); ++i) { - bool has_depfile = !(*i)->rule_->depfile().empty(); + string depfile = (*i)->GetBinding("depfile"); for (vector::iterator ni = (*i)->outputs_.begin(); ni != (*i)->outputs_.end(); ++ni) { // Only delete this output if it was actually modified. This is @@ -622,12 +622,13 @@ void Builder::Cleanup() { // need to rebuild an output because of a modified header file // mentioned in a depfile, and the command touches its depfile // but is interrupted before it touches its output file.) - if (has_depfile || - (*ni)->mtime() != disk_interface_->Stat((*ni)->path())) + if (!depfile.empty() || + (*ni)->mtime() != disk_interface_->Stat((*ni)->path())) { disk_interface_->RemoveFile((*ni)->path()); + } } - if (has_depfile) - disk_interface_->RemoveFile((*i)->EvaluateDepFile()); + if (!depfile.empty()) + disk_interface_->RemoveFile(depfile); } } } @@ -771,11 +772,11 @@ bool Builder::StartEdge(Edge* edge, string* err) { // Create response file, if needed // XXX: this may also block; do we care? - if (edge->HasRspFile()) { - if (!disk_interface_->WriteFile(edge->GetRspFile(), - edge->GetRspFileContent())) { + string rspfile = edge->GetBinding("rspfile"); + if (!rspfile.empty()) { + string content = edge->GetBinding("rspfile_content"); + if (!disk_interface_->WriteFile(rspfile, content)) return false; - } } // start command computing and run it @@ -792,7 +793,7 @@ void Builder::FinishEdge(Edge* edge, bool success, const string& output) { TimeStamp restat_mtime = 0; if (success) { - if (edge->rule().restat() && !config_.dry_run) { + if (edge->GetBindingBool("restat") && !config_.dry_run) { bool node_cleaned = false; for (vector::iterator i = edge->outputs_.begin(); @@ -817,9 +818,9 @@ void Builder::FinishEdge(Edge* edge, bool success, const string& output) { restat_mtime = input_mtime; } - if (restat_mtime != 0 && !edge->rule().depfile().empty()) { - TimeStamp depfile_mtime = - disk_interface_->Stat(edge->EvaluateDepFile()); + string depfile = edge->GetBinding("depfile"); + if (restat_mtime != 0 && !depfile.empty()) { + TimeStamp depfile_mtime = disk_interface_->Stat(depfile); if (depfile_mtime > restat_mtime) restat_mtime = depfile_mtime; } @@ -830,9 +831,10 @@ void Builder::FinishEdge(Edge* edge, bool success, const string& output) { } } - // delete the response file on success (if exists) - if (edge->HasRspFile()) - disk_interface_->RemoveFile(edge->GetRspFile()); + // Delete the response file on success (if exists) + string rspfile = edge->GetBinding("rspfile"); + if (!rspfile.empty()) + disk_interface_->RemoveFile(rspfile); plan_.EdgeFinished(edge); } diff --git a/src/clean.cc b/src/clean.cc index 0b8476b..12afb98 100644 --- a/src/clean.cc +++ b/src/clean.cc @@ -83,11 +83,11 @@ bool Cleaner::IsAlreadyRemoved(const string& path) { } void Cleaner::RemoveEdgeFiles(Edge* edge) { - string depfile = edge->EvaluateDepFile(); + string depfile = edge->GetBinding("depfile"); if (!depfile.empty()) Remove(depfile); - string rspfile = edge->GetRspFile(); + string rspfile = edge->GetBinding("rspfile"); if (!rspfile.empty()) Remove(rspfile); } @@ -117,7 +117,7 @@ int Cleaner::CleanAll(bool generator) { if ((*e)->is_phony()) continue; // Do not remove generator's files unless generator specified. - if (!generator && (*e)->rule().generator()) + if (!generator && (*e)->GetBindingBool("generator")) continue; for (vector::iterator out_node = (*e)->outputs_.begin(); out_node != (*e)->outputs_.end(); ++out_node) { diff --git a/src/eval_env.cc b/src/eval_env.cc index 81a8765..834b7e1 100644 --- a/src/eval_env.cc +++ b/src/eval_env.cc @@ -27,6 +27,22 @@ void BindingEnv::AddBinding(const string& key, const string& val) { bindings_[key] = val; } +string BindingEnv::LookupWithFallback(const string& var, + const EvalString* eval, + Env* env) { + map::iterator i = bindings_.find(var); + if (i != bindings_.end()) + return i->second; + + if (eval) + return eval->Evaluate(env); + + if (parent_) + return parent_->LookupVariable(var); + + return ""; +} + string EvalString::Evaluate(Env* env) const { string result; for (TokenList::const_iterator i = parsed_.begin(); i != parsed_.end(); ++i) { diff --git a/src/eval_env.h b/src/eval_env.h index 6e0a0c0..f3c959a 100644 --- a/src/eval_env.h +++ b/src/eval_env.h @@ -22,6 +22,8 @@ using namespace std; #include "string_piece.h" +struct EvalString; + /// An interface for a scope for variable (e.g. "$foo") lookups. struct Env { virtual ~Env() {} @@ -33,10 +35,20 @@ struct Env { struct BindingEnv : public Env { BindingEnv() : parent_(NULL) {} explicit BindingEnv(Env* parent) : parent_(parent) {} + virtual ~BindingEnv() {} virtual string LookupVariable(const string& var); + void AddBinding(const string& key, const string& val); + /// This is tricky. Edges want lookup scope to go in this order: + /// 1) value set on edge itself (edge_->env_) + /// 2) value set on rule, with expansion in the edge's scope + /// 3) value set on enclosing scope of edge (edge_->env_->parent_) + /// This function takes as parameters the necessary info to do (2). + string LookupWithFallback(const string& var, const EvalString* eval, + Env* env); + private: map bindings_; Env* parent_; diff --git a/src/graph.cc b/src/graph.cc index f9b9c6f..380ca7c 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -32,16 +32,40 @@ bool Node::Stat(DiskInterface* disk_interface) { return mtime_ > 0; } +void Rule::AddBinding(const string& key, const EvalString& val) { + bindings_[key] = val; +} + +const EvalString* Rule::GetBinding(const string& key) const { + map::const_iterator i = bindings_.find(key); + if (i == bindings_.end()) + return NULL; + return &i->second; +} + +// static +bool Rule::IsReservedBinding(const string& var) { + return var == "command" || + var == "depfile" || + var == "description" || + var == "generator" || + var == "pool" || + var == "restat" || + var == "rspfile" || + var == "rspfile_content"; +} + bool DependencyScan::RecomputeDirty(Edge* edge, string* err) { bool dirty = false; edge->outputs_ready_ = true; - if (!edge->rule_->depfile().empty()) { - if (!LoadDepFile(edge, err)) { + string depfile = edge->GetBinding("depfile"); + if (!depfile.empty()) { + if (!LoadDepFile(edge, depfile, err)) { if (!err->empty()) return false; EXPLAIN("Edge targets are dirty because depfile '%s' is missing", - edge->EvaluateDepFile().c_str()); + depfile.c_str()); dirty = true; } } @@ -142,7 +166,7 @@ bool DependencyScan::RecomputeOutputDirty(Edge* edge, // build log. Use that mtime instead, so that the file will only be // considered dirty if an input was modified since the previous run. TimeStamp most_recent_stamp = most_recent_input->mtime(); - if (edge->rule_->restat() && build_log() && + if (edge->GetBindingBool("restat") && build_log() && (entry = build_log()->LookupByOutput(output->path()))) { if (entry->restat_mtime < most_recent_stamp) { EXPLAIN("restat of output %s older than most recent input %s " @@ -162,7 +186,7 @@ bool DependencyScan::RecomputeOutputDirty(Edge* edge, // May also be dirty due to the command changing since the last build. // But if this is a generator rule, the command changing does not make us // dirty. - if (!edge->rule_->generator() && build_log()) { + if (!edge->GetBindingBool("generator") && build_log()) { if (entry || (entry = build_log()->LookupByOutput(output->path()))) { if (BuildLog::LogEntry::HashCommand(command) != entry->command_hash) { EXPLAIN("command line changed for %s", output->path().c_str()); @@ -212,11 +236,11 @@ string EdgeEnv::LookupVariable(const string& var) { return MakePathList(edge_->outputs_.begin(), edge_->outputs_.end(), ' '); - } else if (edge_->env_) { - return edge_->env_->LookupVariable(var); - } else { - return string(); } + + // See notes on BindingEnv::LookupWithFallback. + const EvalString* eval = edge_->rule_->GetBinding(var); + return edge_->env_->LookupWithFallback(var, eval, this); } string EdgeEnv::MakePathList(vector::iterator begin, @@ -239,40 +263,26 @@ string EdgeEnv::MakePathList(vector::iterator begin, } string Edge::EvaluateCommand(bool incl_rsp_file) { - EdgeEnv env(this); - string command = rule_->command().Evaluate(&env); - if (incl_rsp_file && HasRspFile()) - command += ";rspfile=" + GetRspFileContent(); + string command = GetBinding("command"); + if (incl_rsp_file) { + string rspfile_content = GetBinding("rspfile_content"); + if (!rspfile_content.empty()) + command += ";rspfile=" + rspfile_content; + } return command; } -string Edge::EvaluateDepFile() { +string Edge::GetBinding(const string& key) { EdgeEnv env(this); - return rule_->depfile().Evaluate(&env); + return env.LookupVariable(key); } -string Edge::GetDescription() { - EdgeEnv env(this); - return rule_->description().Evaluate(&env); -} - -bool Edge::HasRspFile() { - return !rule_->rspfile().empty(); -} - -string Edge::GetRspFile() { - EdgeEnv env(this); - return rule_->rspfile().Evaluate(&env); -} - -string Edge::GetRspFileContent() { - EdgeEnv env(this); - return rule_->rspfile_content().Evaluate(&env); +bool Edge::GetBindingBool(const string& key) { + return !GetBinding(key).empty(); } -bool DependencyScan::LoadDepFile(Edge* edge, string* err) { +bool DependencyScan::LoadDepFile(Edge* edge, const string& path, string* err) { METRIC_RECORD("depfile load"); - string path = edge->EvaluateDepFile(); string content = disk_interface_->ReadFile(path, err); if (!err->empty()) return false; @@ -317,8 +327,7 @@ bool DependencyScan::LoadDepFile(Edge* edge, string* err) { // create one; this makes us not abort if the input is missing, // but instead will rebuild in that circumstance. if (!node->in_edge()) { - Edge* phony_edge = state_->AddEdge(&State::kPhonyRule, - &State::kDefaultPool); + Edge* phony_edge = state_->AddEdge(&State::kPhonyRule); node->set_in_edge(phony_edge); phony_edge->outputs_.push_back(node); diff --git a/src/graph.h b/src/graph.h index 3c31e19..8b93e29 100644 --- a/src/graph.h +++ b/src/graph.h @@ -102,38 +102,23 @@ private: /// An invokable build command and associated metadata (description, etc.). struct Rule { - explicit Rule(const string& name) - : name_(name), generator_(false), restat_(false) {} + explicit Rule(const string& name) : name_(name) {} const string& name() const { return name_; } - bool generator() const { return generator_; } - bool restat() const { return restat_; } + typedef map Bindings; + void AddBinding(const string& key, const EvalString& val); - const EvalString& command() const { return command_; } - const EvalString& description() const { return description_; } - const EvalString& depfile() const { return depfile_; } - const EvalString& rspfile() const { return rspfile_; } - const EvalString& rspfile_content() const { return rspfile_content_; } + static bool IsReservedBinding(const string& var); - /// Used by a test. - void set_command(const EvalString& command) { command_ = command; } + const EvalString* GetBinding(const string& key) const; private: // Allow the parsers to reach into this object and fill out its fields. friend struct ManifestParser; string name_; - - bool generator_; - bool restat_; - - EvalString command_; - EvalString description_; - EvalString depfile_; - EvalString pool_; - EvalString rspfile_; - EvalString rspfile_content_; + map bindings_; }; struct BuildLog; @@ -153,17 +138,9 @@ struct Edge { /// If incl_rsp_file is enabled, the string will also contain the /// full contents of a response file (if applicable) string EvaluateCommand(bool incl_rsp_file = false); - string EvaluateDepFile(); - string GetDescription(); - - /// Does the edge use a response file? - bool HasRspFile(); - - /// Get the path to the response file - string GetRspFile(); - /// Get the contents of the response file - string GetRspFileContent(); + string GetBinding(const string& key); + bool GetBindingBool(const string& key); void Dump(const char* prefix="") const; @@ -171,7 +148,7 @@ struct Edge { Pool* pool_; vector inputs_; vector outputs_; - Env* env_; + BindingEnv* env_; bool outputs_ready_; const Rule& rule() const { return *rule_; } @@ -220,7 +197,7 @@ struct DependencyScan { bool RecomputeOutputDirty(Edge* edge, Node* most_recent_input, const string& command, Node* output); - bool LoadDepFile(Edge* edge, string* err); + bool LoadDepFile(Edge* edge, const string& path, string* err); BuildLog* build_log() const { return build_log_; diff --git a/src/graph_test.cc b/src/graph_test.cc index 5b25c2f..396def4 100644 --- a/src/graph_test.cc +++ b/src/graph_test.cc @@ -187,3 +187,38 @@ TEST_F(GraphTest, DepfileRemoved) { ASSERT_EQ("", err); EXPECT_TRUE(GetNode("out.o")->dirty()); } + +// Check that rule-level variables are in scope for eval. +TEST_F(GraphTest, RuleVariablesInScope) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"rule r\n" +" depfile = x\n" +" command = depfile is $depfile\n" +"build out: r in\n")); + Edge* edge = GetNode("out")->in_edge(); + EXPECT_EQ("depfile is x", edge->EvaluateCommand()); +} + +// Check that build statements can override rule builtins like depfile. +TEST_F(GraphTest, DepfileOverride) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"rule r\n" +" depfile = x\n" +" command = unused\n" +"build out: r in\n" +" depfile = y\n")); + Edge* edge = GetNode("out")->in_edge(); + EXPECT_EQ("y", edge->GetBinding("depfile")); +} + +// Check that overridden values show up in expansion of rule-level bindings. +TEST_F(GraphTest, DepfileOverrideParent) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"rule r\n" +" depfile = x\n" +" command = depfile is $depfile\n" +"build out: r in\n" +" depfile = y\n")); + Edge* edge = GetNode("out")->in_edge(); + EXPECT_EQ("depfile is y", edge->GetBinding("command")); +} diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc index 2d052b5..9d17932 100644 --- a/src/manifest_parser.cc +++ b/src/manifest_parser.cc @@ -154,22 +154,8 @@ bool ManifestParser::ParseRule(string* err) { if (!ParseLet(&key, &value, err)) return false; - if (key == "command") { - rule->command_ = value; - } else if (key == "depfile") { - rule->depfile_ = value; - } else if (key == "description") { - rule->description_ = value; - } else if (key == "generator") { - rule->generator_ = true; - } else if (key == "restat") { - rule->restat_ = true; - } else if (key == "rspfile") { - rule->rspfile_ = value; - } else if (key == "rspfile_content") { - rule->rspfile_content_ = value; - } else if (key == "pool") { - rule->pool_ = value; + if (Rule::IsReservedBinding(key)) { + rule->AddBinding(key, value); } else { // Die on other keyvals for now; revisit if we want to add a // scope here. @@ -177,12 +163,13 @@ bool ManifestParser::ParseRule(string* err) { } } - if (rule->rspfile_.empty() != rule->rspfile_content_.empty()) { - return lexer_.Error("rspfile and rspfile_content need to be both specified", - err); + if (rule->bindings_["rspfile"].empty() != + rule->bindings_["rspfile_content"].empty()) { + return lexer_.Error("rspfile and rspfile_content need to be " + "both specified", err); } - if (rule->command_.empty()) + if (rule->bindings_["command"].empty()) return lexer_.Error("expected 'command =' line", err); state_->AddRule(rule); @@ -296,42 +283,29 @@ bool ManifestParser::ParseEdge(string* err) { if (!ExpectToken(Lexer::NEWLINE, err)) return false; - // Default to using outer env. - BindingEnv* env = env_; - Pool* pool = NULL; + // XXX scoped_ptr to handle error case. + BindingEnv* env = new BindingEnv(env_); - // But create and fill a nested env if there are variables in scope. - if (lexer_.PeekToken(Lexer::INDENT)) { - // XXX scoped_ptr to handle error case. - env = new BindingEnv(env_); - do { - string key; - EvalString val; - if (!ParseLet(&key, &val, err)) - return false; - if (key == "pool") { - string pool_name = val.Evaluate(env_); - pool = state_->LookupPool(pool_name); - if (pool == NULL) - return lexer_.Error("undefined pool '" + pool_name + "'", err); - } else { - env->AddBinding(key, val.Evaluate(env_)); - } - } while (lexer_.PeekToken(Lexer::INDENT)); - } + while (lexer_.PeekToken(Lexer::INDENT)) { + string key; + EvalString val; + if (!ParseLet(&key, &val, err)) + return false; - if (pool == NULL) { - if (!rule->pool_.empty()) { - pool = state_->LookupPool(rule->pool_.Evaluate(env_)); - if (pool == NULL) - return lexer_.Error("cannot resolve pool for this edge.", err); - } else { - pool = &State::kDefaultPool; - } + env->AddBinding(key, val.Evaluate(env_)); } - Edge* edge = state_->AddEdge(rule, pool); + Edge* edge = state_->AddEdge(rule); edge->env_ = env; + + string pool_name = edge->GetBinding("pool"); + if (!pool_name.empty()) { + Pool* pool = state_->LookupPool(pool_name); + if (pool == NULL) + return lexer_.Error("unknown pool name", err); + edge->pool_ = pool; + } + for (vector::iterator i = ins.begin(); i != ins.end(); ++i) { string path = i->Evaluate(env); string path_err; diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc index 8b00efb..92f52d2 100644 --- a/src/manifest_parser_test.cc +++ b/src/manifest_parser_test.cc @@ -61,7 +61,8 @@ TEST_F(ParserTest, Rules) { ASSERT_EQ(3u, state.rules_.size()); const Rule* rule = state.rules_.begin()->second; EXPECT_EQ("cat", rule->name()); - EXPECT_EQ("[cat ][$in][ > ][$out]", rule->command().Serialize()); + EXPECT_EQ("[cat ][$in][ > ][$out]", + rule->GetBinding("command")->Serialize()); } TEST_F(ParserTest, RuleAttributes) { @@ -92,8 +93,9 @@ TEST_F(ParserTest, IgnoreIndentedComments) { ASSERT_EQ(2u, state.rules_.size()); const Rule* rule = state.rules_.begin()->second; EXPECT_EQ("cat", rule->name()); - EXPECT_TRUE(rule->restat()); - EXPECT_FALSE(rule->generator()); + Edge* edge = state.GetNode("result")->in_edge(); + EXPECT_TRUE(edge->GetBindingBool("restat")); + EXPECT_FALSE(edge->GetBindingBool("generator")); } TEST_F(ParserTest, IgnoreIndentedBlankLines) { @@ -124,9 +126,10 @@ TEST_F(ParserTest, ResponseFiles) { ASSERT_EQ(2u, state.rules_.size()); const Rule* rule = state.rules_.begin()->second; EXPECT_EQ("cat_rsp", rule->name()); - EXPECT_EQ("[cat ][$rspfile][ > ][$out]", rule->command().Serialize()); - EXPECT_EQ("[$rspfile]", rule->rspfile().Serialize()); - EXPECT_EQ("[$in]", rule->rspfile_content().Serialize()); + EXPECT_EQ("[cat ][$rspfile][ > ][$out]", + rule->GetBinding("command")->Serialize()); + EXPECT_EQ("[$rspfile]", rule->GetBinding("rspfile")->Serialize()); + EXPECT_EQ("[$in]", rule->GetBinding("rspfile_content")->Serialize()); } TEST_F(ParserTest, InNewline) { @@ -140,7 +143,8 @@ TEST_F(ParserTest, InNewline) { ASSERT_EQ(2u, state.rules_.size()); const Rule* rule = state.rules_.begin()->second; EXPECT_EQ("cat_rsp", rule->name()); - EXPECT_EQ("[cat ][$in_newline][ > ][$out]", rule->command().Serialize()); + EXPECT_EQ("[cat ][$in_newline][ > ][$out]", + rule->GetBinding("command")->Serialize()); Edge* edge = state.edges_[0]; EXPECT_EQ("cat in\nin2 > out", edge->EvaluateCommand()); @@ -200,7 +204,7 @@ TEST_F(ParserTest, Continuation) { ASSERT_EQ(2u, state.rules_.size()); const Rule* rule = state.rules_.begin()->second; EXPECT_EQ("link", rule->name()); - EXPECT_EQ("[foo bar baz]", rule->command().Serialize()); + EXPECT_EQ("[foo bar baz]", rule->GetBinding("command")->Serialize()); } TEST_F(ParserTest, Backslash) { diff --git a/src/state.cc b/src/state.cc index bb0cc15..b5d2622 100644 --- a/src/state.cc +++ b/src/state.cc @@ -91,10 +91,10 @@ Pool* State::LookupPool(const string& pool_name) { return i->second; } -Edge* State::AddEdge(const Rule* rule, Pool* pool) { +Edge* State::AddEdge(const Rule* rule) { Edge* edge = new Edge(); edge->rule_ = rule; - edge->pool_ = pool; + edge->pool_ = &State::kDefaultPool; edge->env_ = &bindings_; edges_.push_back(edge); return edge; diff --git a/src/state.h b/src/state.h index 918fe09..326fbac 100644 --- a/src/state.h +++ b/src/state.h @@ -92,7 +92,7 @@ struct State { void AddPool(Pool* pool); Pool* LookupPool(const string& pool_name); - Edge* AddEdge(const Rule* rule, Pool* pool); + Edge* AddEdge(const Rule* rule); Node* GetNode(StringPiece path); Node* LookupNode(StringPiece path); diff --git a/src/state_test.cc b/src/state_test.cc index 26177ff..af2bff1 100644 --- a/src/state_test.cc +++ b/src/state_test.cc @@ -29,10 +29,10 @@ TEST(State, Basic) { command.AddSpecial("out"); Rule* rule = new Rule("cat"); - rule->set_command(command); + rule->AddBinding("command", command); state.AddRule(rule); - Edge* edge = state.AddEdge(rule, &State::kDefaultPool); + Edge* edge = state.AddEdge(rule); state.AddIn(edge, "in1"); state.AddIn(edge, "in2"); state.AddOut(edge, "out"); -- 2.7.4