find_package: Fix CPS version parsing

Update cmPackageInfoReader's version parsing to more fully conform to
the specification and to reject non-conforming version strings. Start
adding framework to support version schemas other than "simple". Fix how
cmFindPackageCommand extracts version parts to not fail if more than
four parts are present.

Source/cmFindPackageCommand.cxx

@@ -2758,23 +2758,32 @@ bool cmFindPackageCommand::CheckVersion(std::string const& config_file)
         if (result && hasVersion) {
           this->VersionFound = version;
 
-          std::vector<unsigned> const& versionParts = reader->ParseVersion();
-          this->VersionFoundCount = static_cast<unsigned>(versionParts.size());
-          switch (this->VersionFoundCount) {
-            case 4:
-              this->VersionFoundTweak = versionParts[3];
-              CM_FALLTHROUGH;
-            case 3:
-              this->VersionFoundPatch = versionParts[2];
-              CM_FALLTHROUGH;
-            case 2:
-              this->VersionFoundMinor = versionParts[1];
-              CM_FALLTHROUGH;
-            case 1:
-              this->VersionFoundMajor = versionParts[0];
-              CM_FALLTHROUGH;
-            default:
-              break;
+          cm::optional<cmPackageInfoReader::Pep440Version> const&
+            parsedVersion = reader->ParseVersion();
+          if (parsedVersion) {
+            std::vector<unsigned> const& versionParts =
+              parsedVersion->ReleaseComponents;
+
+            this->VersionFoundCount =
+              static_cast<unsigned>(versionParts.size());
+            switch (std::min(this->VersionFoundCount, 4u)) {
+              case 4:
+                this->VersionFoundTweak = versionParts[3];
+                CM_FALLTHROUGH;
+              case 3:
+                this->VersionFoundPatch = versionParts[2];
+                CM_FALLTHROUGH;
+              case 2:
+                this->VersionFoundMinor = versionParts[1];
+                CM_FALLTHROUGH;
+              case 1:
+                this->VersionFoundMajor = versionParts[0];
+                CM_FALLTHROUGH;
+              default:
+                break;
+            }
+          } else {
+            this->VersionFoundCount = 0;
           }
         }
         this->CpsReader = std::move(reader);

Source/cmPackageInfoReader.cxx

@@ -366,6 +366,62 @@ void AddDefinitions(cmMakefile* makefile, cmTarget* target,
   }
 }
 
+cm::optional<cmPackageInfoReader::Pep440Version> ParseSimpleVersion(
+  std::string const& version)
+{
+  if (version.empty()) {
+    return cm::nullopt;
+  }
+
+  cmPackageInfoReader::Pep440Version result;
+  result.Simple = true;
+
+  cm::string_view remnant{ version };
+  for (;;) {
+    // Find the next part separator.
+    std::string::size_type const n = remnant.find_first_of(".+-"_s);
+    if (n == 0) {
+      // The part is an empty string.
+      return cm::nullopt;
+    }
+
+    // Extract the part as a number.
+    cm::string_view const part = remnant.substr(0, n);
+    std::string::size_type const l = part.size();
+    std::string::size_type p;
+    unsigned long const value = std::stoul(std::string{ part }, &p);
+    if (p != l || value > std::numeric_limits<unsigned>::max()) {
+      // The part was not a valid number or is too big.
+      return cm::nullopt;
+    }
+    result.ReleaseComponents.push_back(static_cast<unsigned>(value));
+
+    // Have we consumed the entire input?
+    if (n == std::string::npos) {
+      return { std::move(result) };
+    }
+
+    // Lop off the current part.
+    char const sep = remnant[n];
+    remnant = remnant.substr(n + 1);
+    if (sep == '+' || sep == '-') {
+      // If we hit the local label, we're done.
+      result.LocalLabel = remnant;
+      return { std::move(result) };
+    }
+
+    // We just consumed a '.'; check that there's more.
+    if (remnant.empty()) {
+      // A trailing part separator is not allowed.
+      return cm::nullopt;
+    }
+
+    // Continue with the remaining input.
+  }
+
+  // Unreachable.
+}
+
 } // namespace
 
 std::unique_ptr<cmPackageInfoReader> cmPackageInfoReader::Read(
@@ -428,40 +484,22 @@ cm::optional<std::string> cmPackageInfoReader::GetVersion() const
   return cm::nullopt;
 }
 
-std::vector<unsigned> cmPackageInfoReader::ParseVersion() const
+cm::optional<cmPackageInfoReader::Pep440Version>
+cmPackageInfoReader::ParseVersion() const
 {
   // Check that we have a version.
   cm::optional<std::string> const& version = this->GetVersion();
   if (!version) {
-    return {};
+    return cm::nullopt;
   }
 
-  std::vector<unsigned> result;
-  cm::string_view remnant{ *version };
-
   // Check if we know how to parse the version.
   Json::Value const& schema = this->Data["version_schema"];
   if (schema.isNull() || cmStrCaseEq(schema.asString(), "simple"_s)) {
-    // Keep going until we run out of parts.
-    while (!remnant.empty()) {
-      std::string::size_type n = remnant.find('.');
-      cm::string_view part = remnant.substr(0, n);
-      if (n == std::string::npos) {
-        remnant = {};
-      } else {
-        remnant = remnant.substr(n + 1);
-      }
-
-      unsigned long const value = std::stoul(std::string{ part }, &n);
-      if (n == 0 || value > std::numeric_limits<unsigned>::max()) {
-        // The part was not a valid number or is too big.
-        return {};
-      }
-      result.push_back(static_cast<unsigned>(value));
-    }
+    return ParseSimpleVersion(*version);
   }
 
-  return result;
+  return cm::nullopt;
 }
 
 std::vector<cmPackageRequirement> cmPackageInfoReader::GetRequirements() const

Source/cmPackageInfoReader.h

@@ -44,10 +44,31 @@ public:
   std::string GetName() const;
   cm::optional<std::string> GetVersion() const;
 
+  // NOTE: The eventual intent is for CPS to support multiple version schemas,
+  // and in particular, we expect to want to support "simple", "custom", "rpm",
+  // "dpkg" and "pep440". Additionally, we desire to be able to parse each of
+  // these to the maximum extent possible; in particular, we want to be able
+  // to decompose "simple" and "pep440" versions into components represented
+  // as numeric types rather than strings, which is not possible with the "rpm"
+  // and "dpkg" schemas. Therefore, we require different data structures to
+  // represent different version schemas.
+
+  struct Pep440Version
+  {
+    // NOTE: This structure is currently incomplete as we only support the
+    // "simple" schema at this time.
+    bool Simple; // "simple" can be represented as a subset of "pep440"
+    std::vector<unsigned> ReleaseComponents;
+    cm::optional<std::string> LocalLabel;
+  };
+
+  // FIXME: Return a sum type (e.g. {cm,std}::variant) of possible versions
+  // when we support more than just the "simple" (and possibly "pep440")
+  // schema(s).
   /// If the package uses the 'simple' version scheme, obtain the version as
-  /// a numeric tuple.  Returns an empty vector for other schemes or if no
-  /// version is specified.
-  std::vector<unsigned> ParseVersion() const;
+  /// a numeric tuple and optional trailing string.  Returns a disengaged
+  /// optional for other schemes or if no version is specified.
+  cm::optional<Pep440Version> ParseVersion() const;
 
   std::vector<cmPackageRequirement> GetRequirements() const;
   std::vector<std::string> GetComponentNames() const;

Tests/FindPackageCpsTest/CMakeLists.txt

@@ -21,24 +21,53 @@ set(CMAKE_FIND_FRAMEWORK FIRST)
 
 add_executable(FindPackageCpsTest FindPackageTest.cxx)
 
+###############################################################################
+
+function(expect PACKAGE VAR OP VALUE WHAT)
+  if(NOT ${PACKAGE}_${VAR} ${OP} ${VALUE})
+    message(SEND_ERROR "${PACKAGE} wrong ${WHAT} ${${PACKAGE}_${VAR}} !")
+  endif()
+endfunction()
+
+function(test_version PACKAGE LITERAL COUNT MAJOR MINOR PATCH TWEAK)
+  if(NOT ${PACKAGE}_FOUND)
+    message(SEND_ERROR "${PACKAGE} not found !")
+  else()
+    expect(${PACKAGE} VERSION STREQUAL "${LITERAL}" "version")
+    expect(${PACKAGE} VERSION_COUNT EQUAL ${COUNT} "version count")
+    expect(${PACKAGE} VERSION_MAJOR EQUAL ${MAJOR} "major version")
+    expect(${PACKAGE} VERSION_MINOR EQUAL ${MINOR} "minor version")
+    expect(${PACKAGE} VERSION_PATCH EQUAL ${PATCH} "patch version")
+    expect(${PACKAGE} VERSION_TWEAK EQUAL ${TWEAK} "tweak version")
+  endif()
+endfunction()
+
+function(test_unparsed_version PACKAGE VERSION)
+  find_package(${PACKAGE} CONFIG)
+  test_version(${PACKAGE} "${VERSION}" 0 0 0 0 0)
+endfunction()
+
 ###############################################################################
 # Test a basic package search.
 # It should NOT find the package's CMake package file.
 
 find_package(Sample CONFIG)
-if(NOT Sample_FOUND)
-  message(SEND_ERROR "Sample not found !")
-elseif(NOT Sample_VERSION STREQUAL "2.10.11")
-  message(SEND_ERROR "Sample wrong version ${Sample_VERSION} !")
-elseif(NOT Sample_VERSION_MAJOR EQUAL 2)
-  message(SEND_ERROR "Sample wrong major version ${Sample_VERSION_MAJOR} !")
-elseif(NOT Sample_VERSION_MINOR EQUAL 10)
-  message(SEND_ERROR "Sample wrong minor version ${Sample_VERSION_MINOR} !")
-elseif(NOT Sample_VERSION_PATCH EQUAL 11)
-  message(SEND_ERROR "Sample wrong patch version ${Sample_VERSION_PATCH} !")
-elseif(NOT Sample_VERSION_TWEAK EQUAL 0)
-  message(SEND_ERROR "Sample wrong tweak version ${Sample_VERSION_TWEAK} !")
-endif()
+test_version(Sample "2.10.11" 3 2 10 11 0)
+
+###############################################################################
+# Test some more complicated version parsing.
+
+find_package(LongVersion CONFIG)
+test_version(LongVersion "1.1.2.3.5.8+fibonacci" 6 1 1 2 3)
+
+find_package(EmptyMarker CONFIG)
+test_version(EmptyMarker "1.1+" 2 1 1 0 0)
+
+test_unparsed_version(BadVersion1 "1..1")
+test_unparsed_version(BadVersion2 "1.1a.0")
+test_unparsed_version(BadVersion3 "1.1.")
+test_unparsed_version(BadVersion4 "+42")
+test_unparsed_version(CustomVersion "VII")
 
 ###############################################################################
 # Test glob sorting.

Tests/FindPackageCpsTest/cps/badversion1.cps

@@ -0,0 +1,8 @@
+{
+  "cps_version": "0.13",
+  "name": "BadVersion1",
+  "version": "1..1",
+  "version_schema": "simple",
+  "cps_path": "@prefix@/cps",
+  "components": {}
+}

Tests/FindPackageCpsTest/cps/badversion2.cps

@@ -0,0 +1,8 @@
+{
+  "cps_version": "0.13",
+  "name": "BadVersion2",
+  "version": "1.1a.0",
+  "version_schema": "simple",
+  "cps_path": "@prefix@/cps",
+  "components": {}
+}

Tests/FindPackageCpsTest/cps/badversion3.cps

@@ -0,0 +1,8 @@
+{
+  "cps_version": "0.13",
+  "name": "BadVersion3",
+  "version": "1.1.",
+  "version_schema": "simple",
+  "cps_path": "@prefix@/cps",
+  "components": {}
+}

Tests/FindPackageCpsTest/cps/badversion4.cps

@@ -0,0 +1,8 @@
+{
+  "cps_version": "0.13",
+  "name": "BadVersion4",
+  "version": "+42",
+  "version_schema": "simple",
+  "cps_path": "@prefix@/cps",
+  "components": {}
+}

Tests/FindPackageCpsTest/cps/customversion.cps

@@ -0,0 +1,8 @@
+{
+  "cps_version": "0.13",
+  "name": "CustomVersion",
+  "version": "VII",
+  "version_schema": "roman",
+  "cps_path": "@prefix@/cps",
+  "components": {}
+}

Tests/FindPackageCpsTest/cps/emptymarker.cps

@@ -0,0 +1,8 @@
+{
+  "cps_version": "0.13",
+  "name": "EmptyMarker",
+  "version": "1.1+",
+  "version_schema": "simple",
+  "cps_path": "@prefix@/cps",
+  "components": {}
+}

Tests/FindPackageCpsTest/cps/longversion.cps

@@ -0,0 +1,8 @@
+{
+  "cps_version": "0.13",
+  "name": "LongVersion",
+  "version": "1.1.2.3.5.8+fibonacci",
+  "version_schema": "simple",
+  "cps_path": "@prefix@/cps",
+  "components": {}
+}