Fix loading optional transitive dependencies

If optional module B depends on an invalid module A (i.e. whos validate
script throws an exception), module B should be not present as well.
Fix that by remembering the initial value of the "required" property in
the Depends item.

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

9 files changed, 51 insertions, 3 deletions

diff --git a/src/lib/corelib/language/item.h b/src/lib/corelib/language/item.h
index c5d8ef980..2d676a7f7 100644
--- a/src/lib/corelib/language/item.h
+++ b/src/lib/corelib/language/item.h
@@ -78,6 +78,7 @@ public:
         QualifiedId name;
         Item *item;
         bool isProduct;
+        bool requiredValue = true; // base value of the required prop
         bool required;
         QVariantMap parameters;
         VersionRange versionRange;
diff --git a/src/lib/corelib/language/moduleloader.cpp b/src/lib/corelib/language/moduleloader.cpp
index 935f18f9e..09bfb00e8 100644
--- a/src/lib/corelib/language/moduleloader.cpp
+++ b/src/lib/corelib/language/moduleloader.cpp
@@ -1489,6 +1489,13 @@ void ModuleLoader::handleProduct(ModuleLoader::ProductContext *productContext)
             continue;
         try {
             m_evaluator->boolValue(module.item, StringConstants::validateProperty());
+            for (const auto &dep : module.item->modules()) {
+                if (dep.requiredValue && !dep.item->isPresentModule()) {
+                    throw ErrorInfo(Tr::tr("Module '%1' depends on module '%2', which was not "
+                                           "loaded successfully")
+                                    .arg(module.name.toString(), dep.name.toString()));
+                }
+            }
         } catch (const ErrorInfo &error) {
             handleModuleSetupError(productContext, module, error);
             if (productContext->info.delayedError.hasError())
@@ -2665,8 +2672,10 @@ void ModuleLoader::resolveDependsItem(DependsContext *dependsContext, Item *pare
         return;
     }
 
+    const bool isRequiredValue =
+            m_evaluator->boolValue(dependsItem, StringConstants::requiredProperty());
     const bool isRequired = !productTypesIsSet
-            && m_evaluator->boolValue(dependsItem, StringConstants::requiredProperty())
+            && isRequiredValue
             && !contains(m_requiredChain, false);
     const Version minVersion = Version::fromString(
                 m_evaluator->stringValue(dependsItem,
@@ -2686,8 +2695,8 @@ void ModuleLoader::resolveDependsItem(DependsContext *dependsContext, Item *pare
         const auto it = std::find_if(moduleResults->begin(), moduleResults->end(),
                 [moduleName](const Item::Module &m) { return m.name == moduleName; });
         if (it != moduleResults->end()) {
-            if (isRequired)
-                it->required = true;
+            it->required = it->required || isRequired;
+            it->requiredValue = it->requiredValue || isRequiredValue;
             it->versionRange.narrowDown(versionRange);
             continue;
         }
@@ -2720,6 +2729,7 @@ void ModuleLoader::resolveDependsItem(DependsContext *dependsContext, Item *pare
         qCDebug(lcModuleLoader) << "module loaded:" << moduleName.toString();
         result.name = moduleName;
         result.item = moduleItem;
+        result.requiredValue = isRequiredValue;
         result.required = isRequired;
         result.parameters = defaultParameters;
         result.versionRange = versionRange;
diff --git a/tests/auto/blackbox/testdata/transitive-invalid-dependencies/modules/a/a.qbs b/tests/auto/blackbox/testdata/transitive-invalid-dependencies/modules/a/a.qbs
new file mode 100644
index 000000000..fd52488fb
--- /dev/null
+++ b/tests/auto/blackbox/testdata/transitive-invalid-dependencies/modules/a/a.qbs
@@ -0,0 +1,5 @@
+Module {
+    validate: {
+        throw "Module cannot be loaded";
+    }
+}
diff --git a/tests/auto/blackbox/testdata/transitive-invalid-dependencies/modules/b/b.qbs b/tests/auto/blackbox/testdata/transitive-invalid-dependencies/modules/b/b.qbs
new file mode 100644
index 000000000..605a2aaee
--- /dev/null
+++ b/tests/auto/blackbox/testdata/transitive-invalid-dependencies/modules/b/b.qbs
@@ -0,0 +1,3 @@
+Module {
+    Depends { name: "a" }
+}
diff --git a/tests/auto/blackbox/testdata/transitive-invalid-dependencies/modules/c/c.qbs b/tests/auto/blackbox/testdata/transitive-invalid-dependencies/modules/c/c.qbs
new file mode 100644
index 000000000..ac7dbbec6
--- /dev/null
+++ b/tests/auto/blackbox/testdata/transitive-invalid-dependencies/modules/c/c.qbs
@@ -0,0 +1,3 @@
+Module {
+    Depends { name: "a"; required: false }
+}
diff --git a/tests/auto/blackbox/testdata/transitive-invalid-dependencies/modules/d/d.qbs b/tests/auto/blackbox/testdata/transitive-invalid-dependencies/modules/d/d.qbs
new file mode 100644
index 000000000..0bdd8c3b7
--- /dev/null
+++ b/tests/auto/blackbox/testdata/transitive-invalid-dependencies/modules/d/d.qbs
@@ -0,0 +1,4 @@
+Module {
+    Depends { name: "b"; }
+    Depends { name: "c"; }
+}
diff --git a/tests/auto/blackbox/testdata/transitive-invalid-dependencies/transitive-invalid-dependencies.qbs b/tests/auto/blackbox/testdata/transitive-invalid-dependencies/transitive-invalid-dependencies.qbs
new file mode 100644
index 000000000..209b1e47d
--- /dev/null
+++ b/tests/auto/blackbox/testdata/transitive-invalid-dependencies/transitive-invalid-dependencies.qbs
@@ -0,0 +1,11 @@
+Product {
+    property bool modulePresent: {
+        console.info("b.present = " + b.present);
+        console.info("c.present = " + c.present);
+        console.info("d.present = " + d.present);
+    }
+
+    Depends { name: "b"; required: false }
+    Depends { name: "c"; required: false }
+    Depends { name: "d"; required: false }
+}
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index 786a43725..1a593c73c 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -8106,6 +8106,16 @@ void TestBlackbox::qbsVersion()
     QVERIFY(runQbs(params) != 0);
 }
 
+void TestBlackbox::transitiveInvalidDependencies()
+{
+    QDir::setCurrent(testDataDir + "/transitive-invalid-dependencies");
+    QbsRunParameters params;
+    QCOMPARE(runQbs(params), 0);
+    QVERIFY2(m_qbsStdout.contains("b.present = false"), m_qbsStdout);
+    QVERIFY2(m_qbsStdout.contains("c.present = true"), m_qbsStdout);
+    QVERIFY2(m_qbsStdout.contains("d.present = false"), m_qbsStdout);
+}
+
 void TestBlackbox::transitiveOptionalDependencies()
 {
     QDir::setCurrent(testDataDir + "/transitive-optional-dependencies");
diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h
index de357169d..8a5d69a02 100644
--- a/tests/auto/blackbox/tst_blackbox.h
+++ b/tests/auto/blackbox/tst_blackbox.h
@@ -313,6 +313,7 @@ private slots:
     void trackRemoveFile();
     void trackRemoveFileTag();
     void trackRemoveProduct();
+    void transitiveInvalidDependencies();
     void transitiveOptionalDependencies();
     void typescript();
     void undefinedTargetPlatform();