From 946e4030197cefb5d1518576ce0091e93b578d51 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Fri, 10 Jun 2022 09:15:53 +0200 Subject: Improve diagnostics: unable to negotiate --- bpkg/package-configuration.cxx | 149 ++++++++++++++++++++++++++++++++++------- bpkg/package-configuration.hxx | 32 ++++++++- 2 files changed, 154 insertions(+), 27 deletions(-) diff --git a/bpkg/package-configuration.cxx b/bpkg/package-configuration.cxx index 560fb3b..9375a7c 100644 --- a/bpkg/package-configuration.cxx +++ b/bpkg/package-configuration.cxx @@ -11,8 +11,8 @@ namespace bpkg { using build2::config::variable_origin; - string config_variable_value:: - serialize_cmdline () const + static string + serialize_cmdline (const string& name, const optional& value) { using namespace build2; @@ -35,6 +35,58 @@ namespace bpkg return r; } + + string config_variable_value:: + serialize_cmdline () const + { + return bpkg::serialize_cmdline (name, value); + } + + string dependent_config_variable_value:: + serialize_cmdline () const + { + return bpkg::serialize_cmdline (name, value); + } + + void package_configuration:: + print (diag_record& dr, + const char* indent, + const dependent_config_variable_values* ovrs) const + { + bool first (true); + for (const config_variable_value& v: *this) + { + if (v.origin != variable_origin::buildfile && + v.origin != variable_origin::override_) + continue; + + if (first) + first = false; + else + dr << '\n'; + + dr << indent; + + if (ovrs != nullptr && v.origin == variable_origin::buildfile) + { + if (const dependent_config_variable_value* ov = ovrs->find (v.name)) + { + dr << ov->serialize_cmdline () << " (set by " << ov->dependent << ')'; + continue; + } + } + + dr << v.serialize_cmdline () << " ("; + + if (v.origin == variable_origin::buildfile) + dr << "set by " << *v.dependent; + else + dr << "user configuration"; + + dr << ')'; + } + } + void to_checksum (sha256& cs, const config_variable_value& v) { @@ -194,7 +246,22 @@ namespace bpkg : dept.evaluate_prefer_accept (depc_cfgs, *da.prefer, *da.accept, pos.first))) { - fail << "unable to negotiate acceptable configuration"; // @@ TODO + diag_record dr (fail); + + dr << "unable to negotiate acceptable configuration with dependent " + << dept.key << " for dependencies "; + + for (size_t i (0); i != depcs.size (); ++i) + dr << (i == 0 ? "" : ", ") << depcs[i].get ().key; + + dr << info << "configuration before negotiation:\n"; + + // Note that we won't print this dependent's values (which we have unset + // above), but that seems like a good thing since they are not the cause + // of this impasse. + // + for (const package_configuration& cfg: depc_cfgs) + cfg.print (dr, " "); // Note 4 spaces since in nested info. } // Check if anything changed by comparing to entries in old_cfgs. @@ -214,20 +281,15 @@ namespace bpkg { if (v.origin == variable_origin::buildfile) { - auto i (find_if (old_cfgs.begin (), old_cfgs.end (), - [&v] (const dependent_config_variable_value& o) - { - return v.name == o.name; - })); - - if (i != old_cfgs.end ()) + if (const dependent_config_variable_value* ov = + old_cfgs.find (v.name)) { - if (i->value == v.value) + if (ov->value == v.value) { // If the value hasn't change, so shouldn't the originating // dependent. // - assert (i->dependent == *v.dependent); + assert (ov->dependent == *v.dependent); if (n) ++*n; @@ -236,7 +298,7 @@ namespace bpkg } else { - assert (i->dependent != *v.dependent); + assert (ov->dependent != *v.dependent); cycle = true; } } @@ -348,20 +410,59 @@ namespace bpkg } } - if (cycle) + if (!cycle) { - // @@ TODO - // - // Here we can analyze the O->N change history and determine the other - // problematic dependent(s). Do we actually know for sure they are all - // problematic? Well, they repeatedly changed the values so I guess so. - // - fail << "unable to negotiate acceptable configuration (cycle)"; + change_history.push_back (move (old_cfgs)); + change_history.push_back (move (new_cfgs)); + + return true; + } + + diag_record dr (fail); + + dr << "unable to negotiate acceptable configuration between dependents " + << dept.key; + + // Analyze the O->N changes and determine the problematic dependent(s). + // Do we actually know for sure they are all problematic? Well, they + // repeatedly changed the values to the ones we don't like, so I guess so. + // + small_vector, 1> depts; // Duplicates. + for (const dependent_config_variable_value& nv: new_cfgs) + { + if (nv.dependent == dept.key) + { + if (const dependent_config_variable_value* ov = old_cfgs.find (nv.name)) + { + if (ov->value != nv.value && ov->dependent != nv.dependent) + { + if (find_if (depts.begin (), depts.end (), + [ov] (reference_wrapper pk) + { + return ov->dependent == pk.get (); + }) == depts.end ()) + { + dr << ", " << ov->dependent; + depts.push_back (ov->dependent); + } + } + } + } } - change_history.push_back (move (old_cfgs)); - change_history.push_back (move (new_cfgs)); + dr << " for dependencies "; + + for (size_t i (0); i != depcs.size (); ++i) + dr << (i == 0 ? "" : ", ") << depcs[i].get ().key; + + dr << info << "configuration before negotiation:\n"; + for (const package_configuration& cfg: depc_cfgs) + cfg.print (dr, " ", &old_cfgs); + + dr << info << "configuration after negotiation:\n"; + for (const package_configuration& cfg: depc_cfgs) + cfg.print (dr, " "); - return true; + dr << endf; } } diff --git a/bpkg/package-configuration.hxx b/bpkg/package-configuration.hxx index 655b414..3ff7b59 100644 --- a/bpkg/package-configuration.hxx +++ b/bpkg/package-configuration.hxx @@ -80,6 +80,10 @@ namespace bpkg string name; optional value; package_key dependent; + + public: + string + serialize_cmdline () const; }; inline bool @@ -89,6 +93,22 @@ namespace bpkg return x.name == y.name && x.value == y.value && x.dependent == y.dependent; } + class dependent_config_variable_values: + public small_vector + { + public: + const dependent_config_variable_value* + find (const string& name) const + { + auto i (find_if (begin (), end (), + [&name] (const dependent_config_variable_value& v) + { + return v.name == name; + })); + return i != end () ? &*i : nullptr; + } + }; + class package_configuration: public vector { public: @@ -118,10 +138,16 @@ namespace bpkg })); return i != end () ? &*i : nullptr; } - }; - using dependent_config_variable_values = - small_vector; + // Print buildfile and override configuration variable values as command + // line overrides one per line with the specified indentation. After each + // variable also print in parenthesis its origin. If overrides is not + // NULL, then it is used to override the value/dependent information. + // + void + print (diag_record&, const char* indent, + const dependent_config_variable_values* overrides = nullptr) const; + }; class package_configurations: public small_vector { -- cgit v1.1