Fix unit test failures.

microsoft · Jan 10, 2024 · ca6d48d · ca6d48d
1 parent d7e5b0b
commit ca6d48d
Show file tree

Hide file tree

Showing 13 changed files with 147 additions and 47 deletions.
diff --git a/include/vcpkg/base/diagnostics.h b/include/vcpkg/base/diagnostics.h
@@ -115,6 +115,7 @@ namespace vcpkg
     };
 
     extern DiagnosticContext& console_diagnostic_context;
+    extern DiagnosticContext& null_diagnostic_context;
 
     // If Ty is an Optional<U>, typename AdaptContextUnwrapOptional<Ty>::type is the type necessary to return U, and fwd
     // is the type necessary to forward U. Otherwise, there are no members ::type or ::fwd

diff --git a/include/vcpkg/base/system.mac.h b/include/vcpkg/base/system.mac.h
@@ -3,6 +3,9 @@
 #include <vcpkg/base/fwd/span.h>
 #include <vcpkg/base/fwd/stringview.h>
 
+#include <vcpkg/base/diagnostics.h>
+#include <vcpkg/base/optional.h>
+
 #include <string>
 
 namespace vcpkg
@@ -13,5 +16,5 @@ namespace vcpkg
     bool validate_mac_address_format(StringView mac);
     bool is_valid_mac_for_telemetry(StringView mac);
     std::string mac_bytes_to_string(const Span<char>& bytes);
-    bool extract_mac_from_getmac_output_line(StringView line, std::string& out);
+    Optional<std::string> extract_mac_from_getmac_output_line(DiagnosticContext& context, StringView line);
 }
diff --git a/src/vcpkg-test/cgroup-parser.cpp b/src/vcpkg-test/cgroup-parser.cpp
@@ -132,7 +132,11 @@ TEST_CASE ("parse proc/pid/stat file", "[cgroup-parser]")
         std::string contents =
             R"(4281 (0123456789abcdefg) S 4099 1676 1676 0 -1 1077936384 51165 303 472 0 81 25 0 0 20 0 10 0 829158 4924583936 39830 18446744073709551615 4194304 14147733 140725993620736 0 0 0 0 16781312 16386 0 0 0 17 1 0 0 5 0 0 16247120 16519160 29999104 140725993622792 140725993622920 140725993622920 140725993627556 0)";
 
-        auto maybe_stat = try_parse_process_stat_file(console_diagnostic_context, {contents, "test"});
+        BufferedDiagnosticContext bdc;
+        auto maybe_stat = try_parse_process_stat_file(bdc, {contents, "test"});
         REQUIRE(!maybe_stat.has_value());
+        REQUIRE(bdc.to_string() == R"(test:1:7: error: expected ')' here
+  on expression: 4281 (0123456789abcdefg) S 4099 1676 1676 0 -1 1077936384 51165 303 472 0 81 25 0 0 20 0 10 0 829158 4924583936 39830 18446744073709551615 4194304 14147733 140725993620736 0 0 0 0 16781312 16386 0 0 0 17 1 0 0 5 0 0 16247120 16519160 29999104 140725993622792 140725993622920 140725993622920 140725993627556 0
+                       ^)");
     }
 }
diff --git a/src/vcpkg-test/manifests.cpp b/src/vcpkg-test/manifests.cpp
@@ -1285,7 +1285,7 @@ TEST_CASE ("license error messages", "[manifests][license]")
     }
     {
         BufferedDiagnosticContext bdc;
-        CHECK(!parse_spdx_license_expression(bdc, "MIT AND unknownlicense").has_value());
+        CHECK(parse_spdx_license_expression(bdc, "MIT AND unknownlicense").has_value());
         CHECK(
             bdc.to_string() ==
             R"(<license string>:1:9: warning: Unknown license identifier 'unknownlicense'. Known values are listed at https://spdx.org/licenses/

diff --git a/src/vcpkg-test/system.mac.cpp b/src/vcpkg-test/system.mac.cpp
@@ -51,16 +51,19 @@ TEST_CASE ("MAC bytes to string", "[metrics.mac]")
     CHECK(long_mac_str.empty());
 }
 
-TEST_CASE ("test getmac ouptut parse", "[metrics.mac]")
+TEST_CASE ("test getmac output parse", "[metrics.mac]")
 {
     std::string mac_str;
 
     static constexpr StringLiteral good_line =
         R"csv("Wi-Fi","Wi-Fi 6, maybe","00-11-22-DD-EE-FF","\Device\Tcip_{GUID}")csv";
-    CHECK(extract_mac_from_getmac_output_line(good_line, mac_str));
-    CHECK(mac_str == "00:11:22:dd:ee:ff");
+    BufferedDiagnosticContext bdc;
+    REQUIRE(extract_mac_from_getmac_output_line(bdc, good_line).value_or_exit(VCPKG_LINE_INFO) == "00:11:22:dd:ee:ff");
+    REQUIRE(bdc.lines.empty());
 
     static constexpr StringLiteral bad_line = "00-11-22-DD-EE-FF      \\Device\\Tcip_{GUID}";
-    CHECK(!extract_mac_from_getmac_output_line(bad_line, mac_str));
-    CHECK(mac_str.empty());
+    REQUIRE(!extract_mac_from_getmac_output_line(bdc, bad_line));
+    REQUIRE(bdc.to_string() == R"(getmac output:1:1: error: expected '"' here
+  on expression: 00-11-22-DD-EE-FF      \Device\Tcip_{GUID}
+                 ^)");
 }
diff --git a/src/vcpkg/base/diagnostics.cpp b/src/vcpkg/base/diagnostics.cpp
@@ -179,3 +179,21 @@ namespace vcpkg
 {
     DiagnosticContext& console_diagnostic_context = console_diagnostic_context_instance;
 }
+
+namespace
+{
+    struct NullDiagnosticContext : DiagnosticContext
+    {
+        virtual void report(const DiagnosticLine&) override
+        {
+            // intentionally empty
+        }
+    };
+
+    NullDiagnosticContext null_diagnostic_context_instance;
+}
+
+namespace vcpkg
+{
+    DiagnosticContext& null_diagnostic_context = null_diagnostic_context_instance;
+}
diff --git a/src/vcpkg/base/git.cpp b/src/vcpkg/base/git.cpp
@@ -147,6 +147,7 @@ namespace vcpkg
         {
             context.report(DiagnosticLine{DiagKind::Note,
                                           msg::format(msgGitUnexpectedCommandOutputCmd, msg::command_line = cmd_line)});
+            results_storage.clear();
         }
 
         return results_storage;

diff --git a/src/vcpkg/base/parse.cpp b/src/vcpkg/base/parse.cpp
@@ -185,19 +185,25 @@ namespace vcpkg
 
     void ParserBase::add_error(LocalizedString&& message, const SourceLoc& loc)
     {
-        message.append_raw('\n');
-        append_caret_line(message, loc);
-        m_context.report(
-            DiagnosticLine{DiagKind::Error, m_origin, TextPosition{loc.row, loc.column}, std::move(message)});
-        m_any_errors = true;
-        // Avoid error loops by skipping to the end
-        skip_to_eof();
+        // avoid cascading errors by only saving the first
+        if (!m_any_errors)
+        {
+            message.append_raw('\n');
+            append_caret_line(message, loc);
+            m_context.report(
+                DiagnosticLine{DiagKind::Error, m_origin, TextPosition{loc.row, loc.column}, std::move(message)});
+            m_any_errors = true;
+            // Avoid error loops by skipping to the end
+            skip_to_eof();
+        }
     }
 
     void ParserBase::add_warning(LocalizedString&& message) { add_warning(std::move(message), cur_loc()); }
 
     void ParserBase::add_warning(LocalizedString&& message, const SourceLoc& loc)
     {
+        message.append_raw('\n');
+        append_caret_line(message, loc);
         m_context.report(
             DiagnosticLine{DiagKind::Warning, m_origin, TextPosition{loc.row, loc.column}, std::move(message)});
     }

diff --git a/src/vcpkg/base/system.mac.cpp b/src/vcpkg/base/system.mac.cpp
@@ -84,51 +84,96 @@ namespace vcpkg
         return std::string(mac_address, non_zero_mac ? MAC_STRING_LENGTH : 0);
     }
 
-    bool extract_mac_from_getmac_output_line(StringView line, std::string& out)
+    Optional<std::string> extract_mac_from_getmac_output_line(DiagnosticContext& context, StringView line)
     {
         // getmac /V /NH /FO CSV
         // outputs each interface on its own comma-separated line
         // "connection name","network adapter","physical address","transport name"
         auto is_quote = [](auto ch) -> bool { return ch == '"'; };
 
-        auto parser = ParserBase(console_diagnostic_context, line, "getmac ouptut");
+        auto parser = ParserBase(context, line, "getmac output");
 
-        out.clear();
+        Optional<std::string> result;
+        auto& out = result.emplace();
 
         // ignore "connection name"
-        if (parser.require_character('"')) return false;
+        if (parser.require_character('"'))
+        {
+            result.clear();
+            return result;
+        }
         parser.match_until(is_quote);
-        if (parser.require_character('"')) return false;
-        if (parser.require_character(',')) return false;
+        if (parser.require_character('"'))
+        {
+            result.clear();
+            return result;
+        }
+        if (parser.require_character(','))
+        {
+            result.clear();
+            return result;
+        }
 
         // ignore "network adapter"
-        if (parser.require_character('"')) return false;
+        if (parser.require_character('"'))
+        {
+            result.clear();
+            return result;
+        }
         parser.match_until(is_quote);
-        if (parser.require_character('"')) return false;
-        if (parser.require_character(',')) return false;
+        if (parser.require_character('"'))
+        {
+            result.clear();
+            return result;
+        }
+        if (parser.require_character(','))
+        {
+            result.clear();
+            return result;
+        }
 
         // get "physical address"
-        if (parser.require_character('"')) return false;
+        if (parser.require_character('"'))
+        {
+            result.clear();
+            return result;
+        }
         out = parser.match_until(is_quote).to_string();
-        if (parser.require_character('"')) return false;
-        if (parser.require_character(',')) return false;
+        if (parser.require_character('"'))
+        {
+            result.clear();
+            return result;
+        }
+        if (parser.require_character(','))
+        {
+            result.clear();
+            return result;
+        }
 
         // ignore "transport name"
-        if (parser.require_character('"')) return false;
+        if (parser.require_character('"'))
+        {
+            result.clear();
+            return result;
+        }
         parser.match_until(is_quote);
-        if (parser.require_character('"')) return false;
+        if (parser.require_character('"'))
+        {
+            result.clear();
+            return result;
+        }
 
         parser.skip_whitespace();
         if (!parser.at_eof())
         {
-            out.clear();
-            return false;
+            result.clear();
+            return result;
         }
 
         // output line was properly formatted
         std::replace(out.begin(), out.end(), '-', ':');
         Strings::inplace_ascii_to_lowercase(out);
-        return true;
+        return result;
     }
 
     std::string get_user_mac_hash()
@@ -143,10 +188,13 @@ namespace vcpkg
         {
             for (auto&& line : Strings::split(getmac->output, '\n'))
             {
-                std::string mac;
-                if (extract_mac_from_getmac_output_line(line, mac) && is_valid_mac_for_telemetry(mac))
+                auto maybe_mac = extract_mac_from_getmac_output_line(null_diagnostic_context, line);
+                if (auto mac = maybe_mac.get())
                 {
-                    return Hash::get_string_hash(mac, Hash::Algorithm::Sha256);
+                    if (is_valid_mac_for_telemetry(*mac))
+                    {
+                        return Hash::get_string_hash(*mac, Hash::Algorithm::Sha256);
+                    }
                 }
             }
         }

diff --git a/src/vcpkg/ci-baseline.cpp b/src/vcpkg/ci-baseline.cpp
@@ -60,15 +60,16 @@ namespace vcpkg
                                                             StringView text,
                                                             StringView origin)
     {
-        std::vector<CiBaselineLine> result;
+        Optional<std::vector<CiBaselineLine>> result_storage;
+        auto& result = result_storage.emplace();
         ParserBase parser(context, text, origin);
         for (;;)
         {
             parser.skip_whitespace();
             if (parser.at_eof())
             {
                 // success
-                return result;
+                return result_storage;
             }
 
             if (parser.cur() == '#')
@@ -148,7 +149,8 @@ namespace vcpkg
         }
 
         // failure
-        return nullopt;
+        result_storage.clear();
+        return result_storage;
     }
 
     CiBaselineData parse_and_apply_ci_baseline(View<CiBaselineLine> lines,

diff --git a/src/vcpkg/packagespec.cpp b/src/vcpkg/packagespec.cpp
@@ -130,7 +130,16 @@ namespace vcpkg
     {
         auto parser = ParserBase(context, input, "<unknown>");
         auto maybe_pqs = parse_qualified_specifier(parser);
-        if (!parser.at_eof()) parser.add_error(msg::format(msgExpectedEof));
+        if (!parser.at_eof())
+        {
+            parser.add_error(msg::format(msgExpectedEof));
+        }
+
+        if (parser.any_errors())
+        {
+            maybe_pqs.clear();
+        }
+
         return maybe_pqs;
     }
 

diff --git a/src/vcpkg/paragraphs.cpp b/src/vcpkg/paragraphs.cpp
@@ -145,24 +145,28 @@ namespace vcpkg
     template<class T, class Message, class F>
     static Optional<std::vector<T>> parse_list_until_eof(Message bad_comma_message, ParserBase& parser, F f)
     {
-        std::vector<T> ret;
+        Optional<std::vector<T>> ret;
+        auto& vec = ret.emplace();
         parser.skip_whitespace();
         if (parser.at_eof()) return std::vector<T>{};
-        do
+        for (;;)
         {
             auto item = f(parser);
-            if (!item) return nullopt;
-            ret.push_back(std::move(item).value_or_exit(VCPKG_LINE_INFO));
+            if (!item) break;
+            vec.push_back(std::move(item).value_or_exit(VCPKG_LINE_INFO));
             parser.skip_whitespace();
-            if (parser.at_eof()) return {std::move(ret)};
+            if (parser.at_eof()) return ret;
             if (parser.cur() != ',')
             {
                 parser.add_error(msg::format(bad_comma_message));
-                return nullopt;
+                break;
             }
             parser.next();
             parser.skip_whitespace();
-        } while (true);
+        }
+
+        ret.clear();
+        return ret;
     }
 
     Optional<std::vector<std::string>> parse_default_features_list_context(DiagnosticContext& context,

diff --git a/src/vcpkg/sourceparagraph.cpp b/src/vcpkg/sourceparagraph.cpp
@@ -889,7 +889,8 @@ namespace vcpkg
             if (cur() == Unicode::end_of_file)
             {
                 add_error(msg::format(msgEmptyLicenseExpression));
-                return nullopt;
+                result.clear();
+                return result;
             }
 
             Expecting expecting = Expecting::License;