Merge pull request #304 from BenchR267/feature/39798231-add-pid-to-extended-result
Added process identifier to command_extended_result
diff --git a/lib/Core/BuildEngine.cpp b/lib/Core/BuildEngine.cpp
index 8312c89..b1efc1c 100644
--- a/lib/Core/BuildEngine.cpp
+++ b/lib/Core/BuildEngine.cpp
@@ -492,6 +492,7 @@
// Create the task for this rule.
Task* task = ruleInfo.rule.action(buildEngine);
+ assert(task && "rule action returned null task");
// Find the task info for this task.
auto taskInfo = getTaskInfo(task);
@@ -1237,7 +1238,9 @@
finishedTaskInfos.clear();
}
}
-
+
+ std::lock_guard<std::mutex> guard(taskInfosMutex);
+
for (auto& it: taskInfos) {
// Cancel the task, marking it incomplete.
//
@@ -1350,10 +1353,12 @@
RuleInfo& addRule(KeyID keyID, Rule&& rule) {
auto result = ruleInfos.emplace(keyID, RuleInfo(keyID, std::move(rule)));
if (!result.second) {
- // FIXME: Error handling.
- std::cerr << "error: attempt to register duplicate rule \""
- << rule.key << "\"\n";
- exit(1);
+ delegate.error("attempt to register duplicate rule \"" + rule.key + "\"\n");
+
+ // Set cancelled, but return something 'valid' for use until it is
+ // processed.
+ buildCancelled = true;
+ return result.first->second;
}
// If we have a database attached, retrieve any stored result.
@@ -1366,8 +1371,10 @@
std::string error;
db->lookupRuleResult(ruleInfo.keyID, ruleInfo.rule, &ruleInfo.result, &error);
if (!error.empty()) {
+ // FIXME: Investigate changing the database error handling model to
+ // allow builds to proceed without the database.
delegate.error(error);
- cancelRemainingTasks();
+ buildCancelled = true;
}
}
@@ -1481,7 +1488,7 @@
void dumpGraphToFile(const std::string& path) {
FILE* fp = ::fopen(path.c_str(), "w");
if (!fp) {
- delegate.error("error: unable to open graph output path \"" + path + "\"");
+ delegate.error("unable to open graph output path \"" + path + "\"");
return;
}
@@ -1551,8 +1558,8 @@
void taskNeedsInput(Task* task, const KeyType& key, uintptr_t inputID) {
// Validate the InputID.
if (inputID > BuildEngine::kMaximumInputID) {
- delegate.error("error: attempt to use reserved input ID");
- cancelRemainingTasks();
+ delegate.error("attempt to use reserved input ID");
+ buildCancelled = true;
return;
}
@@ -1569,8 +1576,8 @@
assert(taskInfo && "cannot request inputs for an unknown task");
if (!taskInfo->forRuleInfo->isInProgressComputing()) {
- delegate.error("error: invalid state for adding discovered dependency");
- cancelRemainingTasks();
+ delegate.error("invalid state for adding discovered dependency");
+ buildCancelled = true;
return;
}
@@ -1586,8 +1593,8 @@
assert(taskInfo && "cannot request inputs for an unknown task");
if (!taskInfo->forRuleInfo->isInProgressComputing()) {
- delegate.error("error: invalid state for marking task complete");
- cancelRemainingTasks();
+ delegate.error("invalid state for marking task complete");
+ buildCancelled = true;
return;
}
diff --git a/unittests/Core/BuildEngineCancellationTest.cpp b/unittests/Core/BuildEngineCancellationTest.cpp
index 8d25910..04b69e1 100644
--- a/unittests/Core/BuildEngineCancellationTest.cpp
+++ b/unittests/Core/BuildEngineCancellationTest.cpp
@@ -28,6 +28,8 @@
namespace {
class SimpleBuildEngineDelegate : public core::BuildEngineDelegate {
+public:
+ bool expectError = false;
private:
virtual core::Rule lookupRule(const core::KeyType& key) override {
// We never expect dynamic rule lookup.
@@ -43,7 +45,8 @@
virtual void error(const Twine& message) override {
fprintf(stderr, "error: %s\n", message.str().c_str());
- abort();
+ if (!expectError)
+ abort();
}
};
@@ -114,6 +117,13 @@
compute)); };
}
+static ActionFn simpleActionExternalTask(const std::vector<KeyType>& inputs,
+ SimpleTask::ComputeFnType compute, Task** task) {
+ return [=] (BuildEngine& engine) {
+ *task = new SimpleTask([inputs]{ return inputs; }, compute);
+ return engine.registerTask(*task); };
+}
+
TEST(BuildEngineCancellationTest, basic) {
std::vector<std::string> builtKeys;
SimpleBuildEngineDelegate delegate;
@@ -152,4 +162,50 @@
EXPECT_EQ("result", builtKeys[1]);
}
+TEST(BuildEngineCancellationDueToDuplicateTaskTest, basic) {
+ std::vector<std::string> builtKeys;
+ SimpleBuildEngineDelegate delegate;
+ core::BuildEngine engine(delegate);
+ Task* taskA = nullptr;
+ engine.addRule({
+ "value-A", simpleActionExternalTask({"value-B"},
+ [&] (const std::vector<int>& inputs) {
+ builtKeys.push_back("value-A");
+ fprintf(stderr, "building A\n");
+ return 2;
+ },
+ &taskA)
+ });
+ engine.addRule({
+ "value-B", simpleAction({}, [&] (const std::vector<int>& inputs) {
+ builtKeys.push_back("value-B");
+ fprintf(stderr, "building B (and reporting A complete early)\n");
+ engine.taskIsComplete(taskA, intToValue(2));
+ return 3; }) });
+ engine.addRule({
+ "result",
+ simpleAction({"value-A", "value-B"},
+ [&] (const std::vector<int>& inputs) {
+ EXPECT_EQ(2U, inputs.size());
+ EXPECT_EQ(2, inputs[0]);
+ EXPECT_EQ(3, inputs[1]);
+ builtKeys.push_back("result");
+ return inputs[0] * 3 + inputs[1];
+ }) });
+
+ // Build the result, expecting error
+ //
+ // This test is triggering the cancellation behavior through what is
+ // clearly broken client behavior (reporting a task complete that should
+ // not have started yet). The engine should cleanly cancel and report the
+ // error, which is what this test is checking. However, the question remains
+ // are there 'legitimate'/non-broken client pathways that could also trigger
+ // that error?
+ delegate.expectError = true;
+ auto result = engine.build("result");
+ EXPECT_EQ(0U, result.size());
+ EXPECT_EQ(1U, builtKeys.size());
+ EXPECT_EQ("value-B", builtKeys[0]);
+}
+
}
