From ca0f379c27aa4887dbc63e3af71a6ebc687649e0 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Mon, 3 Jun 2013 16:56:46 -0700 Subject: Add test that proves `node->AddOutEdge(edge);` in `LoadDepFile()` is needed. No functionality change. Related to issue #590. --- src/build_test.cc | 25 ++++++++++++++++++++++++- src/graph.cc | 7 ++++--- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/build_test.cc b/src/build_test.cc index ed9ade3..5089607 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -1604,7 +1604,30 @@ TEST_F(BuildWithDepsLogTest, DepsIgnoredInDryRun) { } /// Check that a restat rule generating a header cancels compilations correctly. -TEST_F(BuildWithDepsLogTest, RestatDepfileDependency) { +TEST_F(BuildTest, RestatDepfileDependency) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"rule true\n" +" command = true\n" // Would be "write if out-of-date" in reality. +" restat = 1\n" +"build header.h: true header.in\n" +"build out: cat in1\n" +" depfile = in1.d\n")); + + fs_.Create("header.h", ""); + fs_.Create("in1.d", "out: header.h"); + fs_.Tick(); + fs_.Create("header.in", ""); + + string err; + EXPECT_TRUE(builder_.AddTarget("out", &err)); + ASSERT_EQ("", err); + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); +} + +/// Check that a restat rule generating a header cancels compilations correctly, +/// depslog case. +TEST_F(BuildWithDepsLogTest, RestatDepfileDependencyDepsLog) { string err; // Note: in1 was created by the superclass SetUp(). const char* manifest = diff --git a/src/graph.cc b/src/graph.cc index 7a57753..fdd93de 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -417,9 +417,10 @@ bool ImplicitDepLoader::LoadDepsFromLog(Edge* edge, TimeStamp* deps_mtime, vector::iterator implicit_dep = PreallocateSpace(edge, deps->node_count); for (int i = 0; i < deps->node_count; ++i, ++implicit_dep) { - *implicit_dep = deps->nodes[i]; - deps->nodes[i]->AddOutEdge(edge); - CreatePhonyInEdge(*implicit_dep); + Node* node = deps->nodes[i]; + *implicit_dep = node; + node->AddOutEdge(edge); + CreatePhonyInEdge(node); } return true; } -- cgit v0.12 From 456b0d4fdb206b692fb394f35bc626708b318981 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Wed, 5 Jun 2013 18:51:00 -0700 Subject: Add a test for CreatePhonyInEdge() in depsmode path. Removing the `CreatePhonyInEdge(node);` line in `ImplicitDepLoader::LoadDepsFromLog()` made no tests fail before this change. The new test is a port to depsmode of the existing DepFileOK test. --- src/build_test.cc | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/build_test.cc b/src/build_test.cc index 5089607..313a386 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -1689,3 +1689,64 @@ TEST_F(BuildWithDepsLogTest, RestatDepfileDependencyDepsLog) { builder.command_runner_.release(); } } + +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"; + + fs_.Create("foo.c", ""); + + { + State state; + ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest)); + + // Run the build once, everything should be ok. + DepsLog deps_log; + ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err)); + ASSERT_EQ("", err); + + Builder builder(&state, config_, NULL, &deps_log, &fs_); + builder.command_runner_.reset(&command_runner_); + EXPECT_TRUE(builder.AddTarget("foo.o", &err)); + ASSERT_EQ("", err); + fs_.Create("foo.o.d", "foo.o: blah.h bar.h\n"); + EXPECT_TRUE(builder.Build(&err)); + EXPECT_EQ("", err); + + deps_log.Close(); + builder.command_runner_.release(); + } + + { + State state; + ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest)); + + DepsLog deps_log; + ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err)); + ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err)); + ASSERT_EQ("", err); + + Builder builder(&state, config_, NULL, &deps_log, &fs_); + builder.command_runner_.reset(&command_runner_); + + Edge* edge = state.edges_.back(); + + state.GetNode("bar.h")->MarkDirty(); // Mark bar.h as missing. + EXPECT_TRUE(builder.AddTarget("foo.o", &err)); + ASSERT_EQ("", err); + + // Expect three new edges: one generating foo.o, and two more from + // loading the depfile. + ASSERT_EQ(3, (int)state.edges_.size()); + // Expect our edge to now have three inputs: foo.c and two headers. + ASSERT_EQ(3u, edge->inputs_.size()); + + // Expect the command line we generate to only use the original input. + ASSERT_EQ("cc foo.c", edge->EvaluateCommand()); + + deps_log.Close(); + builder.command_runner_.release(); + } +} -- cgit v0.12 From 13bad8d61c1a1d25328ae99daf50ac71294b8bb1 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Thu, 13 Jun 2013 22:11:34 -0700 Subject: Make sure to not write partial deps entries. When two ninja instances run in parallel in the same build directory, one instance could write a deps entry header and a few ids, and then the other instance could write a file name in the middle of the deps header. When ninja reads this deps log on the next run, it will deserialize the file name as indices, which will cause an out-of-bounds read. (This can happen if a user runs a "compile this file" that uses ninja to compile the current buffer in an editor, and also does a full build in a terminal at the same time for example.) While running two ninja instances in parallel in the same build directory isn't a good idea, it happens to mostly work in non-deps mode: There's redundant edge execution, but nothing crashes. This is partially because the command log is line-buffered and a single log entry only consists of a single line. This change makes sure that deps entries are always written in one go, like command log entries currently are. Running two ninja binaries in parallel on the same build directory still isn't a great idea, but it's less likely to lead to crashes. See issue #595. --- src/deps_log.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/deps_log.cc b/src/deps_log.cc index 931cc77..ce9bf06 100644 --- a/src/deps_log.cc +++ b/src/deps_log.cc @@ -32,6 +32,11 @@ const char kFileSignature[] = "# ninjadeps\n"; const int kCurrentVersion = 1; +// Since the size field is 2 bytes and the top bit marks deps entries, a single +// record can be at most 32 kB. Set the buffer size to this and flush the file +// buffer after every record to make sure records aren't written partially. +const int kMaxBufferSize = 1 << 15; + DepsLog::~DepsLog() { Close(); } @@ -48,6 +53,7 @@ bool DepsLog::OpenForWrite(const string& path, string* err) { *err = strerror(errno); return false; } + setvbuf(file_, NULL, _IOFBF, kMaxBufferSize); SetCloseOnExec(fileno(file_)); // Opening a file in append mode doesn't set the file pointer to the file's @@ -64,6 +70,7 @@ bool DepsLog::OpenForWrite(const string& path, string* err) { return false; } } + fflush(file_); return true; } @@ -124,6 +131,7 @@ bool DepsLog::RecordDeps(Node* node, TimeStamp mtime, id = nodes[i]->id(); fwrite(&id, 4, 1, file_); } + fflush(file_); // Update in-memory representation. Deps* deps = new Deps(mtime, node_count); @@ -318,6 +326,7 @@ bool DepsLog::RecordId(Node* node) { uint16_t size = (uint16_t)node->path().size(); fwrite(&size, 2, 1, file_); fwrite(node->path().data(), node->path().size(), 1, file_); + fflush(file_); node->set_id(nodes_.size()); nodes_.push_back(node); -- cgit v0.12 From 5ed055b3f7f5088c3ffc79d19dac14fe15b2afb8 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Fri, 21 Jun 2013 17:40:40 -0700 Subject: Add stdlib.h include for atol(). This attempts to fix issue #600. `man atol` claims that atol() is in stdlib.h, which wasn't included yet. --- src/manifest_parser.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc index 3593567..d742331 100644 --- a/src/manifest_parser.cc +++ b/src/manifest_parser.cc @@ -15,6 +15,7 @@ #include "manifest_parser.h" #include +#include #include #include "graph.h" -- cgit v0.12 From 7b9b1fb94aa44784e33734bef68c76e6a43361ef Mon Sep 17 00:00:00 2001 From: David Hill Date: Sat, 29 Jun 2013 17:34:51 -0400 Subject: support Bitrig --- bootstrap.py | 6 +++--- configure.py | 4 ++-- platform_helper.py | 7 ++++++- src/subprocess-posix.cc | 6 +++--- src/subprocess_test.cc | 4 ++-- 5 files changed, 16 insertions(+), 11 deletions(-) diff --git a/bootstrap.py b/bootstrap.py index 5682bf1..66ec85b 100755 --- a/bootstrap.py +++ b/bootstrap.py @@ -37,7 +37,7 @@ parser.add_option('--platform', help='target platform (' + '/'.join(platform_helper.platforms()) + ')', choices=platform_helper.platforms()) parser.add_option('--force-pselect', action='store_true', - help="ppoll() is used by default on Linux and OpenBSD, but older versions might need to use pselect instead",) + help="ppoll() is used by default on Linux, OpenBSD and Bitrig, but older versions might need to use pselect instead",) (options, conf_args) = parser.parse_args() @@ -53,7 +53,7 @@ def run(*args, **kwargs): # g++ call as well as in the later configure.py. cflags = os.environ.get('CFLAGS', '').split() ldflags = os.environ.get('LDFLAGS', '').split() -if platform.is_freebsd() or platform.is_openbsd(): +if platform.is_freebsd() or platform.is_openbsd() or platform.is_bitrig(): cflags.append('-I/usr/local/include') ldflags.append('-L/usr/local/lib') @@ -109,7 +109,7 @@ else: cflags.append('-D_WIN32_WINNT=0x0501') if options.x64: cflags.append('-m64') -if (platform.is_linux() or platform.is_openbsd()) and not options.force_pselect: +if (platform.is_linux() or platform.is_openbsd() or platform.is_bitrig()) and not options.force_pselect: cflags.append('-DUSE_PPOLL') if options.force_pselect: conf_args.append("--force-pselect") diff --git a/configure.py b/configure.py index 22eb1e5..6b6b3d0 100755 --- a/configure.py +++ b/configure.py @@ -48,7 +48,7 @@ parser.add_option('--with-python', metavar='EXE', help='use EXE as the Python interpreter', default=os.path.basename(sys.executable)) parser.add_option('--force-pselect', action='store_true', - help="ppoll() is used by default on Linux and OpenBSD, but older versions might need to use pselect instead",) + help="ppoll() is used by default on Linux, OpenBSD and Bitrig, but older versions might need to use pselect instead",) (options, args) = parser.parse_args() if args: print('ERROR: extra unparsed command-line arguments:', args) @@ -165,7 +165,7 @@ else: cflags.append('-fno-omit-frame-pointer') libs.extend(['-Wl,--no-as-needed', '-lprofiler']) -if (platform.is_linux() or platform.is_openbsd()) and not options.force_pselect: +if (platform.is_linux() or platform.is_openbsd() or platform.is_bitrig()) and not options.force_pselect: cflags.append('-DUSE_PPOLL') def shell_escape(str): diff --git a/platform_helper.py b/platform_helper.py index 5097f49..e040246 100644 --- a/platform_helper.py +++ b/platform_helper.py @@ -19,7 +19,7 @@ import sys def platforms(): return ['linux', 'darwin', 'freebsd', 'openbsd', 'solaris', 'sunos5', - 'mingw', 'msvc', 'gnukfreebsd8'] + 'mingw', 'msvc', 'gnukfreebsd8', 'bitrig'] class Platform( object ): def __init__( self, platform): @@ -41,6 +41,8 @@ class Platform( object ): self._platform = 'mingw' elif self._platform.startswith('win'): self._platform = 'msvc' + elif self._platform.startswith('bitrig'): + self._platform = 'bitrig' def platform(self): @@ -69,3 +71,6 @@ class Platform( object ): def is_sunos5(self): return self._platform == 'sunos5' + + def is_bitrig(self): + return self._platform == 'bitrig' diff --git a/src/subprocess-posix.cc b/src/subprocess-posix.cc index b396f84..ae010c9 100644 --- a/src/subprocess-posix.cc +++ b/src/subprocess-posix.cc @@ -41,7 +41,7 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) { Fatal("pipe: %s", strerror(errno)); fd_ = output_pipe[0]; #if !defined(USE_PPOLL) - // On Linux and OpenBSD, we use ppoll in DoWork(); elsewhere we use pselect + // On Linux, OpenBSD and Bitrig, we use ppoll in DoWork(); elsewhere we use pselect // and so must avoid overly-large FDs. if (fd_ >= static_cast(FD_SETSIZE)) Fatal("pipe: %s", strerror(EMFILE)); @@ -224,7 +224,7 @@ bool SubprocessSet::DoWork() { return interrupted_; } -#else // linux || __OpenBSD__ +#else // linux || __OpenBSD__ || __Bitrig__ bool SubprocessSet::DoWork() { fd_set set; int nfds = 0; @@ -266,7 +266,7 @@ bool SubprocessSet::DoWork() { return interrupted_; } -#endif // linux || __OpenBSD__ +#endif // linux || __OpenBSD__ || __Bitrig__ Subprocess* SubprocessSet::NextFinished() { if (finished_.empty()) diff --git a/src/subprocess_test.cc b/src/subprocess_test.cc index afd9008..7e98cad 100644 --- a/src/subprocess_test.cc +++ b/src/subprocess_test.cc @@ -152,7 +152,7 @@ TEST_F(SubprocessTest, SetWithMulti) { // OS X's process limit is less than 1025 by default // (|sysctl kern.maxprocperuid| is 709 on 10.7 and 10.8 and less prior to that). -#if defined(linux) || defined(__OpenBSD__) +#if defined(linux) || defined(__OpenBSD__) || defined(__Bitrig__) TEST_F(SubprocessTest, SetWithLots) { // Arbitrary big number; needs to be over 1024 to confirm we're no longer // hostage to pselect. @@ -179,7 +179,7 @@ TEST_F(SubprocessTest, SetWithLots) { } ASSERT_EQ(kNumProcs, subprocs_.finished_.size()); } -#endif // linux || __OpenBSD__ +#endif // linux || __OpenBSD__ || __Bitrig__ // TODO: this test could work on Windows, just not sure how to simply // read stdin. -- cgit v0.12 From cb85ce886da075a57ede902a5fe6b4e3507afb34 Mon Sep 17 00:00:00 2001 From: David Hill Date: Sat, 29 Jun 2013 19:16:51 -0400 Subject: cleanup based on comments from martine --- configure.py | 2 +- platform_helper.py | 2 +- src/subprocess-posix.cc | 2 +- src/subprocess_test.cc | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/configure.py b/configure.py index 6b6b3d0..c838392 100755 --- a/configure.py +++ b/configure.py @@ -48,7 +48,7 @@ parser.add_option('--with-python', metavar='EXE', help='use EXE as the Python interpreter', default=os.path.basename(sys.executable)) parser.add_option('--force-pselect', action='store_true', - help="ppoll() is used by default on Linux, OpenBSD and Bitrig, but older versions might need to use pselect instead",) + help="ppoll() is used by default where available, but some platforms may need to use pselect instead",) (options, args) = parser.parse_args() if args: print('ERROR: extra unparsed command-line arguments:', args) diff --git a/platform_helper.py b/platform_helper.py index e040246..e615660 100644 --- a/platform_helper.py +++ b/platform_helper.py @@ -73,4 +73,4 @@ class Platform( object ): return self._platform == 'sunos5' def is_bitrig(self): - return self._platform == 'bitrig' + return self._platform == 'bitrig' diff --git a/src/subprocess-posix.cc b/src/subprocess-posix.cc index ae010c9..cd22aa9 100644 --- a/src/subprocess-posix.cc +++ b/src/subprocess-posix.cc @@ -41,7 +41,7 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) { Fatal("pipe: %s", strerror(errno)); fd_ = output_pipe[0]; #if !defined(USE_PPOLL) - // On Linux, OpenBSD and Bitrig, we use ppoll in DoWork(); elsewhere we use pselect + // If available, we use ppoll in DoWork(); otherwise we use pselect // and so must avoid overly-large FDs. if (fd_ >= static_cast(FD_SETSIZE)) Fatal("pipe: %s", strerror(EMFILE)); diff --git a/src/subprocess_test.cc b/src/subprocess_test.cc index 7e98cad..9b903c8 100644 --- a/src/subprocess_test.cc +++ b/src/subprocess_test.cc @@ -152,7 +152,7 @@ TEST_F(SubprocessTest, SetWithMulti) { // OS X's process limit is less than 1025 by default // (|sysctl kern.maxprocperuid| is 709 on 10.7 and 10.8 and less prior to that). -#if defined(linux) || defined(__OpenBSD__) || defined(__Bitrig__) +if !defined(__APPLE__) TEST_F(SubprocessTest, SetWithLots) { // Arbitrary big number; needs to be over 1024 to confirm we're no longer // hostage to pselect. -- cgit v0.12 From 43cc6c197878311e78cbb912306ab012b48b5ab9 Mon Sep 17 00:00:00 2001 From: David Hill Date: Sat, 29 Jun 2013 19:21:15 -0400 Subject: ugh, missing # --- src/subprocess_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/subprocess_test.cc b/src/subprocess_test.cc index 9b903c8..f0e193b 100644 --- a/src/subprocess_test.cc +++ b/src/subprocess_test.cc @@ -152,7 +152,7 @@ TEST_F(SubprocessTest, SetWithMulti) { // OS X's process limit is less than 1025 by default // (|sysctl kern.maxprocperuid| is 709 on 10.7 and 10.8 and less prior to that). -if !defined(__APPLE__) +#if !defined(__APPLE__) TEST_F(SubprocessTest, SetWithLots) { // Arbitrary big number; needs to be over 1024 to confirm we're no longer // hostage to pselect. -- cgit v0.12 From eb398301f6a1272c5675835c6ef4f6eecf90433d Mon Sep 17 00:00:00 2001 From: David Hill Date: Mon, 1 Jul 2013 14:11:05 -0400 Subject: Exclude Windows as well --- src/subprocess_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/subprocess_test.cc b/src/subprocess_test.cc index f0e193b..9f8dcea 100644 --- a/src/subprocess_test.cc +++ b/src/subprocess_test.cc @@ -152,7 +152,7 @@ TEST_F(SubprocessTest, SetWithMulti) { // OS X's process limit is less than 1025 by default // (|sysctl kern.maxprocperuid| is 709 on 10.7 and 10.8 and less prior to that). -#if !defined(__APPLE__) +#if !defined(__APPLE__) && !defined(_WIN32) TEST_F(SubprocessTest, SetWithLots) { // Arbitrary big number; needs to be over 1024 to confirm we're no longer // hostage to pselect. @@ -179,7 +179,7 @@ TEST_F(SubprocessTest, SetWithLots) { } ASSERT_EQ(kNumProcs, subprocs_.finished_.size()); } -#endif // linux || __OpenBSD__ || __Bitrig__ +#endif // !__APPLE__ && !_WIN32 // TODO: this test could work on Windows, just not sure how to simply // read stdin. -- cgit v0.12 From 4cc869a7c99227749ff98d4449420d393958c53a Mon Sep 17 00:00:00 2001 From: Maxim Kalaev Date: Mon, 1 Jul 2013 21:33:12 +0300 Subject: Adding error checking on fwrite/fflush in deps_log --- src/build.cc | 16 +++++++++++----- src/build.h | 5 ++++- src/deps_log.cc | 36 ++++++++++++++++++++++++------------ 3 files changed, 39 insertions(+), 18 deletions(-) diff --git a/src/build.cc b/src/build.cc index 5cf9d27..a2f430e 100644 --- a/src/build.cc +++ b/src/build.cc @@ -641,7 +641,10 @@ bool Builder::Build(string* err) { } --pending_commands; - FinishCommand(&result); + if (!FinishCommand(&result, err)) { + status_->BuildFinished(); + return false; + } if (!result.success()) { if (failures_allowed) @@ -704,7 +707,7 @@ bool Builder::StartEdge(Edge* edge, string* err) { return true; } -void Builder::FinishCommand(CommandRunner::Result* result) { +bool Builder::FinishCommand(CommandRunner::Result* result, string* err) { METRIC_RECORD("FinishCommand"); Edge* edge = result->edge; @@ -733,7 +736,7 @@ void Builder::FinishCommand(CommandRunner::Result* result) { // The rest of this function only applies to successful commands. if (!result->success()) - return; + return true; // Restat the edge outputs, if necessary. TimeStamp restat_mtime = 0; @@ -791,9 +794,12 @@ void Builder::FinishCommand(CommandRunner::Result* result) { assert(edge->outputs_.size() == 1 && "should have been rejected by parser"); Node* out = edge->outputs_[0]; TimeStamp deps_mtime = disk_interface_->Stat(out->path()); - scan_.deps_log()->RecordDeps(out, deps_mtime, deps_nodes); + if (!scan_.deps_log()->RecordDeps(out, deps_mtime, deps_nodes)) { + *err = string("Error writing to deps log: ") + strerror(errno); + return false; + } } - + return true; } bool Builder::ExtractDeps(CommandRunner::Result* result, diff --git a/src/build.h b/src/build.h index 2715c0c..33df7d0 100644 --- a/src/build.h +++ b/src/build.h @@ -163,7 +163,10 @@ struct Builder { bool Build(string* err); bool StartEdge(Edge* edge, string* err); - void FinishCommand(CommandRunner::Result* result); + + /// Update status ninja logs following a command termination. + /// @return false if the build can not proceed further due to a fatal error. + bool FinishCommand(CommandRunner::Result* result, string* err); /// Used for tests. void SetBuildLog(BuildLog* log) { diff --git a/src/deps_log.cc b/src/deps_log.cc index ce9bf06..b172d4b 100644 --- a/src/deps_log.cc +++ b/src/deps_log.cc @@ -70,8 +70,10 @@ bool DepsLog::OpenForWrite(const string& path, string* err) { return false; } } - fflush(file_); - + if (fflush(file_) != 0) { + *err = strerror(errno); + return false; + } return true; } @@ -88,12 +90,14 @@ bool DepsLog::RecordDeps(Node* node, TimeStamp mtime, // Assign ids to all nodes that are missing one. if (node->id() < 0) { - RecordId(node); + if (!RecordId(node)) + return false; made_change = true; } for (int i = 0; i < node_count; ++i) { if (nodes[i]->id() < 0) { - RecordId(nodes[i]); + if (!RecordId(nodes[i])) + return false; made_change = true; } } @@ -122,16 +126,21 @@ bool DepsLog::RecordDeps(Node* node, TimeStamp mtime, // Update on-disk representation. uint16_t size = 4 * (1 + 1 + (uint16_t)node_count); size |= 0x8000; // Deps record: set high bit. - fwrite(&size, 2, 1, file_); + if (fwrite(&size, 2, 1, file_) < 1) + return false; int id = node->id(); - fwrite(&id, 4, 1, file_); + if (fwrite(&id, 4, 1, file_) < 1) + return false; int timestamp = mtime; - fwrite(×tamp, 4, 1, file_); + if (fwrite(×tamp, 4, 1, file_) < 1) + return false; for (int i = 0; i < node_count; ++i) { id = nodes[i]->id(); - fwrite(&id, 4, 1, file_); + if (fwrite(&id, 4, 1, file_) < 1) + return false; } - fflush(file_); + if (fflush(file_) != 0) + return false; // Update in-memory representation. Deps* deps = new Deps(mtime, node_count); @@ -324,9 +333,12 @@ bool DepsLog::UpdateDeps(int out_id, Deps* deps) { bool DepsLog::RecordId(Node* node) { uint16_t size = (uint16_t)node->path().size(); - fwrite(&size, 2, 1, file_); - fwrite(node->path().data(), node->path().size(), 1, file_); - fflush(file_); + if (fwrite(&size, 2, 1, file_) < 1) + return false; + if (fwrite(node->path().data(), node->path().size(), 1, file_) < 1) + return false; // assuming node->path().size() > 0 + if (fflush(file_) != 0) + return false; node->set_id(nodes_.size()); nodes_.push_back(node); -- cgit v0.12 From b1f96969b7745e936796edf0b4a9b8c2312589e2 Mon Sep 17 00:00:00 2001 From: David Hill Date: Mon, 1 Jul 2013 16:05:30 -0400 Subject: use !defined(NOT_PPOLL) --- src/subprocess-posix.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/subprocess-posix.cc b/src/subprocess-posix.cc index cd22aa9..a9af756 100644 --- a/src/subprocess-posix.cc +++ b/src/subprocess-posix.cc @@ -224,7 +224,7 @@ bool SubprocessSet::DoWork() { return interrupted_; } -#else // linux || __OpenBSD__ || __Bitrig__ +#else // !defined(USE_PPOLL) bool SubprocessSet::DoWork() { fd_set set; int nfds = 0; @@ -266,7 +266,7 @@ bool SubprocessSet::DoWork() { return interrupted_; } -#endif // linux || __OpenBSD__ || __Bitrig__ +#endif // !defined(USE_PPOLL) Subprocess* SubprocessSet::NextFinished() { if (finished_.empty()) -- cgit v0.12 From 70f18e1138c9af638afae95b8a521c257e42ccce Mon Sep 17 00:00:00 2001 From: Maxim Kalaev Date: Tue, 2 Jul 2013 00:09:43 +0300 Subject: Adding checks for record overflow in deps_log --- src/deps_log.cc | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/deps_log.cc b/src/deps_log.cc index b172d4b..0b19e39 100644 --- a/src/deps_log.cc +++ b/src/deps_log.cc @@ -37,6 +37,9 @@ const int kCurrentVersion = 1; // buffer after every record to make sure records aren't written partially. const int kMaxBufferSize = 1 << 15; +// Record size is currently limited to 15 bit +const size_t kMaxRecordSize = (1 << 15) - 1; + DepsLog::~DepsLog() { Close(); } @@ -124,9 +127,14 @@ bool DepsLog::RecordDeps(Node* node, TimeStamp mtime, return true; // Update on-disk representation. - uint16_t size = 4 * (1 + 1 + (uint16_t)node_count); + size_t size = 4 * (1 + 1 + (uint16_t)node_count); + if (size > kMaxRecordSize) { + errno = ERANGE; + return false; + } size |= 0x8000; // Deps record: set high bit. - if (fwrite(&size, 2, 1, file_) < 1) + uint16_t size16 = (uint16_t)size; + if (fwrite(&size16, 2, 1, file_) < 1) return false; int id = node->id(); if (fwrite(&id, 4, 1, file_) < 1) @@ -332,11 +340,18 @@ bool DepsLog::UpdateDeps(int out_id, Deps* deps) { } bool DepsLog::RecordId(Node* node) { - uint16_t size = (uint16_t)node->path().size(); - if (fwrite(&size, 2, 1, file_) < 1) + size_t size = node->path().size(); + if (size > kMaxRecordSize) { + errno = ERANGE; return false; - if (fwrite(node->path().data(), node->path().size(), 1, file_) < 1) - return false; // assuming node->path().size() > 0 + } + uint16_t size16 = (uint16_t)size; + if (fwrite(&size16, 2, 1, file_) < 1) + return false; + if (fwrite(node->path().data(), node->path().size(), 1, file_) < 1) { + assert(node->path().size() > 0); + return false; + } if (fflush(file_) != 0) return false; -- cgit v0.12 From ad76e867f782e75e0fed620e7b39f7099af154a9 Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Tue, 2 Jul 2013 02:00:22 -0400 Subject: Use fwrite to print whatever the subcommand wrote Subcommands can write things like UTF-16, which some terminals can understand. printf() will interpret the null bytes as the end of the string. In particular, MSVC's assert() will print wide characters by default, and I can't find a way to disable it, leading to clang assertion failures looking like: FAILED: ...clang.exe ... Aninja: build stopped: subcommand failed. With this fix, I get the desired: FAILED: ...clang.exe ... Assertion failed: SymbolMap... ninja: build stopped: subcommand failed. --- src/line_printer.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/line_printer.cc b/src/line_printer.cc index a75eb05..3537e88 100644 --- a/src/line_printer.cc +++ b/src/line_printer.cc @@ -104,6 +104,10 @@ void LinePrinter::Print(string to_print, LineType type) { void LinePrinter::PrintOnNewLine(const string& to_print) { if (!have_blank_line_) printf("\n"); - printf("%s", to_print.c_str()); + if (!to_print.empty()) { + // Avoid printf and C strings, since the actual output might contain null + // bytes like UTF-16 does (yuck). + fwrite(&to_print[0], sizeof(char), to_print.size(), stdout); + } have_blank_line_ = to_print.empty() || *to_print.rbegin() == '\n'; } -- cgit v0.12 From ea378842971c7a5ba1af0f04f07caa7d8cad9006 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Tue, 2 Jul 2013 15:43:40 -0700 Subject: Improve error message for duplicate rules and unknown pools. Also add more tests for invalid manifests. According to gcov, these invalid inputs weren't tested before. --- src/manifest_parser.cc | 8 ++-- src/manifest_parser_test.cc | 102 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 5 deletions(-) diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc index d742331..d4f0007 100644 --- a/src/manifest_parser.cc +++ b/src/manifest_parser.cc @@ -146,10 +146,8 @@ bool ManifestParser::ParseRule(string* err) { if (!ExpectToken(Lexer::NEWLINE, err)) return false; - if (state_->LookupRule(name) != NULL) { - *err = "duplicate rule '" + name + "'"; - return false; - } + if (state_->LookupRule(name) != NULL) + return lexer_.Error("duplicate rule '" + name + "'", err); Rule* rule = new Rule(name); // XXX scoped_ptr @@ -307,7 +305,7 @@ bool ManifestParser::ParseEdge(string* err) { if (!pool_name.empty()) { Pool* pool = state_->LookupPool(pool_name); if (pool == NULL) - return lexer_.Error("unknown pool name", err); + return lexer_.Error("unknown pool name '" + pool_name + "'", err); edge->pool_ = pool; } diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc index 2638edc..b333549 100644 --- a/src/manifest_parser_test.cc +++ b/src/manifest_parser_test.cc @@ -377,6 +377,17 @@ TEST_F(ParserTest, Errors) { State state; ManifestParser parser(&state, NULL); string err; + EXPECT_FALSE(parser.ParseTest("build\n", &err)); + EXPECT_EQ("input:1: expected path\n" + "build\n" + " ^ near here" + , err); + } + + { + State state; + ManifestParser parser(&state, NULL); + string err; EXPECT_FALSE(parser.ParseTest("build x: y z\n", &err)); EXPECT_EQ("input:1: unknown build rule 'y'\n" "build x: y z\n" @@ -422,6 +433,32 @@ TEST_F(ParserTest, Errors) { ManifestParser parser(&state, NULL); string err; EXPECT_FALSE(parser.ParseTest("rule cat\n" + " command = echo\n" + "rule cat\n" + " command = echo\n", &err)); + EXPECT_EQ("input:3: duplicate rule 'cat'\n" + "rule cat\n" + " ^ near here" + , err); + } + + { + State state; + ManifestParser parser(&state, NULL); + string err; + EXPECT_FALSE(parser.ParseTest("rule cat\n" + " command = echo\n" + " rspfile = cat.rsp\n", &err)); + EXPECT_EQ( + "input:4: rspfile and rspfile_content need to be both specified\n", + err); + } + + { + State state; + ManifestParser parser(&state, NULL); + string err; + EXPECT_FALSE(parser.ParseTest("rule cat\n" " command = ${fafsd\n" "foo = bar\n", &err)); @@ -583,6 +620,71 @@ TEST_F(ParserTest, Errors) { " generator = 1\n", &err)); EXPECT_EQ("input:4: unexpected indent\n", err); } + + { + State state; + ManifestParser parser(&state, NULL); + string err; + EXPECT_FALSE(parser.ParseTest("pool\n", &err)); + EXPECT_EQ("input:1: expected pool name\n", err); + } + + { + State state; + ManifestParser parser(&state, NULL); + string err; + EXPECT_FALSE(parser.ParseTest("pool foo\n", &err)); + EXPECT_EQ("input:2: expected 'depth =' line\n", err); + } + + { + State state; + ManifestParser parser(&state, NULL); + string err; + EXPECT_FALSE(parser.ParseTest("pool foo\n" + " depth = 4\n" + "pool foo\n", &err)); + EXPECT_EQ("input:3: duplicate pool 'foo'\n" + "pool foo\n" + " ^ near here" + , err); + } + + { + State state; + ManifestParser parser(&state, NULL); + string err; + EXPECT_FALSE(parser.ParseTest("pool foo\n" + " depth = -1\n", &err)); + EXPECT_EQ("input:2: invalid pool depth\n" + " depth = -1\n" + " ^ near here" + , err); + } + + { + State state; + ManifestParser parser(&state, NULL); + string err; + EXPECT_FALSE(parser.ParseTest("pool foo\n" + " bar = 1\n", &err)); + EXPECT_EQ("input:2: unexpected variable 'bar'\n" + " bar = 1\n" + " ^ near here" + , err); + } + + { + State state; + ManifestParser parser(&state, NULL); + string err; + // Pool names are dereferenced at edge parsing time. + EXPECT_FALSE(parser.ParseTest("rule run\n" + " command = echo\n" + " pool = unnamed_pool\n" + "build out: run in\n", &err)); + EXPECT_EQ("input:5: unknown pool name 'unnamed_pool'\n", err); + } } TEST_F(ParserTest, MissingInput) { -- cgit v0.12 From 8a3a941ef6b08f53f057ef50f17893cbf27e2095 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Sun, 7 Jul 2013 12:22:38 -0700 Subject: Mention pools in the discussion of ninja's toplevel declarations. --- doc/manual.asciidoc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/manual.asciidoc b/doc/manual.asciidoc index aa5644d..2900f28 100644 --- a/doc/manual.asciidoc +++ b/doc/manual.asciidoc @@ -588,6 +588,7 @@ rule cc command = cl /showIncludes -c $in /Fo$out ---- +[[ref_pool]] Pools ~~~~~ @@ -668,6 +669,9 @@ A file is a series of declarations. A declaration can be one of: +include _path_+. The difference between these is explained below <>. +6. A pool declaration, which looks like +pool _poolname_+. Pools are explained + <>. + Lexical syntax ~~~~~~~~~~~~~~ -- cgit v0.12 From e0e12debf82124012ba84e13fe6662c812bba8b1 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Mon, 8 Jul 2013 10:47:57 -0700 Subject: Fix murmur hash implementations to work on strict alignment architectures like OpenBSD/mips64el and OpenBSD/hppa64. --- src/build_log.cc | 25 +++++++++++++------------ src/hash_map.h | 4 +++- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/build_log.cc b/src/build_log.cc index 6b73002..a040ce2 100644 --- a/src/build_log.cc +++ b/src/build_log.cc @@ -54,26 +54,27 @@ uint64_t MurmurHash64A(const void* key, size_t len) { const uint64_t m = BIG_CONSTANT(0xc6a4a7935bd1e995); const int r = 47; uint64_t h = seed ^ (len * m); - const uint64_t * data = (const uint64_t *)key; - const uint64_t * end = data + (len/8); - while (data != end) { - uint64_t k = *data++; + const unsigned char * data = (const unsigned char *)key; + while (len >= 8) { + uint64_t k; + memcpy(&k, data, sizeof k); k *= m; k ^= k >> r; k *= m; h ^= k; h *= m; + data += 8; + len -= 8; } - const unsigned char* data2 = (const unsigned char*)data; switch (len & 7) { - case 7: h ^= uint64_t(data2[6]) << 48; - case 6: h ^= uint64_t(data2[5]) << 40; - case 5: h ^= uint64_t(data2[4]) << 32; - case 4: h ^= uint64_t(data2[3]) << 24; - case 3: h ^= uint64_t(data2[2]) << 16; - case 2: h ^= uint64_t(data2[1]) << 8; - case 1: h ^= uint64_t(data2[0]); + case 7: h ^= uint64_t(data[6]) << 48; + case 6: h ^= uint64_t(data[5]) << 40; + case 5: h ^= uint64_t(data[4]) << 32; + case 4: h ^= uint64_t(data[3]) << 24; + case 3: h ^= uint64_t(data[2]) << 16; + case 2: h ^= uint64_t(data[1]) << 8; + case 1: h ^= uint64_t(data[0]); h *= m; }; h ^= h >> r; diff --git a/src/hash_map.h b/src/hash_map.h index 076f6c0..919b6fc 100644 --- a/src/hash_map.h +++ b/src/hash_map.h @@ -15,6 +15,7 @@ #ifndef NINJA_MAP_H_ #define NINJA_MAP_H_ +#include #include "string_piece.h" // MurmurHash2, by Austin Appleby @@ -26,7 +27,8 @@ unsigned int MurmurHash2(const void* key, size_t len) { unsigned int h = seed ^ len; const unsigned char * data = (const unsigned char *)key; while (len >= 4) { - unsigned int k = *(unsigned int *)data; + unsigned int k; + memcpy(&k, data, sizeof k); k *= m; k ^= k >> r; k *= m; -- cgit v0.12 From 8de46a9685f562b75319bb48263e8c787e86d48b Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Mon, 8 Jul 2013 12:34:46 -0700 Subject: Add a 'recompact' tool, which forces recompaction of the build and deps logs. This is useful for performance comparisons between two build directories. --- src/build_log.cc | 2 +- src/deps_log.cc | 2 +- src/ninja.cc | 36 ++++++++++++++++++++++++++++++++---- 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/src/build_log.cc b/src/build_log.cc index 6b73002..2478e22 100644 --- a/src/build_log.cc +++ b/src/build_log.cc @@ -109,7 +109,6 @@ BuildLog::~BuildLog() { bool BuildLog::OpenForWrite(const string& path, string* err) { if (needs_recompaction_) { - Close(); if (!Recompact(path, err)) return false; } @@ -351,6 +350,7 @@ bool BuildLog::Recompact(const string& path, string* err) { METRIC_RECORD(".ninja_log recompact"); printf("Recompacting log...\n"); + Close(); string temp_path = path + ".recompact"; FILE* f = fopen(temp_path.c_str(), "wb"); if (!f) { diff --git a/src/deps_log.cc b/src/deps_log.cc index ce9bf06..63e0d95 100644 --- a/src/deps_log.cc +++ b/src/deps_log.cc @@ -43,7 +43,6 @@ DepsLog::~DepsLog() { bool DepsLog::OpenForWrite(const string& path, string* err) { if (needs_recompaction_) { - Close(); if (!Recompact(path, err)) return false; } @@ -265,6 +264,7 @@ bool DepsLog::Recompact(const string& path, string* err) { METRIC_RECORD(".ninja_deps recompact"); printf("Recompacting deps...\n"); + Close(); string temp_path = path + ".recompact"; // OpenForWrite() opens for append. Make sure it's not appending to a diff --git a/src/ninja.cc b/src/ninja.cc index 3b381b7..0c9bee1 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -110,15 +110,16 @@ struct NinjaMain { int ToolCommands(int argc, char* argv[]); int ToolClean(int argc, char* argv[]); int ToolCompilationDatabase(int argc, char* argv[]); + int ToolRecompact(int argc, char* argv[]); int ToolUrtle(int argc, char** argv); /// Open the build log. /// @return false on error. - bool OpenBuildLog(); + bool OpenBuildLog(bool recompact_only = false); /// Open the deps log: load it, then open for writing. /// @return false on error. - bool OpenDepsLog(); + bool OpenDepsLog(bool recompact_only = false); /// Ensure the build directory exists, creating it if necessary. /// @return false on error. @@ -602,6 +603,17 @@ int NinjaMain::ToolCompilationDatabase(int argc, char* argv[]) { return 0; } +int NinjaMain::ToolRecompact(int argc, char* argv[]) { + if (!EnsureBuildDirExists()) + return 1; + + if (!OpenBuildLog(/*recompact_only=*/true) || + !OpenDepsLog(/*recompact_only=*/true)) + return 1; + + return 0; +} + int NinjaMain::ToolUrtle(int argc, char** argv) { // RLE encoded. const char* urtle = @@ -652,6 +664,8 @@ const Tool* ChooseTool(const string& tool_name) { Tool::RUN_AFTER_LOAD, &NinjaMain::ToolTargets }, { "compdb", "dump JSON compilation database to stdout", Tool::RUN_AFTER_LOAD, &NinjaMain::ToolCompilationDatabase }, + { "recompact", "recompacts ninja-internal data structures", + Tool::RUN_AFTER_LOAD, &NinjaMain::ToolRecompact }, { "urtle", NULL, Tool::RUN_AFTER_FLAGS, &NinjaMain::ToolUrtle }, { NULL, NULL, Tool::RUN_AFTER_FLAGS, NULL } @@ -712,7 +726,7 @@ bool DebugEnable(const string& name) { } } -bool NinjaMain::OpenBuildLog() { +bool NinjaMain::OpenBuildLog(bool recompact_only) { string log_path = ".ninja_log"; if (!build_dir_.empty()) log_path = build_dir_ + "/" + log_path; @@ -728,6 +742,13 @@ bool NinjaMain::OpenBuildLog() { err.clear(); } + if (recompact_only) { + bool success = build_log_.Recompact(log_path, &err); + if (!success) + Error("failed recompaction: %s", err.c_str()); + return success; + } + if (!config_.dry_run) { if (!build_log_.OpenForWrite(log_path, &err)) { Error("opening build log: %s", err.c_str()); @@ -740,7 +761,7 @@ bool NinjaMain::OpenBuildLog() { /// Open the deps log: load it, then open for writing. /// @return false on error. -bool NinjaMain::OpenDepsLog() { +bool NinjaMain::OpenDepsLog(bool recompact_only) { string path = ".ninja_deps"; if (!build_dir_.empty()) path = build_dir_ + "/" + path; @@ -756,6 +777,13 @@ bool NinjaMain::OpenDepsLog() { err.clear(); } + if (recompact_only) { + bool success = deps_log_.Recompact(path, &err); + if (!success) + Error("failed recompaction: %s", err.c_str()); + return success; + } + if (!config_.dry_run) { if (!deps_log_.OpenForWrite(path, &err)) { Error("opening deps log: %s", err.c_str()); -- cgit v0.12 From 5d94cf12741da06bfbea860441f492ebdb2e3651 Mon Sep 17 00:00:00 2001 From: Maxim Kalaev Date: Sun, 30 Jun 2013 22:59:54 +0300 Subject: Introducing tool 'deps' dumping ninja deps log entries --- src/ninja.cc | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/ninja.cc b/src/ninja.cc index 3b381b7..1e5590d 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -104,6 +104,7 @@ struct NinjaMain { // The various subcommands, run via "-t XXX". int ToolGraph(int argc, char* argv[]); int ToolQuery(int argc, char* argv[]); + int ToolDeps(int argc, char* argv[]); int ToolBrowse(int argc, char* argv[]); int ToolMSVC(int argc, char* argv[]); int ToolTargets(int argc, char* argv[]); @@ -437,6 +438,45 @@ int ToolTargetsList(State* state) { return 0; } +int NinjaMain::ToolDeps(int argc, char** argv) { + vector nodes; + if (argc == 0) { + for (vector::const_iterator ni = deps_log_.nodes().begin(); + ni != deps_log_.nodes().end(); ++ni) { + // Only query for targets with an incoming edge and deps + Edge* e = (*ni)->in_edge(); + if (e && !e->GetBinding("deps").empty()) + nodes.push_back(*ni); + } + } else { + string err; + if (!CollectTargetsFromArgs(argc, argv, &nodes, &err)) { + Error("%s", err.c_str()); + return 1; + } + } + + RealDiskInterface disk_interface; + for (vector::iterator it = nodes.begin(), end = nodes.end(); + it != end; ++it) { + DepsLog::Deps* deps = deps_log_.GetDeps(*it); + if (!deps) { + printf("%s: deps not found\n", (*it)->path().c_str()); + continue; + } + + TimeStamp mtime = disk_interface.Stat((*it)->path()); + printf("%s: #deps %d, deps mtime %d (%s)\n", + (*it)->path().c_str(), deps->node_count, deps->mtime, + (!mtime || mtime > deps->mtime ? "STALE":"VALID")); + for (int i = 0; i < deps->node_count; ++i) + printf(" %s\n", deps->nodes[i]->path().c_str()); + printf("\n"); + } + + return 0; +} + int NinjaMain::ToolTargets(int argc, char* argv[]) { int depth = 1; if (argc >= 1) { @@ -644,6 +684,8 @@ const Tool* ChooseTool(const string& tool_name) { Tool::RUN_AFTER_LOAD, &NinjaMain::ToolClean }, { "commands", "list all commands required to rebuild given targets", Tool::RUN_AFTER_LOAD, &NinjaMain::ToolCommands }, + { "deps", "show dependencies stored in the deps log", + Tool::RUN_AFTER_LOGS, &NinjaMain::ToolDeps }, { "graph", "output graphviz dot file for targets", Tool::RUN_AFTER_LOAD, &NinjaMain::ToolGraph }, { "query", "show inputs/outputs for a path", -- cgit v0.12 From adaa91a33eb2cf23b88333b5cc2e2e456a5f1803 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Wed, 10 Jul 2013 17:30:16 -0700 Subject: Reuse ManifestParser::Load() in ManifestParser::ParseFileInclude(). ParseFileInclude() was doing the work that Load() was doing. Instead, just make it call Load(). Also, remove a FIXME about using ReadPath() in ParseFileInclude() -- it's already being used. (The FIXME was added in the same commit that added the call to ReadPath() -- 8a0c96075786c19 -- it probably should've been deleted before that commit.) Also, remove a `contents.resize(contents.size() + 10);`. It's not clear what it's for, but if it was important then ManifestParser::ParseFileInclude() would have needed that call too, and it didn't have it. No intended behavior change. --- src/manifest_parser.cc | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc index d4f0007..88c16ab 100644 --- a/src/manifest_parser.cc +++ b/src/manifest_parser.cc @@ -28,6 +28,7 @@ ManifestParser::ManifestParser(State* state, FileReader* file_reader) : state_(state), file_reader_(file_reader) { env_ = &state->bindings_; } + bool ManifestParser::Load(const string& filename, string* err) { string contents; string read_err; @@ -35,7 +36,6 @@ bool ManifestParser::Load(const string& filename, string* err) { *err = "loading '" + filename + "': " + read_err; return false; } - contents.resize(contents.size() + 10); return Parse(filename, contents, err); } @@ -338,17 +338,11 @@ bool ManifestParser::ParseEdge(string* err) { } bool ManifestParser::ParseFileInclude(bool new_scope, string* err) { - // XXX this should use ReadPath! EvalString eval; if (!lexer_.ReadPath(&eval, err)) return false; string path = eval.Evaluate(env_); - string contents; - string read_err; - if (!file_reader_->ReadFile(path, &contents, &read_err)) - return lexer_.Error("loading '" + path + "': " + read_err, err); - ManifestParser subparser(state_, file_reader_); if (new_scope) { subparser.env_ = new BindingEnv(env_); @@ -356,8 +350,8 @@ bool ManifestParser::ParseFileInclude(bool new_scope, string* err) { subparser.env_ = env_; } - if (!subparser.Parse(path, contents, err)) - return false; + if (!subparser.Load(path, err)) + return lexer_.Error(string(*err), err); if (!ExpectToken(Lexer::NEWLINE, err)) return false; -- cgit v0.12 From 4c7802baf8ddead0cbfddefb1f9fa3587c6dd089 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Wed, 10 Jul 2013 17:43:29 -0700 Subject: Let the ".ninja parse" metric include the time to read the toplevel ninja file. Loads of included ninja files were covered by the ".ninja parse" in Parse() further up the stack from the toplevel file, but the load of the toplevel file wasn't counted. Fix that. --- src/manifest_parser.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc index 88c16ab..f5e7eac 100644 --- a/src/manifest_parser.cc +++ b/src/manifest_parser.cc @@ -30,6 +30,7 @@ ManifestParser::ManifestParser(State* state, FileReader* file_reader) } bool ManifestParser::Load(const string& filename, string* err) { + METRIC_RECORD(".ninja parse"); string contents; string read_err; if (!file_reader_->ReadFile(filename, &contents, &read_err)) { @@ -41,7 +42,6 @@ bool ManifestParser::Load(const string& filename, string* err) { bool ManifestParser::Parse(const string& filename, const string& input, string* err) { - METRIC_RECORD(".ninja parse"); lexer_.Start(filename, input); for (;;) { -- cgit v0.12 From cce4807703a0595fd8db8c57db61184b0d00a187 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Wed, 17 Jul 2013 17:55:36 -0700 Subject: Add back contents.resize(), but with a comment and just 1 instead of 10. --- src/manifest_parser.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc index f5e7eac..a1bb904 100644 --- a/src/manifest_parser.cc +++ b/src/manifest_parser.cc @@ -37,6 +37,14 @@ bool ManifestParser::Load(const string& filename, string* err) { *err = "loading '" + filename + "': " + read_err; return false; } + + // The lexer needs a nul byte at the end of its input, to know when it's done. + // It takes a StringPiece, and StringPiece's string constructor uses + // string::data(). data()'s return value isn't guaranteed to be + // null-terminated (although in practice - libc++, libstdc++, msvc's stl -- + // it is, and C++11 demands that too), so add an explicit nul byte. + contents.resize(contents.size() + 1); + return Parse(filename, contents, err); } -- cgit v0.12 From 6bf7c4cb6bcbc1c90fcc3b8ea365838e8417beee Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Thu, 18 Jul 2013 14:39:41 -0700 Subject: Add test for a manifest ending in the middle of a keyword. --- src/manifest_parser_test.cc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc index b333549..be63e52 100644 --- a/src/manifest_parser_test.cc +++ b/src/manifest_parser_test.cc @@ -302,6 +302,17 @@ TEST_F(ParserTest, Errors) { State state; ManifestParser parser(&state, NULL); string err; + EXPECT_FALSE(parser.ParseTest(string("subn", 4), &err)); + EXPECT_EQ("input:1: expected '=', got eof\n" + "subn\n" + " ^ near here" + , err); + } + + { + State state; + ManifestParser parser(&state, NULL); + string err; EXPECT_FALSE(parser.ParseTest("foobar", &err)); EXPECT_EQ("input:1: expected '=', got eof\n" "foobar\n" -- cgit v0.12 From a2df2c7d491ca530e16c90d8f9f7012f08c2e0ae Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Thu, 18 Jul 2013 17:51:10 -0700 Subject: Fix diagnostic formatting regression caused by adaa91a33eb2cf23b88. Ninja regressed to include a location for every file on the include stack for nested diagnostics, i.e. it would print: input:1: include.ninja:1: expected path Fix this so that it prints only the current file location, like it used to: include.ninja:1: expected path Also add a test for this. --- src/manifest_parser.cc | 8 +++++--- src/manifest_parser.h | 2 +- src/manifest_parser_test.cc | 11 +++++++++++ 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc index a1bb904..20be7f3 100644 --- a/src/manifest_parser.cc +++ b/src/manifest_parser.cc @@ -29,12 +29,14 @@ ManifestParser::ManifestParser(State* state, FileReader* file_reader) env_ = &state->bindings_; } -bool ManifestParser::Load(const string& filename, string* err) { +bool ManifestParser::Load(const string& filename, string* err, Lexer* parent) { METRIC_RECORD(".ninja parse"); string contents; string read_err; if (!file_reader_->ReadFile(filename, &contents, &read_err)) { *err = "loading '" + filename + "': " + read_err; + if (parent) + parent->Error(string(*err), err); return false; } @@ -358,8 +360,8 @@ bool ManifestParser::ParseFileInclude(bool new_scope, string* err) { subparser.env_ = env_; } - if (!subparser.Load(path, err)) - return lexer_.Error(string(*err), err); + if (!subparser.Load(path, err, &lexer_)) + return false; if (!ExpectToken(Lexer::NEWLINE, err)) return false; diff --git a/src/manifest_parser.h b/src/manifest_parser.h index 967dfdd..5212f72 100644 --- a/src/manifest_parser.h +++ b/src/manifest_parser.h @@ -35,7 +35,7 @@ struct ManifestParser { ManifestParser(State* state, FileReader* file_reader); /// Load and parse a file. - bool Load(const string& filename, string* err); + bool Load(const string& filename, string* err, Lexer* parent=NULL); /// Parse a text string of input. Used by tests. bool ParseTest(const string& input, string* err) { diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc index b333549..67c422b 100644 --- a/src/manifest_parser_test.cc +++ b/src/manifest_parser_test.cc @@ -762,6 +762,17 @@ TEST_F(ParserTest, Include) { EXPECT_EQ("inner", state.bindings_.LookupVariable("var")); } +TEST_F(ParserTest, BrokenInclude) { + files_["include.ninja"] = "build\n"; + ManifestParser parser(&state, this); + string err; + EXPECT_FALSE(parser.ParseTest("include include.ninja\n", &err)); + EXPECT_EQ("include.ninja:1: expected path\n" + "build\n" + " ^ near here" + , err); +} + TEST_F(ParserTest, Implicit) { ASSERT_NO_FATAL_FAILURE(AssertParse( "rule cat\n" -- cgit v0.12 From b209096873434eaee96068251d02b352ff58eb8f Mon Sep 17 00:00:00 2001 From: Maxim Kalaev Date: Thu, 27 Jun 2013 21:55:38 +0300 Subject: Removing a redundant stat() call with 'deps' and 'restat = 1' --- src/build.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/build.cc b/src/build.cc index 5cf9d27..52dac87 100644 --- a/src/build.cc +++ b/src/build.cc @@ -763,7 +763,7 @@ void Builder::FinishCommand(CommandRunner::Result* result) { } string depfile = edge->GetBinding("depfile"); - if (restat_mtime != 0 && !depfile.empty()) { + if (restat_mtime != 0 && deps_type.empty() && !depfile.empty()) { TimeStamp depfile_mtime = disk_interface_->Stat(depfile); if (depfile_mtime > restat_mtime) restat_mtime = depfile_mtime; -- cgit v0.12 From d864d8446ce465781127294f6f2d5efa86645967 Mon Sep 17 00:00:00 2001 From: Maxim Kalaev Date: Fri, 28 Jun 2013 20:48:30 +0300 Subject: minor: removing noop call to MarkDirty(), fixing comment --- src/build.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/build.cc b/src/build.cc index 52dac87..2fbfdec 100644 --- a/src/build.cc +++ b/src/build.cc @@ -431,14 +431,13 @@ void Plan::CleanNode(DependencyScan* scan, Node* node) { if (scan->RecomputeOutputDirty(*ei, most_recent_input, 0, command, *ni)) { - (*ni)->MarkDirty(); all_outputs_clean = false; } else { CleanNode(scan, *ni); } } - // If we cleaned all outputs, mark the node as not wanted. + // If we cleaned all outputs, mark the edge as not wanted. if (all_outputs_clean) { want_i->second = false; --wanted_edges_; -- cgit v0.12 From 9635dae5558b2bb04f021699ed66b1f6f7634acd Mon Sep 17 00:00:00 2001 From: Chris Hopman Date: Wed, 3 Jul 2013 14:26:25 -0700 Subject: Fix restat dirty check logic for edges with multiple outputs In a normal dependency scan (see DependencyScan::RecomputeDirty) we mark all outputs of an Edge as dirty if any of the outputs is dirty. This is the correct behavior because if any output is dirty, we will run the command for that Edge and that can touch any of the outputs of the Edge and so all the outputs should be marked dirty. When updating the dirty state of Node's for a restat check, we were not applying this logic, instead only those outputs that were actually "dirty" were marked dirty. Before this patch, restat edges with multiple outputs caused not all dependent edges to run. --- src/build.cc | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/build.cc b/src/build.cc index 2fbfdec..143aeb2 100644 --- a/src/build.cc +++ b/src/build.cc @@ -422,23 +422,22 @@ void Plan::CleanNode(DependencyScan* scan, Node* node) { } string command = (*ei)->EvaluateCommand(true); - // Now, recompute the dirty state of each output. - bool all_outputs_clean = true; + // Now, this edge is dirty if any of the outputs are dirty. + bool dirty = false; for (vector::iterator ni = (*ei)->outputs_.begin(); - ni != (*ei)->outputs_.end(); ++ni) { - if (!(*ni)->dirty()) - continue; + !dirty && ni != (*ei)->outputs_.end(); ++ni) { + dirty = scan->RecomputeOutputDirty(*ei, most_recent_input, 0, + command, *ni); + } - if (scan->RecomputeOutputDirty(*ei, most_recent_input, 0, - command, *ni)) { - all_outputs_clean = false; - } else { + // If the edge isn't dirty, clean the outputs and mark the edge as not + // wanted. + if (!dirty) { + for (vector::iterator ni = (*ei)->outputs_.begin(); + ni != (*ei)->outputs_.end(); ++ni) { CleanNode(scan, *ni); } - } - // If we cleaned all outputs, mark the edge as not wanted. - if (all_outputs_clean) { want_i->second = false; --wanted_edges_; if (!(*ei)->is_phony()) -- cgit v0.12 From 88d1b073af89a28edaa223d20bfa63356606916f Mon Sep 17 00:00:00 2001 From: Chris Hopman Date: Tue, 16 Jul 2013 16:00:28 -0700 Subject: Add test for correct restat logic When a single output of an edge is dirty, the restat check should leave all outputs of that edge dirty. --- src/build_test.cc | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/build_test.cc b/src/build_test.cc index 313a386..d1ac0ef 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -1094,6 +1094,46 @@ TEST_F(BuildWithLogTest, RestatMissingFile) { ASSERT_EQ(1u, command_runner_.commands_ran_.size()); } +TEST_F(BuildWithLogTest, RestatSingleDependentOutputDirty) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, + "rule true\n" + " command = true\n" + " restat = 1\n" + "rule touch\n" + " command = touch\n" + "build out1: true in\n" + "build out2 out3: touch out1\n" + "build out4: touch out2\n" + )); + + // Create the necessary files + fs_.Create("in", ""); + + string err; + EXPECT_TRUE(builder_.AddTarget("out4", &err)); + ASSERT_EQ("", err); + EXPECT_TRUE(builder_.Build(&err)); + ASSERT_EQ("", err); + ASSERT_EQ(3u, command_runner_.commands_ran_.size()); + + fs_.Tick(); + fs_.Create("in", ""); + fs_.RemoveFile("out3"); + + // Since "in" is missing, out1 will be built. Since "out3" is missing, + // out2 and out3 will be built even though "in" is not touched when built. + // Then, since out2 is rebuilt, out4 should be rebuilt -- the restat on the + // "true" rule should not lead to the "touch" edge writing out2 and out3 being + // cleard. + command_runner_.commands_ran_.clear(); + state_.Reset(); + EXPECT_TRUE(builder_.AddTarget("out4", &err)); + ASSERT_EQ("", err); + EXPECT_TRUE(builder_.Build(&err)); + ASSERT_EQ("", err); + ASSERT_EQ(3u, command_runner_.commands_ran_.size()); +} + // Test scenario, in which an input file is removed, but output isn't changed // https://github.com/martine/ninja/issues/295 TEST_F(BuildWithLogTest, RestatMissingInput) { -- cgit v0.12 From 2eecb07bdaedca1a37cf499ce7d6e6491e2046c8 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Wed, 24 Jul 2013 14:07:12 -0700 Subject: Build log write error checking. Like f6f00aa40f0c541df06, but for the build log instead of the deps log. --- src/build.cc | 7 +++++-- src/build_log.cc | 21 ++++++++++++++------- src/build_log.h | 4 ++-- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/build.cc b/src/build.cc index 67e4634..45f6849 100644 --- a/src/build.cc +++ b/src/build.cc @@ -784,8 +784,11 @@ bool Builder::FinishCommand(CommandRunner::Result* result, string* err) { disk_interface_->RemoveFile(rspfile); if (scan_.build_log()) { - scan_.build_log()->RecordCommand(edge, start_time, end_time, - restat_mtime); + if (!scan_.build_log()->RecordCommand(edge, start_time, end_time, + restat_mtime)) { + *err = string("Error writing to build log: ") + strerror(errno); + return false; + } } if (!deps_type.empty() && !config_.dry_run) { diff --git a/src/build_log.cc b/src/build_log.cc index 6fb179a..1374bd0 100644 --- a/src/build_log.cc +++ b/src/build_log.cc @@ -136,7 +136,7 @@ bool BuildLog::OpenForWrite(const string& path, string* err) { return true; } -void BuildLog::RecordCommand(Edge* edge, int start_time, int end_time, +bool BuildLog::RecordCommand(Edge* edge, int start_time, int end_time, TimeStamp restat_mtime) { string command = edge->EvaluateCommand(true); uint64_t command_hash = LogEntry::HashCommand(command); @@ -156,9 +156,12 @@ void BuildLog::RecordCommand(Edge* edge, int start_time, int end_time, log_entry->end_time = end_time; log_entry->restat_mtime = restat_mtime; - if (log_file_) - WriteEntry(log_file_, *log_entry); + if (log_file_) { + if (!WriteEntry(log_file_, *log_entry)) + return false; + } } + return true; } void BuildLog::Close() { @@ -341,10 +344,10 @@ BuildLog::LogEntry* BuildLog::LookupByOutput(const string& path) { return NULL; } -void BuildLog::WriteEntry(FILE* f, const LogEntry& entry) { - fprintf(f, "%d\t%d\t%d\t%s\t%" PRIx64 "\n", +bool BuildLog::WriteEntry(FILE* f, const LogEntry& entry) { + return fprintf(f, "%d\t%d\t%d\t%s\t%" PRIx64 "\n", entry.start_time, entry.end_time, entry.restat_mtime, - entry.output.c_str(), entry.command_hash); + entry.output.c_str(), entry.command_hash) > 0; } bool BuildLog::Recompact(const string& path, string* err) { @@ -366,7 +369,11 @@ bool BuildLog::Recompact(const string& path, string* err) { } for (Entries::iterator i = entries_.begin(); i != entries_.end(); ++i) { - WriteEntry(f, *i->second); + if (!WriteEntry(f, *i->second)) { + *err = strerror(errno); + fclose(f); + return false; + } } fclose(f); diff --git a/src/build_log.h b/src/build_log.h index 6eae89f..eeac5b3 100644 --- a/src/build_log.h +++ b/src/build_log.h @@ -37,7 +37,7 @@ struct BuildLog { ~BuildLog(); bool OpenForWrite(const string& path, string* err); - void RecordCommand(Edge* edge, int start_time, int end_time, + bool RecordCommand(Edge* edge, int start_time, int end_time, TimeStamp restat_mtime = 0); void Close(); @@ -69,7 +69,7 @@ struct BuildLog { LogEntry* LookupByOutput(const string& path); /// Serialize an entry into a log file. - void WriteEntry(FILE* f, const LogEntry& entry); + bool WriteEntry(FILE* f, const LogEntry& entry); /// Rewrite the known log entries, throwing away old data. bool Recompact(const string& path, string* err); -- cgit v0.12 From 66822baa2b1a58e40dc886158099f1a691b095a2 Mon Sep 17 00:00:00 2001 From: Tony Chang Date: Fri, 26 Jul 2013 14:24:52 -0700 Subject: Fix the browse tool. ReadFlags was altering argv in real_main causing us to pass the wrong value into the NinjaMain constructor. Fix this by storing the ninja command before real_main runs. --- src/ninja.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ninja.cc b/src/ninja.cc index 80d68e7..0586bdc 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -1006,6 +1006,7 @@ int real_main(int argc, char** argv) { options.input_file = "build.ninja"; setvbuf(stdout, NULL, _IOLBF, BUFSIZ); + const char* ninja_command = argv[0]; int exit_code = ReadFlags(&argc, &argv, &options, &config); if (exit_code >= 0) @@ -1014,7 +1015,7 @@ int real_main(int argc, char** argv) { if (options.tool && options.tool->when == Tool::RUN_AFTER_FLAGS) { // None of the RUN_AFTER_FLAGS actually use a NinjaMain, but it's needed // by other tools. - NinjaMain ninja(argv[0], config); + NinjaMain ninja(ninja_command, config); return (ninja.*options.tool->func)(argc, argv); } @@ -1034,7 +1035,7 @@ int real_main(int argc, char** argv) { // The build can take up to 2 passes: one to rebuild the manifest, then // another to build the desired target. for (int cycle = 0; cycle < 2; ++cycle) { - NinjaMain ninja(argv[0], config); + NinjaMain ninja(ninja_command, config); RealFileReader file_reader; ManifestParser parser(&ninja.state_, &file_reader); -- cgit v0.12 From 3913d73e2025555d84f72a7c2d9b31b300535900 Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Fri, 26 Jul 2013 15:27:30 -0700 Subject: Use fwrite in the msvc tool instead of printf This allows wide characters from the compiler to propagate through the msvc tool. Similar to ad76e867f782e75e0fed620e7b39f7099af154a9. --- src/msvc_helper_main-win32.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/msvc_helper_main-win32.cc b/src/msvc_helper_main-win32.cc index 8a0479c..ff9bc8e 100644 --- a/src/msvc_helper_main-win32.cc +++ b/src/msvc_helper_main-win32.cc @@ -129,7 +129,9 @@ int MSVCHelperMain(int argc, char** argv) { // CLWrapper's output already as \r\n line endings, make sure the C runtime // doesn't expand this to \r\r\n. _setmode(_fileno(stdout), _O_BINARY); - printf("%s", output.c_str()); + // Avoid printf and C strings, since the actual output might contain null + // bytes like UTF-16 does (yuck). + fwrite(&output[0], sizeof(char), output.size(), stdout); return exit_code; } -- cgit v0.12 From ba71bc32312800fa1bc897d6b8c0280bee488acd Mon Sep 17 00:00:00 2001 From: Kim Grasman Date: Mon, 5 Aug 2013 17:52:51 +0200 Subject: Fix tabs/spaces inconsistency. --- platform_helper.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/platform_helper.py b/platform_helper.py index e615660..b7447a1 100644 --- a/platform_helper.py +++ b/platform_helper.py @@ -41,9 +41,8 @@ class Platform( object ): self._platform = 'mingw' elif self._platform.startswith('win'): self._platform = 'msvc' - elif self._platform.startswith('bitrig'): - self._platform = 'bitrig' - + elif self._platform.startswith('bitrig'): + self._platform = 'bitrig' def platform(self): return self._platform -- cgit v0.12 From 3f03746c458a92cfb6d2c8a89169ead8db5aaf1f Mon Sep 17 00:00:00 2001 From: Richard Geary Date: Sat, 10 Aug 2013 20:13:41 -0400 Subject: Add to .gitignore : SublimeText project files, *.patch files, ..ninja_deps, .ninja_log --- .gitignore | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.gitignore b/.gitignore index 3cee921..501a02d 100644 --- a/.gitignore +++ b/.gitignore @@ -15,8 +15,16 @@ TAGS /doc/manual.html /doc/doxygen /gtest-1.6.0 +*.patch # Eclipse project files .project .cproject +# SublimeText project files +*.sublime-project +*.sublime-workspace + +# Ninja output +.ninja_deps +.ninja_log -- cgit v0.12 From b78b27cba23e3bd2fce1cea4bf33a85eb645113d Mon Sep 17 00:00:00 2001 From: Richard Geary Date: Sat, 10 Aug 2013 20:14:49 -0400 Subject: Fix for missing "no work to do." message if all build edges are phony rules. Added NestedPhonyPrintsDone unit test --- src/build.cc | 5 ++++- src/build.h | 2 +- src/graph_test.cc | 20 ++++++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/build.cc b/src/build.cc index 45f6849..6d23f3b 100644 --- a/src/build.cc +++ b/src/build.cc @@ -362,8 +362,11 @@ void Plan::ResumeDelayedJobs(Edge* edge) { void Plan::EdgeFinished(Edge* edge) { map::iterator i = want_.find(edge); assert(i != want_.end()); - if (i->second) + if (i->second) { --wanted_edges_; + if (!edge->is_phony()) + --command_edges_; + } want_.erase(i); edge->outputs_ready_ = true; diff --git a/src/build.h b/src/build.h index 33df7d0..a872f6c 100644 --- a/src/build.h +++ b/src/build.h @@ -51,7 +51,7 @@ struct Plan { Edge* FindWork(); /// Returns true if there's more work to be done. - bool more_to_do() const { return wanted_edges_; } + bool more_to_do() const { return (command_edges_ > 0); } /// Dumps the current state of the plan. void Dump(); diff --git a/src/graph_test.cc b/src/graph_test.cc index 63d5757..8521216 100644 --- a/src/graph_test.cc +++ b/src/graph_test.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "graph.h" +#include "build.h" #include "test.h" @@ -226,3 +227,22 @@ TEST_F(GraphTest, DepfileOverrideParent) { Edge* edge = GetNode("out")->in_edge(); EXPECT_EQ("depfile is y", edge->GetBinding("command")); } + +// Verify that building a nested phony rule prints "no work to do" +TEST_F(GraphTest, NestedPhonyPrintsDone) { + AssertParse(&state_, +"build n1: phony \n" +"build n2: phony n1\n" + ); + string err; + Edge* edge = GetNode("n2")->in_edge(); + EXPECT_TRUE(scan_.RecomputeDirty(edge, &err)); + ASSERT_EQ("", err); + + Plan plan_; + EXPECT_TRUE(plan_.AddTarget(GetNode("n2"), &err)); + ASSERT_EQ("", err); + + EXPECT_EQ(0, plan_.command_edge_count()); + ASSERT_FALSE(plan_.more_to_do()); +} -- cgit v0.12 From 3382976433fa09b4b069c25242ec29e302a4c691 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johan=20Sundstr=C3=B6m?= Date: Mon, 12 Aug 2013 18:47:06 -0700 Subject: Proof reading --- doc/manual.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/manual.asciidoc b/doc/manual.asciidoc index 2900f28..a735257 100644 --- a/doc/manual.asciidoc +++ b/doc/manual.asciidoc @@ -54,7 +54,7 @@ Here are the design goals of Ninja: higher-level build systems have different opinions about how code should be built; for example, should built objects live alongside the sources or should all build output go into a separate directory? - Is there an "package" rule that builds a distributable package of + Is there a "package" rule that builds a distributable package of the project? Sidestep these decisions by trying to allow either to be implemented, rather than choosing, even if that results in more verbosity. -- cgit v0.12 From 808ba411d4484617363b79f175569d6ae2f86c01 Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Mon, 19 Aug 2013 16:44:16 -0700 Subject: Avoid indexing into an empty string. --- src/msvc_helper_main-win32.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/msvc_helper_main-win32.cc b/src/msvc_helper_main-win32.cc index ff9bc8e..e3a7846 100644 --- a/src/msvc_helper_main-win32.cc +++ b/src/msvc_helper_main-win32.cc @@ -126,12 +126,15 @@ int MSVCHelperMain(int argc, char** argv) { WriteDepFileOrDie(output_filename, parser); } + if (output.empty()) + return exit_code; + // CLWrapper's output already as \r\n line endings, make sure the C runtime // doesn't expand this to \r\r\n. _setmode(_fileno(stdout), _O_BINARY); // Avoid printf and C strings, since the actual output might contain null // bytes like UTF-16 does (yuck). - fwrite(&output[0], sizeof(char), output.size(), stdout); + fwrite(&output[0], 1, output.size(), stdout); return exit_code; } -- cgit v0.12 From 9e9102a8b84ffea87e2afc97c8fd1981e8742274 Mon Sep 17 00:00:00 2001 From: Richard Geary Date: Thu, 22 Aug 2013 18:08:12 -0400 Subject: Fix for incorrect number of total edges in the status message --- src/build.cc | 5 +---- src/build.h | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/build.cc b/src/build.cc index 6d23f3b..45f6849 100644 --- a/src/build.cc +++ b/src/build.cc @@ -362,11 +362,8 @@ void Plan::ResumeDelayedJobs(Edge* edge) { void Plan::EdgeFinished(Edge* edge) { map::iterator i = want_.find(edge); assert(i != want_.end()); - if (i->second) { + if (i->second) --wanted_edges_; - if (!edge->is_phony()) - --command_edges_; - } want_.erase(i); edge->outputs_ready_ = true; diff --git a/src/build.h b/src/build.h index a872f6c..aa96512 100644 --- a/src/build.h +++ b/src/build.h @@ -51,7 +51,7 @@ struct Plan { Edge* FindWork(); /// Returns true if there's more work to be done. - bool more_to_do() const { return (command_edges_ > 0); } + bool more_to_do() const { return (wanted_edges_ > 0) && (command_edges_ > 0); } /// Dumps the current state of the plan. void Dump(); -- cgit v0.12 From 3e6a3bcc139f15800f69eca27c5fda74b01c5563 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Thu, 22 Aug 2013 19:24:04 -0400 Subject: Add a test for issue #638. --- src/build_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/build_test.cc b/src/build_test.cc index d1ac0ef..94f3994 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -1018,6 +1018,7 @@ TEST_F(BuildWithLogTest, RestatTest) { ASSERT_EQ("", err); EXPECT_TRUE(builder_.Build(&err)); ASSERT_EQ("", err); + EXPECT_EQ("[3/3]", builder_.status_->FormatProgressStatus("[%s/%t]")); command_runner_.commands_ran_.clear(); state_.Reset(); -- cgit v0.12 From de3380aab6e18f4f31653f0efa502ad31db93797 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Fri, 23 Aug 2013 15:20:55 -0400 Subject: Minor style fixes, no behavior change. --- src/build.h | 2 +- src/build_log.cc | 2 +- src/build_test.cc | 4 ++-- src/disk_interface.cc | 2 +- src/hash_map.h | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/build.h b/src/build.h index aa96512..5b6c83c 100644 --- a/src/build.h +++ b/src/build.h @@ -51,7 +51,7 @@ struct Plan { Edge* FindWork(); /// Returns true if there's more work to be done. - bool more_to_do() const { return (wanted_edges_ > 0) && (command_edges_ > 0); } + bool more_to_do() const { return wanted_edges_ > 0 && command_edges_ > 0; } /// Dumps the current state of the plan. void Dump(); diff --git a/src/build_log.cc b/src/build_log.cc index 1374bd0..b92a06f 100644 --- a/src/build_log.cc +++ b/src/build_log.cc @@ -54,7 +54,7 @@ uint64_t MurmurHash64A(const void* key, size_t len) { const uint64_t m = BIG_CONSTANT(0xc6a4a7935bd1e995); const int r = 47; uint64_t h = seed ^ (len * m); - const unsigned char * data = (const unsigned char *)key; + const unsigned char* data = (const unsigned char*)key; while (len >= 8) { uint64_t k; memcpy(&k, data, sizeof k); diff --git a/src/build_test.cc b/src/build_test.cc index 94f3994..66d0954 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -1167,7 +1167,7 @@ TEST_F(BuildWithLogTest, RestatMissingInput) { // See that an entry in the logfile is created, capturing // the right mtime - BuildLog::LogEntry * log_entry = build_log_.LookupByOutput("out1"); + BuildLog::LogEntry* log_entry = build_log_.LookupByOutput("out1"); ASSERT_TRUE(NULL != log_entry); ASSERT_EQ(restat_mtime, log_entry->restat_mtime); @@ -1338,7 +1338,7 @@ TEST_F(BuildWithLogTest, RspFileCmdLineChange) { // 3. Alter the entry in the logfile // (to simulate a change in the command line between 2 builds) - BuildLog::LogEntry * log_entry = build_log_.LookupByOutput("out"); + BuildLog::LogEntry* log_entry = build_log_.LookupByOutput("out"); ASSERT_TRUE(NULL != log_entry); ASSERT_NO_FATAL_FAILURE(AssertHash( "cat out.rsp > out;rspfile=Original very long command", diff --git a/src/disk_interface.cc b/src/disk_interface.cc index ee3e99a..3233144 100644 --- a/src/disk_interface.cc +++ b/src/disk_interface.cc @@ -121,7 +121,7 @@ TimeStamp RealDiskInterface::Stat(const string& path) { } bool RealDiskInterface::WriteFile(const string& path, const string& contents) { - FILE * fp = fopen(path.c_str(), "w"); + FILE* fp = fopen(path.c_str(), "w"); if (fp == NULL) { Error("WriteFile(%s): Unable to create file. %s", path.c_str(), strerror(errno)); diff --git a/src/hash_map.h b/src/hash_map.h index 919b6fc..c63aa88 100644 --- a/src/hash_map.h +++ b/src/hash_map.h @@ -25,7 +25,7 @@ unsigned int MurmurHash2(const void* key, size_t len) { const unsigned int m = 0x5bd1e995; const int r = 24; unsigned int h = seed ^ len; - const unsigned char * data = (const unsigned char *)key; + const unsigned char* data = (const unsigned char*)key; while (len >= 4) { unsigned int k; memcpy(&k, data, sizeof k); -- cgit v0.12 From 65ad26801e19b35a386e82491ed32085adbf1a85 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Mon, 26 Aug 2013 15:19:21 -0700 Subject: Suffix depslog path records with their expected index. ninja assumes that numbering path entries in the deps log in order produces valid dense integer ids. If two ninja processes write to the same deps log concurrently, this is not true. Store the expected indices of path records in the log and treat the rest of a deps log as invalid if the dense id of a path record doesn't match the expected id stored in the log. Addresses the rest of issue #595. This requires bumping the deps log file format version. Do not migrate the old version to the new, as the old format did not have this protection, and might hence contain invalid data. (Unlikely, but possible.) Also store the record id as one's complement, to make them look less like regular deps record values. Since each record is written atomically this isn't really necessary, but it makes the format somewhat more robust (and easier to read in a hex editor). --- src/deps_log.cc | 37 +++++++++++++++++++++++++++++++------ src/deps_log.h | 4 +++- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/deps_log.cc b/src/deps_log.cc index 2c4e3c2..ee49d6b 100644 --- a/src/deps_log.cc +++ b/src/deps_log.cc @@ -30,7 +30,7 @@ // The version is stored as 4 bytes after the signature and also serves as a // byte order mark. Signature and version combined are 16 bytes long. const char kFileSignature[] = "# ninjadeps\n"; -const int kCurrentVersion = 1; +const int kCurrentVersion = 2; // Since the size field is 2 bytes and the top bit marks deps entries, a single // record can be at most 32 kB. Set the buffer size to this and flush the file @@ -179,9 +179,15 @@ bool DepsLog::Load(const string& path, State* state, string* err) { int version = 0; if (!fgets(buf, sizeof(buf), f) || fread(&version, 4, 1, f) < 1) valid_header = false; + // Note: For version differences, this should migrate to the new format. + // But the v1 format could sometimes (rarely) end up with invalid data, so + // don't migrate v1 to v2 to force a rebuild. if (!valid_header || strcmp(buf, kFileSignature) != 0 || version != kCurrentVersion) { - *err = "bad deps log signature or version; starting over"; + if (version == 1) + *err = "deps log potentially corrupt; rebuilding"; + else + *err = "bad deps log signature or version; starting over"; fclose(f); unlink(path.c_str()); // Don't report this as a failure. An empty deps log will cause @@ -229,10 +235,25 @@ bool DepsLog::Load(const string& path, State* state, string* err) { if (!UpdateDeps(out_id, deps)) ++unique_dep_record_count; } else { - StringPiece path(buf, size); + int path_size = size - 4; + StringPiece path(buf, path_size); Node* node = state->GetNode(path); + + // Check that the expected index matches the actual index. This can only + // happen if two ninja processes write to the same deps log concurrently. + // (This uses unary complement to make the checksum look less like a + // dependency record entry.) + unsigned checksum; + memcpy(&checksum, buf + path_size, sizeof checksum); + int expected_id = ~checksum; + int id = nodes_.size(); + if (id != expected_id) { + read_failed = true; + break; + } + assert(node->id() < 0); - node->set_id(nodes_.size()); + node->set_id(id); nodes_.push_back(node); } } @@ -340,7 +361,7 @@ bool DepsLog::UpdateDeps(int out_id, Deps* deps) { } bool DepsLog::RecordId(Node* node) { - size_t size = node->path().size(); + size_t size = node->path().size() + 4; if (size > kMaxRecordSize) { errno = ERANGE; return false; @@ -352,10 +373,14 @@ bool DepsLog::RecordId(Node* node) { assert(node->path().size() > 0); return false; } + int id = nodes_.size(); + unsigned checksum = ~(unsigned)id; + if (fwrite(&checksum, 4, 1, file_) < 1) + return false; if (fflush(file_) != 0) return false; - node->set_id(nodes_.size()); + node->set_id(id); nodes_.push_back(node); return true; diff --git a/src/deps_log.h b/src/deps_log.h index de0fe63..47ade03 100644 --- a/src/deps_log.h +++ b/src/deps_log.h @@ -52,7 +52,9 @@ struct State; /// Concretely, a record is: /// two bytes record length, high bit indicates record type /// (implies max record length 32k) -/// path records contain just the string name of the path +/// path records contain the string name of the path, followed by the +/// one's complement of the expected index of the record (to detect +/// concurrent writes of multiple ninja processes to the log). /// dependency records are an array of 4-byte integers /// [output path id, output path mtime, input path id, input path id...] /// (The mtime is compared against the on-disk output path mtime -- cgit v0.12 From 9940196d57d46eb8aeb1cc28a57dee290034fccf Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Wed, 28 Aug 2013 13:29:02 -0700 Subject: Deps log format v3 This fixes two bugs. 1.) It makes the record size field 4 byte instead of 2, and allows a max record size of 512kB. This fixes #605, ninja "only" supporting 8k dependencies per file -- 512kB have room for 128k dependencies. (If that's still not enough, the current design can support at least 4x that if the constant is tweaked.) 2.) It makes all records 4-byte aligned, which is probably needed to make the `reinterpret_cast(buf)` work on SPARC (cf issue #481), and it's generally nicer too. (Since there's already one reinterpret_cast, convert a memcpy() to reinterpret_cast in another branch too). --- src/deps_log.cc | 59 +++++++++++++++++++++++++++++++-------------------------- src/deps_log.h | 7 ++++--- 2 files changed, 36 insertions(+), 30 deletions(-) diff --git a/src/deps_log.cc b/src/deps_log.cc index ee49d6b..a5d726c 100644 --- a/src/deps_log.cc +++ b/src/deps_log.cc @@ -30,15 +30,11 @@ // The version is stored as 4 bytes after the signature and also serves as a // byte order mark. Signature and version combined are 16 bytes long. const char kFileSignature[] = "# ninjadeps\n"; -const int kCurrentVersion = 2; +const int kCurrentVersion = 3; -// Since the size field is 2 bytes and the top bit marks deps entries, a single -// record can be at most 32 kB. Set the buffer size to this and flush the file -// buffer after every record to make sure records aren't written partially. -const int kMaxBufferSize = 1 << 15; - -// Record size is currently limited to 15 bit -const size_t kMaxRecordSize = (1 << 15) - 1; +// Record size is currently limited to less than the full 32 bit, due to +// internal buffers having to have this size. +const unsigned kMaxRecordSize = (1 << 19) - 1; DepsLog::~DepsLog() { Close(); @@ -55,7 +51,9 @@ bool DepsLog::OpenForWrite(const string& path, string* err) { *err = strerror(errno); return false; } - setvbuf(file_, NULL, _IOFBF, kMaxBufferSize); + // Set the buffer size to this and flush the file buffer after every record + // to make sure records aren't written partially. + setvbuf(file_, NULL, _IOFBF, kMaxRecordSize + 1); SetCloseOnExec(fileno(file_)); // Opening a file in append mode doesn't set the file pointer to the file's @@ -126,14 +124,13 @@ bool DepsLog::RecordDeps(Node* node, TimeStamp mtime, return true; // Update on-disk representation. - size_t size = 4 * (1 + 1 + (uint16_t)node_count); + unsigned size = 4 * (1 + 1 + (uint16_t)node_count); if (size > kMaxRecordSize) { errno = ERANGE; return false; } - size |= 0x8000; // Deps record: set high bit. - uint16_t size16 = (uint16_t)size; - if (fwrite(&size16, 2, 1, file_) < 1) + size |= 0x80000000; // Deps record: set high bit. + if (fwrite(&size, 4, 1, file_) < 1) return false; int id = node->id(); if (fwrite(&id, 4, 1, file_) < 1) @@ -147,7 +144,7 @@ bool DepsLog::RecordDeps(Node* node, TimeStamp mtime, return false; } if (fflush(file_) != 0) - return false; + return false; // Update in-memory representation. Deps* deps = new Deps(mtime, node_count); @@ -166,7 +163,7 @@ void DepsLog::Close() { bool DepsLog::Load(const string& path, State* state, string* err) { METRIC_RECORD(".ninja_deps load"); - char buf[32 << 10]; + char buf[kMaxRecordSize + 1]; FILE* f = fopen(path.c_str(), "rb"); if (!f) { if (errno == ENOENT) @@ -181,7 +178,8 @@ bool DepsLog::Load(const string& path, State* state, string* err) { valid_header = false; // Note: For version differences, this should migrate to the new format. // But the v1 format could sometimes (rarely) end up with invalid data, so - // don't migrate v1 to v2 to force a rebuild. + // don't migrate v1 to v3 to force a rebuild. (v2 only existed for a few days, + // and there was no release with it, so pretend that it never happened.) if (!valid_header || strcmp(buf, kFileSignature) != 0 || version != kCurrentVersion) { if (version == 1) @@ -202,16 +200,16 @@ bool DepsLog::Load(const string& path, State* state, string* err) { for (;;) { offset = ftell(f); - uint16_t size; - if (fread(&size, 2, 1, f) < 1) { + unsigned size; + if (fread(&size, 4, 1, f) < 1) { if (!feof(f)) read_failed = true; break; } - bool is_deps = (size >> 15) != 0; - size = size & 0x7FFF; + bool is_deps = (size >> 31) != 0; + size = size & 0x7FFFFFFF; - if (fread(buf, size, 1, f) < 1) { + if (fread(buf, size, 1, f) < 1 || size > kMaxRecordSize) { read_failed = true; break; } @@ -236,6 +234,10 @@ bool DepsLog::Load(const string& path, State* state, string* err) { ++unique_dep_record_count; } else { int path_size = size - 4; + // There can be up to 3 bytes of padding. + if (buf[path_size - 1] == '\0') --path_size; + if (buf[path_size - 1] == '\0') --path_size; + if (buf[path_size - 1] == '\0') --path_size; StringPiece path(buf, path_size); Node* node = state->GetNode(path); @@ -243,8 +245,7 @@ bool DepsLog::Load(const string& path, State* state, string* err) { // happen if two ninja processes write to the same deps log concurrently. // (This uses unary complement to make the checksum look less like a // dependency record entry.) - unsigned checksum; - memcpy(&checksum, buf + path_size, sizeof checksum); + unsigned checksum = *reinterpret_cast(buf + size - 4); int expected_id = ~checksum; int id = nodes_.size(); if (id != expected_id) { @@ -361,18 +362,22 @@ bool DepsLog::UpdateDeps(int out_id, Deps* deps) { } bool DepsLog::RecordId(Node* node) { - size_t size = node->path().size() + 4; + int path_size = node->path().size(); + int padding = (4 - path_size % 4) % 4; // Pad path to 4 byte boundary. + + unsigned size = path_size + padding + 4; if (size > kMaxRecordSize) { errno = ERANGE; return false; } - uint16_t size16 = (uint16_t)size; - if (fwrite(&size16, 2, 1, file_) < 1) + if (fwrite(&size, 4, 1, file_) < 1) return false; - if (fwrite(node->path().data(), node->path().size(), 1, file_) < 1) { + if (fwrite(node->path().data(), path_size, 1, file_) < 1) { assert(node->path().size() > 0); return false; } + if (padding && fwrite("\0\0", padding, 1, file_) < 1) + return false; int id = nodes_.size(); unsigned checksum = ~(unsigned)id; if (fwrite(&checksum, 4, 1, file_) < 1) diff --git a/src/deps_log.h b/src/deps_log.h index 47ade03..babf828 100644 --- a/src/deps_log.h +++ b/src/deps_log.h @@ -50,9 +50,10 @@ struct State; /// A dependency list maps an output id to a list of input ids. /// /// Concretely, a record is: -/// two bytes record length, high bit indicates record type -/// (implies max record length 32k) -/// path records contain the string name of the path, followed by the +/// four bytes record length, high bit indicates record type +/// (but max record sizes are capped at 512kB) +/// path records contain the string name of the path, followed by up to 3 +/// padding bytes to align on 4 byte boundaries, followed by the /// one's complement of the expected index of the record (to detect /// concurrent writes of multiple ninja processes to the log). /// dependency records are an array of 4-byte integers -- cgit v0.12 From 427614f89ffa407f028db2d8ab3aedccacac23ca Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Wed, 28 Aug 2013 14:03:53 -0700 Subject: document an assumption --- src/deps_log.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/deps_log.cc b/src/deps_log.cc index a5d726c..64ef58f 100644 --- a/src/deps_log.cc +++ b/src/deps_log.cc @@ -234,6 +234,7 @@ bool DepsLog::Load(const string& path, State* state, string* err) { ++unique_dep_record_count; } else { int path_size = size - 4; + assert(path_size > 0); // CanonicalizePath() rejects empty paths. // There can be up to 3 bytes of padding. if (buf[path_size - 1] == '\0') --path_size; if (buf[path_size - 1] == '\0') --path_size; -- cgit v0.12 From 65120c69372cd35809168115a9fc2a12868eb4aa Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Wed, 28 Aug 2013 15:00:05 -0700 Subject: Fix an issue with more than 64k deps, spotted by maximuska. Also add a test for this case, which would have spotted the issue too. --- src/deps_log.cc | 2 +- src/deps_log_test.cc | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/deps_log.cc b/src/deps_log.cc index 64ef58f..ceee68a 100644 --- a/src/deps_log.cc +++ b/src/deps_log.cc @@ -124,7 +124,7 @@ bool DepsLog::RecordDeps(Node* node, TimeStamp mtime, return true; // Update on-disk representation. - unsigned size = 4 * (1 + 1 + (uint16_t)node_count); + unsigned size = 4 * (1 + 1 + node_count); if (size > kMaxRecordSize) { errno = ERANGE; return false; diff --git a/src/deps_log_test.cc b/src/deps_log_test.cc index 3b32963..4e6cbac 100644 --- a/src/deps_log_test.cc +++ b/src/deps_log_test.cc @@ -82,6 +82,39 @@ TEST_F(DepsLogTest, WriteRead) { ASSERT_EQ("bar2.h", log_deps->nodes[1]->path()); } +TEST_F(DepsLogTest, LotsOfDeps) { + const int kNumDeps = 100000; // More than 64k. + + State state1; + DepsLog log1; + string err; + EXPECT_TRUE(log1.OpenForWrite(kTestFilename, &err)); + ASSERT_EQ("", err); + + { + vector deps; + for (int i = 0; i < kNumDeps; ++i) { + char buf[32]; + sprintf(buf, "file%d.h", i); + deps.push_back(state1.GetNode(buf)); + } + log1.RecordDeps(state1.GetNode("out.o"), 1, deps); + + DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o")); + ASSERT_EQ(kNumDeps, log_deps->node_count); + } + + log1.Close(); + + State state2; + DepsLog log2; + EXPECT_TRUE(log2.Load(kTestFilename, &state2, &err)); + ASSERT_EQ("", err); + + DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out.o")); + ASSERT_EQ(kNumDeps, log_deps->node_count); +} + // Verify that adding the same deps twice doesn't grow the file. TEST_F(DepsLogTest, DoubleEntry) { // Write some deps to the file and grab its size. -- cgit v0.12 From 4a72182c6c65d44f894da68a72fb797b34c8d9ec Mon Sep 17 00:00:00 2001 From: Benedikt Meurer Date: Sun, 1 Sep 2013 11:12:23 +0200 Subject: Simplify implementation of GetProcessorCount(). The current implementation is unnecessarily complex, because: - The BSD derived systems implement sysconf(_SC_NPROCESSORS_ONLN) in terms of sysctl({CTL_HW,HW_NCPU}). - get_nprocs() is a GNU extension, and glibc implements sysconf(_SC_NPROCESSORS_ONLN) in terms of get_nprocs(). --- src/util.cc | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/src/util.cc b/src/util.cc index b9c2c0d..6ba3c6c 100644 --- a/src/util.cc +++ b/src/util.cc @@ -300,35 +300,15 @@ string StripAnsiEscapeCodes(const string& in) { return stripped; } -#if defined(linux) || defined(__GLIBC__) -int GetProcessorCount() { - return get_nprocs(); -} -#elif defined(__APPLE__) || defined(__FreeBSD__) -int GetProcessorCount() { - int processors; - size_t processors_size = sizeof(processors); - int name[] = {CTL_HW, HW_NCPU}; - if (sysctl(name, sizeof(name) / sizeof(int), - &processors, &processors_size, - NULL, 0) < 0) { - return 0; - } - return processors; -} -#elif defined(_WIN32) int GetProcessorCount() { +#ifdef _WIN32 SYSTEM_INFO info; GetSystemInfo(&info); return info.dwNumberOfProcessors; -} #else -// This is what get_nprocs() should be doing in the Linux implementation -// above, but in a more standard way. -int GetProcessorCount() { return sysconf(_SC_NPROCESSORS_ONLN); -} #endif +} #if defined(_WIN32) || defined(__CYGWIN__) double GetLoadAverage() { -- cgit v0.12 From d7a46654a7be1a46a777a8d8a51f065ac98ce05d Mon Sep 17 00:00:00 2001 From: Maxim Kalaev Date: Mon, 2 Sep 2013 15:24:26 -0700 Subject: Check depslog timestamp in LoadDepsFromLog(), not in RecomputeOutputDirty(). RecomputeOutputDirty() is called from two places: 1. RecomputeDirty(), which calls LoadDeps(). 2. CleanNode(), which always passes 0 for the deps mtime. So this is no behavior change in either case. deps_mtime was nonzero only in deps mode, and it got passed all over the place. This makes things simpler. --- src/build.cc | 2 +- src/graph.cc | 36 +++++++++++++++++------------------- src/graph.h | 11 +++++------ 3 files changed, 23 insertions(+), 26 deletions(-) diff --git a/src/build.cc b/src/build.cc index 45f6849..73dc1ff 100644 --- a/src/build.cc +++ b/src/build.cc @@ -426,7 +426,7 @@ void Plan::CleanNode(DependencyScan* scan, Node* node) { bool dirty = false; for (vector::iterator ni = (*ei)->outputs_.begin(); !dirty && ni != (*ei)->outputs_.end(); ++ni) { - dirty = scan->RecomputeOutputDirty(*ei, most_recent_input, 0, + dirty = scan->RecomputeOutputDirty(*ei, most_recent_input, command, *ni); } diff --git a/src/graph.cc b/src/graph.cc index fdd93de..b58e17f 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -61,8 +61,7 @@ bool DependencyScan::RecomputeDirty(Edge* edge, string* err) { bool dirty = false; edge->outputs_ready_ = true; - TimeStamp deps_mtime = 0; - if (!dep_loader_.LoadDeps(edge, &deps_mtime, err)) { + if (!dep_loader_.LoadDeps(edge, err)) { if (!err->empty()) return false; // Failed to load dependency info: rebuild to regenerate it. @@ -113,8 +112,7 @@ bool DependencyScan::RecomputeDirty(Edge* edge, string* err) { for (vector::iterator i = edge->outputs_.begin(); i != edge->outputs_.end(); ++i) { (*i)->StatIfNecessary(disk_interface_); - if (RecomputeOutputDirty(edge, most_recent_input, deps_mtime, - command, *i)) { + if (RecomputeOutputDirty(edge, most_recent_input, command, *i)) { dirty = true; break; } @@ -143,7 +141,6 @@ bool DependencyScan::RecomputeDirty(Edge* edge, string* err) { bool DependencyScan::RecomputeOutputDirty(Edge* edge, Node* most_recent_input, - TimeStamp deps_mtime, const string& command, Node* output) { if (edge->is_phony()) { @@ -185,13 +182,6 @@ bool DependencyScan::RecomputeOutputDirty(Edge* edge, } } - // Dirty if the output is newer than the deps. - if (deps_mtime && output->mtime() > deps_mtime) { - EXPLAIN("stored deps info out of date for for %s (%d vs %d)", - output->path().c_str(), deps_mtime, output->mtime()); - return true; - } - // 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. @@ -332,10 +322,10 @@ void Node::Dump(const char* prefix) const { } } -bool ImplicitDepLoader::LoadDeps(Edge* edge, TimeStamp* mtime, string* err) { +bool ImplicitDepLoader::LoadDeps(Edge* edge, string* err) { string deps_type = edge->GetBinding("deps"); if (!deps_type.empty()) { - if (!LoadDepsFromLog(edge, mtime, err)) { + if (!LoadDepsFromLog(edge, err)) { if (!err->empty()) return false; EXPLAIN("deps for %s are missing", edge->outputs_[0]->path().c_str()); @@ -406,13 +396,21 @@ bool ImplicitDepLoader::LoadDepFile(Edge* edge, const string& path, return true; } -bool ImplicitDepLoader::LoadDepsFromLog(Edge* edge, TimeStamp* deps_mtime, - string* err) { - DepsLog::Deps* deps = deps_log_->GetDeps(edge->outputs_[0]); - if (!deps) +bool ImplicitDepLoader::LoadDepsFromLog(Edge* edge, string* err) { + // NOTE: deps are only supported for single-target edges. + Node* output = edge->outputs_[0]; + DepsLog::Deps* deps = deps_log_->GetDeps(output); + if (!deps) { + EXPLAIN("deps for '%s' are missing", output->path().c_str()); return false; + } - *deps_mtime = deps->mtime; + // Deps are invalid if the output is newer than the deps. + if (output->mtime() > deps->mtime) { + EXPLAIN("stored deps info out of date for for '%s' (%d vs %d)", + output->path().c_str(), deps->mtime, output->mtime()); + return false; + } vector::iterator implicit_dep = PreallocateSpace(edge, deps->node_count); diff --git a/src/graph.h b/src/graph.h index 428ba01..bd0cb34 100644 --- a/src/graph.h +++ b/src/graph.h @@ -192,10 +192,10 @@ struct ImplicitDepLoader { DiskInterface* disk_interface) : state_(state), disk_interface_(disk_interface), deps_log_(deps_log) {} - /// Load implicit dependencies for \a edge. May fill in \a mtime with - /// the timestamp of the loaded information. - /// @return false on error (without filling \a err if info is just missing). - bool LoadDeps(Edge* edge, TimeStamp* mtime, string* err); + /// Load implicit dependencies for \a edge. + /// @return false on error (without filling \a err if info is just missing + // or out of date). + bool LoadDeps(Edge* edge, string* err); DepsLog* deps_log() const { return deps_log_; @@ -208,7 +208,7 @@ struct ImplicitDepLoader { /// Load implicit dependencies for \a edge from the DepsLog. /// @return false on error (without filling \a err if info is just missing). - bool LoadDepsFromLog(Edge* edge, TimeStamp* mtime, string* err); + bool LoadDepsFromLog(Edge* edge, string* err); /// Preallocate \a count spaces in the input array on \a edge, returning /// an iterator pointing at the first new space. @@ -243,7 +243,6 @@ struct DependencyScan { /// Recompute whether a given single output should be marked dirty. /// Returns true if so. bool RecomputeOutputDirty(Edge* edge, Node* most_recent_input, - TimeStamp deps_mtime, const string& command, Node* output); BuildLog* build_log() const { -- cgit v0.12 From 41c6258ec1928cbcfc5642504cb9c2b3659fa897 Mon Sep 17 00:00:00 2001 From: Maxim Kalaev Date: Mon, 2 Sep 2013 15:32:19 -0700 Subject: Share more code between CleanNode() and RecomputeDirty(). Move a common loop into the new function RecomputeOutputsDirty(). Simplifies things a bit, and makes it harder for the restat path to have different behavior from the regular path. No dramatic behavior change (the restat path now also calls RestatIfNecessary()). --- src/build.cc | 12 ++---------- src/graph.cc | 26 ++++++++++++++------------ src/graph.h | 10 +++++++--- 3 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/build.cc b/src/build.cc index 73dc1ff..76d317f 100644 --- a/src/build.cc +++ b/src/build.cc @@ -414,25 +414,17 @@ void Plan::CleanNode(DependencyScan* scan, Node* node) { begin = (*ei)->inputs_.begin(), end = (*ei)->inputs_.end() - (*ei)->order_only_deps_; if (find_if(begin, end, mem_fun(&Node::dirty)) == end) { - // Recompute most_recent_input and command. + // Recompute most_recent_input. Node* most_recent_input = NULL; for (vector::iterator ni = begin; ni != end; ++ni) { if (!most_recent_input || (*ni)->mtime() > most_recent_input->mtime()) most_recent_input = *ni; } - string command = (*ei)->EvaluateCommand(true); // Now, this edge is dirty if any of the outputs are dirty. - bool dirty = false; - for (vector::iterator ni = (*ei)->outputs_.begin(); - !dirty && ni != (*ei)->outputs_.end(); ++ni) { - dirty = scan->RecomputeOutputDirty(*ei, most_recent_input, - command, *ni); - } - // If the edge isn't dirty, clean the outputs and mark the edge as not // wanted. - if (!dirty) { + if (!scan->RecomputeOutputsDirty(*ei, most_recent_input)) { for (vector::iterator ni = (*ei)->outputs_.begin(); ni != (*ei)->outputs_.end(); ++ni) { CleanNode(scan, *ni); diff --git a/src/graph.cc b/src/graph.cc index b58e17f..eeb3db1 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -106,18 +106,8 @@ bool DependencyScan::RecomputeDirty(Edge* edge, string* err) { // We may also be dirty due to output state: missing outputs, out of // date outputs, etc. Visit all outputs and determine whether they're dirty. - if (!dirty) { - string command = edge->EvaluateCommand(true); - - for (vector::iterator i = edge->outputs_.begin(); - i != edge->outputs_.end(); ++i) { - (*i)->StatIfNecessary(disk_interface_); - if (RecomputeOutputDirty(edge, most_recent_input, command, *i)) { - dirty = true; - break; - } - } - } + if (!dirty) + dirty = RecomputeOutputsDirty(edge, most_recent_input); // Finally, visit each output to mark off that we've visited it, and update // their dirty state if necessary. @@ -139,6 +129,18 @@ bool DependencyScan::RecomputeDirty(Edge* edge, string* err) { return true; } +bool DependencyScan::RecomputeOutputsDirty(Edge* edge, + Node* most_recent_input) { + string command = edge->EvaluateCommand(true); + for (vector::iterator i = edge->outputs_.begin(); + i != edge->outputs_.end(); ++i) { + (*i)->StatIfNecessary(disk_interface_); + if (RecomputeOutputDirty(edge, most_recent_input, command, *i)) + return true; + } + return false; +} + bool DependencyScan::RecomputeOutputDirty(Edge* edge, Node* most_recent_input, const string& command, diff --git a/src/graph.h b/src/graph.h index bd0cb34..d5d0f4f 100644 --- a/src/graph.h +++ b/src/graph.h @@ -240,10 +240,9 @@ struct DependencyScan { /// Returns false on failure. bool RecomputeDirty(Edge* edge, string* err); - /// Recompute whether a given single output should be marked dirty. + /// Recompute whether any output of the edge is dirty. /// Returns true if so. - bool RecomputeOutputDirty(Edge* edge, Node* most_recent_input, - const string& command, Node* output); + bool RecomputeOutputsDirty(Edge* edge, Node* most_recent_input); BuildLog* build_log() const { return build_log_; @@ -257,6 +256,11 @@ struct DependencyScan { } private: + /// Recompute whether a given single output should be marked dirty. + /// Returns true if so. + bool RecomputeOutputDirty(Edge* edge, Node* most_recent_input, + const string& command, Node* output); + BuildLog* build_log_; DiskInterface* disk_interface_; ImplicitDepLoader dep_loader_; -- cgit v0.12 From 25a3c97a8203dff5589bc13e835b9f362a51f3ab Mon Sep 17 00:00:00 2001 From: Maxim Kalaev Date: Mon, 2 Sep 2013 15:44:32 -0700 Subject: Fix restat rebuild if deps are missing. Fixes issue #603. Apparently, the problem was caused by my fix r "consider target dirty if depfile is missing" (bcc8ad1), which was not working correctly with restat rules cleaning nodes. Switching to deps only triggered an easily observable issue. Fix by setting a flag in edges with invalid deps, and not cleaning edges with that flag set. --- src/build.cc | 4 ++ src/build_test.cc | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++- src/graph.cc | 3 +- src/graph.h | 5 ++- 4 files changed, 126 insertions(+), 4 deletions(-) diff --git a/src/build.cc b/src/build.cc index 76d317f..8a93632 100644 --- a/src/build.cc +++ b/src/build.cc @@ -408,6 +408,10 @@ void Plan::CleanNode(DependencyScan* scan, Node* node) { if (want_i == want_.end() || !want_i->second) continue; + // Don't attempt to clean an edge if it failed to load deps. + if ((*ei)->deps_missing_) + continue; + // If all non-order-only inputs for this edge are now clean, // we might have changed the dirty state of the outputs. vector::iterator diff --git a/src/build_test.cc b/src/build_test.cc index 66d0954..4b1c829 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -435,6 +435,13 @@ struct BuildTest : public StateTestWithBuiltinRules { builder_.command_runner_.release(); } + /// Rebuild target in the 'working tree' (fs_). + /// State of command_runner_ and logs contents (if specified) ARE MODIFIED. + /// Handy to check for NOOP builds, and higher-level rebuild tests. + void RebuildTarget(const string& target, const char* manifest, + const char* log_path = NULL, + const char* deps_path = NULL); + // Mark a path dirty. void Dirty(const string& path); @@ -452,6 +459,39 @@ struct BuildTest : public StateTestWithBuiltinRules { BuildStatus status_; }; +void BuildTest::RebuildTarget(const string& target, const char* manifest, + const char* log_path, const char* deps_path) { + State state; + ASSERT_NO_FATAL_FAILURE(AddCatRule(&state)); + AssertParse(&state, manifest); + + string err; + BuildLog build_log, *pbuild_log = NULL; + if (log_path) { + ASSERT_TRUE(build_log.Load(log_path, &err)); + ASSERT_TRUE(build_log.OpenForWrite(log_path, &err)); + ASSERT_EQ("", err); + pbuild_log = &build_log; + } + + DepsLog deps_log, *pdeps_log = NULL; + if (deps_path) { + ASSERT_TRUE(deps_log.Load(deps_path, &state, &err)); + ASSERT_TRUE(deps_log.OpenForWrite(deps_path, &err)); + ASSERT_EQ("", err); + pdeps_log = &deps_log; + } + + Builder builder(&state, config_, pbuild_log, pdeps_log, &fs_); + EXPECT_TRUE(builder.AddTarget(target, &err)); + + command_runner_.commands_ran_.clear(); + builder.command_runner_.reset(&command_runner_); + bool build_res = builder.Build(&err); + builder.command_runner_.release(); + EXPECT_TRUE(build_res) << "builder.Build(&err)"; +} + bool FakeCommandRunner::CanRunMore() { // Only run one at a time. return last_command_ == NULL; @@ -1780,7 +1820,7 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) { // Expect three new edges: one generating foo.o, and two more from // loading the depfile. - ASSERT_EQ(3, (int)state.edges_.size()); + ASSERT_EQ(3u, state.edges_.size()); // Expect our edge to now have three inputs: foo.c and two headers. ASSERT_EQ(3u, edge->inputs_.size()); @@ -1791,3 +1831,79 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) { builder.command_runner_.release(); } } + +/// Check that a restat rule doesn't clear an edge if the depfile is missing. +/// Follows from: https://github.com/martine/ninja/issues/603 +TEST_F(BuildTest, RestatMissingDepfile) { +const char* manifest = +"rule true\n" +" command = true\n" // Would be "write if out-of-date" in reality. +" restat = 1\n" +"build header.h: true header.in\n" +"build out: cat header.h\n" +" depfile = out.d\n"; + + fs_.Create("header.h", ""); + fs_.Tick(); + fs_.Create("out", ""); + fs_.Create("header.in", ""); + + // Normally, only 'header.h' would be rebuilt, as + // its rule doesn't touch the output and has 'restat=1' set. + // But we are also missing the depfile for 'out', + // which should force its command to run anyway! + RebuildTarget("out", manifest); + ASSERT_EQ(2u, command_runner_.commands_ran_.size()); +} + +/// Check that a restat rule doesn't clear an edge if the deps are missing. +/// https://github.com/martine/ninja/issues/603 +TEST_F(BuildWithDepsLogTest, RestatMissingDepfileDepslog) { + string err; + const char* manifest = +"rule true\n" +" command = true\n" // Would be "write if out-of-date" in reality. +" restat = 1\n" +"build header.h: true header.in\n" +"build out: cat header.h\n" +" deps = gcc\n" +" depfile = out.d\n"; + + // Build once to populate ninja deps logs from out.d + fs_.Create("header.in", ""); + fs_.Create("out.d", "out: header.h"); + fs_.Create("header.h", ""); + + RebuildTarget("out", manifest, "build_log", "ninja_deps"); + ASSERT_EQ(2u, command_runner_.commands_ran_.size()); + + // Sanity: this rebuild should be NOOP + RebuildTarget("out", manifest, "build_log", "ninja_deps"); + ASSERT_EQ(0u, command_runner_.commands_ran_.size()); + + // Touch 'header.in', blank dependencies log (create a different one). + // Building header.h triggers 'restat' outputs cleanup. + // Validate that out is rebuilt netherless, as deps are missing. + fs_.Tick(); + fs_.Create("header.in", ""); + + // (switch to a new blank deps_log "ninja_deps2") + RebuildTarget("out", manifest, "build_log", "ninja_deps2"); + ASSERT_EQ(2u, command_runner_.commands_ran_.size()); + + // Sanity: this build should be NOOP + RebuildTarget("out", manifest, "build_log", "ninja_deps2"); + ASSERT_EQ(0u, command_runner_.commands_ran_.size()); + + // Check that invalidating deps by target timestamp also works here + // Repeat the test but touch target instead of blanking the log. + fs_.Tick(); + fs_.Create("header.in", ""); + fs_.Create("out", ""); + RebuildTarget("out", manifest, "build_log", "ninja_deps2"); + ASSERT_EQ(2u, command_runner_.commands_ran_.size()); + + // And this build should be NOOP again + RebuildTarget("out", manifest, "build_log", "ninja_deps2"); + ASSERT_EQ(0u, command_runner_.commands_ran_.size()); +} diff --git a/src/graph.cc b/src/graph.cc index eeb3db1..3616199 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -60,12 +60,13 @@ bool Rule::IsReservedBinding(const string& var) { bool DependencyScan::RecomputeDirty(Edge* edge, string* err) { bool dirty = false; edge->outputs_ready_ = true; + edge->deps_missing_ = false; if (!dep_loader_.LoadDeps(edge, err)) { if (!err->empty()) return false; // Failed to load dependency info: rebuild to regenerate it. - dirty = true; + dirty = edge->deps_missing_ = true; } // Visit all inputs; we're dirty if any of the inputs are dirty. diff --git a/src/graph.h b/src/graph.h index d5d0f4f..868413c 100644 --- a/src/graph.h +++ b/src/graph.h @@ -135,8 +135,8 @@ struct Rule { /// An edge in the dependency graph; links between Nodes using Rules. struct Edge { - Edge() : rule_(NULL), env_(NULL), outputs_ready_(false), implicit_deps_(0), - order_only_deps_(0) {} + Edge() : rule_(NULL), env_(NULL), outputs_ready_(false), deps_missing_(false), + implicit_deps_(0), order_only_deps_(0) {} /// Return true if all inputs' in-edges are ready. bool AllInputsReady() const; @@ -157,6 +157,7 @@ struct Edge { vector outputs_; BindingEnv* env_; bool outputs_ready_; + bool deps_missing_; const Rule& rule() const { return *rule_; } Pool* pool() const { return pool_; } -- cgit v0.12 From 39b57bcd3950f6c28a33c21a6c7dd918075cd80f Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Mon, 2 Sep 2013 16:01:46 -0700 Subject: Delete a line I failed to delete in d7a46654a7be1. --- src/graph.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/graph.cc b/src/graph.cc index 3616199..8d507f7 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -331,7 +331,6 @@ bool ImplicitDepLoader::LoadDeps(Edge* edge, string* err) { if (!LoadDepsFromLog(edge, err)) { if (!err->empty()) return false; - EXPLAIN("deps for %s are missing", edge->outputs_[0]->path().c_str()); return false; } return true; -- cgit v0.12 From d23e718acb7cb4038473c96555cecf15556ed227 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Mon, 2 Sep 2013 16:09:00 -0700 Subject: Simplify. --- src/graph.cc | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/src/graph.cc b/src/graph.cc index 8d507f7..6191998 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -327,25 +327,12 @@ void Node::Dump(const char* prefix) const { bool ImplicitDepLoader::LoadDeps(Edge* edge, string* err) { string deps_type = edge->GetBinding("deps"); - if (!deps_type.empty()) { - if (!LoadDepsFromLog(edge, err)) { - if (!err->empty()) - return false; - return false; - } - return true; - } + if (!deps_type.empty()) + return LoadDepsFromLog(edge, err); string depfile = edge->GetBinding("depfile"); - if (!depfile.empty()) { - if (!LoadDepFile(edge, depfile, err)) { - if (!err->empty()) - return false; - EXPLAIN("depfile '%s' is missing", depfile.c_str()); - return false; - } - return true; - } + if (!depfile.empty()) + return LoadDepFile(edge, depfile, err); // No deps to load. return true; @@ -360,8 +347,10 @@ bool ImplicitDepLoader::LoadDepFile(Edge* edge, const string& path, return false; } // On a missing depfile: return false and empty *err. - if (content.empty()) + if (content.empty()) { + EXPLAIN("depfile '%s' is missing", path.c_str()); return false; + } DepfileParser depfile; string depfile_err; -- cgit v0.12 From c58caad3d19bf2734281a6c056b7776486bf5487 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Fri, 6 Sep 2013 14:29:32 -0700 Subject: Make depslog v1->v3 message less scary. See the comment 5 lines up for details. --- src/deps_log.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deps_log.cc b/src/deps_log.cc index ceee68a..4f1214a 100644 --- a/src/deps_log.cc +++ b/src/deps_log.cc @@ -183,7 +183,7 @@ bool DepsLog::Load(const string& path, State* state, string* err) { if (!valid_header || strcmp(buf, kFileSignature) != 0 || version != kCurrentVersion) { if (version == 1) - *err = "deps log potentially corrupt; rebuilding"; + *err = "deps log version change; rebuilding"; else *err = "bad deps log signature or version; starting over"; fclose(f); -- cgit v0.12 From e674c51e4dfd38c8f7b318a98e1ee7637af9a14e Mon Sep 17 00:00:00 2001 From: Richard Geary Date: Sat, 7 Sep 2013 10:03:00 -0400 Subject: Fix for build test for BuildWithDepsLogTest.RestatMissingDepfileDepslog --- src/build_test.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/build_test.cc b/src/build_test.cc index 4b1c829..3c117f4 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -487,7 +487,10 @@ void BuildTest::RebuildTarget(const string& target, const char* manifest, command_runner_.commands_ran_.clear(); builder.command_runner_.reset(&command_runner_); - bool build_res = builder.Build(&err); + bool build_res = true; + if (!builder.AlreadyUpToDate()) { + build_res = builder.Build(&err); + } builder.command_runner_.release(); EXPECT_TRUE(build_res) << "builder.Build(&err)"; } -- cgit v0.12 From 97f9766537af211d2a40ca8036a75adc2504f980 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Sat, 7 Sep 2013 19:16:21 -0700 Subject: Simplify. --- src/build_test.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/build_test.cc b/src/build_test.cc index 3c117f4..e206cd8 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -487,12 +487,11 @@ void BuildTest::RebuildTarget(const string& target, const char* manifest, command_runner_.commands_ran_.clear(); builder.command_runner_.reset(&command_runner_); - bool build_res = true; if (!builder.AlreadyUpToDate()) { - build_res = builder.Build(&err); + bool build_res = builder.Build(&err); + EXPECT_TRUE(build_res) << "builder.Build(&err)"; } builder.command_runner_.release(); - EXPECT_TRUE(build_res) << "builder.Build(&err)"; } bool FakeCommandRunner::CanRunMore() { -- cgit v0.12 From cc89c1aaec13487dc633cd69f7022fcb72fc1c10 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Tue, 10 Sep 2013 15:34:01 -0700 Subject: add -d keeprsp to preserve @rsp files on success on windows --- configure.py | 2 +- src/build.cc | 3 ++- src/debug_flags.cc | 17 +++++++++++++++++ src/debug_flags.h | 29 +++++++++++++++++++++++++++++ src/explain.cc | 15 --------------- src/explain.h | 27 --------------------------- src/graph.cc | 2 +- src/ninja.cc | 6 +++++- 8 files changed, 55 insertions(+), 46 deletions(-) create mode 100644 src/debug_flags.cc create mode 100644 src/debug_flags.h delete mode 100644 src/explain.cc delete mode 100644 src/explain.h diff --git a/configure.py b/configure.py index c838392..9fe3be8 100755 --- a/configure.py +++ b/configure.py @@ -263,12 +263,12 @@ n.comment('Core source files all build into ninja library.') for name in ['build', 'build_log', 'clean', + 'debug_flags', 'depfile_parser', 'deps_log', 'disk_interface', 'edit_distance', 'eval_env', - 'explain', 'graph', 'graphviz', 'lexer', diff --git a/src/build.cc b/src/build.cc index 8a93632..9718f85 100644 --- a/src/build.cc +++ b/src/build.cc @@ -25,6 +25,7 @@ #endif #include "build_log.h" +#include "debug_flags.h" #include "depfile_parser.h" #include "deps_log.h" #include "disk_interface.h" @@ -776,7 +777,7 @@ bool Builder::FinishCommand(CommandRunner::Result* result, string* err) { // Delete any left over response file. string rspfile = edge->GetBinding("rspfile"); - if (!rspfile.empty()) + if (!rspfile.empty() && !g_keep_rsp) disk_interface_->RemoveFile(rspfile); if (scan_.build_log()) { diff --git a/src/debug_flags.cc b/src/debug_flags.cc new file mode 100644 index 0000000..75f1ea5 --- /dev/null +++ b/src/debug_flags.cc @@ -0,0 +1,17 @@ +// Copyright 2012 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +bool g_explaining = false; + +bool g_keep_rsp = false; diff --git a/src/debug_flags.h b/src/debug_flags.h new file mode 100644 index 0000000..ba3ebf3 --- /dev/null +++ b/src/debug_flags.h @@ -0,0 +1,29 @@ +// Copyright 2012 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef NINJA_EXPLAIN_H_ +#define NINJA_EXPLAIN_H_ + +#include + +#define EXPLAIN(fmt, ...) { \ + if (g_explaining) \ + fprintf(stderr, "ninja explain: " fmt "\n", __VA_ARGS__); \ +} + +extern bool g_explaining; + +extern bool g_keep_rsp; + +#endif // NINJA_EXPLAIN_H_ diff --git a/src/explain.cc b/src/explain.cc deleted file mode 100644 index 4e14c25..0000000 --- a/src/explain.cc +++ /dev/null @@ -1,15 +0,0 @@ -// Copyright 2012 Google Inc. All Rights Reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -bool g_explaining = false; diff --git a/src/explain.h b/src/explain.h deleted file mode 100644 index d4f6a6c..0000000 --- a/src/explain.h +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright 2012 Google Inc. All Rights Reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#ifndef NINJA_EXPLAIN_H_ -#define NINJA_EXPLAIN_H_ - -#include - -#define EXPLAIN(fmt, ...) { \ - if (g_explaining) \ - fprintf(stderr, "ninja explain: " fmt "\n", __VA_ARGS__); \ -} - -extern bool g_explaining; - -#endif // NINJA_EXPLAIN_H_ diff --git a/src/graph.cc b/src/graph.cc index 6191998..9801a7b 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -18,10 +18,10 @@ #include #include "build_log.h" +#include "debug_flags.h" #include "depfile_parser.h" #include "deps_log.h" #include "disk_interface.h" -#include "explain.h" #include "manifest_parser.h" #include "metrics.h" #include "state.h" diff --git a/src/ninja.cc b/src/ninja.cc index 0586bdc..a313ecb 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -32,8 +32,8 @@ #include "build_log.h" #include "deps_log.h" #include "clean.h" +#include "debug_flags.h" #include "disk_interface.h" -#include "explain.h" #include "graph.h" #include "graphviz.h" #include "manifest_parser.h" @@ -747,6 +747,7 @@ bool DebugEnable(const string& name) { printf("debugging modes:\n" " stats print operation counts/timing info\n" " explain explain what caused a command to execute\n" +" keeprsp don't delete @response files on success\n" "multiple modes can be enabled via -d FOO -d BAR\n"); return false; } else if (name == "stats") { @@ -755,6 +756,9 @@ bool DebugEnable(const string& name) { } else if (name == "explain") { g_explaining = true; return true; + } else if (name == "keeprsp") { + g_keep_rsp = true; + return true; } else { const char* suggestion = SpellcheckString(name.c_str(), "stats", "explain", NULL); -- cgit v0.12 From 6f7ea464bb9161ce2e15deb97977886de152c12d Mon Sep 17 00:00:00 2001 From: Evan Martin Date: Wed, 11 Sep 2013 19:05:21 -0700 Subject: mark this 1.4.0.git, update RELEASING --- RELEASING | 18 ++++++++++++++---- src/version.cc | 2 +- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/RELEASING b/RELEASING index 1110f0b..25926db 100644 --- a/RELEASING +++ b/RELEASING @@ -1,12 +1,22 @@ Notes to myself on all the steps to make for a Ninja release. +Push new release branch: 1. update src/version.cc with new version (with ".git") 2. git checkout release; git merge master 3. fix version number in src/version.cc (it will likely conflict in the above) 4. fix version in doc/manual.asciidoc -5. rebuild manual, put in place on website -6. commit, tag, push (don't forget to push --tags) -7. construct release notes from prior notes +5. commit, tag, push (don't forget to push --tags) +6. construct release notes from prior notes credits: git shortlog -s --no-merges REV.. -8. update home page mention of latest version. +Release on github: +1. (haven't tried this yet) + https://github.com/blog/1547-release-your-software + +Make announcement on mailing list: +1. copy old mail + +Update website: +(note to self: website is now in github.com/martine/martine.github.io) +1. rebuild manual, put in place on website +2. update home page mention of latest version. diff --git a/src/version.cc b/src/version.cc index 18fa96a..1406d91 100644 --- a/src/version.cc +++ b/src/version.cc @@ -18,7 +18,7 @@ #include "util.h" -const char* kNinjaVersion = "1.3.0.git"; +const char* kNinjaVersion = "1.4.0.git"; void ParseVersion(const string& version, int* major, int* minor) { size_t end = version.find('.'); -- cgit v0.12