diff options
author | Christian Kandeler <christian.kandeler@qt.io> | 2018-07-20 15:01:58 +0200 |
---|---|---|
committer | Christian Kandeler <christian.kandeler@qt.io> | 2018-07-30 13:07:38 +0000 |
commit | 1f327e8321a594d886ad226630bac9313c31d049 (patch) | |
tree | 5fb3e1418afebe2bc0f3c94b12eee33604748978 | |
parent | cffe2129465d7fa5dd86c7a7beabeb9cbbf0389c (diff) | |
download | qbs-1f327e8321a594d886ad226630bac9313c31d049.tar.gz |
Fix false positives in artifacts map change tracking
We used to invalidate a script if it accessed the artifacts map and that
map's keys are now different from what they were. However, that logic
was too coarse: All rules that run after the command will add new
artifacts, so such changes will be the norm, at least when the project
is built for the first time. As a result, some artifacts might get
rebuilt unnecessarily on the next qbs invocation.
The reason why we did it that way was that we needed to consider the
case where a script iterated over the elements of an artifacts map, e.g.
using a for..in loop. In such a case, we cannot know which part of the
map the code is interested in and the script needs to be re-run if the
keys change. This is now solved with the help of a QScriptClass, which
informs us if a script enumerates the keys. In all other cases, the
script only accesses well-known tags, so we only have to compare the
respective artifact lists and do not care about other keys getting added
or removed.
Change-Id: I182a50c34ece28f7ff3f7eba7a8f246e9b79b30e
Reviewed-by: Joerg Bornemann <joerg.bornemann@qt.io>
-rw-r--r-- | src/lib/corelib/buildgraph/artifactsscriptvalue.cpp | 63 | ||||
-rw-r--r-- | src/lib/corelib/buildgraph/requestedartifacts.cpp | 49 | ||||
-rw-r--r-- | src/lib/corelib/buildgraph/requestedartifacts.h | 5 | ||||
-rw-r--r-- | src/lib/corelib/language/scriptengine.h | 15 | ||||
-rw-r--r-- | src/lib/corelib/tools/persistence.cpp | 2 | ||||
-rw-r--r-- | tests/auto/blackbox/testdata/artifacts-map-change-tracking/artifacts-map-change-tracking.qbs | 27 | ||||
-rw-r--r-- | tests/auto/blackbox/tst_blackbox.cpp | 24 |
7 files changed, 161 insertions, 24 deletions
diff --git a/src/lib/corelib/buildgraph/artifactsscriptvalue.cpp b/src/lib/corelib/buildgraph/artifactsscriptvalue.cpp index 0fcede324..c32e95b2a 100644 --- a/src/lib/corelib/buildgraph/artifactsscriptvalue.cpp +++ b/src/lib/corelib/buildgraph/artifactsscriptvalue.cpp @@ -45,6 +45,7 @@ #include <language/language.h> #include <language/scriptengine.h> +#include <QtScript/qscriptclass.h> #include <QtScript/qscriptcontext.h> namespace qbs { @@ -52,7 +53,44 @@ namespace Internal { enum BuildGraphScriptValueCommonPropertyKeys : quint32 { CachedValueKey, - FileTagKey + FileTagKey, + ProductPtrKey, +}; + +class ArtifactsScriptClass : public QScriptClass +{ +public: + ArtifactsScriptClass(QScriptEngine *engine) : QScriptClass(engine) { } + +private: + QueryFlags queryProperty(const QScriptValue &object, const QScriptString &name, + QueryFlags flags, uint *id) override + { + getProduct(object); + qbsEngine()->setNonExistingArtifactSetRequested(m_product, name.toString()); + return QScriptClass::queryProperty(object, name, flags, id); + } + + QScriptClassPropertyIterator *newIterator(const QScriptValue &object) override + { + getProduct(object); + qbsEngine()->setArtifactsEnumerated(m_product); + return QScriptClass::newIterator(object); + } + + void getProduct(const QScriptValue &object) + { + if (m_lastObjectId != object.objectId()) { + m_lastObjectId = object.objectId(); + m_product = reinterpret_cast<const ResolvedProduct *>( + object.data().property(ProductPtrKey).toVariant().value<quintptr>()); + } + } + + ScriptEngine *qbsEngine() const { return static_cast<ScriptEngine *>(engine()); } + + qint64 m_lastObjectId = 0; + const ResolvedProduct *m_product = nullptr; }; static bool isRelevantArtifact(const ResolvedProduct *, const Artifact *artifact) @@ -74,6 +112,27 @@ static ArtifactSetByFileTag artifactsMap(const ResolvedModule *module) return artifactsMap(module->product); } +static QScriptValue createArtifactsObject(const ResolvedProduct *product, ScriptEngine *engine) +{ + QScriptClass *scriptClass = engine->artifactsScriptClass(); + if (!scriptClass) { + scriptClass = new ArtifactsScriptClass(engine); + engine->setArtifactsScriptClass(scriptClass); + } + QScriptValue artifactsObj = engine->newObject(scriptClass); + QScriptValue data = engine->newObject(); + QVariant v; + v.setValue<quintptr>(reinterpret_cast<quintptr>(product)); + data.setProperty(ProductPtrKey, engine->newVariant(v)); + artifactsObj.setData(data); + return artifactsObj; +} + +static QScriptValue createArtifactsObject(const ResolvedModule *, ScriptEngine *engine) +{ + return engine->newObject(); +} + static bool checkAndSetArtifactsMapUpToDateFlag(const ResolvedProduct *p) { return p->buildData->checkAndSetJsArtifactsMapUpToDateFlag(); @@ -119,7 +178,7 @@ template<class ProductOrModule> static QScriptValue js_artifacts( QScriptValue artifactsObj = ctx->callee().property(CachedValueKey); if (artifactsObj.isObject() && checkAndSetArtifactsMapUpToDateFlag(productOrModule)) return artifactsObj; - artifactsObj = engine->newObject(); + artifactsObj = createArtifactsObject(productOrModule, engine); ctx->callee().setProperty(CachedValueKey, artifactsObj); const auto &map = artifactsMap(productOrModule); for (auto it = map.cbegin(); it != map.cend(); ++it) { diff --git a/src/lib/corelib/buildgraph/requestedartifacts.cpp b/src/lib/corelib/buildgraph/requestedartifacts.cpp index 0028c21bb..200f226ce 100644 --- a/src/lib/corelib/buildgraph/requestedartifacts.cpp +++ b/src/lib/corelib/buildgraph/requestedartifacts.cpp @@ -95,6 +95,20 @@ void RequestedArtifacts::setArtifactsForTag(const ResolvedProduct *product, filePaths.insert(a->filePath()); } +void RequestedArtifacts::setNonExistingTagRequested(const ResolvedProduct *product, + const QString &tag) +{ + RequestedArtifactsPerProduct &ra = m_requestedArtifactsPerProduct[product->uniqueName()]; + QBS_ASSERT(!ra.allTags.empty(), ;); + Set<QString> &filePaths = ra.requestedTags[tag]; + QBS_CHECK(filePaths.empty()); +} + +void RequestedArtifacts::setArtifactsEnumerated(const ResolvedProduct *product) +{ + m_requestedArtifactsPerProduct[product->uniqueName()].artifactsEnumerated = true; +} + void RequestedArtifacts::unite(const RequestedArtifacts &other) { for (auto it = other.m_requestedArtifactsPerProduct.begin(); @@ -129,24 +143,33 @@ bool RequestedArtifacts::RequestedArtifactsPerProduct::isUpToDate( << "is now disabled"; return false; } + + if (!artifactsEnumerated && requestedTags.empty()) + return true; + const ArtifactSetByFileTag currentArtifacts = product->buildData->artifactsByFileTag(); - Set<QString> currentTags; - for (auto it = currentArtifacts.begin(); it != currentArtifacts.end(); ++it) { - const QString tagString = it.key().toString(); - currentTags.insert(tagString); - const auto reqTagsIt = requestedTags.find(tagString); - if (reqTagsIt == requestedTags.end()) - continue; + for (auto reqIt = requestedTags.cbegin(); reqIt != requestedTags.cend(); ++reqIt) { + const FileTag tag = FileTag(reqIt->first.toUtf8()); + const auto currentIt = currentArtifacts.constFind(tag); Set<QString> currentFilePathsForTag; - for (const Artifact * const a : it.value()) - currentFilePathsForTag.insert(a->filePath()); - if (currentFilePathsForTag != reqTagsIt->second) { + if (currentIt != currentArtifacts.constEnd()) { + for (const Artifact * const a : currentIt.value()) + currentFilePathsForTag.insert(a->filePath()); + } + if (currentFilePathsForTag != reqIt->second) { qCDebug(lcBuildGraph) << "artifacts map not up to date: requested artifact set for " - "file tag" << tagString << "in product" + "file tag" << reqIt->first << "in product" << product->uniqueName() << "differs from the current one"; return false; } } + + if (!artifactsEnumerated) + return true; + + Set<QString> currentTags; + for (auto it = currentArtifacts.begin(); it != currentArtifacts.end(); ++it) + currentTags.insert(it.key().toString()); if (currentTags != allTags) { qCDebug(lcBuildGraph) << "artifacts map not up to date: overall file tags differ for " << "product" << product->uniqueName(); @@ -169,10 +192,8 @@ void RequestedArtifacts::RequestedArtifactsPerProduct::unite( void RequestedArtifacts::RequestedArtifactsPerProduct::doSanityChecks() const { - Set<QString> requestedTagsSet; for (auto it = requestedTags.begin(); it != requestedTags.end(); ++it) - requestedTagsSet.insert(it->first); - QBS_CHECK(allTags.contains(requestedTagsSet)); + QBS_CHECK(allTags.contains(it->first) || it->second.empty()); } void RequestedArtifacts::RequestedArtifactsPerProduct::load(PersistentPool &pool) diff --git a/src/lib/corelib/buildgraph/requestedartifacts.h b/src/lib/corelib/buildgraph/requestedartifacts.h index 4d4a01297..82c393803 100644 --- a/src/lib/corelib/buildgraph/requestedartifacts.h +++ b/src/lib/corelib/buildgraph/requestedartifacts.h @@ -61,6 +61,8 @@ public: void clear() { m_requestedArtifactsPerProduct.clear(); } void setAllArtifactTags(const ResolvedProduct *product); void setArtifactsForTag(const ResolvedProduct *product, const FileTag &tag); + void setNonExistingTagRequested(const ResolvedProduct *product, const QString &tag); + void setArtifactsEnumerated(const ResolvedProduct *product); void unite(const RequestedArtifacts &other); void doSanityChecks() const; @@ -78,6 +80,7 @@ private: { Set<QString> allTags; std::unordered_map<QString, Set<QString>> requestedTags; + bool artifactsEnumerated = false; bool isUpToDate(const ResolvedProduct *product) const; @@ -87,7 +90,7 @@ private: template<PersistentPool::OpType opType> void serializationOp(PersistentPool &pool) { - pool.serializationOp<opType>(allTags, requestedTags); + pool.serializationOp<opType>(allTags, requestedTags, artifactsEnumerated); } void load(PersistentPool &pool); diff --git a/src/lib/corelib/language/scriptengine.h b/src/lib/corelib/language/scriptengine.h index 11ed51a49..bfe238b09 100644 --- a/src/lib/corelib/language/scriptengine.h +++ b/src/lib/corelib/language/scriptengine.h @@ -128,6 +128,14 @@ public: { m_requestedArtifacts.setArtifactsForTag(product, tag); } + void setNonExistingArtifactSetRequested(const ResolvedProduct *product, const QString &tag) + { + m_requestedArtifacts.setNonExistingTagRequested(product, tag); + } + void setArtifactsEnumerated(const ResolvedProduct *product) + { + m_requestedArtifacts.setArtifactsEnumerated(product); + } void addPropertyRequestedFromArtifact(const Artifact *artifact, const Property &property); void addRequestedExport(const ResolvedProduct *product) { m_requestedExports.insert(product); } void clearRequestedProperties() { @@ -238,6 +246,12 @@ public: m_productPropertyScriptClass = productPropertyScriptClass; } + QScriptClass *artifactsScriptClass() const { return m_artifactsScriptClass; } + void setArtifactsScriptClass(QScriptClass *artifactsScriptClass) + { + m_artifactsScriptClass = artifactsScriptClass; + } + void addResourceAcquiringScriptObject(ResourceAcquiringScriptObject *obj); void releaseResourcesOfScriptObjects(); @@ -298,6 +312,7 @@ private: ScriptImporter *m_scriptImporter; QScriptClass *m_modulePropertyScriptClass; QScriptClass *m_productPropertyScriptClass = nullptr; + QScriptClass *m_artifactsScriptClass = nullptr; QHash<JsImport, QScriptValue> m_jsImportCache; bool m_propertyCacheEnabled; bool m_active; diff --git a/src/lib/corelib/tools/persistence.cpp b/src/lib/corelib/tools/persistence.cpp index 4ad024c8d..e705cb2da 100644 --- a/src/lib/corelib/tools/persistence.cpp +++ b/src/lib/corelib/tools/persistence.cpp @@ -49,7 +49,7 @@ namespace qbs { namespace Internal { -static const char QBS_PERSISTENCE_MAGIC[] = "QBSPERSISTENCE-118"; +static const char QBS_PERSISTENCE_MAGIC[] = "QBSPERSISTENCE-119"; NoBuildGraphError::NoBuildGraphError(const QString &filePath) : ErrorInfo(Tr::tr("Build graph not found for configuration '%1'. Expected location was '%2'.") diff --git a/tests/auto/blackbox/testdata/artifacts-map-change-tracking/artifacts-map-change-tracking.qbs b/tests/auto/blackbox/testdata/artifacts-map-change-tracking/artifacts-map-change-tracking.qbs index 92579ad7c..0d3c361f6 100644 --- a/tests/auto/blackbox/testdata/artifacts-map-change-tracking/artifacts-map-change-tracking.qbs +++ b/tests/auto/blackbox/testdata/artifacts-map-change-tracking/artifacts-map-change-tracking.qbs @@ -56,4 +56,31 @@ Project { } } } + + Product { + name: "p" + type: "p_type" + Rule { + multiplex: true + Artifact { filePath: "dummy1"; fileTags: "d_type" } + prepare: { + var cmd = new JavaScriptCommand(); + cmd.description = "generating " + output.fileName; + cmd.sourceCode = function() { + var blubb = product.artifacts.qbs; + }; + return cmd; + } + } + Rule { + inputs: "d_type" + Artifact { filePath: "dummy2"; fileTags: "p_type" } + prepare: { + var cmd = new JavaScriptCommand(); + cmd.description = "generating " + output.fileName; + cmd.sourceCode = function() { }; + return cmd; + } + } + } } diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp index 3aac681eb..2a229954f 100644 --- a/tests/auto/blackbox/tst_blackbox.cpp +++ b/tests/auto/blackbox/tst_blackbox.cpp @@ -376,12 +376,14 @@ void TestBlackbox::artifactsMapChangeTracking() QVERIFY2(m_qbsStdout.contains("running rule for test.cpp"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("creating test.cpp"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("linking"), m_qbsStdout.constData()); - QCOMPARE(runQbs(QStringList{"-p", "meta"}), 0); + QCOMPARE(runQbs(QStringList{"-p", "meta,p"}), 0); QVERIFY2(m_qbsStdout.contains("printing artifacts"), m_qbsStdout.constData()); QVERIFY2(!m_qbsStdout.contains("test.txt"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("main.cpp"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("test.cpp"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("TheBinary"), m_qbsStdout.constData()); + QVERIFY2(m_qbsStdout.contains("dummy1"), m_qbsStdout.constData()); + QVERIFY2(m_qbsStdout.contains("dummy2"), m_qbsStdout.constData()); // Change name of target binary. Command must be re-run, because the file name of an // artifact changed. @@ -393,13 +395,15 @@ void TestBlackbox::artifactsMapChangeTracking() QVERIFY2(!m_qbsStdout.contains("running rule for test.cpp"), m_qbsStdout.constData()); QVERIFY2(!m_qbsStdout.contains("creating test.cpp"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("linking"), m_qbsStdout.constData()); - QCOMPARE(runQbs(QStringList{"-p", "meta"}), 0); + QCOMPARE(runQbs(QStringList{"-p", "meta,p"}), 0); QVERIFY2(m_qbsStdout.contains("printing artifacts"), m_qbsStdout.constData()); QVERIFY2(!m_qbsStdout.contains("test.txt"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("main.cpp"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("test.cpp"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("TheNewBinary"), m_qbsStdout.constData()); QVERIFY2(!m_qbsStdout.contains("TheBinary"), m_qbsStdout.constData()); + QVERIFY2(!m_qbsStdout.contains("dummy1"), m_qbsStdout.constData()); + QVERIFY2(!m_qbsStdout.contains("dummy2"), m_qbsStdout.constData()); // Add file tag to generated artifact. Command must be re-run, because it enumerates the keys // of the artifacts map. @@ -409,12 +413,14 @@ void TestBlackbox::artifactsMapChangeTracking() QVERIFY2(m_qbsStdout.contains("running rule for test.cpp"), m_qbsStdout.constData()); QVERIFY2(!m_qbsStdout.contains("creating test.cpp"), m_qbsStdout.constData()); QVERIFY2(!m_qbsStdout.contains("linking"), m_qbsStdout.constData()); - QCOMPARE(runQbs(QStringList{"-p", "meta"}), 0); + QCOMPARE(runQbs(QStringList{"-p", "meta,p"}), 0); QVERIFY2(m_qbsStdout.contains("printing artifacts"), m_qbsStdout.constData()); QVERIFY2(!m_qbsStdout.contains("test.txt"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("main.cpp"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("test.cpp"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("TheNewBinary"), m_qbsStdout.constData()); + QVERIFY2(!m_qbsStdout.contains("dummy1"), m_qbsStdout.constData()); + QVERIFY2(!m_qbsStdout.contains("dummy2"), m_qbsStdout.constData()); // Add redundant file tag to generated artifact. Command must not be re-run, because // the artifacts map has not changed. @@ -426,8 +432,10 @@ void TestBlackbox::artifactsMapChangeTracking() QVERIFY2(!m_qbsStdout.contains("running rule for test.cpp"), m_qbsStdout.constData()); QVERIFY2(!m_qbsStdout.contains("creating test.cpp"), m_qbsStdout.constData()); QVERIFY2(!m_qbsStdout.contains("linking"), m_qbsStdout.constData()); - QCOMPARE(runQbs(QStringList{"-p", "meta"}), 0); + QCOMPARE(runQbs(QStringList{"-p", "meta,p"}), 0); QVERIFY2(!m_qbsStdout.contains("printing artifacts"), m_qbsStdout.constData()); + QVERIFY2(!m_qbsStdout.contains("dummy1"), m_qbsStdout.constData()); + QVERIFY2(!m_qbsStdout.contains("dummy2"), m_qbsStdout.constData()); // Rebuild the app. Command must not be re-run, because the artifacts map has not changed. WAIT_FOR_NEW_TIMESTAMP(); @@ -436,8 +444,10 @@ void TestBlackbox::artifactsMapChangeTracking() QVERIFY2(!m_qbsStdout.contains("running rule for test.cpp"), m_qbsStdout.constData()); QVERIFY2(!m_qbsStdout.contains("creating test.cpp"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("linking"), m_qbsStdout.constData()); - QCOMPARE(runQbs(QStringList{"-p", "meta"}), 0); + QCOMPARE(runQbs(QStringList{"-p", "meta,p"}), 0); QVERIFY2(!m_qbsStdout.contains("printing artifacts"), m_qbsStdout.constData()); + QVERIFY2(!m_qbsStdout.contains("dummy1"), m_qbsStdout.constData()); + QVERIFY2(!m_qbsStdout.contains("dummy2"), m_qbsStdout.constData()); // Add source file to app. Command must be re-run, because the artifacts map has changed. WAIT_FOR_NEW_TIMESTAMP(); @@ -447,12 +457,14 @@ void TestBlackbox::artifactsMapChangeTracking() QVERIFY2(!m_qbsStdout.contains("running rule for test.cpp"), m_qbsStdout.constData()); QVERIFY2(!m_qbsStdout.contains("creating test.cpp"), m_qbsStdout.constData()); QVERIFY2(!m_qbsStdout.contains("linking"), m_qbsStdout.constData()); - QCOMPARE(runQbs(QStringList{"-p", "meta"}), 0); + QCOMPARE(runQbs(QStringList{"-p", "meta,p"}), 0); QVERIFY2(m_qbsStdout.contains("printing artifacts"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("test.txt"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("main.cpp"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("test.cpp"), m_qbsStdout.constData()); QVERIFY2(m_qbsStdout.contains("TheNewBinary"), m_qbsStdout.constData()); + QVERIFY2(!m_qbsStdout.contains("dummy1"), m_qbsStdout.constData()); + QVERIFY2(!m_qbsStdout.contains("dummy2"), m_qbsStdout.constData()); } void TestBlackbox::artifactsMapInvalidation() |