aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBoris Kolpackov <boris@codesynthesis.com>2022-06-09 08:58:12 +0200
committerBoris Kolpackov <boris@codesynthesis.com>2022-06-10 07:16:07 +0200
commitb9e8ca2f16bc06fdaa9b2b7c3bfffca01ec590ac (patch)
tree296472addaa10212a904ab852933a07dc23991a6
parent843627b2d3f801a7e9e45af0cc2cd4654077fac8 (diff)
Handle bogus configuration variables
-rw-r--r--bpkg/package-configuration.cxx54
-rw-r--r--bpkg/package-configuration.hxx24
-rw-r--r--bpkg/package-skeleton.cxx33
-rw-r--r--bpkg/pkg-build.cxx174
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 <bpkg/package-configuration.hxx>
+#include <sstream>
+
#include <bpkg/package-skeleton.hxx>
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<uint8_t> (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<package_key> 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<build2::names>& 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<build2::names>
@@ -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<size_t, size_t> 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<find_database_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<package_key> 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,