From 34d0f2e678087ded43f68faadfa88d9e72e6321e Mon Sep 17 00:00:00 2001
From: Rainer Keller <rainer.keller@qt.io>
Date: Wed, 19 Oct 2022 13:30:33 +0200
Subject: [PATCH] McuSupport: Fix invalid version detector being instantiatied

In case the JSON entry was missing completely, a directory version detector
was instantiated.
Instantiate version dectectors only if there is enough and valid
information available.

Change-Id: Iaf9cd40165e4d75124f3e0ddda66db2f4dab5ff7
Reviewed-by: Daniele Bortolotti <daniele.bortolotti@qt.io>
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: Yasser Grimes <yasser.grimes@qt.io>
Reviewed-by: Christian Stenger <christian.stenger@qt.io>
---
 src/plugins/mcusupport/mcutargetfactory.cpp |  7 ++-
 src/plugins/mcusupport/test/unittest.cpp    | 64 +++++++++++++++++++++
 src/plugins/mcusupport/test/unittest.h      |  4 ++
 3 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/src/plugins/mcusupport/mcutargetfactory.cpp b/src/plugins/mcusupport/mcutargetfactory.cpp
index 50cf51f327f..d17263ca351 100644
--- a/src/plugins/mcusupport/mcutargetfactory.cpp
+++ b/src/plugins/mcusupport/mcutargetfactory.cpp
@@ -36,10 +36,15 @@ McuPackageVersionDetector *createVersionDetection(const VersionDetection &versio
                                                            versionDetection.filePattern),
                                                        QStringList{versionDetection.executableArgs},
                                                        versionDetection.regex};
-    else
+    else if (!versionDetection.filePattern.isEmpty() && !versionDetection.regex.isEmpty())
         return new McuPackageDirectoryVersionDetector(versionDetection.filePattern,
                                                       versionDetection.regex,
                                                       versionDetection.isFile);
+    else {
+        // In this case the JSON entry is either invalid or missing.
+        // After refactoring, this should raise a JSON error to the user.
+        return nullptr;
+    }
 }
 
 static void removeEmptyPackages(Packages &packages)
diff --git a/src/plugins/mcusupport/test/unittest.cpp b/src/plugins/mcusupport/test/unittest.cpp
index 6d8e84c7979..8f97f1c17ae 100644
--- a/src/plugins/mcusupport/test/unittest.cpp
+++ b/src/plugins/mcusupport/test/unittest.cpp
@@ -1638,4 +1638,68 @@ void McuSupportTest::test_addToSystemPathFlag()
     QCOMPARE(freeRtosPackage.shouldAddToSystemPath, false);
 }
 
+void McuSupportTest::test_nonemptyVersionDetector()
+{
+    PackageDescription pkgDesc;
+    pkgDesc.label = "GNU Arm Embedded Toolchain";
+    pkgDesc.envVar = "ARMGCC_DIR";
+    // pkgDesc.cmakeVar left empty
+    // pkgDesc.description left empty
+    pkgDesc.setting = "GNUArmEmbeddedToolchain";
+    // pkgDesc.defaultPath left empty
+    // pkgDesc.validationPath left empty
+    // pkgDesc.versions left empty
+    pkgDesc.versionDetection.filePattern = "bin/arm-none-eabi-g++";
+    pkgDesc.versionDetection.regex = "\\bv?(\\d+\\.\\d+\\.\\d+)\\b";
+    pkgDesc.versionDetection.executableArgs = "--version";
+    // pkgDesc.versionDetection.xmlElement left empty
+    // pkgDesc.versionDetection.xmlAttribute left empty
+    pkgDesc.shouldAddToSystemPath = false;
+
+    const auto package = targetFactory.createPackage(pkgDesc);
+    QVERIFY(package->getVersionDetector() != nullptr);
+    QCOMPARE(typeid(*package->getVersionDetector()).name(),
+             typeid(McuPackageExecutableVersionDetector).name());
+}
+
+void McuSupportTest::test_emptyVersionDetector()
+{
+    PackageDescription pkgDesc;
+    pkgDesc.label = "GNU Arm Embedded Toolchain";
+    pkgDesc.envVar = "ARMGCC_DIR";
+    // pkgDesc.cmakeVar left empty
+    // pkgDesc.description left empty
+    pkgDesc.setting = "GNUArmEmbeddedToolchain";
+    // pkgDesc.defaultPath left empty
+    // pkgDesc.validationPath left empty
+    // pkgDesc.versions left empty
+    // Version detection left completely empty
+        // pkgDesc.versionDetection.filePattern
+        // pkgDesc.versionDetection.regex
+        // pkgDesc.versionDetection.executableArgs
+        // pkgDesc.versionDetection.xmlElement
+        // pkgDesc.versionDetection.xmlAttribute
+    pkgDesc.shouldAddToSystemPath = false;
+
+    const auto package = targetFactory.createPackage(pkgDesc);
+    QVERIFY(package->getVersionDetector() == nullptr);
+}
+
+void McuSupportTest::test_emptyVersionDetectorFromJson()
+{
+    const auto targetDescription = parseDescriptionJson(armgcc_mimxrt1050_evk_freertos_json);
+    auto [targets, packages] = targetFactory.createTargets(targetDescription, sdkPackagePtr);
+
+    auto freeRtos = findOrDefault(packages, [](const McuPackagePtr &pkg) {
+        return (pkg->cmakeVariableName() == freeRtosCMakeVar);
+    });
+
+    QVERIFY2(!freeRtos->cmakeVariableName().isEmpty(), "The freeRTOS package was not found, but there should be one.");
+
+    // For the FreeRTOS package there used to be no version check defined. Use this package to check if this was
+    // considered correctly
+    QVERIFY(freeRtos->getVersionDetector() == nullptr);
+}
+
+
 } // namespace McuSupport::Internal::Test
diff --git a/src/plugins/mcusupport/test/unittest.h b/src/plugins/mcusupport/test/unittest.h
index 25982d71bd3..50e823d10b2 100644
--- a/src/plugins/mcusupport/test/unittest.h
+++ b/src/plugins/mcusupport/test/unittest.h
@@ -103,6 +103,10 @@ private slots:
     void test_differentValueForEachOperationSystem();
     void test_addToSystemPathFlag();
 
+    void test_nonemptyVersionDetector();
+    void test_emptyVersionDetector();
+    void test_emptyVersionDetectorFromJson();
+
 private:
     QVersionNumber currentQulVersion{2, 0};
     PackageMock *freeRtosPackage{new PackageMock};