Billy CRs for version parsing

microsoft · Mar 15, 2022 · 5143398 · 5143398
1 parent e11e4da
commit 5143398
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 43 deletions.
diff --git a/include/vcpkg/tools.h b/include/vcpkg/tools.h
@@ -6,6 +6,7 @@
 #include <vcpkg/fwd/vcpkgpaths.h>
 
 #include <vcpkg/base/stringliteral.h>
+#include <vcpkg/base/optional.h>
 
 #include <string>
 #include <utility>
@@ -44,5 +45,7 @@ namespace vcpkg
         virtual const std::string& get_tool_version(const VcpkgPaths& paths, StringView tool) const = 0;
     };
 
+    Optional<std::array<int, 3>> parse_tool_version_string(StringView string_version);
+
     std::unique_ptr<ToolCache> get_tool_cache(RequireExactVersions abiToolVersionHandling);
 }
diff --git a/src/vcpkg-test/tools.cpp b/src/vcpkg-test/tools.cpp
@@ -0,0 +1,43 @@
+#include <catch2/catch.hpp>
+
+#include <vcpkg/tools.h>
+
+#include <array>
+
+using namespace vcpkg;
+
+TEST_CASE("parse_tool_version_string", "[tools]")
+{
+    auto result = parse_tool_version_string("1.2.3");
+    REQUIRE(result.has_value());
+    CHECK(*result.get() == std::array<int, 3>{1, 2, 3});
+
+    result = parse_tool_version_string("3.22.3");
+    REQUIRE(result.has_value());
+    CHECK(*result.get() == std::array<int, 3>{3, 22, 3});
+
+    result = parse_tool_version_string("4.65");
+    REQUIRE(result.has_value());
+    CHECK(*result.get() == std::array<int, 3>{4, 65, 0});
+
+    result = parse_tool_version_string(R"(cmake version 3.22.2
+CMake suite maintained and supported by Kitware (kitware.com/cmake).)");
+    REQUIRE(result.has_value());
+    CHECK(*result.get() == std::array<int, 3>{3, 22, 2});
+
+    result = parse_tool_version_string(R"(aria2 version 1.35.0
+Copyright (C) 2006, 2019 Tatsuhiro Tsujikawa)");
+    REQUIRE(result.has_value());
+    CHECK(*result.get() == std::array<int, 3>{1, 35, 0});
+
+    result = parse_tool_version_string(R"(git version 2.17.1.windows.2)");
+    REQUIRE(result.has_value());
+    CHECK(*result.get() == std::array<int, 3>{2, 17, 1});
+
+    result = parse_tool_version_string(R"(git version 2.17.windows.2)");
+    REQUIRE(result.has_value());
+    CHECK(*result.get() == std::array<int, 3>{2, 17, 0});
+
+    result = parse_tool_version_string("4");
+    CHECK_FALSE(result.has_value());
+}
diff --git a/src/vcpkg/tools.cpp b/src/vcpkg/tools.cpp
@@ -26,39 +26,32 @@ namespace vcpkg
     };
 
     // /\d+\.\d+(\.\d+)?/
-    static Optional<std::array<int, 3>> parse_version_string(StringView string_version)
+    Optional<std::array<int, 3>> parse_tool_version_string(StringView string_version)
     {
         using P = Parse::ParserBase;
 
         // first, find the beginning of the version
         auto first = string_version.begin();
-        auto last = string_version.end();
+        const auto last = string_version.end();
 
         // we're looking for the first instance of `<digits>.<digits>`
+        ParsedExternalVersion parsed_version{};
         for (;;)
         {
             first = std::find_if(first, last, P::is_ascii_digit);
-            auto end_of_field = std::find_if_not(first, last, P::is_ascii_digit);
-            if (end_of_field == last)
+            if (first == last)
             {
-                break;
+                return nullopt;
             }
-            if (*end_of_field == '.')
+
+            if (try_parse_external_dot_version(parsed_version, StringView{first, last}) && !parsed_version.minor.empty())
             {
-                ++end_of_field;
-                if (end_of_field != last && P::is_ascii_digit(*end_of_field))
-                {
-                    break;
-                }
+                break;
             }
-            first = end_of_field;
-        }
 
-        ParsedExternalVersion parsed_version;
-        if (!try_parse_external_dot_version(parsed_version, StringView{first, last}))
-        {
-            return {};
+            first = std::find_if_not(first, last, P::is_ascii_digit);
         }
+
         parsed_version.normalize();
 
         std::string buffer;
@@ -144,7 +137,7 @@ namespace vcpkg
         const std::string sha512 = Strings::find_exactly_one_enclosed(tool_data, "<sha512>", "</sha512>").to_string();
         auto archive_name = Strings::find_at_most_one_enclosed(tool_data, "<archiveName>", "</archiveName>");
 
-        const Optional<std::array<int, 3>> version = parse_version_string(version_as_string);
+        const Optional<std::array<int, 3>> version = parse_tool_version_string(version_as_string);
         Checks::check_exit(VCPKG_LINE_INFO,
                            version.has_value(),
                            "Could not parse version for tool %s. Version string was: %s",
@@ -197,7 +190,7 @@ namespace vcpkg
             auto maybe_version = tool_provider.get_version(paths, candidate);
             const auto version = maybe_version.get();
             if (!version) continue;
-            const auto parsed_version = parse_version_string(*version);
+            const auto parsed_version = parse_tool_version_string(*version);
             if (!parsed_version) continue;
             auto& actual_version = *parsed_version.get();
             if (!accept_version(actual_version)) continue;

diff --git a/src/vcpkg/versions.cpp b/src/vcpkg/versions.cpp
@@ -326,19 +326,11 @@ namespace vcpkg
 
     ExpectedL<DateVersion> DateVersion::try_parse(StringView version)
     {
-        if (version.size() < 10) return format_invalid_date_version(version);
-
-        bool valid = Parse::ParserBase::is_ascii_digit(version[0]);
-        valid |= Parse::ParserBase::is_ascii_digit(version[1]);
-        valid |= Parse::ParserBase::is_ascii_digit(version[2]);
-        valid |= Parse::ParserBase::is_ascii_digit(version[3]);
-        valid |= version[4] != '-';
-        valid |= Parse::ParserBase::is_ascii_digit(version[5]);
-        valid |= Parse::ParserBase::is_ascii_digit(version[6]);
-        valid |= version[7] != '-';
-        valid |= Parse::ParserBase::is_ascii_digit(version[8]);
-        valid |= Parse::ParserBase::is_ascii_digit(version[9]);
-        if (!valid) return format_invalid_date_version(version);
+        ParsedExternalVersion parsed;
+        if (!try_parse_external_date_version(parsed, version))
+        {
+            return format_invalid_date_version(version);
+        }
 
         DateVersion ret;
         ret.original_string.assign(version.data(), version.size());
@@ -469,8 +461,6 @@ namespace vcpkg
 
         out.major = out.minor = out.patch = StringView{};
 
-        if (first == last) return false;
-        if (*first == 'v') ++first;
         if (first == last) return false;
 
         auto major_last = std::find_if_not(first, last, P::is_ascii_digit);
@@ -485,22 +475,15 @@ namespace vcpkg
         }
 
         auto minor_last = std::find_if_not(major_last + 1, last, P::is_ascii_digit);
-        if (minor_last == major_last + 1)
-        {
-            return false;
-        }
         out.minor = StringView{major_last + 1, minor_last};
-        if (minor_last == last || *minor_last != '.')
+        if (minor_last == last || minor_last == major_last + 1 || *minor_last != '.')
         {
             return true;
         }
 
         auto patch_last = std::find_if_not(minor_last + 1, last, P::is_ascii_digit);
-        if (minor_last == major_last + 1)
-        {
-            return false;
-        }
         out.patch = StringView{minor_last + 1, patch_last};
+
         return true;
     }
 }