Module providers: Fix some change tracking problems

- We must not remember our temporary files.
- We need to use a different reference time stamp for the created
modules.
- Collecting the results of File.exists() & friends can lead to false
positives, so we disable it for now.

Change-Id: Id64685b510606f1991e83eb825c36a1b3ec4a4e1
Reviewed-by: Joerg Bornemann <joerg.bornemann@qt.io>

12 files changed, 65 insertions, 32 deletions

diff --git a/src/lib/corelib/api/project.cpp b/src/lib/corelib/api/project.cpp
index d1cb351be..f8d50df9b 100644
--- a/src/lib/corelib/api/project.cpp
+++ b/src/lib/corelib/api/project.cpp
@@ -1256,7 +1256,7 @@ ErrorInfo Project::addGroup(const ProductData &product, const QString &groupName
         QBS_CHECK(isValid());
         d->prepareChangeToProject();
         d->addGroup(product, groupName);
-        d->internalProject->lastResolveTime = FileTime::currentTime();
+        d->internalProject->lastStartResolveTime = FileTime::currentTime();
         d->internalProject->store(d->logger);
         return ErrorInfo();
     } catch (ErrorInfo errorInfo) {
@@ -1283,7 +1283,7 @@ ErrorInfo Project::addFiles(const ProductData &product, const GroupData &group,
         QBS_CHECK(isValid());
         d->prepareChangeToProject();
         d->addFiles(product, group, filePaths);
-        d->internalProject->lastResolveTime = FileTime::currentTime();
+        d->internalProject->lastStartResolveTime = FileTime::currentTime();
         d->internalProject->store(d->logger);
         return ErrorInfo();
     } catch (ErrorInfo errorInfo) {
@@ -1309,7 +1309,7 @@ ErrorInfo Project::removeFiles(const ProductData &product, const GroupData &grou
         QBS_CHECK(isValid());
         d->prepareChangeToProject();
         d->removeFiles(product, group, filePaths);
-        d->internalProject->lastResolveTime = FileTime::currentTime();
+        d->internalProject->lastStartResolveTime = FileTime::currentTime();
         d->internalProject->store(d->logger);
         return ErrorInfo();
     } catch (ErrorInfo errorInfo) {
@@ -1330,7 +1330,7 @@ ErrorInfo Project::removeGroup(const ProductData &product, const GroupData &grou
         QBS_CHECK(isValid());
         d->prepareChangeToProject();
         d->removeGroup(product, group);
-        d->internalProject->lastResolveTime = FileTime::currentTime();
+        d->internalProject->lastStartResolveTime = FileTime::currentTime();
         d->internalProject->store(d->logger);
         return ErrorInfo();
     } catch (ErrorInfo errorInfo) {
diff --git a/src/lib/corelib/buildgraph/buildgraphloader.cpp b/src/lib/corelib/buildgraph/buildgraphloader.cpp
index e6d1cb75a..51f7403b0 100644
--- a/src/lib/corelib/buildgraph/buildgraphloader.cpp
+++ b/src/lib/corelib/buildgraph/buildgraphloader.cpp
@@ -307,7 +307,7 @@ void BuildGraphLoader::trackProjectChanges()
     bool reResolvingNecessary = false;
     if (!checkConfigCompatibility())
         reResolvingNecessary = true;
-    if (hasProductFileChanged(allRestoredProducts, restoredProject->lastResolveTime,
+    if (hasProductFileChanged(allRestoredProducts, restoredProject->lastStartResolveTime,
                               buildSystemFiles, changedProducts)) {
         reResolvingNecessary = true;
     }
@@ -317,7 +317,7 @@ void BuildGraphLoader::trackProjectChanges()
     // having been touched. In such a case, the build data for that product will have to be set up
     // anew.
     if (probeExecutionForced(restoredProject, allRestoredProducts)
-            || hasBuildSystemFileChanged(buildSystemFiles, restoredProject->lastResolveTime)
+            || hasBuildSystemFileChanged(buildSystemFiles, restoredProject.get())
             || hasEnvironmentChanged(restoredProject)
             || hasCanonicalFilePathResultChanged(restoredProject)
             || hasFileExistsResultChanged(restoredProject)
@@ -342,7 +342,7 @@ void BuildGraphLoader::trackProjectChanges()
     ldr.setOldProjectProbes(restoredProject->probes);
     if (!m_parameters.forceProbeExecution())
         ldr.setStoredModuleProviderInfo(restoredProject->moduleProviderInfo);
-    ldr.setLastResolveTime(restoredProject->lastResolveTime);
+    ldr.setLastResolveTime(restoredProject->lastStartResolveTime);
     QHash<QString, std::vector<ProbeConstPtr>> restoredProbes;
     for (const auto &restoredProduct : qAsConst(allRestoredProducts))
         restoredProbes.insert(restoredProduct->uniqueName(), restoredProduct->probes);
@@ -630,12 +630,24 @@ bool BuildGraphLoader::hasProductFileChanged(const std::vector<ResolvedProductPt
 }
 
 bool BuildGraphLoader::hasBuildSystemFileChanged(const Set<QString> &buildSystemFiles,
-                                                 const FileTime &referenceTime)
+                                                 const TopLevelProject *restoredProject)
 {
     for (const QString &file : buildSystemFiles) {
         const FileInfo fi(file);
-        if (!fi.exists() || referenceTime < fi.lastModified()) {
-            qCDebug(lcBuildGraph) << "A qbs or js file changed, must re-resolve project.";
+        if (!fi.exists()) {
+            qCDebug(lcBuildGraph) << "Project file" << file
+                                  << "no longer exists, must re-resolve project.";
+            return true;
+        }
+        const auto generatedChecker = [&file, restoredProject](const ModuleProviderInfo &mpi) {
+            return file.startsWith(mpi.outputDirPath(restoredProject->buildDirectory));
+        };
+        const bool fileWasCreatedByModuleProvider = any_of(restoredProject->moduleProviderInfo,
+                                                           generatedChecker);
+        const FileTime referenceTime = fileWasCreatedByModuleProvider
+                ? restoredProject->lastEndResolveTime : restoredProject->lastStartResolveTime;
+        if (referenceTime < fi.lastModified()) {
+            qCDebug(lcBuildGraph) << "Project file" << file << "changed, must re-resolve project.";
             return true;
         }
     }
diff --git a/src/lib/corelib/buildgraph/buildgraphloader.h b/src/lib/corelib/buildgraph/buildgraphloader.h
index 30d7b9b45..761c80603 100644
--- a/src/lib/corelib/buildgraph/buildgraphloader.h
+++ b/src/lib/corelib/buildgraph/buildgraphloader.h
@@ -97,7 +97,7 @@ private:
                                Set<QString> &remainingBuildSystemFiles,
                                std::vector<ResolvedProductPtr> &productsWithChangedFiles);
     bool hasBuildSystemFileChanged(const Set<QString> &buildSystemFiles,
-                                   const FileTime &referenceTime);
+                                   const TopLevelProject *restoredProject);
     void markTransformersForChangeTracking(const std::vector<ResolvedProductPtr> &restoredProducts);
     void checkAllProductsForChanges(const std::vector<ResolvedProductPtr> &restoredProducts,
             std::vector<ResolvedProductPtr> &changedProducts);
diff --git a/src/lib/corelib/language/language.cpp b/src/lib/corelib/language/language.cpp
index 2a9eb5b0e..86056f2a6 100644
--- a/src/lib/corelib/language/language.cpp
+++ b/src/lib/corelib/language/language.cpp
@@ -588,7 +588,7 @@ void ResolvedProject::store(PersistentPool &pool)
 
 
 TopLevelProject::TopLevelProject()
-    : bgLocker(nullptr), locked(false), lastResolveTime(FileTime::oldestTime())
+    : bgLocker(nullptr), locked(false), lastStartResolveTime(FileTime::oldestTime())
 {
 }
 
diff --git a/src/lib/corelib/language/language.h b/src/lib/corelib/language/language.h
index 994ad6c55..82d9f94a2 100644
--- a/src/lib/corelib/language/language.h
+++ b/src/lib/corelib/language/language.h
@@ -703,7 +703,8 @@ public:
     bool locked; // This is the API-level lock for the project instance.
 
     Set<QString> buildSystemFiles;
-    FileTime lastResolveTime;
+    FileTime lastStartResolveTime;
+    FileTime lastEndResolveTime;
     QList<ErrorInfo> warningsEncountered;
 
     void setBuildConfiguration(const QVariantMap &config);
@@ -724,8 +725,8 @@ private:
         pool.serializationOp<opType>(m_id, canonicalFilePathResults, fileExistsResults,
                                      directoryEntriesResults, fileLastModifiedResults, environment,
                                      probes, profileConfigs, overriddenValues, buildSystemFiles,
-                                     lastResolveTime, warningsEncountered, buildData,
-                                     moduleProviderInfo);
+                                     lastStartResolveTime, lastEndResolveTime, warningsEncountered,
+                                     buildData, moduleProviderInfo);
     }
     void load(PersistentPool &pool) override;
     void store(PersistentPool &pool) override;
diff --git a/src/lib/corelib/language/loader.cpp b/src/lib/corelib/language/loader.cpp
index 4d2eef983..e27ccca74 100644
--- a/src/lib/corelib/language/loader.cpp
+++ b/src/lib/corelib/language/loader.cpp
@@ -170,7 +170,8 @@ TopLevelProjectPtr Loader::loadProject(const SetupProjectParameters &_parameters
     ProjectResolver resolver(&evaluator, loadResult, parameters, m_logger);
     resolver.setProgressObserver(m_progressObserver);
     const TopLevelProjectPtr project = resolver.resolve();
-    project->lastResolveTime = resolveTime;
+    project->lastStartResolveTime = resolveTime;
+    project->lastEndResolveTime = FileTime::currentTime();
 
     // E.g. if the top-level project is disabled.
     if (m_progressObserver)
diff --git a/src/lib/corelib/language/moduleloader.cpp b/src/lib/corelib/language/moduleloader.cpp
index 58dab062a..fe71df43f 100644
--- a/src/lib/corelib/language/moduleloader.cpp
+++ b/src/lib/corelib/language/moduleloader.cpp
@@ -361,7 +361,7 @@ ModuleLoaderResult ModuleLoader::load(const SetupProjectParameters &parameters)
     handleTopLevelProject(&result, root, buildDirectory,
                   Set<QString>() << QDir::cleanPath(parameters.projectFilePath()));
     result.root = root;
-    result.qbsFiles = m_reader->filesRead();
+    result.qbsFiles = m_reader->filesRead() - m_tempQbsFiles;
     for (auto it = m_localProfiles.cbegin(); it != m_localProfiles.cend(); ++it)
         result.profileConfigs.remove(it.key());
     printProfilingInfo();
@@ -2156,7 +2156,10 @@ void ModuleLoader::setSearchPathsForProduct(ModuleLoader::ProductContext *produc
     if (!m_moduleProviderInfo.empty()) {
         const QVariantMap configForProduct = moduleProviderConfig(*product);
         for (const ModuleProviderInfo &c : m_moduleProviderInfo) {
-            if (configForProduct.value(c.name.toString()) == c.config) {
+            if (configForProduct.value(c.name.toString()).toMap() == c.config) {
+                qCDebug(lcModuleLoader) << "re-using search paths" << c.searchPaths
+                                        << "from module provider" << c.name
+                                        << "for product" << product->name;
                 product->knownModuleProviders.insert(c.name);
                 product->searchPaths << c.searchPaths;
             }
@@ -2622,10 +2625,6 @@ void ModuleLoader::resolveDependsItem(DependsContext *dependsContext, Item *pare
             }
             ErrorInfo e(Tr::tr("Dependency '%1' not found for product '%2'.")
                         .arg(moduleName.toString(), productName), dependsItem->location());
-            if (moduleName.size() == 2 && moduleName.front() == QStringLiteral("Qt")) {
-                e.append(Tr::tr("Please create a Qt profile using the qbs-setup-qt tool "
-                                "if you haven't already done so."));
-            }
             throw e;
         }
         if (result.isProduct && !m_dependsChain.empty() && !m_dependsChain.back().isProduct) {
@@ -3761,6 +3760,7 @@ ModuleLoader::ModuleProviderResult ModuleLoader::findModuleProvider(const Qualif
                                    "for dependency '%1': %2").arg(name.toString(),
                                                                   dummyItemFile.errorString()));
         }
+        m_tempQbsFiles << dummyItemFile.fileName();
         qCDebug(lcModuleLoader) << "Instantiating module provider at" << providerFile;
         const QString projectBuildDir = product.project->item->variantProperty(
                     StringConstants::buildDirectoryProperty())->value().toString();
@@ -3789,6 +3789,7 @@ ModuleLoader::ModuleProviderResult ModuleLoader::findModuleProvider(const Qualif
                 .arg(providerFile, providerItem->typeName(),
                      BuiltinDeclarations::instance().nameForType(ItemType::ModuleProvider)));
         }
+        providerItem->setParent(product.item);
         const QVariantMap configMap = moduleConfig.toMap();
         for (auto it = configMap.begin(); it != configMap.end(); ++it) {
             const PropertyDeclaration decl = providerItem->propertyDeclaration(it.key());
@@ -3798,7 +3799,7 @@ ModuleLoader::ModuleProviderResult ModuleLoader::findModuleProvider(const Qualif
             }
             providerItem->setProperty(it.key(), VariantValue::create(it.value()));
         }
-        EvalContextSwitcher contextSwitcher(m_evaluator->engine(), EvalContext::ProbeExecution);
+        EvalContextSwitcher contextSwitcher(m_evaluator->engine(), EvalContext::ModuleProvider);
         const QStringList searchPaths
                 = m_evaluator->stringListValue(providerItem, QStringLiteral("searchPaths"));
         if (searchPaths.empty()) {
diff --git a/src/lib/corelib/language/moduleloader.h b/src/lib/corelib/language/moduleloader.h
index 9e45908c3..a0a555748 100644
--- a/src/lib/corelib/language/moduleloader.h
+++ b/src/lib/corelib/language/moduleloader.h
@@ -450,6 +450,7 @@ private:
     Set<Item *> m_exportsWithDeferredDependsItems;
 
     ModuleProviderInfoList m_moduleProviderInfo;
+    Set<QString> m_tempQbsFiles;
 
     SetupProjectParameters m_parameters;
     std::unique_ptr<Settings> m_settings;
diff --git a/src/lib/corelib/language/scriptengine.cpp b/src/lib/corelib/language/scriptengine.cpp
index fb4706b3b..7b2c69aa1 100644
--- a/src/lib/corelib/language/scriptengine.cpp
+++ b/src/lib/corelib/language/scriptengine.cpp
@@ -234,6 +234,7 @@ void ScriptEngine::checkContext(const QString &operation,
                         + Tr::tr("Should this call be in a JavaScriptCommand instead?");
             }
             break;
+        case EvalContext::ModuleProvider:
         case EvalContext::ProbeExecution:
         case EvalContext::JsCommand:
             QBS_ASSERT(false, continue);
@@ -574,25 +575,30 @@ void ScriptEngine::releaseResourcesOfScriptObjects()
 void ScriptEngine::addCanonicalFilePathResult(const QString &filePath,
                                               const QString &resultFilePath)
 {
-    m_canonicalFilePathResult.insert(filePath, resultFilePath);
+    if (gatherFileResults())
+        m_canonicalFilePathResult.insert(filePath, resultFilePath);
 }
 
 void ScriptEngine::addFileExistsResult(const QString &filePath, bool exists)
 {
-    m_fileExistsResult.insert(filePath, exists);
+    if (gatherFileResults())
+        m_fileExistsResult.insert(filePath, exists);
 }
 
 void ScriptEngine::addDirectoryEntriesResult(const QString &path, QDir::Filters filters,
                                              const QStringList &entries)
 {
-    m_directoryEntriesResult.insert(
-                std::pair<QString, quint32>(path, static_cast<quint32>(filters)),
-                entries);
+    if (gatherFileResults()) {
+        m_directoryEntriesResult.insert(
+                    std::pair<QString, quint32>(path, static_cast<quint32>(filters)),
+                    entries);
+    }
 }
 
 void ScriptEngine::addFileLastModifiedResult(const QString &filePath, const FileTime &fileTime)
 {
-    m_fileLastModifiedResult.insert(filePath, fileTime);
+    if (gatherFileResults())
+        m_fileLastModifiedResult.insert(filePath, fileTime);
 }
 
 Set<QString> ScriptEngine::imports() const
@@ -655,6 +661,12 @@ void ScriptEngine::abort()
     abortEvaluation(m_cancelationError);
 }
 
+bool ScriptEngine::gatherFileResults() const
+{
+    return evalContext() == EvalContext::PropertyEvaluation
+            || evalContext() == EvalContext::ProbeExecution;
+}
+
 class JSTypeExtender
 {
 public:
diff --git a/src/lib/corelib/language/scriptengine.h b/src/lib/corelib/language/scriptengine.h
index 226cf16a9..89863189a 100644
--- a/src/lib/corelib/language/scriptengine.h
+++ b/src/lib/corelib/language/scriptengine.h
@@ -72,7 +72,9 @@ class PrepareScriptObserver;
 class ScriptImporter;
 class ScriptPropertyObserver;
 
-enum class EvalContext { PropertyEvaluation, ProbeExecution, RuleExecution, JsCommand };
+enum class EvalContext {
+    PropertyEvaluation, ProbeExecution, ModuleProvider, RuleExecution, JsCommand
+};
 class DubiousContext
 {
 public:
@@ -279,6 +281,8 @@ private:
 
     void abort();
 
+    bool gatherFileResults() const;
+
     void installQbsBuiltins();
     void extendJavaScriptBuiltins();
     void installFunction(const QString &name, int length, QScriptValue *functionValue,
diff --git a/src/lib/corelib/tools/persistence.cpp b/src/lib/corelib/tools/persistence.cpp
index b50084702..5591c9761 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-125";
+static const char QBS_PERSISTENCE_MAGIC[] = "QBSPERSISTENCE-126";
 
 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/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index 82f29f320..8154e7262 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -6396,7 +6396,8 @@ void TestBlackbox::fallbackModuleProvider()
     static const auto b2s = [](bool b) { return QString(b ? "true" : "false"); };
     QbsRunParameters resolveParams("resolve",
         QStringList{"modules.pkgconfig.libDirs:" + pkgConfigLibDirs.join(','),
-                    "products.p.fallbacksEnabled:" + b2s(fallbacksEnabledInProduct)});
+                    "products.p.fallbacksEnabled:" + b2s(fallbacksEnabledInProduct),
+                    "--force-probe-execution"});
     if (!fallbacksEnabledGlobally)
         resolveParams.arguments << "--no-fallback-module-provider";
     QCOMPARE(runQbs(resolveParams), 0);