Take into account qbs module props used by providers when caching

Otherwise, we end up with wrong modules used by products that override
qbs module props such as sysroot.

Change-Id: Id5a74e8198217e737fb02a506ac7a9bb216b4d60
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>

8 files changed, 125 insertions, 9 deletions

diff --git a/src/lib/corelib/language/moduleproviderinfo.h b/src/lib/corelib/language/moduleproviderinfo.h
index 8ed6f008d..d4c375f4a 100644
--- a/src/lib/corelib/language/moduleproviderinfo.h
+++ b/src/lib/corelib/language/moduleproviderinfo.h
@@ -92,7 +92,12 @@ using ModuleProviderInfoList = std::vector<ModuleProviderInfo>;
 // Persistent info stored between sessions
 struct StoredModuleProviderInfo
 {
-    using CacheKey = std::tuple<QString /*name*/, QVariantMap /*config*/, int /*lookup*/>;
+    using CacheKey = std::tuple<
+        QString /*name*/,
+        QVariantMap /*config*/,
+        QVariantMap /*qbsModule*/,
+        int /*lookup*/
+    >;
     using ModuleProvidersCache = QHash<CacheKey, ModuleProviderInfo>;
 
     ModuleProvidersCache providers;
diff --git a/src/lib/corelib/language/moduleproviderloader.cpp b/src/lib/corelib/language/moduleproviderloader.cpp
index 8ff08a1b7..3e62d9ed9 100644
--- a/src/lib/corelib/language/moduleproviderloader.cpp
+++ b/src/lib/corelib/language/moduleproviderloader.cpp
@@ -116,10 +116,11 @@ ModuleProviderLoader::ModuleProviderResult ModuleProviderLoader::executeModulePr
         return {};
     QStringList allSearchPaths;
     ModuleProviderResult result;
+    const auto qbsModule = evaluateQbsModule(product);
     for (const auto &[name, lookupType] : providers) {
         const QVariantMap config = getModuleProviderConfig(product).value(name.toString()).toMap();
-        ModuleProviderInfo &info =
-                m_storedModuleProviderInfo.providers[{name.toString(), config, int(lookupType)}];
+        ModuleProviderInfo &info = m_storedModuleProviderInfo.providers[
+            {name.toString(), config, qbsModule, int(lookupType)}];
         const bool fromCache = !info.name.isEmpty();
         if (!fromCache) {
             info.name = name;
@@ -128,7 +129,7 @@ ModuleProviderLoader::ModuleProviderResult ModuleProviderLoader::executeModulePr
             if (!info.providerFile.isEmpty()) {
                 qCDebug(lcModuleLoader) << "Running provider" << name << "at" << info.providerFile;
                 info.searchPaths = evaluateModuleProvider(
-                        product, dependsItemLocation, name, info.providerFile, config);
+                        product, dependsItemLocation, name, info.providerFile, config, qbsModule);
                 info.transientOutput = m_parameters.dryRun();
             }
         }
@@ -260,12 +261,47 @@ QString ModuleProviderLoader::findModuleProviderFile(
     return {};
 }
 
+QVariantMap ModuleProviderLoader::evaluateQbsModule(ProductContext &product) const
+{
+    const QString properties[] = {
+        QStringLiteral("sysroot"),
+    };
+    const auto qbsItemValue = std::static_pointer_cast<ItemValue>(
+        product.item->property(StringConstants::qbsModule()));
+    QVariantMap result;
+    for (const auto &property : properties) {
+        auto value = m_evaluator->value(qbsItemValue->item(), property).toVariant();
+        if (value.isValid())
+            result[property] = std::move(value);
+    }
+    return result;
+}
+
+Item *ModuleProviderLoader::createProviderScope(
+    ProductContext &product, const QVariantMap &qbsModule)
+{
+    const auto qbsItemValue = std::static_pointer_cast<ItemValue>(
+        product.item->property(StringConstants::qbsModule()));
+
+    Item *fakeQbsModule = Item::create(product.item->pool(), ItemType::Scope);
+
+    for (auto it = qbsModule.begin(), end = qbsModule.end(); it != end; ++it) {
+        fakeQbsModule->setProperty(it.key(), VariantValue::create(it.value()));
+    }
+
+    Item *scope = Item::create(product.item->pool(), ItemType::Scope);
+    scope->setFile(qbsItemValue->item()->file());
+    scope->setProperty(StringConstants::qbsModule(), ItemValue::create(fakeQbsModule));
+    return scope;
+}
+
 QStringList ModuleProviderLoader::evaluateModuleProvider(
         ProductContext &product,
         const CodeLocation &dependsItemLocation,
         const QualifiedId &name,
         const QString &providerFile,
-        const QVariantMap &moduleConfig)
+        const QVariantMap &moduleConfig,
+        const QVariantMap &qbsModule)
 {
     QTemporaryFile dummyItemFile;
     if (!dummyItemFile.open()) {
@@ -278,6 +314,11 @@ QStringList ModuleProviderLoader::evaluateModuleProvider(
     const QString projectBuildDir = product.project->item->variantProperty(
                 StringConstants::buildDirectoryProperty())->value().toString();
     const QString searchPathBaseDir = ModuleProviderInfo::outputDirPath(projectBuildDir, name);
+
+    // include qbs module into hash
+    auto jsConfig = moduleConfig;
+    jsConfig[StringConstants::qbsModule()] = qbsModule;
+
     QTextStream stream(&dummyItemFile);
     using Qt::endl;
     setupDefaultCodec(stream);
@@ -286,7 +327,7 @@ QStringList ModuleProviderLoader::evaluateModuleProvider(
     stream << "import '" << providerFile << "' as Provider" << endl;
     stream << "Provider {" << endl;
     stream << "    name: " << toJSLiteral(name.toString()) << endl;
-    stream << "    property var config: (" << toJSLiteral(moduleConfig) << ')' << endl;
+    stream << "    property var config: (" << toJSLiteral(jsConfig) << ')' << endl;
     stream << "    outputBaseDir: FileInfo.joinPaths(baseDirPrefix, "
               "        Utilities.getHash(JSON.stringify(config)))" << endl;
     stream << "    property string baseDirPrefix: " << toJSLiteral(searchPathBaseDir) << endl;
@@ -303,7 +344,9 @@ QStringList ModuleProviderLoader::evaluateModuleProvider(
             .arg(providerFile, providerItem->typeName(),
                  BuiltinDeclarations::instance().nameForType(ItemType::ModuleProvider)));
     }
-    providerItem->setParent(product.item);
+
+    providerItem->setScope(createProviderScope(product, qbsModule));
+
     providerItem->overrideProperties(moduleConfig, name.toString(), m_parameters, m_logger);
 
     m_probesResolver->resolveProbes(&product, providerItem);
diff --git a/src/lib/corelib/language/moduleproviderloader.h b/src/lib/corelib/language/moduleproviderloader.h
index ce1ec7f0a..91a32ec80 100644
--- a/src/lib/corelib/language/moduleproviderloader.h
+++ b/src/lib/corelib/language/moduleproviderloader.h
@@ -111,12 +111,15 @@ private:
     std::optional<std::vector<QualifiedId>> getModuleProviders(Item *item);
 
     QString findModuleProviderFile(const QualifiedId &name, ModuleProviderLookup lookupType);
+    QVariantMap evaluateQbsModule(ProductContext &product) const;
+    Item *createProviderScope(ProductContext &product, const QVariantMap &qbsModule);
     QStringList evaluateModuleProvider(
             ProductContext &product,
             const CodeLocation &location,
             const QualifiedId &name,
             const QString &providerFile,
-            const QVariantMap &moduleConfig);
+            const QVariantMap &moduleConfig,
+            const QVariantMap &qbsModule);
 
 private:
     ItemReader *const m_reader{nullptr};
diff --git a/src/lib/corelib/tools/persistence.cpp b/src/lib/corelib/tools/persistence.cpp
index 95211e894..1940b19de 100644
--- a/src/lib/corelib/tools/persistence.cpp
+++ b/src/lib/corelib/tools/persistence.cpp
@@ -48,7 +48,7 @@
 namespace qbs {
 namespace Internal {
 
-static const char QBS_PERSISTENCE_MAGIC[] = "QBSPERSISTENCE-130";
+static const char QBS_PERSISTENCE_MAGIC[] = "QBSPERSISTENCE-131";
 
 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/qbs-module-properties-in-providers/module-providers/provider_a.qbs b/tests/auto/blackbox/testdata/qbs-module-properties-in-providers/module-providers/provider_a.qbs
new file mode 100644
index 000000000..95c89cd1c
--- /dev/null
+++ b/tests/auto/blackbox/testdata/qbs-module-properties-in-providers/module-providers/provider_a.qbs
@@ -0,0 +1,9 @@
+import "../../qbs-module-providers-helpers.js" as Helpers
+
+ModuleProvider {
+    property string sysroot: qbs.sysroot
+    relativeSearchPaths: {
+        Helpers.writeModule(outputBaseDir, "qbsmetatestmodule", sysroot);
+        return "";
+    }
+}
diff --git a/tests/auto/blackbox/testdata/qbs-module-properties-in-providers/qbs-module-properties-in-providers.qbs b/tests/auto/blackbox/testdata/qbs-module-properties-in-providers/qbs-module-properties-in-providers.qbs
new file mode 100644
index 000000000..a338a220d
--- /dev/null
+++ b/tests/auto/blackbox/testdata/qbs-module-properties-in-providers/qbs-module-properties-in-providers.qbs
@@ -0,0 +1,34 @@
+Project {
+    qbsModuleProviders: "provider_a"
+    name: "project"
+
+    Profile {
+        name: "profile1"
+        qbs.sysroot: "sysroot1"
+    }
+
+    Profile {
+        name: "profile2"
+        qbs.sysroot: "sysroot2"
+    }
+
+    Product {
+        name: "product1"
+        Depends { name: "qbsmetatestmodule" }
+        property bool dummy: {
+            console.info("product1.qbsmetatestmodule.prop: " + qbsmetatestmodule.prop);
+        }
+        // multiplex over profiles, sysroot should not be cached
+        qbs.profiles: ["profile1", "profile2"]
+    }
+
+    Product {
+        name: "product2"
+        Depends { name: "qbsmetatestmodule" }
+        property bool dummy: {
+            console.info("product2.qbsmetatestmodule.prop: " + qbsmetatestmodule.prop);
+        }
+        // multiplex over profiles, sysroot should not be cached
+        qbs.profiles: ["profile1", "profile2"]
+    }
+}
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index fde8c61f4..679972d37 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -6111,6 +6111,27 @@ void TestBlackbox::qbsConfigAddProfile_data()
                                     << QString("Profile properties must be key/value pairs");
 }
 
+// checks that we can set qbs module properties in providers and provider cache works corectly
+void TestBlackbox::qbsModulePropertiesInProviders()
+{
+    QDir::setCurrent(testDataDir + "/qbs-module-properties-in-providers");
+
+    QbsRunParameters params("resolve");
+
+    QCOMPARE(runQbs(params), 0);
+
+    // We have 2 products in 2 configurations, but second product should use the cached value
+    // so we should have only 2 copies of the module, not 4.
+    QCOMPARE(m_qbsStdout.count("Running setup script for qbsmetatestmodule"), 2);
+
+    // Check that products get correct values from modules
+    QVERIFY2(m_qbsStdout.contains(("product1.qbsmetatestmodule.prop: sysroot1")), m_qbsStdout);
+    QVERIFY2(m_qbsStdout.contains(("product1.qbsmetatestmodule.prop: sysroot2")), m_qbsStdout);
+
+    QVERIFY2(m_qbsStdout.contains(("product2.qbsmetatestmodule.prop: sysroot1")), m_qbsStdout);
+    QVERIFY2(m_qbsStdout.contains(("product2.qbsmetatestmodule.prop: sysroot2")), m_qbsStdout);
+}
+
 // Tests whether it is possible to set qbsModuleProviders in Product and Project items
 // and that the order of providers results in correct priority
 void TestBlackbox::qbsModuleProviders()
diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h
index ea3a8597d..dc184989c 100644
--- a/tests/auto/blackbox/tst_blackbox.h
+++ b/tests/auto/blackbox/tst_blackbox.h
@@ -262,6 +262,7 @@ private slots:
     void qbsConfig();
     void qbsConfigAddProfile();
     void qbsConfigAddProfile_data();
+    void qbsModulePropertiesInProviders();
     void qbsModuleProviders();
     void qbsModuleProviders_data();
     void qbsModuleProvidersCliOverride();