Fix module provider search paths

Originally, provider added paths to the reader and they were
reseted to project state deeper in the recursion.

We lookup modules in the clean state so we do not pollute
the search paths added by providers, otherwise existingModulePathCache
is polluted with search paths from previous invocations of the fallback
module provider.

This amends fb52fed84a1510a7de0172e643d6fd66a780e2e8.

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

10 files changed, 77 insertions, 51 deletions

diff --git a/src/lib/corelib/language/itemreader.cpp b/src/lib/corelib/language/itemreader.cpp
index 8fcda8f24..e71eba43d 100644
--- a/src/lib/corelib/language/itemreader.cpp
+++ b/src/lib/corelib/language/itemreader.cpp
@@ -220,5 +220,19 @@ Item *ItemReader::setupItemFromFile(
     return item;
 }
 
+SearchPathsManager::SearchPathsManager(ItemReader &itemReader, const QStringList &extraSearchPaths)
+    : m_itemReader(itemReader),
+      m_oldSize(itemReader.extraSearchPathsStack().size())
+{
+    if (!extraSearchPaths.isEmpty())
+        m_itemReader.pushExtraSearchPaths(extraSearchPaths);
+}
+
+SearchPathsManager::~SearchPathsManager()
+{
+    while (m_itemReader.extraSearchPathsStack().size() > m_oldSize)
+        m_itemReader.popExtraSearchPaths();
+}
+
 } // namespace Internal
 } // namespace qbs
diff --git a/src/lib/corelib/language/itemreader.h b/src/lib/corelib/language/itemreader.h
index 0ca845a00..751c9b1b6 100644
--- a/src/lib/corelib/language/itemreader.h
+++ b/src/lib/corelib/language/itemreader.h
@@ -105,6 +105,16 @@ private:
     qint64 m_elapsedTime = -1;
 };
 
+class SearchPathsManager {
+public:
+    SearchPathsManager(ItemReader &itemReader, const QStringList &extraSearchPaths = {});
+    ~SearchPathsManager();
+
+private:
+    ItemReader &m_itemReader;
+    size_t m_oldSize{0};
+};
+
 } // namespace Internal
 } // namespace qbs
 
diff --git a/src/lib/corelib/language/moduleloader.cpp b/src/lib/corelib/language/moduleloader.cpp
index 27ce2cd77..1772ee808 100644
--- a/src/lib/corelib/language/moduleloader.cpp
+++ b/src/lib/corelib/language/moduleloader.cpp
@@ -189,6 +189,8 @@ ModuleLoader::Result ModuleLoader::searchAndLoadModuleFile(
         return result;
     };
 
+    SearchPathsManager searchPathsManager(d->itemReader);
+
     Result loadResult;
     auto existingPaths = findExistingModulePaths();
     if (existingPaths.isEmpty()) { // no suitable names found, try to use providers
@@ -205,9 +207,10 @@ ModuleLoader::Result ModuleLoader::searchAndLoadModuleFile(
         loadResult.providerProbes << result.probes;
         if (!providerConfig)
             providerConfig = result.providerConfig;
-        if (result.providerAddedSearchPaths) {
+        if (result.searchPaths) {
             qCDebug(lcModuleLoader) << "Re-checking for module" << moduleName.toString()
                                     << "with newly added search paths from module provider";
+            d->itemReader.pushExtraSearchPaths(*result.searchPaths);
             existingPaths = findExistingModulePaths();
         }
     }
diff --git a/src/lib/corelib/language/moduleproviderloader.cpp b/src/lib/corelib/language/moduleproviderloader.cpp
index 5b7cfa7a2..22605bf9f 100644
--- a/src/lib/corelib/language/moduleproviderloader.cpp
+++ b/src/lib/corelib/language/moduleproviderloader.cpp
@@ -147,7 +147,7 @@ ModuleProviderLoader::ModuleProviderResult ModuleProviderLoader::executeModulePr
             continue;
         }
         if (fromCache)
-            qCDebug(lcModuleLoader) << "Re-using provider" << name << "from cache";
+            qCDebug(lcModuleLoader) << "Re-using provider" << name.toString() << "from cache";
 
         result.providerFound = true;
         if (info.searchPaths.empty()) {
@@ -163,8 +163,7 @@ ModuleProviderLoader::ModuleProviderResult ModuleProviderLoader::executeModulePr
     if (allSearchPaths.isEmpty())
         return result;
 
-    m_reader.pushExtraSearchPaths(allSearchPaths);
-    result.providerAddedSearchPaths = true;
+    result.searchPaths = std::move(allSearchPaths);
 
     return result;
 }
diff --git a/src/lib/corelib/language/moduleproviderloader.h b/src/lib/corelib/language/moduleproviderloader.h
index 8fb0d6f45..b6240cb03 100644
--- a/src/lib/corelib/language/moduleproviderloader.h
+++ b/src/lib/corelib/language/moduleproviderloader.h
@@ -78,14 +78,10 @@ public:
 
     struct ModuleProviderResult
     {
-        ModuleProviderResult() = default;
-        ModuleProviderResult(bool ran, bool added)
-            : providerFound(ran), providerAddedSearchPaths(added) {}
-
         std::vector<ProbeConstPtr> probes;
         QVariantMap providerConfig;
         bool providerFound = false;
-        bool providerAddedSearchPaths = false;
+        std::optional<QStringList> searchPaths;
     };
 
     const StoredModuleProviderInfo &storedModuleProviderInfo() const
diff --git a/src/lib/corelib/language/projecttreebuilder.cpp b/src/lib/corelib/language/projecttreebuilder.cpp
index bc1e4ab9a..4667e8a1b 100644
--- a/src/lib/corelib/language/projecttreebuilder.cpp
+++ b/src/lib/corelib/language/projecttreebuilder.cpp
@@ -202,21 +202,6 @@ using ShadowProductInfo = std::pair<bool, QString>;
 enum class Deferral { Allowed, NotAllowed };
 enum class HandleDependency { Use, Ignore, Defer };
 
-class SearchPathsManager {
-public:
-    SearchPathsManager(ItemReader &itemReader, const QStringList &extraSearchPaths);
-    SearchPathsManager(ItemReader &itemReader, ProductContext &product);
-    ~SearchPathsManager();
-
-private:
-    SearchPathsManager(ItemReader &itemReader, ProductContext *product,
-                       const QStringList &extraSearchPaths);
-
-    ItemReader &m_itemReader;
-    ProductContext * const m_product = nullptr;
-    size_t m_oldSize{0};
-};
-
 class TimingData {
 public:
     qint64 prepareProducts = 0;
@@ -946,7 +931,7 @@ bool ProjectTreeBuilder::Private::resolveDependencies(ProductContext &product, D
                     ProductContext::ResolvedAndMultiplexedDependsItem::makeBaseDependency());
     }
 
-    SearchPathsManager searchPathsMgr(reader, product);
+    SearchPathsManager searchPathsMgr(reader, product.searchPaths);
 
     while (!product.resolveDependenciesState.empty()) {
 fixme:
@@ -2170,32 +2155,6 @@ ProjectTreeBuilder::Private::multiplexDependency(
     return dependencies;
 }
 
-SearchPathsManager::SearchPathsManager(ItemReader &itemReader, const QStringList &extraSearchPaths)
-    : SearchPathsManager(itemReader, nullptr, extraSearchPaths) {}
-
-SearchPathsManager::SearchPathsManager(ItemReader &itemReader, ProductContext &product)
-    : SearchPathsManager(itemReader, &product, product.searchPaths) {}
-
-SearchPathsManager::SearchPathsManager(ItemReader &itemReader, ProductContext *product,
-                                       const QStringList &extraSearchPaths)
-    : m_itemReader(itemReader), m_product(product),
-      m_oldSize(itemReader.extraSearchPathsStack().size())
-{
-    if (!extraSearchPaths.isEmpty())
-        m_itemReader.pushExtraSearchPaths(extraSearchPaths);
-}
-
-SearchPathsManager::~SearchPathsManager()
-{
-    while (m_itemReader.extraSearchPathsStack().size() > m_oldSize) {
-        if (m_product && m_itemReader.extraSearchPathsStack().size() > m_oldSize + 1) {
-            for (const QString &pathFromProvider : m_itemReader.extraSearchPathsStack().back())
-                m_product->searchPaths.prepend(pathFromProvider);
-        }
-        m_itemReader.popExtraSearchPaths();
-    }
-}
-
 QString ProductContext::uniqueName() const
 {
     return ResolvedProduct::uniqueName(name, multiplexConfigurationId);
diff --git a/tests/auto/blackbox/testdata/module-providers-cache/module-providers-cache.qbs b/tests/auto/blackbox/testdata/module-providers-cache/module-providers-cache.qbs
new file mode 100644
index 000000000..508ed84d2
--- /dev/null
+++ b/tests/auto/blackbox/testdata/module-providers-cache/module-providers-cache.qbs
@@ -0,0 +1,11 @@
+Project {
+    qbsModuleProviders: ["provider_a"]
+    name: "project"
+    property string dummyProp
+
+    Product {
+        name: "p1"
+        Depends { name: "qbsothermodule" }
+        Depends { name: "qbsmetatestmodule" }
+    }
+}
diff --git a/tests/auto/blackbox/testdata/module-providers-cache/module-providers/provider_a.qbs b/tests/auto/blackbox/testdata/module-providers-cache/module-providers/provider_a.qbs
new file mode 100644
index 000000000..782cf7d25
--- /dev/null
+++ b/tests/auto/blackbox/testdata/module-providers-cache/module-providers/provider_a.qbs
@@ -0,0 +1,9 @@
+import "../../qbs-module-providers-helpers.js" as Helpers
+
+ModuleProvider {
+    relativeSearchPaths: {
+        Helpers.writeModule(outputBaseDir, "qbsmetatestmodule", "from_provider_a");
+        Helpers.writeModule(outputBaseDir, "qbsothermodule", "from_provider_a");
+        return "";
+    }
+}
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index d07b9c1bf..98184ef2d 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -8105,6 +8105,30 @@ void TestBlackbox::moduleProviders()
     QVERIFY2(m_qbsStdout.contains("The letters are Z and Y"), m_qbsStdout.constData());
 }
 
+// Checks regression - when loading 2 modules from the same provider, the second module should
+// come from provider cache
+void TestBlackbox::moduleProvidersCache()
+{
+    QDir::setCurrent(testDataDir + "/module-providers-cache");
+
+    QbsRunParameters params("resolve", {"-v"});
+    QCOMPARE(runQbs(params), 0);
+    const auto qbsmetatestmoduleMessage = "Re-checking for module \"qbsmetatestmodule\" with "
+                                          "newly added search paths from module provider";
+    const auto qbsothermoduleMessage = "Re-checking for module \"qbsothermodule\" with "
+                                       "newly added search paths from module provider";
+    QCOMPARE(m_qbsStderr.count(qbsmetatestmoduleMessage), 1);
+    QCOMPARE(m_qbsStderr.count(qbsothermoduleMessage), 1);
+    QCOMPARE(m_qbsStderr.count("Re-using provider \"provider_a\" from cache"), 1);
+
+    // We didn't change providers, so both modules should come from cache.
+    params.arguments << "project.dummyProp:value";
+    QCOMPARE(runQbs(params), 0);
+    QCOMPARE(m_qbsStderr.count(qbsmetatestmoduleMessage), 1);
+    QCOMPARE(m_qbsStderr.count(qbsothermoduleMessage), 1);
+    QCOMPARE(m_qbsStderr.count("Re-using provider \"provider_a\" from cache"), 2);
+}
+
 void TestBlackbox::fallbackModuleProvider_data()
 {
     QTest::addColumn<bool>("fallbacksEnabledGlobally");
diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h
index f4e001360..0bfdfc271 100644
--- a/tests/auto/blackbox/tst_blackbox.h
+++ b/tests/auto/blackbox/tst_blackbox.h
@@ -198,6 +198,7 @@ private slots:
     void maximumCLanguageVersion();
     void maximumCxxLanguageVersion();
     void moduleProviders();
+    void moduleProvidersCache();
     void fallbackModuleProvider_data();
     void fallbackModuleProvider();
     void minimumSystemVersion();