From b9e8ca2f16bc06fdaa9b2b7c3bfffca01ec590ac Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Thu, 9 Jun 2022 08:58:12 +0200 Subject: Handle bogus configuration variables --- bpkg/package-configuration.cxx | 54 ++++++++++++- bpkg/package-configuration.hxx | 24 ++++++ bpkg/package-skeleton.cxx | 33 ++------ bpkg/pkg-build.cxx | 174 ++++++++++++++++++++++++++++++++++++++--- 4 files changed, 244 insertions(+), 41 deletions(-) diff --git a/bpkg/package-configuration.cxx b/bpkg/package-configuration.cxx index 624de18..560fb3b 100644 --- a/bpkg/package-configuration.cxx +++ b/bpkg/package-configuration.cxx @@ -3,12 +3,62 @@ #include +#include + #include namespace bpkg { using build2::config::variable_origin; + string config_variable_value:: + serialize_cmdline () const + { + using namespace build2; + + string r (name + '='); + + if (!value) + r += "[null]"; + else + { + if (!value->empty ()) + { + // Note: we need to use command-line (effective) quoting. + // + ostringstream os; + to_stream (os, *value, quote_mode::effective, '@'); + r += os.str (); + } + } + + return r; + } + + void + to_checksum (sha256& cs, const config_variable_value& v) + { + using namespace build2; + + cs.append (v.name); + cs.append (static_cast (v.origin)); + if (v.type) + cs.append (*v.type); + + if (v.origin != variable_origin::undefined) + { + if (v.value) + for (const name& n: *v.value) + to_checksum (cs, n); + + if (v.origin == variable_origin::buildfile) + { + cs.append (v.dependent->string ()); + cs.append (v.confirmed); + } + } + } + bool up_negotiate_configuration ( package_configurations& cfgs, @@ -118,9 +168,7 @@ namespace bpkg // Note that we will not reload it to default in case of require. // - v.origin = variable_origin::undefined; - v.value = nullopt; - v.dependent = nullopt; + v.undefine (); } else old_cfgs.push_back ( diff --git a/bpkg/package-configuration.hxx b/bpkg/package-configuration.hxx index d6fc989..655b414 100644 --- a/bpkg/package-configuration.hxx +++ b/bpkg/package-configuration.hxx @@ -46,8 +46,32 @@ namespace bpkg // first set this variable to this value. // optional dependent; + + // If origin is buildfile, then this flag indicates whether the + // originating dependent has been encountered during the negotiation + // retry. + // + bool confirmed; + + public: + void + undefine () + { + origin = build2::config::variable_origin::undefined; + value = nullopt; + dependent = nullopt; + confirmed = false; + } + + // Serialize the variable value as a command line override. + // + string + serialize_cmdline () const; }; + void + to_checksum (sha256&, const config_variable_value&); + // A subset of config_variable_value for variable values set by the // dependents (origin is buildfile). Used to track change history. // diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx index 4813651..eaebf26 100644 --- a/bpkg/package-skeleton.cxx +++ b/bpkg/package-skeleton.cxx @@ -262,30 +262,6 @@ namespace bpkg return r; } - static string - serialize_cmdline (const string& var, const optional& val) - { - using namespace build2; - - string r (var + '='); - - if (!val) - r += "[null]"; - else - { - if (!val->empty ()) - { - // Note: we need to use command-line (effective) quoting. - // - ostringstream os; - to_stream (os, *val, quote_mode::effective, '@'); - r += os.str (); - } - } - - return r; - } - // Reverse value to names. // static optional @@ -317,7 +293,7 @@ namespace bpkg for (const config_variable_value& v: cfg) { if (v.origin == variable_origin::buildfile) - r.push_back (serialize_cmdline (v.name, v.value)); + r.push_back (v.serialize_cmdline ()); } return r; @@ -411,7 +387,7 @@ namespace bpkg case variable_origin::override_: case variable_origin::undefined: { - config_variable_value v {var.name, ol.first, {}, {}, {}}; + config_variable_value v {var.name, ol.first, {}, {}, {}, false}; // Override could mean user override from config_vars_ or the // dependent override that we have merged above. @@ -424,6 +400,7 @@ namespace bpkg v.origin = variable_origin::buildfile; v.dependent = move (ov->dependent); + v.confirmed = true; } } @@ -1171,6 +1148,7 @@ namespace bpkg v.value = move (ns); v.dependent = key; // We are the originating dependent. + v.confirmed = true; break; } case variable_origin::default_: @@ -1448,6 +1426,7 @@ namespace bpkg v.value = move (ns); v.dependent = key; // We are the originating dependent. + v.confirmed = true; } } } @@ -1792,7 +1771,7 @@ namespace bpkg for (const config_variable_value& v: cfg) { if (v.origin == variable_origin::override_) - dependency_vars.push_back (serialize_cmdline (v.name, v.value)); + dependency_vars.push_back (v.serialize_cmdline ()); } } diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx index 63bbc36..296a41c 100644 --- a/bpkg/pkg-build.cxx +++ b/bpkg/pkg-build.cxx @@ -2073,7 +2073,9 @@ namespace bpkg { size_t depth; package_key dependent; +#if 0 // @@ TMP pair position; +#endif }; struct merge_configuration @@ -4624,7 +4626,7 @@ namespace bpkg // dep_chain.clear (); - throw retry_configuration {cfg.depth, move (pk), dp}; + throw retry_configuration {cfg.depth, move (pk)/*, dp*/}; // @@ TMP } l5 ([&]{trace << "configuration for cfg-postponed " @@ -5570,6 +5572,7 @@ namespace bpkg postponed_packages& postponed_alts, postponed_dependencies& postponed_deps, postponed_configurations& postponed_cfgs, + strings& postponed_cfgs_history, postponed_positions& postponed_poss, const function& fdb, const repointed_dependents& rpt_depts, @@ -5578,6 +5581,8 @@ namespace bpkg { // Snapshot of the package builds collection state. // + // Note: should not include postponed_cfgs_history. + // class snapshot { public: @@ -6440,6 +6445,7 @@ namespace bpkg postponed_alts, postponed_deps, postponed_cfgs, + postponed_cfgs_history, postponed_poss, fdb, rpt_depts, @@ -6507,7 +6513,7 @@ namespace bpkg break; } - catch (retry_configuration& e) + catch (const retry_configuration& e) { // If this is not "our problem", then keep looking. // @@ -6529,21 +6535,32 @@ namespace bpkg pc = &postponed_cfgs[ci]; + // @@ TMP: "shadow" seems no longer correct. + // l5 ([&]{trace << "cfg-negotiation of " << *pc << " failed due " << "to dependent " << e.dependent << ", adding " << "shadow dependent and re-negotiating";}); - // @@ TODO - // - // Note that there is also a possibility of having a "bogus" - // dependent/dependencies (i.e., we add them to the cluster but - // they never get re-visited). While seems harmless (or not), - // maybe we need to prune the part of the configuration that was - // added by them? Maybe we should have the confirmed flag which - // is set when the originating dependent confirms the value?! - // Copy over the configuration for further refinement. // + // Note that there is also a possibility of ending up with + // "bogus" configuration variables that were set by a dependent + // during up-negotiation but, due to changes to the overall + // configuration, such a dependent were never re-visited. + // + // The way we are going to deal with this is by detecting such + // bogus variables based on the confirmed flag, cleaning them + // out, and doing another retry. Here we clear the confirmed + // flag and the detection happens in collect_build_postponed() + // after we have processed everything postponed (since that's + // the only time we can be certain there could no longer be a + // re-visit). + // + for (package_configuration& cfg: cfgs) + for (config_variable_value& v: cfg) + if (v.dependent) + v.confirmed = false; + pc->dependency_configurations = move (cfgs); #if 0 @@ -6810,6 +6827,139 @@ namespace bpkg postponed_deps.cancel_bogus (trace, false /* initial_collection */); } + // Check if any negotiatiated configurations ended up with any bogus + // variables (see retry_configuration catch block for background). + // + // Note that we could potentially end up yo-yo'ing: we remove a bogus + // and that causes the original dependent to get re-visited which in + // turn re-introduces the bogus. In other words, one of the bogus + // variables which we have removed are actually the cause of no longer + // needing the dependent that introduced it. Feels like the correct + // outcome of this should be keeping the bogus variable that triggered + // yo-yo'ing. Of course, there could be some that we should keep and + // some that we should drop and figuring this out would require retrying + // all possible combinations. An alternative solution would be to detect + // yo-yo'ing, print the bogus variables involved, and ask the user to + // choose (with an override) which ones to keep. Let's go with this for + // now. + // + { + // On the first pass see if we have anything bogus. + // + bool bogus (false); + for (postponed_configuration& pcfg: postponed_cfgs) + { + if (pcfg.negotiated && *pcfg.negotiated) // Negotiated. + { + for (package_configuration& cfg: pcfg.dependency_configurations) + { + for (config_variable_value& v: cfg) + { + if (v.dependent && !v.confirmed) + { + bogus = true; + break; + } + } + if (bogus) break; + } + if (bogus) break; + } + } + + if (bogus) + { + // On the second pass calculate the checksum of all the negotiated + // clusters. + // + sha256 cs; + for (postponed_configuration& pcfg: postponed_cfgs) + { + if (pcfg.negotiated && *pcfg.negotiated) + { + for (package_configuration& cfg: pcfg.dependency_configurations) + { + for (config_variable_value& v: cfg) + { + if (v.dependent) + to_checksum (cs, v); + } + } + } + } + + bool cycle; + { + string s (cs.string ()); + if (find (postponed_cfgs_history.begin (), + postponed_cfgs_history.end (), + s) == postponed_cfgs_history.end ()) + { + postponed_cfgs_history.push_back (move (s)); + cycle = false; + } + else + cycle = true; + } + + // On the third pass we either retry or diagnose. + // + diag_record dr; + if (cycle) + { + dr << + fail << "unable to remove bogus configuration values without " + << "causing configuration refinement cycle" << + info << "consider manually specifying one or more of the " + << "following variables as user configuration"; + } + + for (postponed_configuration& pcfg: postponed_cfgs) + { + optional dept; // Bogus dependent. + + if (pcfg.negotiated && *pcfg.negotiated) + { + for (package_configuration& cfg: pcfg.dependency_configurations) + { + // Note that the entire dependency configuration may end up + // being "bogus" (i.e., it does not contain any configuration + // variables with a confirmed dependent). But that will be + // handled naturally: we will either no longer have this + // dependency in the cluster and thus never call its + // skeleton's dependent_config() or this call will be no-op + // since it won't find any dependent variables. + // + for (config_variable_value& v: cfg) + { + if (v.dependent && !v.confirmed) + { + if (!dept) + dept = move (v.dependent); + + if (cycle) + dr << info << v.serialize_cmdline (); + else + v.undefine (); + } + } + } + + if (dept) + { + if (cycle) + break; + else + throw retry_configuration {pcfg.depth, move (*dept)}; + } + } + + if (dept) + break; + } + } + } + // If any postponed_{repo,alts} builds remained, then perform the // diagnostics run. Naturally we shouldn't have any postponed_cfgs // without one of the former. @@ -10826,6 +10976,7 @@ namespace bpkg postponed_packages postponed_repo; postponed_packages postponed_alts; postponed_configurations postponed_cfgs; + strings postponed_cfgs_history; try { @@ -11099,6 +11250,7 @@ namespace bpkg postponed_alts, postponed_deps, postponed_cfgs, + postponed_cfgs_history, postponed_poss, find_prereq_database, rpt_depts, -- cgit v1.1