test-features: Suggest cascade lines with support expressions

azure-pipelines/end-to-end-tests-dir/test-features.ps1

@@ -148,6 +148,14 @@ $ciFeatureBaseline = "$PSScriptRoot/../e2e-assets/ci-feature-baseline/vcpkg-self
 Run-VcpkgAndCaptureOutput x-test-features @commonArgs $vcpkgSelfCascadePortsArg --all --ci-feature-baseline $ciFeatureBaseline
 Throw-IfFailed
 
+$ciFeatureBaseline = "$PSScriptRoot/../e2e-assets/ci-feature-baseline/empty-baseline.txt"
+$output = Run-VcpkgAndCaptureOutput x-test-features @commonArgs $vcpkgSelfCascadePortsArg --all --ci-feature-baseline $ciFeatureBaseline
+Throw-IfNotFailed
+Throw-IfNonContains -Expected @"
+note: consider adding the following line:
+vcpkg-self-cascade[cascade](!windows | windows) = cascade
+"@ -Actual $output
+
 # Ensure that the baseline without a feature can be overwritten by feature specific baselines.
 $ciFeatureBaseline = "$PSScriptRoot/../e2e-assets/ci-feature-baseline/vcpkg-fail-or-cascade.txt"
 $output = Run-VcpkgAndCaptureOutput x-test-features @commonArgs "--x-builtin-ports-root=$PSScriptRoot/../e2e-ports" vcpkg-fail-feature-cascades --ci-feature-baseline $ciFeatureBaseline

include/vcpkg/base/message-data.inc.h

@@ -2978,6 +2978,7 @@ DECLARE_MESSAGE(
     "",
     "{feature_spec} was unexpectedly a cascading failure because the following dependencies are unavailable:")
 DECLARE_MESSAGE(UnexpectedStateCascadePortNote, (), "", "consider changing this to =cascade instead")
+DECLARE_MESSAGE(UnexpectedStateCascadeSuggestLine, (), "", "consider adding the following line:")
 DECLARE_MESSAGE(UnexpectedSwitch,
                 (msg::option),
                 "Switch is a command line switch like --switch",

include/vcpkg/platform-expression.h

@@ -64,6 +64,10 @@ namespace vcpkg::PlatformExpression
 
         static const Expr always_true;
 
+        Expr& negate();
+
+        Expr& simplify();
+
     private:
         std::unique_ptr<detail::ExprImpl> underlying_;
     };

locales/messages.json

@@ -1553,6 +1553,7 @@
   "UnexpectedStateCascade": "{feature_spec} was unexpectedly a cascading failure because the following dependencies are unavailable:",
   "_UnexpectedStateCascade.comment": "An example of {feature_spec} is zlib[featurea,featureb].",
   "UnexpectedStateCascadePortNote": "consider changing this to =cascade instead",
+  "UnexpectedStateCascadeSuggestLine": "consider adding the following line:",
   "UnexpectedStateFailedCascade": "{feature_spec} build failed but was expected to be a cascaded failure",
   "_UnexpectedStateFailedCascade.comment": "An example of {feature_spec} is zlib[featurea,featureb].",
   "UnexpectedStateFailedNoteConsiderSkippingPort": "consider adding `{package_name}=fail`, or `{spec}=fail`, or equivalent skips",

src/vcpkg-test/platform-expression.cpp

@@ -570,3 +570,34 @@ TEST_CASE ("invalid alternate expressions", "[platform-expression]")
     CHECK_FALSE(parse_expr("not! windows"));
     CHECK_FALSE(parse_expr("notx64 windows"));
 }
+
+TEST_CASE ("negate expression", "[platform-expression]")
+{
+    auto m_expr = parse_expr("uwp & !xbox, (windows | osx)");
+    REQUIRE(m_expr);
+    m_expr.get()->negate();
+    to_string(*m_expr.get());
+
+    CHECK(to_string(*m_expr.get()) == "(!uwp | xbox) & (!windows & !osx)");
+}
+
+static std::string simplyfy(StringView expr)
+{
+    auto m_expr = parse_expr(expr);
+    REQUIRE(m_expr);
+    m_expr.get()->simplify();
+    return to_string(*m_expr.get());
+}
+
+TEST_CASE ("simplify expression", "[platform-expression]")
+{
+    CHECK(simplyfy("(uwp & (xbox & (uwp & xbox))) , !windows | (uwp, !windows)") == "(uwp & xbox), !windows, uwp");
+    CHECK(simplyfy("uwp & uwp") == "uwp");
+    CHECK(simplyfy("uwp , uwp | uwp") == "uwp");
+    CHECK(simplyfy("!uwp & !uwp") == "!uwp");
+    CHECK(simplyfy("uwp & (uwp & (windows & uwp))") == "uwp & windows");
+    CHECK(simplyfy("uwp | (uwp , (windows | uwp))") == "uwp | windows");
+    CHECK(simplyfy("!(!uwp)") == "uwp");
+    CHECK(simplyfy("!(uwp & uwp)") == "!uwp");
+    CHECK(simplyfy("!(!uwp & !uwp)") == "uwp");
+}

src/vcpkg/commands.test-features.cpp

@@ -176,7 +176,8 @@ namespace
                                             const FullPackageSpec& spec,
                                             const std::string* ci_feature_baseline_file_name,
                                             const CiFeatureBaselineEntry* baseline,
-                                            std::string&& cascade_reason)
+                                            std::string&& cascade_reason,
+                                            LocalizedString&& fix_msg = {})
     {
         auto outcome = expected_outcome(baseline, spec.features);
         switch (outcome.value)
@@ -192,6 +193,10 @@ namespace
             case CiFeatureBaselineOutcome::ConfigurationFail:
                 add_build_cascade_diagnostic(
                     diagnostics, spec, ci_feature_baseline_file_name, outcome.loc, std::move(cascade_reason));
+                if (!fix_msg.empty())
+                {
+                    diagnostics.push_back(DiagnosticLine{DiagKind::Note, std::move(fix_msg)});
+                }
                 break;
             case CiFeatureBaselineOutcome::PortMarkedFail:
             case CiFeatureBaselineOutcome::FeatureFail:
@@ -201,6 +206,10 @@ namespace
                                                      *ci_feature_baseline_file_name,
                                                      TextRowCol{outcome.loc.row, outcome.loc.column},
                                                      msg::format(msgUnexpectedStateCascadePortNote)});
+                if (!fix_msg.empty())
+                {
+                    diagnostics.push_back(DiagnosticLine{DiagKind::Note, std::move(fix_msg)});
+                }
                 break;
             case CiFeatureBaselineOutcome::PortMarkedCascade:
             case CiFeatureBaselineOutcome::FeatureCascade:
@@ -727,21 +736,50 @@ namespace vcpkg
             if (!install_plan.unsupported_features.empty())
             {
                 std::vector<std::string> out;
+                std::vector<PlatformExpression::Expr> exprs;
                 for (const auto& entry : install_plan.unsupported_features)
                 {
                     out.push_back(msg::format(msgOnlySupports,
                                               msg::feature_spec = entry.first,
                                               msg::supports_expression = to_string(entry.second))
                                       .extract_data());
+                    if (spec.kind != SpecToTestKind::Combined && ci_feature_baseline_file_name)
+                    {
+                        exprs.push_back(entry.second);
+                    }
+                }
+                LocalizedString fix_msg;
+                if (!exprs.empty())
+                {
+                    auto all_or = PlatformExpression::Expr::And(std::move(exprs)).negate().simplify();
+                    fix_msg = msg::format(msgUnexpectedStateCascadeSuggestLine).append_raw('\n');
+                    if (spec.kind == SpecToTestKind::Core)
+                    {
+                        fix_msg.append_raw(
+                            fmt::format("{}({}) = cascade", spec.package_spec.name(), to_string(all_or)));
+                    }
+                    else if (spec.kind == SpecToTestKind::Separate)
+                    {
+                        fix_msg.append_raw(fmt::format("{}[{}]({}) = cascade",
+                                                       spec.package_spec.name(),
+                                                       spec.separate_feature,
+                                                       to_string(all_or)));
+                    }
                 }
+
                 msg::print(msg::format(msgSkipTestingOfPort,
                                        msg::feature_spec = install_plan.install_actions.back().display_name(),
                                        msg::triplet = target_triplet)
                                .append_raw('\n')
                                .append_raw(Strings::join("\n", out))
                                .append_raw('\n'));
-                handle_cascade_feature_test_result(
-                    diagnostics, all_ports, spec, ci_feature_baseline_file_name, baseline, Strings::join(", ", out));
+                handle_cascade_feature_test_result(diagnostics,
+                                                   all_ports,
+                                                   spec,
+                                                   ci_feature_baseline_file_name,
+                                                   baseline,
+                                                   Strings::join(", ", out),
+                                                   std::move(fix_msg));
                 continue;
             }
 

src/vcpkg/platform-expression.cpp

@@ -1,3 +1,5 @@
+#include <vcpkg/base/checks.h>
+#include <vcpkg/base/lineinfo.h>
 #include <vcpkg/base/parse.h>
 #include <vcpkg/base/strings.h>
 #include <vcpkg/base/util.h>
@@ -124,6 +126,117 @@ namespace vcpkg::PlatformExpression
                 return std::make_unique<ExprImpl>(
                     ExprImpl{kind, identifier, Util::fmap(exprs, [](auto&& p) { return p->clone(); })});
             }
+
+            bool operator==(const ExprImpl& other) const noexcept
+            {
+                if (kind != other.kind) return false;
+                if (kind == ExprKind::identifier)
+                {
+                    return identifier == other.identifier;
+                }
+                return std::equal(exprs.begin(),
+                                  exprs.end(),
+                                  other.exprs.begin(),
+                                  other.exprs.end(),
+                                  [](const std::unique_ptr<detail::ExprImpl>& lhs,
+                                     const std::unique_ptr<detail::ExprImpl>& rhs) noexcept { return *lhs == *rhs; });
+            }
+
+            bool operator!=(const ExprImpl& other) const { return !(*this == other); }
+
+            void negate()
+            {
+                switch (kind)
+                {
+                    case ExprKind::identifier:
+                    {
+                        exprs.push_back(std::make_unique<ExprImpl>(std::move(*this)));
+                        kind = ExprKind::op_not;
+                        return;
+                    }
+                    case ExprKind::op_not:
+                    {
+                        auto sub_expr = std::move(*exprs.at(0));
+                        *this = std::move(sub_expr);
+                        return;
+                    }
+                    case ExprKind::op_and:
+                    case ExprKind::op_or:
+                    case ExprKind::op_list:
+                    {
+                        kind = (kind == ExprKind::op_and ? ExprKind::op_or : ExprKind::op_and);
+                        for (auto& expr : exprs)
+                        {
+                            expr->negate();
+                        }
+                        return;
+                    }
+                    case ExprKind::op_empty:
+                    case ExprKind::op_invalid: return;
+                }
+                Checks::unreachable(VCPKG_LINE_INFO);
+            }
+
+            static void add_if_not_contains(std::vector<std::unique_ptr<ExprImpl>>& exprs,
+                                            std::unique_ptr<ExprImpl>&& new_expr)
+            {
+                if (Util::all_of(exprs, [&](const auto& expr) { return *expr != *new_expr; }))
+                {
+                    exprs.push_back(std::move(new_expr));
+                }
+            }
+
+            static bool is_orish(ExprKind kind) { return kind == ExprKind::op_or || kind == ExprKind::op_list; }
+
+            void simplify()
+            {
+                switch (kind)
+                {
+                    case ExprKind::op_not:
+                    {
+                        exprs.at(0)->simplify();
+                        if (exprs.at(0)->kind == ExprKind::op_not)
+                        {
+                            auto sub_sub_expr = std::move(*exprs.at(0)->exprs.at(0));
+                            *this = std::move(sub_sub_expr);
+                            simplify();
+                        }
+                        return;
+                    }
+                    case ExprKind::op_and:
+                    case ExprKind::op_or:
+                    case ExprKind::op_list:
+                    {
+                        auto old_exprs = std::move(exprs);
+                        for (auto&& expr : old_exprs)
+                        {
+                            expr->simplify();
+                            if ((kind == ExprKind::op_and && expr->kind == ExprKind::op_and) ||
+                                (is_orish(kind) && is_orish(expr->kind)))
+                            {
+                                for (auto&& sub_expr : expr->exprs)
+                                {
+                                    add_if_not_contains(exprs, std::move(sub_expr));
+                                }
+                            }
+                            else
+                            {
+                                add_if_not_contains(exprs, std::move(expr));
+                            }
+                        }
+                        if (exprs.size() == 1)
+                        {
+                            auto sub_expr = std::move(*exprs[0]);
+                            *this = std::move(sub_expr);
+                        }
+                        return;
+                    }
+                    case ExprKind::identifier:
+                    case ExprKind::op_empty:
+                    case ExprKind::op_invalid: return;
+                }
+                Checks::unreachable(VCPKG_LINE_INFO);
+            }
         };
 
         struct ExpressionParser : ParserBase
@@ -666,6 +779,18 @@ namespace vcpkg::PlatformExpression
         return Impl{}(underlying_);
     }
 
+    Expr& Expr::negate()
+    {
+        underlying_->negate();
+        return *this;
+    }
+
+    Expr& Expr::simplify()
+    {
+        underlying_->simplify();
+        return *this;
+    }
+
     ExpectedL<Expr> parse_platform_expression(StringView expression, MultipleBinaryOperators multiple_binary_operators)
     {
         ExpressionParser parser(expression, multiple_binary_operators);
@@ -681,39 +806,11 @@ namespace vcpkg::PlatformExpression
 
     bool structurally_equal(const Expr& lhs, const Expr& rhs)
     {
-        struct Impl
-        {
-            bool operator()(const std::unique_ptr<detail::ExprImpl>& lhs,
-                            const std::unique_ptr<detail::ExprImpl>& rhs) const
-            {
-                return (*this)(*lhs, *rhs);
-            }
-            bool operator()(const detail::ExprImpl& lhs, const detail::ExprImpl& rhs) const
-            {
-                if (lhs.kind != rhs.kind) return false;
-
-                if (lhs.kind == ExprKind::identifier)
-                {
-                    return lhs.identifier == rhs.identifier;
-                }
-                else
-                {
-                    const auto& exprs_l = lhs.exprs;
-                    const auto& exprs_r = rhs.exprs;
-                    return std::equal(exprs_l.begin(), exprs_l.end(), exprs_r.begin(), exprs_r.end(), *this);
-                }
-            }
-        };
-
-        if (lhs.is_empty())
-        {
-            return rhs.is_empty();
-        }
-        if (rhs.is_empty())
+        if (lhs.underlying_ && rhs.underlying_)
         {
-            return false;
+            return *lhs.underlying_ == *rhs.underlying_;
         }
-        return Impl{}(lhs.underlying_, rhs.underlying_);
+        return !lhs.underlying_ && !rhs.underlying_;
     }
 
     int compare(const Expr& lhs, const Expr& rhs)