From 87e31015d671e324e699f8bfc9a9e40f126fe393 Mon Sep 17 00:00:00 2001 From: Christian Kandeler Date: Tue, 27 Nov 2012 18:56:24 +0100 Subject: Sanitize the Executor code flow. The current behavior is not exactly well-defined, mostly due to signals calling back on the Executor while it is calling a function on the object emitting the signal (e.g. waitForFinished()). These traps are now gone. Also make sure error messages are in a sensible order when a build is canceled. In addition, remove unused functionality. Change-Id: Icf35eb378995a55b5ba4fc6caea9492eccb43527 Reviewed-by: Joerg Bornemann --- src/lib/api/internaljobs.cpp | 22 ++--- src/lib/api/internaljobs.h | 5 +- src/lib/buildgraph/executor.cpp | 131 ++++++++++++-------------- src/lib/buildgraph/executor.h | 37 +++----- src/lib/buildgraph/jscommandexecutor.cpp | 3 +- src/lib/buildgraph/processcommandexecutor.cpp | 3 +- src/lib/tools/error.h | 2 +- 7 files changed, 87 insertions(+), 116 deletions(-) diff --git a/src/lib/api/internaljobs.cpp b/src/lib/api/internaljobs.cpp index 6fb182f00..d74c516ab 100644 --- a/src/lib/api/internaljobs.cpp +++ b/src/lib/api/internaljobs.cpp @@ -225,25 +225,19 @@ void InternalBuildJob::build(const QList &products, void InternalBuildJob::start() { - Executor * const executor = new Executor(this); - connect(executor, SIGNAL(finished()), SLOT(handleFinished())); - connect(executor, SIGNAL(error(Error)), SLOT(handleError(Error))); - executor->setEngine(new ScriptEngine(this)); - executor->setBuildOptions(buildOptions()); - executor->setProgressObserver(observer()); - executor->build(products()); + m_executor = new Executor(this); + connect(m_executor, SIGNAL(finished()), SLOT(handleFinished())); + m_executor->setEngine(new ScriptEngine(this)); + m_executor->setBuildOptions(buildOptions()); + m_executor->setProgressObserver(observer()); + m_executor->build(products()); } void InternalBuildJob::handleFinished() { storeBuildGraph(); - if (!hasError()) // Already emitted finished() in that case. - emit finished(this); -} - -void InternalBuildJob::handleError(const Error &error) -{ - setError(error); + if (m_executor->hasError()) + setError(m_executor->error()); emit finished(this); } diff --git a/src/lib/api/internaljobs.h b/src/lib/api/internaljobs.h index 8eae7682d..ddb3dabe8 100644 --- a/src/lib/api/internaljobs.h +++ b/src/lib/api/internaljobs.h @@ -41,6 +41,7 @@ namespace qbs { namespace Internal { +class Executor; class JobObserver; class InternalJob : public QObject @@ -130,7 +131,9 @@ public: private slots: void start(); void handleFinished(); - void handleError(const Error &error); + +private: + Executor *m_executor; }; diff --git a/src/lib/buildgraph/executor.cpp b/src/lib/buildgraph/executor.cpp index d1c609e98..9e9c778bb 100644 --- a/src/lib/buildgraph/executor.cpp +++ b/src/lib/buildgraph/executor.cpp @@ -43,6 +43,7 @@ #include #include +#include #include @@ -88,7 +89,6 @@ Executor::Executor(QObject *parent) , m_engine(0) , m_progressObserver(0) , m_state(ExecutorIdle) - , m_buildResult(SuccessfulBuild) { m_inputArtifactScanContext = new InputArtifactScannerContext(&m_scanResultCache); m_autoMoc = new AutoMoc; @@ -124,7 +124,8 @@ void Executor::build(const QList &productsToBuild) try { doBuild(productsToBuild); } catch (const Error &e) { - setError(e); + m_error = e; + QTimer::singleShot(0, this, SLOT(finish())); } } @@ -132,10 +133,11 @@ void Executor::doBuild(const QList &productsToBuild) { Q_ASSERT(m_buildOptions.maxJobCount > 0); Q_ASSERT(m_engine); - Q_ASSERT(m_state != ExecutorRunning); + Q_ASSERT(m_state == ExecutorIdle); m_leaves.clear(); - m_buildResult = SuccessfulBuild; m_productsToBuild = productsToBuild; + m_error.clear(); + m_explicitlyCanceled = false; QSet projects; foreach (const BuildProduct::ConstPtr &buildProduct, productsToBuild) @@ -187,18 +189,10 @@ void Executor::doBuild(const QList &productsToBuild) if (sourceFilesChanged) runAutoMoc(); initLeaves(changedArtifacts); - if (!buildNextArtifact()) - finish(); -} - -void Executor::cancelBuild() -{ - if (m_state != ExecutorRunning) - return; - setState(ExecutorCanceled); - cancelJobs(); - m_buildResult = FailedBuild; - emit error(Error(Tr::tr("Build canceled."))); + if (!scheduleJobs()) { + qbsTrace() << "Nothing to do at all, finishing."; + QTimer::singleShot(0, this, SLOT(finish())); // Don't call back on the caller. + } } void Executor::setEngine(ScriptEngine *engine) @@ -272,24 +266,13 @@ void Executor::initLeavesTopDown(Artifact *artifact, QSet &seenArtif } } -/** - * Returns true if there are still artifacts to traverse. - */ -bool Executor::buildNextArtifact() +// Returns true if some artifacts are still waiting to be built or currently building. +bool Executor::scheduleJobs() { - while (m_state == ExecutorRunning) { - if (m_leaves.isEmpty()) - return !m_processingJobs.isEmpty(); - - if (m_availableJobs.isEmpty()) { - if (qbsLogLevel(LoggerDebug)) - qbsDebug("[EXEC] No jobs available. Trying later."); - return true; - } - + Q_ASSERT(m_state == ExecutorRunning); + while (!m_leaves.isEmpty() && !m_availableJobs.isEmpty()) buildArtifact(m_leaves.takeFirst()); - } - return false; + return !m_leaves.isEmpty() || !m_processingJobs.isEmpty(); } static bool isUpToDate(Artifact *artifact) @@ -340,6 +323,8 @@ static bool mustExecuteTransformer(const QSharedPointer &transforme void Executor::buildArtifact(Artifact *artifact) { + Q_ASSERT(!m_availableJobs.isEmpty()); + if (qbsLogLevel(LoggerDebug)) qbsDebug() << "[EXEC] " << fileName(artifact); @@ -451,25 +436,39 @@ void Executor::buildArtifact(Artifact *artifact) job->run(artifact->transformer.data(), artifact->product); } -void Executor::finishJob(ExecutorJob *job) +void Executor::finishJob(ExecutorJob *job, bool success) { Q_ASSERT(job); + Q_ASSERT(m_state != ExecutorIdle); - Artifact *processedArtifact = m_processingJobs.value(job); - Q_ASSERT(processedArtifact); - - m_processingJobs.remove(job); + const QHash::Iterator it = m_processingJobs.find(job); + Q_ASSERT(it != m_processingJobs.end()); + if (success) + finishArtifact(it.value()); + m_processingJobs.erase(it); m_availableJobs.append(job); - finishArtifact(processedArtifact); + if (!success) + cancelJobs(); + + if (m_state == ExecutorRunning && m_progressObserver && m_progressObserver->canceled()) { + qbsTrace() << "Received cancel request; canceling build."; + m_explicitlyCanceled = true; + cancelJobs(); + } - if (m_progressObserver && m_progressObserver->canceled()) { - cancelBuild(); + if (m_state == ExecutorCanceling) { + if (m_processingJobs.isEmpty()) { + qbsTrace() << "All pending jobs are done, finishing."; + finish(); + } return; } - if (!buildNextArtifact()) + if (!scheduleJobs()) { + qbsTrace() << "Nothing left to build; finishing."; finish(); + } } static bool allChildrenBuilt(Artifact *artifact) @@ -549,11 +548,11 @@ void Executor::insertLeavesAfterAddingDependencies(QVector dependenc void Executor::cancelJobs() { + qbsTrace() << "Canceling all jobs."; + setState(ExecutorCanceling); QList jobs = m_processingJobs.keys(); foreach (ExecutorJob *job, jobs) job->cancel(); - foreach (ExecutorJob *job, jobs) - job->waitForFinished(); } void Executor::setupProgressObserver(bool mocWillRun) @@ -581,10 +580,8 @@ void Executor::addExecutorJobs(int jobNumber) job->setMainThreadScriptEngine(m_engine); job->setObjectName(QString(QLatin1String("J%1")).arg(i)); m_availableJobs.append(job); - connect(job, SIGNAL(error(QString)), - this, SLOT(onProcessError(QString))); - connect(job, SIGNAL(success()), - this, SLOT(onProcessSuccess())); + connect(job, SIGNAL(error(QString)), this, SLOT(onProcessError(QString))); + connect(job, SIGNAL(success()), this, SLOT(onProcessSuccess())); } } @@ -621,17 +618,14 @@ void Executor::runAutoMoc() m_progressObserver->incrementProgressValue(m_mocEffort); } -void Executor::onProcessError(QString errorString) +void Executor::onProcessError(const QString &errorString) { - m_buildResult = FailedBuild; - if (m_buildOptions.keepGoing) { - qbsWarning() << tr("ignoring error: %1").arg(errorString); - ExecutorJob * const job = qobject_cast(sender()); - finishJob(job); - } else { - setError(Error(errorString)); - finish(); - } + if (m_buildOptions.keepGoing) + qbsWarning() << Tr::tr("ignoring error: %1").arg(errorString); + else + qbsError() << errorString; + ExecutorJob * const job = qobject_cast(sender()); + finishJob(job, false); } void Executor::onProcessSuccess() @@ -650,13 +644,12 @@ void Executor::onProcessSuccess() artifact->timestamp = FileInfo(artifact->filePath()).lastModified(); } - finishJob(job); + finishJob(job, true); } void Executor::finish() { - if (m_state == ExecutorIdle) - return; + Q_ASSERT(m_state != ExecutorIdle); QStringList unbuiltProductNames; foreach (BuildProduct::Ptr buildProduct, m_productsToBuild) { @@ -669,13 +662,14 @@ void Executor::finish() } if (unbuiltProductNames.isEmpty()) { qbsInfo() << DontPrintLogLevel << LogOutputStdOut << TextColorGreen - << "Build done."; + << Tr::tr("Build done."); } else { - qbsError() << tr("The following products could not be built: %1.").arg(unbuiltProductNames.join(", ")); - qbsInfo() << DontPrintLogLevel << LogOutputStdOut << TextColorRed - << "Build failed."; + m_error = Error(Tr::tr("The following products could not be built: %1.") + .arg(unbuiltProductNames.join(", "))); } + if (m_explicitlyCanceled) + m_error.append(Tr::tr("Build was canceled due to user request.")); setState(ExecutorIdle); if (m_progressObserver) m_progressObserver->setFinished(); @@ -760,14 +754,5 @@ void Executor::setState(ExecutorState s) m_state = s; } -void Executor::setError(const Error &e) -{ - if (m_state != ExecutorRunning) - return; - setState(ExecutorError); - cancelJobs(); - emit error(e); -} - } // namespace Internal } // namespace qbs diff --git a/src/lib/buildgraph/executor.h b/src/lib/buildgraph/executor.h index e6365e044..1d0cd2aa1 100644 --- a/src/lib/buildgraph/executor.h +++ b/src/lib/buildgraph/executor.h @@ -35,6 +35,7 @@ #include #include #include +#include #include #include @@ -57,53 +58,40 @@ public: void build(const QList &productsToBuild); - enum ExecutorState { - ExecutorIdle, - ExecutorRunning, - ExecutorCanceled, - ExecutorError - }; - - enum BuildResult { - SuccessfulBuild, - FailedBuild - }; - void setEngine(ScriptEngine *engine); void setBuildOptions(const BuildOptions &buildOptions); void setProgressObserver(ProgressObserver *observer) { m_progressObserver = observer; } - ExecutorState state() const { return m_state; } - BuildResult buildResult() const { return m_buildResult; } + + Error error() const { return m_error; } + bool hasError() const { return !error().entries().isEmpty(); } signals: - void error(const Error &error); void finished(); private slots: - void onProcessError(QString errorString); + void onProcessError(const QString &errorString); void onProcessSuccess(); + void finish(); private: + enum ExecutorState { ExecutorIdle, ExecutorRunning, ExecutorCanceling }; + void doBuild(const QList &productsToBuild); - void cancelBuild(); void prepareBuildGraph(const Artifact::BuildState buildState, bool *sourceFilesChanged); void prepareBuildGraph_impl(Artifact *artifact, const Artifact::BuildState buildState, bool *sourceFilesChanged); void updateBuildGraph(Artifact::BuildState buildState); void updateBuildGraph_impl(Artifact *artifact, Artifact::BuildState buildState, QSet &seenArtifacts); void initLeaves(const QList &changedArtifacts); void initLeavesTopDown(Artifact *artifact, QSet &seenArtifacts); - bool buildNextArtifact(); + bool scheduleJobs(); void buildArtifact(Artifact *artifact); - void finishJob(ExecutorJob *job); + void finishJob(ExecutorJob *job, bool success); void finishArtifact(Artifact *artifact); - void finish(); void initializeArtifactsState(); void setState(ExecutorState); - void setError(const Error &e); void addExecutorJobs(int jobNumber); void removeExecutorJobs(int jobNumber); void runAutoMoc(); - void printScanningMessageOnce(); void insertLeavesAfterAddingDependencies(QVector dependencies); void cancelJobs(); void setupProgressObserver(bool mocWillRun); @@ -114,7 +102,6 @@ private: QList m_availableJobs; QHash m_processingJobs; ExecutorState m_state; - BuildResult m_buildResult; QList m_productsToBuild; QList m_roots; QList m_leaves; @@ -122,8 +109,8 @@ private: InputArtifactScannerContext *m_inputArtifactScanContext; AutoMoc *m_autoMoc; int m_mocEffort; - - friend class ExecutorJob; + Error m_error; + bool m_explicitlyCanceled; }; } // namespace Internal diff --git a/src/lib/buildgraph/jscommandexecutor.cpp b/src/lib/buildgraph/jscommandexecutor.cpp index 4b668b2ea..fa94e7d9a 100644 --- a/src/lib/buildgraph/jscommandexecutor.cpp +++ b/src/lib/buildgraph/jscommandexecutor.cpp @@ -48,6 +48,7 @@ #include #include #include +#include namespace qbs { namespace Internal { @@ -150,7 +151,7 @@ void JsCommandExecutor::waitForFinished() void JsCommandExecutor::doStart() { if (dryRun()) { - emit finished(); + QTimer::singleShot(0, this, SIGNAL(finished())); // Don't call back on the caller. return; } QFuture future = QtConcurrent::run(JSRunner(jsCommand()), transformer()); diff --git a/src/lib/buildgraph/processcommandexecutor.cpp b/src/lib/buildgraph/processcommandexecutor.cpp index 1dffb3d08..333c9c151 100644 --- a/src/lib/buildgraph/processcommandexecutor.cpp +++ b/src/lib/buildgraph/processcommandexecutor.cpp @@ -43,6 +43,7 @@ #include #include #include +#include namespace qbs { namespace Internal { @@ -103,7 +104,7 @@ void ProcessCommandExecutor::doStart() qbsInfo() << DontPrintLogLevel << LogOutputStdOut << m_commandLine; qbsDebug() << "[EXEC] " << m_commandLine; if (dryRun()) { - emit finished(); + QTimer::singleShot(0, this, SIGNAL(finished())); // Don't call back on the caller. return; } diff --git a/src/lib/tools/error.h b/src/lib/tools/error.h index 349f98ef7..f0bfad0ca 100644 --- a/src/lib/tools/error.h +++ b/src/lib/tools/error.h @@ -60,7 +60,7 @@ class Error public: Error(); Error(const Error &rhs); - Error(const QString &description, + explicit Error(const QString &description, const QString &file = QString(), int line = 0, int column = 0); Error(const QString &description, const CodeLocation &location); -- cgit v1.2.3