Make "depfile=$out.d" work if $out contains escaped characters, rspfile too.

Fixes #730.  This has always been broken, but due to #690 more paths are now
escaped (e.g. paths containing + characters, like file.c++).  Also see
discussion in #689.

The approach is to give EdgeEnv an enum deciding on whether or not to escape
file names, and provide functions that evaluate depfile and rspfile with that
set that to kNoEscape.  (depfile=$out.d doesn't make sense on edges with
multiple outputs.)

This should be relatively safe, as $in and $out can't be used on edges, only
on rules (#687).

diff --git a/doc/manual.asciidoc b/doc/manual.asciidoc
index 18760dd..aea1784 100644 (file)
--- a/doc/manual.asciidoc
+++ b/doc/manual.asciidoc
@@ -783,7 +783,8 @@ keys.
 `depfile`:: path to an optional `Makefile` that contains extra
   _implicit dependencies_ (see <<ref_dependencies,the reference on
   dependency types>>).  This is explicitly to support C/C++ header
-  dependencies; see <<ref_headers,the full discussion>>.
+  dependencies; see <<ref_headers,the full discussion>>.  `out`, `in`, and
+  `in_newline` are not shell-quoted when used to set `depfile`.
 
 `deps`:: _(Available since Ninja 1.3.)_ if present, must be one of
   `gcc` or `msvc` to specify special dependency processing.  See
@@ -830,7 +831,8 @@ keys.
   response file for the given command, i.e. write the selected string
   (`rspfile_content`) to the given file (`rspfile`) before calling the
   command and delete the file after successful execution of the
-  command.
+  command.  `out`, `in`, and `in_newline` are not shell-quoted when used to set
+  `rspfile`.
 +
 This is particularly useful on Windows OS, where the maximal length of
 a command line is limited and response files must be used instead.

diff --git a/src/build.cc b/src/build.cc
index 91f1754..64bcea3 100644 (file)
--- a/src/build.cc
+++ b/src/build.cc
@@ -541,7 +541,7 @@ void Builder::Cleanup() {
 
     for (vector<Edge*>::iterator i = active_edges.begin();
          i != active_edges.end(); ++i) {
-      string depfile = (*i)->GetBinding("depfile");
+      string depfile = (*i)->GetUnescapedDepfile();
       for (vector<Node*>::iterator ni = (*i)->outputs_.begin();
            ni != (*i)->outputs_.end(); ++ni) {
         // Only delete this output if it was actually modified.  This is
@@ -696,7 +696,7 @@ bool Builder::StartEdge(Edge* edge, string* err) {
 
   // Create response file, if needed
   // XXX: this may also block; do we care?
-  string rspfile = edge->GetBinding("rspfile");
+  string rspfile = edge->GetUnescapedRspfile();
   if (!rspfile.empty()) {
     string content = edge->GetBinding("rspfile_content");
     if (!disk_interface_->WriteFile(rspfile, content))
@@ -772,7 +772,7 @@ bool Builder::FinishCommand(CommandRunner::Result* result, string* err) {
           restat_mtime = input_mtime;
       }
 
-      string depfile = edge->GetBinding("depfile");
+      string depfile = edge->GetUnescapedDepfile();
       if (restat_mtime != 0 && deps_type.empty() && !depfile.empty()) {
         TimeStamp depfile_mtime = disk_interface_->Stat(depfile);
         if (depfile_mtime > restat_mtime)
@@ -788,7 +788,7 @@ bool Builder::FinishCommand(CommandRunner::Result* result, string* err) {
   plan_.EdgeFinished(edge);
 
   // Delete any left over response file.
-  string rspfile = edge->GetBinding("rspfile");
+  string rspfile = edge->GetUnescapedRspfile();
   if (!rspfile.empty() && !g_keep_rsp)
     disk_interface_->RemoveFile(rspfile);
 
@@ -828,7 +828,7 @@ bool Builder::ExtractDeps(CommandRunner::Result* result,
   } else
 #endif
   if (deps_type == "gcc") {
-    string depfile = result->edge->GetBinding("depfile");
+    string depfile = result->edge->GetUnescapedDepfile();
     if (depfile.empty()) {
       *err = string("edge with deps=gcc but no depfile makes no sense");
       return false;

diff --git a/src/build_test.cc b/src/build_test.cc
index c414c88..dad69dc 100644 (file)
--- a/src/build_test.cc
+++ b/src/build_test.cc
@@ -521,6 +521,7 @@ bool FakeCommandRunner::StartCommand(Edge* edge) {
   commands_ran_.push_back(edge->EvaluateCommand());
   if (edge->rule().name() == "cat"  ||
       edge->rule().name() == "cat_rsp" ||
+      edge->rule().name() == "cat_rsp_out" ||
       edge->rule().name() == "cc" ||
       edge->rule().name() == "touch" ||
       edge->rule().name() == "touch-interrupt") {
@@ -775,13 +776,13 @@ TEST_F(BuildTest, DepFileMissing) {
   string err;
   ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
 "rule cc\n  command = cc $in\n  depfile = $out.d\n"
-"build foo.o: cc foo.c\n"));
+"build fo$ o.o: cc foo.c\n"));
   fs_.Create("foo.c", "");
 
-  EXPECT_TRUE(builder_.AddTarget("foo.o", &err));
+  EXPECT_TRUE(builder_.AddTarget("fo o.o", &err));
   ASSERT_EQ("", err);
   ASSERT_EQ(1u, fs_.files_read_.size());
-  EXPECT_EQ("foo.o.d", fs_.files_read_[0]);
+  EXPECT_EQ("fo o.o.d", fs_.files_read_[0]);
 }
 
 TEST_F(BuildTest, DepFileOK) {
@@ -1302,14 +1303,20 @@ TEST_F(BuildTest, RspFileSuccess)
     "  command = cat $rspfile > $out\n"
     "  rspfile = $rspfile\n"
     "  rspfile_content = $long_command\n"
+    "rule cat_rsp_out\n"
+    "  command = cat $rspfile > $out\n"
+    "  rspfile = $out.rsp\n"
+    "  rspfile_content = $long_command\n"
     "build out1: cat in\n"
     "build out2: cat_rsp in\n"
-    "  rspfile = out2.rsp\n"
+    "  rspfile = out 2.rsp\n"
+    "  long_command = Some very long command\n"
+    "build out$ 3: cat_rsp_out in\n"
     "  long_command = Some very long command\n"));
 
   fs_.Create("out1", "");
   fs_.Create("out2", "");
-  fs_.Create("out3", "");
+  fs_.Create("out 3", "");
 
   fs_.Tick();
 
@@ -1320,20 +1327,24 @@ TEST_F(BuildTest, RspFileSuccess)
   ASSERT_EQ("", err);
   EXPECT_TRUE(builder_.AddTarget("out2", &err));
   ASSERT_EQ("", err);
+  EXPECT_TRUE(builder_.AddTarget("out 3", &err));
+  ASSERT_EQ("", err);
 
   size_t files_created = fs_.files_created_.size();
   size_t files_removed = fs_.files_removed_.size();
 
   EXPECT_TRUE(builder_.Build(&err));
-  ASSERT_EQ(2u, command_runner_.commands_ran_.size()); // cat + cat_rsp
+  ASSERT_EQ(3u, command_runner_.commands_ran_.size());
 
-  // The RSP file was created
-  ASSERT_EQ(files_created + 1, fs_.files_created_.size());
-  ASSERT_EQ(1u, fs_.files_created_.count("out2.rsp"));
+  // The RSP files were created
+  ASSERT_EQ(files_created + 2, fs_.files_created_.size());
+  ASSERT_EQ(1u, fs_.files_created_.count("out 2.rsp"));
+  ASSERT_EQ(1u, fs_.files_created_.count("out 3.rsp"));
 
-  // The RSP file was removed
-  ASSERT_EQ(files_removed + 1, fs_.files_removed_.size());
-  ASSERT_EQ(1u, fs_.files_removed_.count("out2.rsp"));
+  // The RSP files were removed
+  ASSERT_EQ(files_removed + 2, fs_.files_removed_.size());
+  ASSERT_EQ(1u, fs_.files_removed_.count("out 2.rsp"));
+  ASSERT_EQ(1u, fs_.files_removed_.count("out 3.rsp"));
 }
 
 // Test that RSP file is created but not removed for commands, which fail
@@ -1804,7 +1815,7 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) {
   string err;
   const char* manifest =
       "rule cc\n  command = cc $in\n  depfile = $out.d\n  deps = gcc\n"
-      "build foo.o: cc foo.c\n";
+      "build fo$ o.o: cc foo.c\n";
 
   fs_.Create("foo.c", "");
 
@@ -1819,9 +1830,9 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) {
 
     Builder builder(&state, config_, NULL, &deps_log, &fs_);
     builder.command_runner_.reset(&command_runner_);
-    EXPECT_TRUE(builder.AddTarget("foo.o", &err));
+    EXPECT_TRUE(builder.AddTarget("fo o.o", &err));
     ASSERT_EQ("", err);
-    fs_.Create("foo.o.d", "foo.o: blah.h bar.h\n");
+    fs_.Create("fo o.o.d", "fo\\ o.o: blah.h bar.h\n");
     EXPECT_TRUE(builder.Build(&err));
     EXPECT_EQ("", err);
 
@@ -1844,10 +1855,10 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) {
     Edge* edge = state.edges_.back();
 
     state.GetNode("bar.h")->MarkDirty();  // Mark bar.h as missing.
-    EXPECT_TRUE(builder.AddTarget("foo.o", &err));
+    EXPECT_TRUE(builder.AddTarget("fo o.o", &err));
     ASSERT_EQ("", err);
 
-    // Expect three new edges: one generating foo.o, and two more from
+    // Expect three new edges: one generating fo o.o, and two more from
     // loading the depfile.
     ASSERT_EQ(3u, state.edges_.size());
     // Expect our edge to now have three inputs: foo.c and two headers.

diff --git a/src/clean.cc b/src/clean.cc
index 5d1974e..98c638c 100644 (file)
--- a/src/clean.cc
+++ b/src/clean.cc
@@ -80,11 +80,11 @@ bool Cleaner::IsAlreadyRemoved(const string& path) {
 }
 
 void Cleaner::RemoveEdgeFiles(Edge* edge) {
-  string depfile = edge->GetBinding("depfile");
+  string depfile = edge->GetUnescapedDepfile();
   if (!depfile.empty())
     Remove(depfile);
 
-  string rspfile = edge->GetBinding("rspfile");
+  string rspfile = edge->GetUnescapedRspfile();
   if (!rspfile.empty())
     Remove(rspfile);
 }

diff --git a/src/clean_test.cc b/src/clean_test.cc
index 4a00fd8..5869bbb 100644 (file)
--- a/src/clean_test.cc
+++ b/src/clean_test.cc
@@ -368,3 +368,31 @@ TEST_F(CleanTest, CleanPhony) {
   EXPECT_EQ(2, cleaner.cleaned_files_count());
   EXPECT_NE(0, fs_.Stat("phony"));
 }
+
+TEST_F(CleanTest, CleanDepFileAndRspFileWithSpaces) {
+  ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
+"rule cc_dep\n"
+"  command = cc $in > $out\n"
+"  depfile = $out.d\n"
+"rule cc_rsp\n"
+"  command = cc $in > $out\n"
+"  rspfile = $out.rsp\n"
+"  rspfile_content = $in\n"
+"build out$ 1: cc_dep in$ 1\n"
+"build out$ 2: cc_rsp in$ 1\n"
+));
+  fs_.Create("out 1", "");
+  fs_.Create("out 2", "");
+  fs_.Create("out 1.d", "");
+  fs_.Create("out 2.rsp", "");
+
+  Cleaner cleaner(&state_, config_, &fs_);
+  EXPECT_EQ(0, cleaner.CleanAll());
+  EXPECT_EQ(4, cleaner.cleaned_files_count());
+  EXPECT_EQ(4u, fs_.files_removed_.size());
+
+  EXPECT_EQ(0, fs_.Stat("out 1"));
+  EXPECT_EQ(0, fs_.Stat("out 2"));
+  EXPECT_EQ(0, fs_.Stat("out 1.d"));
+  EXPECT_EQ(0, fs_.Stat("out 2.rsp"));
+}

diff --git a/src/graph.cc b/src/graph.cc
index 7121342..aa9c0e8 100644 (file)
--- a/src/graph.cc
+++ b/src/graph.cc
@@ -215,7 +215,10 @@ bool Edge::AllInputsReady() const {
 
 /// An Env for an Edge, providing $in and $out.
 struct EdgeEnv : public Env {
-  explicit EdgeEnv(Edge* edge) : edge_(edge) {}
+  enum EscapeKind { kShellEscape, kDoNotEscape };
+
+  explicit EdgeEnv(Edge* edge, EscapeKind escape)
+      : edge_(edge), escape_in_out_(escape) {}
   virtual string LookupVariable(const string& var);
 
   /// Given a span of Nodes, construct a list of paths suitable for a command
@@ -225,6 +228,7 @@ struct EdgeEnv : public Env {
                       char sep);
 
   Edge* edge_;
+  EscapeKind escape_in_out_;
 };
 
 string EdgeEnv::LookupVariable(const string& var) {
@@ -250,13 +254,18 @@ string EdgeEnv::MakePathList(vector<Node*>::iterator begin,
                              char sep) {
   string result;
   for (vector<Node*>::iterator i = begin; i != end; ++i) {
-    if (!result.empty()) result.push_back(sep);
+    if (!result.empty())
+      result.push_back(sep);
     const string& path = (*i)->path();
+    if (escape_in_out_ == kShellEscape) {
 #if _WIN32
-    GetWin32EscapedString(path, &result);
+      GetWin32EscapedString(path, &result);
 #else
-    GetShellEscapedString(path, &result);
+      GetShellEscapedString(path, &result);
 #endif
+    } else {
+      result.append(path);
+    }
   }
   return result;
 }
@@ -272,7 +281,7 @@ string Edge::EvaluateCommand(bool incl_rsp_file) {
 }
 
 string Edge::GetBinding(const string& key) {
-  EdgeEnv env(this);
+  EdgeEnv env(this, EdgeEnv::kShellEscape);
   return env.LookupVariable(key);
 }
 
@@ -280,6 +289,16 @@ bool Edge::GetBindingBool(const string& key) {
   return !GetBinding(key).empty();
 }
 
+string Edge::GetUnescapedDepfile() {
+  EdgeEnv env(this, EdgeEnv::kDoNotEscape);
+  return env.LookupVariable("depfile");
+}
+
+string Edge::GetUnescapedRspfile() {
+  EdgeEnv env(this, EdgeEnv::kDoNotEscape);
+  return env.LookupVariable("rspfile");
+}
+
 void Edge::Dump(const char* prefix) const {
   printf("%s[ ", prefix);
   for (vector<Node*>::const_iterator i = inputs_.begin();
@@ -331,7 +350,7 @@ bool ImplicitDepLoader::LoadDeps(Edge* edge, string* err) {
   if (!deps_type.empty())
     return LoadDepsFromLog(edge, err);
 
-  string depfile = edge->GetBinding("depfile");
+  string depfile = edge->GetUnescapedDepfile();
   if (!depfile.empty())
     return LoadDepFile(edge, depfile, err);
 

diff --git a/src/graph.h b/src/graph.h
index 6cd7f25..66e31b5 100644 (file)
--- a/src/graph.h
+++ b/src/graph.h
@@ -146,9 +146,15 @@ struct Edge {
   /// full contents of a response file (if applicable)
   string EvaluateCommand(bool incl_rsp_file = false);
 
+  /// Returns the shell-escaped value of |key|.
   string GetBinding(const string& key);
   bool GetBindingBool(const string& key);
 
+  /// Like GetBinding("depfile"), but without shell escaping.
+  string GetUnescapedDepfile();
+  /// Like GetBinding("rspfile"), but without shell escaping.
+  string GetUnescapedRspfile();
+
   void Dump(const char* prefix="") const;
 
   const Rule* rule_;