From b07e183e0eb6225e34a3d592e3dff63bcf00df81 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Fri, 9 Dec 2011 17:26:38 +0200 Subject: Clean up how the build line is managed at the end of the build --- src/build.cc | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/build.cc b/src/build.cc index cc877e5..8c9981c 100644 --- a/src/build.cc +++ b/src/build.cc @@ -39,6 +39,7 @@ struct BuildStatus { void BuildEdgeStarted(Edge* edge); void BuildEdgeFinished(Edge* edge, bool success, const string& output, int* start_time, int* end_time); + void BuildFinished(); private: void PrintStatus(Edge* edge); @@ -52,6 +53,8 @@ struct BuildStatus { int started_edges_, finished_edges_, total_edges_; + bool have_blank_line_; + /// Map of running edge to time the edge started running. typedef map RunningEdgeMap; RunningEdgeMap running_edges_; @@ -64,7 +67,8 @@ BuildStatus::BuildStatus(const BuildConfig& config) : config_(config), start_time_millis_(GetTimeMillis()), last_update_millis_(start_time_millis_), - started_edges_(0), finished_edges_(0), total_edges_(0) { + started_edges_(0), finished_edges_(0), total_edges_(0), + have_blank_line_(true) { #ifndef _WIN32 const char* term = getenv("TERM"); smart_terminal_ = isatty(1) && term && string(term) != "dumb"; @@ -115,10 +119,7 @@ void BuildStatus::BuildEdgeFinished(Edge* edge, PrintStatus(edge); if (success && output.empty()) { - if (smart_terminal_) { - if (finished_edges_ == total_edges_) - printf("\n"); - } else { + if (!smart_terminal_) { if (total_time > 5*1000) { printf("%.1f%% %d/%d\n", finished_edges_ * 100 / (float)total_edges_, finished_edges_, total_edges_); @@ -153,9 +154,16 @@ void BuildStatus::BuildEdgeFinished(Edge* edge, if (!final_output.empty()) printf("%s", final_output.c_str()); + + have_blank_line_ = true; } } +void BuildStatus::BuildFinished() { + if (smart_terminal_ && !have_blank_line_) + printf("\n"); +} + void BuildStatus::PrintStatus(Edge* edge) { if (config_.verbosity == BuildConfig::QUIET) return; @@ -195,6 +203,7 @@ void BuildStatus::PrintStatus(Edge* edge) { if (smart_terminal_ && !force_full_command) { printf("\x1B[K"); // Clear to end of line. fflush(stdout); + have_blank_line_ = false; } else { printf("\n"); } @@ -507,8 +516,10 @@ bool Builder::Build(string* err) { // See if we can start any more commands. if (command_runner_->CanRunMore()) { if (Edge* edge = plan_.FindWork()) { - if (!StartEdge(edge, err)) + if (!StartEdge(edge, err)) { + status_->BuildFinished(); return false; + } if (edge->is_phony()) FinishEdge(edge, true, ""); @@ -545,11 +556,13 @@ bool Builder::Build(string* err) { // If we get here, we can neither enqueue new commands nor are any running. if (pending_commands) { + status_->BuildFinished(); *err = "stuck: pending commands but none to wait for? [this is a bug]"; return false; } // If we get here, we cannot make any more progress. + status_->BuildFinished(); if (failures_allowed < config_.swallow_failures) { *err = "cannot make progress due to previous errors"; return false; @@ -559,6 +572,7 @@ bool Builder::Build(string* err) { } } + status_->BuildFinished(); return true; } -- cgit v0.12 From 85ff781fa30fff63c01ccd30faaad39d766e1505 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Sun, 13 Nov 2011 05:49:16 +0000 Subject: Implement cleanup-on-interrupt This causes us to clean up by deleting any output files belonging to currently-running commands before we quit if we are interrupted (either by Ctrl-C or by a command failing). Fixes issue #110. --- src/build.cc | 76 +++++++++++++++++++++++------- src/build.h | 15 +++++- src/build_test.cc | 15 ++++-- src/exit_status.h | 24 ++++++++++ src/subprocess-win32.cc | 63 +++++++++++++++++++++---- src/subprocess.cc | 120 +++++++++++++++++++++++++++++++++++++----------- src/subprocess.h | 33 +++++++++---- src/subprocess_test.cc | 56 +++++++++++++++------- 8 files changed, 318 insertions(+), 84 deletions(-) create mode 100644 src/exit_status.h diff --git a/src/build.cc b/src/build.cc index 8c9981c..4c15de9 100644 --- a/src/build.cc +++ b/src/build.cc @@ -396,35 +396,52 @@ struct RealCommandRunner : public CommandRunner { virtual ~RealCommandRunner() {} virtual bool CanRunMore(); virtual bool StartCommand(Edge* edge); - virtual Edge* WaitForCommand(bool* success, string* output); + virtual Edge* WaitForCommand(ExitStatus* status, string* output); + virtual vector GetActiveEdges(); + virtual void Abort(); const BuildConfig& config_; SubprocessSet subprocs_; map subproc_to_edge_; }; +vector RealCommandRunner::GetActiveEdges() { + vector edges; + for (map::iterator i = subproc_to_edge_.begin(); + i != subproc_to_edge_.end(); ++i) + edges.push_back(i->second); + return edges; +} + +void RealCommandRunner::Abort() { + subprocs_.Clear(); +} + bool RealCommandRunner::CanRunMore() { return ((int)subprocs_.running_.size()) < config_.parallelism; } bool RealCommandRunner::StartCommand(Edge* edge) { string command = edge->EvaluateCommand(); - Subprocess* subproc = new Subprocess; - subproc_to_edge_.insert(make_pair(subproc, edge)); - if (!subproc->Start(&subprocs_, command)) + Subprocess* subproc = subprocs_.Add(command); + if (!subproc) return false; - - subprocs_.Add(subproc); + subproc_to_edge_.insert(make_pair(subproc, edge)); + return true; } -Edge* RealCommandRunner::WaitForCommand(bool* success, string* output) { +Edge* RealCommandRunner::WaitForCommand(ExitStatus* status, string* output) { Subprocess* subproc; while ((subproc = subprocs_.NextFinished()) == NULL) { - subprocs_.DoWork(); + bool interrupted = subprocs_.DoWork(); + if (interrupted) { + *status = ExitInterrupted; + return 0; + } } - *success = subproc->Finish(); + *status = subproc->Finish(); *output = subproc->GetOutput(); map::iterator i = subproc_to_edge_.find(subproc); @@ -445,10 +462,12 @@ struct DryRunCommandRunner : public CommandRunner { finished_.push(edge); return true; } - virtual Edge* WaitForCommand(bool* success, string* output) { - if (finished_.empty()) + virtual Edge* WaitForCommand(ExitStatus* status, string* /* output */) { + if (finished_.empty()) { + *status = ExitFailure; return NULL; - *success = true; + } + *status = ExitSuccess; Edge* edge = finished_.front(); finished_.pop(); return edge; @@ -461,13 +480,29 @@ Builder::Builder(State* state, const BuildConfig& config) : state_(state), config_(config) { disk_interface_ = new RealDiskInterface; if (config.dry_run) - command_runner_ = new DryRunCommandRunner; + command_runner_.reset(new DryRunCommandRunner); else - command_runner_ = new RealCommandRunner(config); + command_runner_.reset(new RealCommandRunner(config)); status_ = new BuildStatus(config); log_ = state->build_log_; } +Builder::~Builder() { + if (command_runner_.get()) { + vector active_edges = command_runner_->GetActiveEdges(); + command_runner_->Abort(); + + for (vector::iterator i = active_edges.begin(); + i != active_edges.end(); ++i) { + for (vector::iterator ni = (*i)->outputs_.begin(); + ni != (*i)->outputs_.end(); ++ni) + disk_interface_->RemoveFile((*ni)->path()); + if (!(*i)->rule_->depfile_.empty()) + disk_interface_->RemoveFile((*i)->EvaluateDepFile()); + } + } +} + Node* Builder::AddTarget(const string& name, string* err) { Node* node = state_->LookupNode(name); if (!node) { @@ -533,10 +568,11 @@ bool Builder::Build(string* err) { // See if we can reap any finished commands. if (pending_commands) { - bool success; + ExitStatus status; string output; - Edge* edge; - if ((edge = command_runner_->WaitForCommand(&success, &output))) { + Edge* edge = command_runner_->WaitForCommand(&status, &output); + if (edge && status != ExitInterrupted) { + bool success = (status == ExitSuccess); --pending_commands; FinishEdge(edge, success, output); if (!success) { @@ -552,6 +588,12 @@ bool Builder::Build(string* err) { // We made some progress; start the main loop over. continue; } + + if (status == ExitInterrupted) { + status_->BuildFinished(); + *err = "interrupted by user"; + return false; + } } // If we get here, we can neither enqueue new commands nor are any running. diff --git a/src/build.h b/src/build.h index 586d1ff..c75bde1 100644 --- a/src/build.h +++ b/src/build.h @@ -20,8 +20,11 @@ #include #include #include +#include using namespace std; +#include "exit_status.h" + struct BuildLog; struct Edge; struct DiskInterface; @@ -87,7 +90,9 @@ struct CommandRunner { virtual bool CanRunMore() = 0; virtual bool StartCommand(Edge* edge) = 0; /// Wait for a command to complete. - virtual Edge* WaitForCommand(bool* success, string* output) = 0; + virtual Edge* WaitForCommand(ExitStatus* status, string* output) = 0; + virtual vector GetActiveEdges() { return vector(); } + virtual void Abort() {} }; /// Options (e.g. verbosity, parallelism) passed to a build. @@ -109,6 +114,7 @@ struct BuildConfig { /// Builder wraps the build process: starting commands, updating status. struct Builder { Builder(State* state, const BuildConfig& config); + ~Builder(); Node* AddTarget(const string& name, string* err); @@ -130,9 +136,14 @@ struct Builder { const BuildConfig& config_; Plan plan_; DiskInterface* disk_interface_; - CommandRunner* command_runner_; + auto_ptr command_runner_; struct BuildStatus* status_; struct BuildLog* log_; + +private: + // Unimplemented copy ctor and operator= ensure we don't copy the auto_ptr. + Builder(const Builder &other); // DO NOT IMPLEMENT + void operator=(const Builder &other); // DO NOT IMPLEMENT }; #endif // NINJA_BUILD_H_ diff --git a/src/build_test.cc b/src/build_test.cc index 0fa23ed..de5ddc1 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -181,7 +181,7 @@ struct BuildTest : public StateTestWithBuiltinRules, BuildTest() : config_(MakeConfig()), builder_(&state_, config_), now_(1), last_command_(NULL) { builder_.disk_interface_ = &fs_; - builder_.command_runner_ = this; + builder_.command_runner_.reset(this); AssertParse(&state_, "build cat1: cat in1\n" "build cat2: cat in1 in2\n" @@ -191,13 +191,17 @@ struct BuildTest : public StateTestWithBuiltinRules, fs_.Create("in2", now_, ""); } + ~BuildTest() { + builder_.command_runner_.release(); + } + // Mark a path dirty. void Dirty(const string& path); // CommandRunner impl virtual bool CanRunMore(); virtual bool StartCommand(Edge* edge); - virtual Edge* WaitForCommand(bool* success, string* output); + virtual Edge* WaitForCommand(ExitStatus* status, string* output); BuildConfig MakeConfig() { BuildConfig config; @@ -251,15 +255,16 @@ bool BuildTest::StartCommand(Edge* edge) { return true; } -Edge* BuildTest::WaitForCommand(bool* success, string* output) { +Edge* BuildTest::WaitForCommand(ExitStatus* status, string* /* output */) { if (Edge* edge = last_command_) { if (edge->rule().name() == "fail") - *success = false; + *status = ExitFailure; else - *success = true; + *status = ExitSuccess; last_command_ = NULL; return edge; } + *status = ExitFailure; return NULL; } diff --git a/src/exit_status.h b/src/exit_status.h new file mode 100644 index 0000000..a714ece --- /dev/null +++ b/src/exit_status.h @@ -0,0 +1,24 @@ +// Copyright 2011 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_EXIT_STATUS_H_ +#define NINJA_EXIT_STATUS_H_ + +enum ExitStatus { + ExitSuccess, + ExitFailure, + ExitInterrupted +}; + +#endif // NINJA_EXIT_STATUS_H_ diff --git a/src/subprocess-win32.cc b/src/subprocess-win32.cc index e757f7e..6b7e942 100644 --- a/src/subprocess-win32.cc +++ b/src/subprocess-win32.cc @@ -32,6 +32,10 @@ Subprocess::Subprocess() : child_(NULL) , overlapped_() { } Subprocess::~Subprocess() { + if (pipe_) { + if (!CloseHandle(pipe_)) + Win32Fatal("CloseHandle"); + } // Reap child if forgotten. if (child_) Finish(); @@ -92,7 +96,7 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) { // Do not prepend 'cmd /c' on Windows, this breaks command // lines greater than 8,191 chars. if (!CreateProcessA(NULL, (char*)command.c_str(), NULL, NULL, - /* inherit handles */ TRUE, 0, + /* inherit handles */ TRUE, CREATE_NEW_PROCESS_GROUP, NULL, NULL, &startup_info, &process_info)) { DWORD error = GetLastError(); @@ -149,10 +153,9 @@ void Subprocess::OnPipeReady() { // function again later and get them at that point. } -bool Subprocess::Finish() { - if (! child_) { - return false; - } +ExitStatus Subprocess::Finish() { + if (!child_) + return ExitFailure; // TODO: add error handling for all of these. WaitForSingleObject(child_, INFINITE); @@ -163,7 +166,9 @@ bool Subprocess::Finish() { CloseHandle(child_); child_ = NULL; - return exit_code == 0; + return exit_code == 0 ? ExitSuccess : + exit_code == CONTROL_C_EXIT ? ExitInterrupted : + ExitFailure; } bool Subprocess::Done() const { @@ -174,24 +179,47 @@ const string& Subprocess::GetOutput() const { return buf_; } +HANDLE SubprocessSet::ioport_; + SubprocessSet::SubprocessSet() { ioport_ = ::CreateIoCompletionPort(INVALID_HANDLE_VALUE, NULL, 0, 1); if (!ioport_) Win32Fatal("CreateIoCompletionPort"); + if (!SetConsoleCtrlHandler(NotifyInterrupted, TRUE)) + Win32Fatal("SetConsoleCtrlHandler"); } SubprocessSet::~SubprocessSet() { + Clear(); + + SetConsoleCtrlHandler(NotifyInterrupted, FALSE); CloseHandle(ioport_); } -void SubprocessSet::Add(Subprocess* subprocess) { +BOOL WINAPI SubprocessSet::NotifyInterrupted(DWORD dwCtrlType) { + if (dwCtrlType == CTRL_C_EVENT || dwCtrlType == CTRL_BREAK_EVENT) { + if (!PostQueuedCompletionStatus(ioport_, 0, 0, NULL)) + Win32Fatal("PostQueuedCompletionStatus"); + return TRUE; + } + + return FALSE; +} + +Subprocess *SubprocessSet::Add(const string &command) { + Subprocess *subprocess = new Subprocess; + if (!subprocess->Start(this, command)) { + delete subprocess; + return 0; + } if (subprocess->child_) running_.push_back(subprocess); else finished_.push(subprocess); + return subprocess; } -void SubprocessSet::DoWork() { +bool SubprocessSet::DoWork() { DWORD bytes_read; Subprocess* subproc; OVERLAPPED* overlapped; @@ -202,6 +230,10 @@ void SubprocessSet::DoWork() { Win32Fatal("GetQueuedCompletionStatus"); } + if (!subproc) // A NULL subproc indicates that we were interrupted and is + // delivered by NotifyInterrupted above. + return true; + subproc->OnPipeReady(); if (subproc->Done()) { @@ -212,6 +244,8 @@ void SubprocessSet::DoWork() { running_.resize(end - running_.begin()); } } + + return false; } Subprocess* SubprocessSet::NextFinished() { @@ -221,3 +255,16 @@ Subprocess* SubprocessSet::NextFinished() { finished_.pop(); return subproc; } + +void SubprocessSet::Clear() { + for (vector::iterator i = running_.begin(); + i != running_.end(); ++i) { + if ((*i)->child_) + if (!GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, GetProcessId((*i)->child_))) + Win32Fatal("GenerateConsoleCtrlEvent"); + } + for (vector::iterator i = running_.begin(); + i != running_.end(); ++i) + delete *i; + running_.clear(); +} diff --git a/src/subprocess.cc b/src/subprocess.cc index 74eded0..d4a7d03 100644 --- a/src/subprocess.cc +++ b/src/subprocess.cc @@ -42,6 +42,10 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) { if (pipe(output_pipe) < 0) Fatal("pipe: %s", strerror(errno)); fd_ = output_pipe[0]; + // fd_ may be a member of the pselect set in SubprocessSet::DoWork. Check + // that it falls below the system limit. + if (fd_ >= FD_SETSIZE) + Fatal("pipe: %s", strerror(EMFILE)); SetCloseOnExec(fd_); pid_ = fork(); @@ -54,6 +58,14 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) { // Track which fd we use to report errors on. int error_pipe = output_pipe[1]; do { + if (setpgid(0, 0) < 0) + break; + + if (sigaction(SIGINT, &set->old_act_, 0) < 0) + break; + if (sigprocmask(SIG_SETMASK, &set->old_mask_, 0) < 0) + break; + // Open /dev/null over stdin. int devnull = open("/dev/null", O_WRONLY); if (devnull < 0) @@ -100,7 +112,7 @@ void Subprocess::OnPipeReady() { } } -bool Subprocess::Finish() { +ExitStatus Subprocess::Finish() { assert(pid_ != -1); int status; if (waitpid(pid_, &status, 0) < 0) @@ -110,9 +122,12 @@ bool Subprocess::Finish() { if (WIFEXITED(status)) { int exit = WEXITSTATUS(status); if (exit == 0) - return true; + return ExitSuccess; + } else if (WIFSIGNALED(status)) { + if (WTERMSIG(status) == SIGINT) + return ExitInterrupted; } - return false; + return ExitFailure; } bool Subprocess::Done() const { @@ -123,48 +138,89 @@ const string& Subprocess::GetOutput() const { return buf_; } -SubprocessSet::SubprocessSet() {} -SubprocessSet::~SubprocessSet() {} +bool SubprocessSet::interrupted_; + +void SubprocessSet::SetInterruptedFlag(int signum) { + (void) signum; + interrupted_ = true; +} + +SubprocessSet::SubprocessSet() { + interrupted_ = false; + + sigset_t set; + sigemptyset(&set); + sigaddset(&set, SIGINT); + if (sigprocmask(SIG_BLOCK, &set, &old_mask_) < 0) + Fatal("sigprocmask: %s", strerror(errno)); + + struct sigaction act; + memset(&act, 0, sizeof(act)); + act.sa_handler = SetInterruptedFlag; + if (sigaction(SIGINT, &act, &old_act_) < 0) + Fatal("sigaction: %s", strerror(errno)); +} + +SubprocessSet::~SubprocessSet() { + Clear(); -void SubprocessSet::Add(Subprocess* subprocess) { + if (sigaction(SIGINT, &old_act_, 0) < 0) + Fatal("sigaction: %s", strerror(errno)); + if (sigprocmask(SIG_SETMASK, &old_mask_, 0) < 0) + Fatal("sigprocmask: %s", strerror(errno)); +} + +Subprocess *SubprocessSet::Add(const string &command) { + Subprocess *subprocess = new Subprocess; + if (!subprocess->Start(this, command)) { + delete subprocess; + return 0; + } running_.push_back(subprocess); + return subprocess; } -void SubprocessSet::DoWork() { - vector fds; +bool SubprocessSet::DoWork() { + fd_set set; + int nfds = 0; + FD_ZERO(&set); - map fd_to_subprocess; for (vector::iterator i = running_.begin(); i != running_.end(); ++i) { int fd = (*i)->fd_; if (fd >= 0) { - fd_to_subprocess[fd] = *i; - fds.resize(fds.size() + 1); - pollfd* newfd = &fds.back(); - newfd->fd = fd; - newfd->events = POLLIN; - newfd->revents = 0; + FD_SET(fd, &set); + if (nfds < fd+1) + nfds = fd+1; } } - int ret = poll(&fds.front(), fds.size(), -1); + int ret = pselect(nfds, &set, 0, 0, 0, &old_mask_); if (ret == -1) { - if (errno != EINTR) - perror("ninja: poll"); - return; + if (errno != EINTR) { + perror("ninja: pselect"); + return false; + } + bool interrupted = interrupted_; + interrupted_ = false; + return interrupted; } - for (size_t i = 0; i < fds.size(); ++i) { - if (fds[i].revents) { - Subprocess* subproc = fd_to_subprocess[fds[i].fd]; - subproc->OnPipeReady(); - if (subproc->Done()) { - finished_.push(subproc); - std::remove(running_.begin(), running_.end(), subproc); - running_.resize(running_.size() - 1); + for (vector::iterator i = running_.begin(); + i != running_.end(); ) { + int fd = (*i)->fd_; + if (fd >= 0 && FD_ISSET(fd, &set)) { + (*i)->OnPipeReady(); + if ((*i)->Done()) { + finished_.push(*i); + running_.erase(i); + continue; } } + ++i; } + + return false; } Subprocess* SubprocessSet::NextFinished() { @@ -174,3 +230,13 @@ Subprocess* SubprocessSet::NextFinished() { finished_.pop(); return subproc; } + +void SubprocessSet::Clear() { + for (vector::iterator i = running_.begin(); + i != running_.end(); ++i) + kill(-(*i)->pid_, SIGINT); + for (vector::iterator i = running_.begin(); + i != running_.end(); ++i) + delete *i; + running_.clear(); +} diff --git a/src/subprocess.h b/src/subprocess.h index 9828bf4..5150eea 100644 --- a/src/subprocess.h +++ b/src/subprocess.h @@ -22,25 +22,32 @@ using namespace std; #ifdef _WIN32 #include +#else +#include #endif +#include "exit_status.h" + /// Subprocess wraps a single async subprocess. It is entirely /// passive: it expects the caller to notify it when its fds are ready /// for reading, as well as call Finish() to reap the child once done() /// is true. struct Subprocess { - Subprocess(); ~Subprocess(); - bool Start(struct SubprocessSet* set, const string& command); - void OnPipeReady(); - /// Returns true on successful process exit. - bool Finish(); + + /// Returns ExitSuccess on successful process exit, ExitInterrupted if + /// the process was interrupted, ExitFailure if it otherwise failed. + ExitStatus Finish(); bool Done() const; const string& GetOutput() const; private: + Subprocess(); + bool Start(struct SubprocessSet* set, const string& command); + void OnPipeReady(); + string buf_; #ifdef _WIN32 @@ -60,22 +67,30 @@ struct Subprocess { friend struct SubprocessSet; }; -/// SubprocessSet runs a poll() loop around a set of Subprocesses. +/// SubprocessSet runs a pselect() loop around a set of Subprocesses. /// DoWork() waits for any state change in subprocesses; finished_ /// is a queue of subprocesses as they finish. struct SubprocessSet { SubprocessSet(); ~SubprocessSet(); - void Add(Subprocess* subprocess); - void DoWork(); + Subprocess *Add(const string &command); + bool DoWork(); Subprocess* NextFinished(); + void Clear(); vector running_; queue finished_; #ifdef _WIN32 - HANDLE ioport_; + static BOOL WINAPI NotifyInterrupted(DWORD dwCtrlType); + static HANDLE ioport_; +#else + static void SetInterruptedFlag(int signum); + static bool interrupted_; + + struct sigaction old_act_; + sigset_t old_mask_; #endif }; diff --git a/src/subprocess_test.cc b/src/subprocess_test.cc index 840287b..5b3e8a3 100644 --- a/src/subprocess_test.cc +++ b/src/subprocess_test.cc @@ -32,46 +32,71 @@ struct SubprocessTest : public testing::Test { // Run a command that fails and emits to stderr. TEST_F(SubprocessTest, BadCommandStderr) { - Subprocess* subproc = new Subprocess; - EXPECT_TRUE(subproc->Start(&subprocs_, "cmd /c ninja_no_such_command")); - subprocs_.Add(subproc); + Subprocess* subproc = subprocs_.Add("cmd /c ninja_no_such_command"); + ASSERT_NE((Subprocess *) 0, subproc); while (!subproc->Done()) { // Pretend we discovered that stderr was ready for writing. subprocs_.DoWork(); } - EXPECT_FALSE(subproc->Finish()); + EXPECT_EQ(ExitFailure, subproc->Finish()); EXPECT_NE("", subproc->GetOutput()); } // Run a command that does not exist TEST_F(SubprocessTest, NoSuchCommand) { - Subprocess* subproc = new Subprocess; - EXPECT_TRUE(subproc->Start(&subprocs_, "ninja_no_such_command")); - subprocs_.Add(subproc); + Subprocess* subproc = subprocs_.Add("ninja_no_such_command"); + ASSERT_NE((Subprocess *) 0, subproc); while (!subproc->Done()) { // Pretend we discovered that stderr was ready for writing. subprocs_.DoWork(); } - EXPECT_FALSE(subproc->Finish()); + EXPECT_EQ(ExitFailure, subproc->Finish()); EXPECT_NE("", subproc->GetOutput()); #ifdef _WIN32 ASSERT_EQ("CreateProcess failed: The system cannot find the file specified.\n", subproc->GetOutput()); #endif } +#ifndef _WIN32 + +TEST_F(SubprocessTest, InterruptChild) { + Subprocess* subproc = subprocs_.Add("kill -INT $$"); + ASSERT_NE((Subprocess *) 0, subproc); + + while (!subproc->Done()) { + subprocs_.DoWork(); + } + + EXPECT_EQ(ExitInterrupted, subproc->Finish()); +} + +TEST_F(SubprocessTest, InterruptParent) { + Subprocess* subproc = subprocs_.Add("kill -INT $PPID ; sleep 1"); + ASSERT_NE((Subprocess *) 0, subproc); + + while (!subproc->Done()) { + bool interrupted = subprocs_.DoWork(); + if (interrupted) + return; + } + + ADD_FAILURE() << "We should have been interrupted"; +} + +#endif + TEST_F(SubprocessTest, SetWithSingle) { - Subprocess* subproc = new Subprocess; - EXPECT_TRUE(subproc->Start(&subprocs_, kSimpleCommand)); - subprocs_.Add(subproc); + Subprocess* subproc = subprocs_.Add(kSimpleCommand); + ASSERT_NE((Subprocess *) 0, subproc); while (!subproc->Done()) { subprocs_.DoWork(); } - ASSERT_TRUE(subproc->Finish()); + ASSERT_EQ(ExitSuccess, subproc->Finish()); ASSERT_NE("", subproc->GetOutput()); ASSERT_EQ(1u, subprocs_.finished_.size()); @@ -91,9 +116,8 @@ TEST_F(SubprocessTest, SetWithMulti) { }; for (int i = 0; i < 3; ++i) { - processes[i] = new Subprocess; - EXPECT_TRUE(processes[i]->Start(&subprocs_, kCommands[i])); - subprocs_.Add(processes[i]); + processes[i] = subprocs_.Add(kCommands[i]); + ASSERT_NE((Subprocess *) 0, processes[i]); } ASSERT_EQ(3u, subprocs_.running_.size()); @@ -112,7 +136,7 @@ TEST_F(SubprocessTest, SetWithMulti) { ASSERT_EQ(3u, subprocs_.finished_.size()); for (int i = 0; i < 3; ++i) { - ASSERT_TRUE(processes[i]->Finish()); + ASSERT_EQ(ExitSuccess, processes[i]->Finish()); ASSERT_NE("", processes[i]->GetOutput()); delete processes[i]; } -- cgit v0.12 From 44f58aeca923d6e3001636f8c0d24a6d513d8ea2 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Thu, 1 Dec 2011 20:55:18 +0000 Subject: If a command fails, wait for all running commands to terminate before we do Previously, if a command fails, the fate of the other child processes running in parallel was inadequately controlled. On POSIX platforms, the processes were orphaned. Normally they would run to completion, but were liable to being killed by a SIGPIPE. On Windows, the child processes would terminate with the parent. The cleanup-on-interrupt patch caused the SubprocessSet and Builder destructors to clean up after themselves by killing any running child processes and deleting their output files, making the behaviour more predictable and consistent across platforms. If the build is interrupted by the user, this is correct behaviour. But in the case where the build is stopped by a failed command, this would be inconsistent with user expectations. In the latter case, we now let any remaining child processes run to completion before leaving the main loop in Builder::Build. --- src/build.cc | 27 +++++++++++++-------------- src/build.h | 4 ++-- src/build_test.cc | 4 ++-- src/ninja.cc | 8 +++++--- 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/build.cc b/src/build.cc index 4c15de9..265c805 100644 --- a/src/build.cc +++ b/src/build.cc @@ -538,7 +538,7 @@ bool Builder::Build(string* err) { status_->PlanHasTotalEdges(plan_.command_edge_count()); int pending_commands = 0; - int failures_allowed = config_.swallow_failures; + int failures_allowed = config_.failures_allowed; // This main loop runs the entire build process. // It is structured like this: @@ -549,7 +549,7 @@ bool Builder::Build(string* err) { // an error. while (plan_.more_to_do()) { // See if we can start any more commands. - if (command_runner_->CanRunMore()) { + if (failures_allowed && command_runner_->CanRunMore()) { if (Edge* edge = plan_.FindWork()) { if (!StartEdge(edge, err)) { status_->BuildFinished(); @@ -576,13 +576,8 @@ bool Builder::Build(string* err) { --pending_commands; FinishEdge(edge, success, output); if (!success) { - if (failures_allowed-- == 0) { - if (config_.swallow_failures != 0) - *err = "subcommands failed"; - else - *err = "subcommand failed"; - return false; - } + if (failures_allowed) + failures_allowed--; } // We made some progress; start the main loop over. @@ -605,13 +600,17 @@ bool Builder::Build(string* err) { // If we get here, we cannot make any more progress. status_->BuildFinished(); - if (failures_allowed < config_.swallow_failures) { + if (failures_allowed == 0) { + if (config_.failures_allowed > 1) + *err = "subcommands failed"; + else + *err = "subcommand failed"; + } else if (failures_allowed < config_.failures_allowed) *err = "cannot make progress due to previous errors"; - return false; - } else { + else *err = "stuck [this is a bug]"; - return false; - } + + return false; } status_->BuildFinished(); diff --git a/src/build.h b/src/build.h index c75bde1..778d59d 100644 --- a/src/build.h +++ b/src/build.h @@ -98,7 +98,7 @@ struct CommandRunner { /// Options (e.g. verbosity, parallelism) passed to a build. struct BuildConfig { BuildConfig() : verbosity(NORMAL), dry_run(false), parallelism(1), - swallow_failures(0) {} + failures_allowed(1) {} enum Verbosity { NORMAL, @@ -108,7 +108,7 @@ struct BuildConfig { Verbosity verbosity; bool dry_run; int parallelism; - int swallow_failures; + int failures_allowed; }; /// Builder wraps the build process: starting commands, updating status. diff --git a/src/build_test.cc b/src/build_test.cc index de5ddc1..7c3a729 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -661,7 +661,7 @@ TEST_F(BuildTest, SwallowFailures) { "build all: phony out1 out2 out3\n")); // Swallow two failures, die on the third. - config_.swallow_failures = 2; + config_.failures_allowed = 3; string err; EXPECT_TRUE(builder_.AddTarget("all", &err)); @@ -682,7 +682,7 @@ TEST_F(BuildTest, SwallowFailuresLimit) { "build final: cat out1 out2 out3\n")); // Swallow ten failures; we should stop before building final. - config_.swallow_failures = 10; + config_.failures_allowed = 11; string err; EXPECT_TRUE(builder_.AddTarget("final", &err)); diff --git a/src/ninja.cc b/src/ninja.cc index 1822e54..6cc2276 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -13,6 +13,7 @@ // limitations under the License. #include +#include #include #include #include @@ -613,9 +614,10 @@ int main(int argc, char** argv) { if (*end != 0) Fatal("-k parameter not numeric; did you mean -k0?"); - // We want to go until N jobs fail, which means we should ignore - // the first N-1 that fail and then stop. - globals.config.swallow_failures = value - 1; + // We want to go until N jobs fail, which means we should allow + // N failures and then stop. For N <= 0, INT_MAX is close enough + // to infinite for most sane builds. + globals.config.failures_allowed = value > 0 ? value : INT_MAX; break; } case 'n': -- cgit v0.12