Ensure that outputs are files, not directories
We've been making this assumption implicitly, and while it is possible
to get the behaviors correct with dependencies, it's quite difficult.
We'd rather all outputs be files instead. So make it a warning by
default, letting us turn it into an error with "-w outputdir=err"
Until the previous change, it was an internal ninja error if an Edge
that created an output directory failed, as we'd try to remove the
directory as if it was a file. The previous change skipped the
RemoveFile if it was detected to be a directory.
It's uncommon to use directories as build nodes, as it is significantly
problematic with how ninja works today -- if you output a directory of
headers, then compile using that directory, the depfile will refer to
individual headers and ninja won't connect them in the build graph. If
there's already an input or implicit dependency that may be okay.
Outputting a directory would also require careful work to ensure that
the timestamp of that directory is valid, and only the correct files
exist -- if you're always doing `rm -rf` and recreating them that works.
But it doesn't protect if some other action (or the user) goes and
modified one or more of the files in the directory (or subdirs).
In Android, we often use dependencies on directories in the source tree,
to detect when files have been added or removed from a directory so that
we can re-run globs and similar. So those have not been restricted.
Test: run ninja_test (build-tools build does this)
Change-Id: Idc51a8bd4b28fae7f425e71217e6def53d139e35
diff --git a/src/build.cc b/src/build.cc
index f0662e7..0ce39a1 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -380,10 +380,11 @@
// mentioned in a depfile, and the command touches its depfile
// but is interrupted before it touches its output file.)
string err;
- TimeStamp new_mtime = disk_interface_->LStat((*o)->path(), &err);
+ bool is_dir = false;
+ TimeStamp new_mtime = disk_interface_->LStat((*o)->path(), &is_dir, &err);
if (new_mtime == -1) // Log and ignore LStat() errors.
status_->Error("%s", err.c_str());
- if (!depfile.empty() || (*o)->mtime() != new_mtime)
+ if (!is_dir && (!depfile.empty() || (*o)->mtime() != new_mtime))
disk_interface_->RemoveFile((*o)->path());
}
if (!depfile.empty())
@@ -584,38 +585,46 @@
end_time_millis = GetTimeMillis() - start_time_millis_;
running_edges_.erase(i);
- status_->BuildEdgeFinished(edge, end_time_millis, result);
-
- // The rest of this function only applies to successful commands.
- if (!result->success()) {
- plan_.EdgeFinished(edge, Plan::kEdgeFailed);
- return true;
- }
-
// Restat the edge outputs
TimeStamp output_mtime = 0;
- bool restat = edge->IsRestat();
- if (!config_.dry_run) {
- bool node_cleaned = false;
+ if (result->success() && !config_.dry_run) {
+ bool restat = edge->IsRestat();
+ vector<Node*> nodes_cleaned;
for (vector<Node*>::iterator o = edge->outputs_.begin();
o != edge->outputs_.end(); ++o) {
- TimeStamp new_mtime = disk_interface_->LStat((*o)->path(), err);
+ bool is_dir = false;
+ TimeStamp new_mtime = disk_interface_->LStat((*o)->path(), &is_dir, err);
if (new_mtime == -1)
return false;
+ if (is_dir) {
+ if (!result->output.empty())
+ result->output.append("\n");
+ result->output.append("ninja: outputs should be files, not directories: ");
+ result->output.append((*o)->path());
+ if (config_.output_directory_should_err) {
+ result->status = ExitFailure;
+ }
+ }
if (new_mtime > output_mtime)
output_mtime = new_mtime;
if ((*o)->mtime() == new_mtime && restat) {
+ nodes_cleaned.push_back(*o);
+ }
+ }
+
+ status_->BuildEdgeFinished(edge, end_time_millis, result);
+
+ if (result->success() && !nodes_cleaned.empty()) {
+ for (vector<Node*>::iterator o = nodes_cleaned.begin();
+ o != nodes_cleaned.end(); ++o) {
// The rule command did not change the output. Propagate the clean
// state through the build graph.
// Note that this also applies to nonexistent outputs (mtime == 0).
if (!plan_.CleanNode(&scan_, *o, err))
return false;
- node_cleaned = true;
}
- }
- if (node_cleaned) {
TimeStamp restat_mtime = 0;
// If any output was cleaned, find the most recent mtime of any
// (existing) non-order-only input or the depfile.
@@ -623,7 +632,7 @@
i != edge->inputs_.end() - edge->order_only_deps_; ++i) {
TimeStamp input_mtime;
if ((*i)->in_edge() != nullptr) {
- input_mtime = disk_interface_->LStat((*i)->path(), err);
+ input_mtime = disk_interface_->LStat((*i)->path(), nullptr, err);
} else {
input_mtime = disk_interface_->Stat((*i)->path(), err);
}
@@ -648,9 +657,16 @@
output_mtime = restat_mtime;
}
+ } else {
+ status_->BuildEdgeFinished(edge, end_time_millis, result);
}
- plan_.EdgeFinished(edge, Plan::kEdgeSucceeded);
+ plan_.EdgeFinished(edge, result->success() ? Plan::kEdgeSucceeded : Plan::kEdgeFailed);
+
+ // The rest of this function only applies to successful commands.
+ if (!result->success()) {
+ return true;
+ }
// Delete any left over response file.
string rspfile = edge->GetUnescapedRspfile();
@@ -667,7 +683,7 @@
if (!deps_type.empty() && !config_.dry_run) {
Node* out = edge->outputs_[0];
- TimeStamp deps_mtime = disk_interface_->LStat(out->path(), err);
+ TimeStamp deps_mtime = disk_interface_->LStat(out->path(), nullptr, err);
if (deps_mtime == -1)
return false;
if (!scan_.deps_log()->RecordDeps(out, deps_mtime, deps_nodes)) {
diff --git a/src/build.h b/src/build.h
index 08389c8..6cd2ae7 100644
--- a/src/build.h
+++ b/src/build.h
@@ -143,7 +143,8 @@
BuildConfig() : verbosity(NORMAL), dry_run(false), parallelism(1),
failures_allowed(1), max_load_average(-0.0f),
frontend(NULL), frontend_file(NULL),
- missing_depfile_should_err(false) {}
+ missing_depfile_should_err(false),
+ output_directory_should_err(false) {}
enum Verbosity {
NORMAL,
@@ -166,6 +167,9 @@
/// Whether a missing depfile should warn or print an error.
bool missing_depfile_should_err;
+
+ /// Whether an output can be a directory
+ bool output_directory_should_err;
};
/// Builder wraps the build process: starting commands, updating status.
diff --git a/src/build_test.cc b/src/build_test.cc
index a0668db..ab6773b 100644
--- a/src/build_test.cc
+++ b/src/build_test.cc
@@ -609,7 +609,8 @@
} else if (edge->rule().name() == "true" ||
edge->rule().name() == "fail" ||
edge->rule().name() == "interrupt" ||
- edge->rule().name() == "console") {
+ edge->rule().name() == "console" ||
+ edge->rule().name() == "mkdir") {
// Don't do anything.
} else {
printf("unknown command\n");
@@ -642,6 +643,18 @@
return true;
}
+ if (edge->rule().name() == "mkdir") {
+ result->status = ExitSuccess;
+ for (vector<Node*>::iterator out = edge->outputs_.begin();
+ out != edge->outputs_.end(); ++out) {
+ if (!fs_->MakeDir((*out)->path())) {
+ result->status = ExitFailure;
+ }
+ }
+ last_command_ = NULL;
+ return true;
+ }
+
if (edge->rule().name() == "fail" ||
(edge->rule().name() == "touch-fail-tick2" && fs_->now_ == 2))
result->status = ExitFailure;
@@ -2340,6 +2353,43 @@
ASSERT_EQ(1u, command_runner_.commands_ran_.size());
}
+TEST_F(BuildTest, OutputDirectoryWarning) {
+ ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
+"rule mkdir\n"
+" command = mkdir $out\n"
+"build outdir: mkdir\n"));
+
+ string err;
+ EXPECT_TRUE(builder_.AddTarget("outdir", &err));
+ EXPECT_EQ("", err);
+ EXPECT_TRUE(builder_.Build(&err));
+ EXPECT_EQ("", err);
+
+ EXPECT_EQ("ninja: outputs should be files, not directories: outdir", status_.last_output_);
+}
+
+TEST_F(BuildTest, OutputDirectoryError) {
+ ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
+"rule mkdir\n"
+" command = mkdir $out\n"
+"build outdir: mkdir\n"));
+
+ config_.output_directory_should_err = true;
+
+ Builder builder(&state_, config_, NULL, NULL, &fs_, &status_, 0);
+ builder.command_runner_.reset(&command_runner_);
+
+ string err;
+ EXPECT_TRUE(builder.AddTarget("outdir", &err));
+ EXPECT_EQ("", err);
+ EXPECT_FALSE(builder.Build(&err));
+ EXPECT_EQ("subcommand failed", err);
+
+ EXPECT_EQ("ninja: outputs should be files, not directories: outdir", status_.last_output_);
+
+ builder.command_runner_.release();
+}
+
TEST_F(BuildWithDepsLogTest, MissingDepfileWarning) {
ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
"build out1: cat in1\n"
@@ -2390,4 +2440,4 @@
EXPECT_EQ("depfile is missing", status_.last_output_);
builder.command_runner_.release();
-}
+}
\ No newline at end of file
diff --git a/src/deps_log.cc b/src/deps_log.cc
index 8acf73f..807dbd3 100644
--- a/src/deps_log.cc
+++ b/src/deps_log.cc
@@ -709,7 +709,7 @@
// If the current manifest does not define this edge, skip if it's missing
// from the disk.
string err;
- TimeStamp mtime = disk.LStat(node->path(), &err);
+ TimeStamp mtime = disk.LStat(node->path(), nullptr, &err);
if (mtime == -1)
Error("%s", err.c_str()); // log and ignore LStat() errors
if (mtime == 0)
diff --git a/src/disk_interface.cc b/src/disk_interface.cc
index dcab2b6..0044c5b 100644
--- a/src/disk_interface.cc
+++ b/src/disk_interface.cc
@@ -229,7 +229,7 @@
#endif
}
-TimeStamp RealDiskInterface::LStat(const string& path, string* err) const {
+TimeStamp RealDiskInterface::LStat(const string& path, bool* is_dir, string* err) const {
METRIC_RECORD("node lstat");
#ifdef _WIN32
#error unimplemented
@@ -241,6 +241,9 @@
*err = "lstat(" + path + "): " + strerror(errno);
return -1;
}
+ if (is_dir != nullptr) {
+ *is_dir = S_ISDIR(st.st_mode);
+ }
return StatTimestamp(st);
#endif
}
diff --git a/src/disk_interface.h b/src/disk_interface.h
index 56bc94f..f8b1b0d 100644
--- a/src/disk_interface.h
+++ b/src/disk_interface.h
@@ -111,7 +111,7 @@
/// other errors. Does not traverse symlinks, and returns whether the
/// path represents a directory. Thread-safe iff IsStatThreadSafe
/// returns true.
- virtual TimeStamp LStat(const string& path, string* err) const = 0;
+ virtual TimeStamp LStat(const string& path, bool* is_dir, string* err) const = 0;
/// True if Stat() can be called from multiple threads concurrently.
virtual bool IsStatThreadSafe() const = 0;
@@ -144,7 +144,7 @@
{}
virtual ~RealDiskInterface() {}
virtual TimeStamp Stat(const string& path, string* err) const;
- virtual TimeStamp LStat(const string& path, string* err) const;
+ virtual TimeStamp LStat(const string& path, bool* is_dir, string* err) const;
virtual bool IsStatThreadSafe() const;
virtual bool MakeDir(const string& path);
virtual bool WriteFile(const string& path, const string& contents);
diff --git a/src/disk_interface_test.cc b/src/disk_interface_test.cc
index b68ee72..e968b78 100644
--- a/src/disk_interface_test.cc
+++ b/src/disk_interface_test.cc
@@ -217,7 +217,7 @@
// DiskInterface implementation.
virtual TimeStamp Stat(const string& path, string* err) const;
- virtual TimeStamp LStat(const string& path, string* err) const;
+ virtual TimeStamp LStat(const string& path, bool* is_dir, string* err) const;
virtual bool IsStatThreadSafe() const;
virtual bool WriteFile(const string& path, const string& contents) {
assert(false);
@@ -248,14 +248,16 @@
};
TimeStamp StatTest::Stat(const string& path, string* err) const {
- return LStat(path, err);
+ return LStat(path, nullptr, err);
}
-TimeStamp StatTest::LStat(const string& path, string* err) const {
+TimeStamp StatTest::LStat(const string& path, bool* is_dir, string* err) const {
stats_.push_back(path);
map<string, TimeStamp>::const_iterator i = mtimes_.find(path);
if (i == mtimes_.end())
return 0; // File not found.
+ if (is_dir != nullptr)
+ *is_dir = false;
return i->second;
}
diff --git a/src/graph.cc b/src/graph.cc
index 364b076..ee9e629 100644
--- a/src/graph.cc
+++ b/src/graph.cc
@@ -29,7 +29,7 @@
bool Node::PrecomputeStat(DiskInterface* disk_interface, std::string* err) {
if (in_edge() != nullptr) {
- return (precomputed_mtime_ = disk_interface->LStat(path_.str(), err)) != -1;
+ return (precomputed_mtime_ = disk_interface->LStat(path_.str(), nullptr, err)) != -1;
} else {
return (precomputed_mtime_ = disk_interface->Stat(path_.str(), err)) != -1;
}
@@ -37,7 +37,7 @@
bool Node::Stat(DiskInterface* disk_interface, string* err) {
if (in_edge() != nullptr) {
- return (mtime_ = disk_interface->LStat(path_.str(), err)) != -1;
+ return (mtime_ = disk_interface->LStat(path_.str(), nullptr, err)) != -1;
} else {
return (mtime_ = disk_interface->Stat(path_.str(), err)) != -1;
}
diff --git a/src/graph_test.cc b/src/graph_test.cc
index b67ae48..18808b9 100644
--- a/src/graph_test.cc
+++ b/src/graph_test.cc
@@ -416,8 +416,36 @@
EXPECT_TRUE(GetNode("out")->dirty());
}
+TEST_F(GraphTest, InputDirectoryUpToDate) {
+ ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
+"build out: cat inputs\n"));
-// TODO: tests around directory timestamps
+ fs_.Create("out", "");
+ EXPECT_TRUE(fs_.MakeDir("inputs"));
+ EXPECT_TRUE(fs_.WriteFile("inputs/foo", ""));
+
+ string err;
+ EXPECT_TRUE(scan_.RecomputeDirty(GetNode("out"), &err));
+ ASSERT_EQ("", err);
+
+ EXPECT_FALSE(GetNode("out")->dirty());
+}
+
+TEST_F(GraphTest, InputDirectoryChanged) {
+ ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
+"build out: cat inputs\n"));
+
+ fs_.Create("out", "");
+ EXPECT_TRUE(fs_.MakeDir("inputs"));
+ fs_.Tick();
+ EXPECT_TRUE(fs_.WriteFile("inputs/foo", ""));
+
+ string err;
+ EXPECT_TRUE(scan_.RecomputeDirty(GetNode("out"), &err));
+ ASSERT_EQ("", err);
+
+ EXPECT_TRUE(GetNode("out")->dirty());
+}
TEST_F(GraphTest, DependencyCycle) {
AssertParse(&state_,
diff --git a/src/ninja.cc b/src/ninja.cc
index a89c3b3..26692a1 100644
--- a/src/ninja.cc
+++ b/src/ninja.cc
@@ -176,7 +176,7 @@
// Do keep entries around for files which still exist on disk, for
// generators that want to use this information.
string err;
- TimeStamp mtime = disk_interface_.LStat(s.AsString(), &err);
+ TimeStamp mtime = disk_interface_.LStat(s.AsString(), nullptr, &err);
if (mtime == -1)
Error("%s", err.c_str()); // Log and ignore Stat() errors.
return mtime == 0;
@@ -988,7 +988,8 @@
printf("warning flags:\n"
" dupbuild={err,warn} multiple build lines for one target\n"
" phonycycle={err,warn} phony build statement references itself\n"
-" missingdepfile={err,warn} how to treat missing depfiles\n");
+" missingdepfile={err,warn} how to treat missing depfiles\n"
+" outputdir={err,warn} how to treat outputs that are directories\n");
return false;
} else if (name == "dupbuild=err") {
options->dupe_edges_should_err = true;
@@ -1008,11 +1009,18 @@
} else if (name == "missingdepfile=warn") {
config->missing_depfile_should_err = false;
return true;
+ } else if (name == "outputdir=err") {
+ config->output_directory_should_err = true;
+ return true;
+ } else if (name == "outputdir=warn") {
+ config->output_directory_should_err = false;
+ return true;
} else {
const char* suggestion =
SpellcheckString(name.c_str(), "dupbuild=err", "dupbuild=warn",
"phonycycle=err", "phonycycle=warn",
- "missingdepfile=err", "missingdepfile=warn", NULL);
+ "missingdepfile=err", "missingdepfile=warn",
+ "outputdir=err", "outputdir=warn", NULL);
if (suggestion) {
Error("unknown warning flag '%s', did you mean '%s'?",
name.c_str(), suggestion);
diff --git a/src/test.cc b/src/test.cc
index c2aa22b..86983cc 100644
--- a/src/test.cc
+++ b/src/test.cc
@@ -184,6 +184,11 @@
}
TimeStamp VirtualFileSystem::Stat(const string& path, string* err) const {
+ DirMap::const_iterator d = dirs_.find(path);
+ if (d != dirs_.end()) {
+ *err = d->second.stat_error;
+ return d->second.mtime;
+ }
FileMap::const_iterator i = files_.find(path);
if (i != files_.end()) {
if (i->second.is_symlink) {
@@ -195,9 +200,18 @@
return 0;
}
-TimeStamp VirtualFileSystem::LStat(const string& path, string* err) const {
+TimeStamp VirtualFileSystem::LStat(const string& path, bool* is_dir, string* err) const {
+ DirMap::const_iterator d = dirs_.find(path);
+ if (d != dirs_.end()) {
+ if (is_dir != nullptr)
+ *is_dir = true;
+ *err = d->second.stat_error;
+ return d->second.mtime;
+ }
FileMap::const_iterator i = files_.find(path);
if (i != files_.end()) {
+ if (is_dir != nullptr)
+ *is_dir = false;
*err = i->second.stat_error;
return i->second.mtime;
}
@@ -209,11 +223,42 @@
}
bool VirtualFileSystem::WriteFile(const string& path, const string& contents) {
+ if (files_.find(path) == files_.end()) {
+ if (dirs_.find(path) != dirs_.end())
+ return false;
+
+ string::size_type slash_pos = path.find_last_of("/");
+ if (slash_pos != string::npos) {
+ DirMap::iterator d = dirs_.find(path.substr(0, slash_pos));
+ if (d != dirs_.end()) {
+ d->second.mtime = now_;
+ } else {
+ return false;
+ }
+ }
+ }
+
Create(path, contents);
return true;
}
bool VirtualFileSystem::MakeDir(const string& path) {
+ if (dirs_.find(path) != dirs_.end())
+ return true;
+ if (files_.find(path) != files_.end())
+ return false;
+
+ string::size_type slash_pos = path.find_last_of("/");
+ if (slash_pos != string::npos) {
+ DirMap::iterator d = dirs_.find(path.substr(0, slash_pos));
+ if (d != dirs_.end()) {
+ d->second.mtime = now_;
+ } else {
+ return false;
+ }
+ }
+
+ dirs_[path].mtime = now_;
directories_made_.push_back(path);
return true; // success
}
@@ -249,11 +294,18 @@
}
int VirtualFileSystem::RemoveFile(const string& path) {
- if (find(directories_made_.begin(), directories_made_.end(), path)
- != directories_made_.end())
+ if (dirs_.find(path) != dirs_.end())
return -1;
FileMap::iterator i = files_.find(path);
if (i != files_.end()) {
+ string::size_type slash_pos = path.find_last_of("/");
+ if (slash_pos != string::npos) {
+ DirMap::iterator d = dirs_.find(path.substr(0, slash_pos));
+ if (d != dirs_.end()) {
+ d->second.mtime = now_;
+ }
+ }
+
files_.erase(i);
files_removed_.insert(path);
return 0;
diff --git a/src/test.h b/src/test.h
index 1413acd..5d49b0a 100644
--- a/src/test.h
+++ b/src/test.h
@@ -148,7 +148,7 @@
// DiskInterface
virtual TimeStamp Stat(const string& path, string* err) const;
- virtual TimeStamp LStat(const string& path, string* err) const;
+ virtual TimeStamp LStat(const string& path, bool* is_dir, string* err) const;
virtual bool IsStatThreadSafe() const;
virtual bool WriteFile(const string& path, const string& contents);
virtual bool MakeDir(const string& path);
@@ -165,9 +165,15 @@
string contents;
bool is_symlink = false;
};
+ struct DirEntry {
+ int mtime;
+ string stat_error; // If mtime is -1.
+ };
vector<string> directories_made_;
vector<string> files_read_;
+ typedef map<string, DirEntry> DirMap;
+ DirMap dirs_;
typedef map<string, Entry> FileMap;
FileMap files_;
set<string> files_removed_;
