aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBoris Kolpackov <boris@codesynthesis.com>2022-05-06 14:02:14 +0200
committerBoris Kolpackov <boris@codesynthesis.com>2022-05-06 14:02:14 +0200
commitc9140670af0361bf89c752115515257b44c3bee0 (patch)
treee38d3a589035d155979dbca75991c751458f212d
parent7609921b4c3cf29e1679b96f2ff31066681f0d5a (diff)
Review
-rw-r--r--bpkg/pkg-build.cxx201
1 files changed, 169 insertions, 32 deletions
diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index 5065641..9725d63 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -1357,10 +1357,17 @@ namespace bpkg
// Merging negotiated configurations results in a non-negotiated one.
//
+ // @@ On the shadow dependent perhaps, but probably not on the shadow
+ // cluster.
+ //
negotiated = nullopt;
// Merge shadow dependents.
//
+ // This is only necessary if all the merged non-shadow configurations
+ // have already been negotiated (otherwise we throw and replace the
+ // shadow with newly merged non-shadow).
+ //
for (auto& d: shadow_dependents)
{
for (dependency& p: d.second.dependencies)
@@ -3637,16 +3644,30 @@ namespace bpkg
postponed_cfgs);
}
- // Postpone a dependent that has configuration clauses and the
- // postponed dependencies.
- //
- // Note that such a dependent will be recursively recollected right
- // after the configuration negotiation.
- //
- // @@ Is the above comment still accurate?
+ // If this dependent has any dependencies with configruations
+ // clauses, then we need to deal with that.
//
if (!cfg_deps.empty ())
{
+ // As a first step add this dependent/dependencies to one of the
+ // new/existing postponed_configuration clusters, which could
+ // potentially cause some of them to be merged. Here are the
+ // possibilities and what we should do in each case.
+ //
+ // 1. Got added to a new cluster -- this dependent got postponed
+ // and we return false.
+ //
+ // 2. Got added to an existing non-yet-negotiated cluster (which
+ // could potentially involve merging a bunch of them) -- ditto.
+ //
+ // 3. Got added to an existing already-[being]-negotiated cluster
+ // (which could potentially involve merging a bunch of them,
+ // some negotiated, some being negotiated, and some not yet
+ // negotiated) -- see below logic.
+ //
+ // Note that if a dependent is postponed, it will be recursively
+ // recollected right after the configuration negotiation.
+
// Note: don't move the argument from since may be needed for
// constructing exception.
//
@@ -3662,24 +3683,100 @@ namespace bpkg
//
postponed_configuration& cfg (r.first);
- if (cfg.depth != 0)
+ if (cfg.depth == 0)
+ return false; // Cases (1) or (2).
+ else
{
- l5 ([&]{trace << "adding cfg-postponing dependent "
- << pkg.available_name_version_db ()
- << " involves negotiated configurations and "
- << "results in " << cfg
- << ", throwing retry_configuration";});
-
- // @@ When we get here after retry_configuration was handled
- // we throw merge_configuration instead if r.second is false.
+ // Cases (3).
//
- throw retry_configuration {cfg.depth,
- move (cp),
- di + 1,
- move (cfg_deps)};
- }
+ // If this is the first time we are here (see below), then we
+ // up-negotiate the configuration and throw retry_configuration
+ // to the try/catch frame corresponding to the negotiation of
+ // the outermost merged cluster in order to retry the same steps
+ // (potentially refining the configuration as we go along) and
+ // likely (but not necessarily) ending up here again, at which
+ // point we do not need to re-do the up-negotiation and can just
+ // merge this dependent/dependency into the cluster. To
+ // distinguish these "first time"/"second time" cases the
+ // cluster contains the shadow dependents map (see the catch
+ // site for details).
+ //
+ // @@ Feels like we need dependency alternative position rather
+ // that cfg_deps to track this.
+ //
+ if (!cfg.contains_shadow (cp, di + 1, cfg_deps))
+ {
+ // The "first time" case.
+ //
+ l5 ([&]{trace << "adding cfg-postponing dependent "
+ << pkg.available_name_version_db ()
+ << " involves negotiated configurations and "
+ << "results in " << cfg
+ << ", throwing retry_configuration";});
- return false;
+ // up_negotiate (...);
+
+ throw retry_configuration {
+ cfg.depth, move (cp), di + 1, move (cfg_deps)};
+ }
+ else
+ {
+ // The "second time" case.
+ //
+ // There is just one complication: If all the merged clusters
+ // are already negotiated, then all is good: all the
+ // dependencies in cfg_deps have been collected recursively as
+ // part of the configuration negotiation (because everything
+ // in this cluster is already negotiated) and we can return
+ // true (no need to postpone any further steps).
+ //
+ // But if we do have clusters not yet negotiated, or, worse,
+ // being in the middle of negotiation, then we need to get
+ // this cluster into the fully negotiated state.
+ //
+ if (r.second)
+ return true; // The everything already negotiated case.
+ else
+ {
+ // The partially negotiated case.
+ //
+ // Handling this in a straightforward way is not easy due to
+ // the being negotiated cases -- we have code up the stack
+ // that is in the middle of the negotiation logic.
+ //
+ // Another idea is to again throw to the outer try/catch
+ // frame (thus unwinding all the being negotiated code) and
+ // complete the work there. The problem with this approach
+ // is that without restoring the state we may end up with
+ // unrelated clusters that will have no corresponding
+ // try-catch frames (because we may unwind them in the
+ // process).
+ //
+ // So the approach we will use is the "shadow" idea but for
+ // merging clusters rather than up-negotiating a
+ // configuration. Specifically, we throw retry_configuration
+ // to the outer try/catch. At the catch site we make the
+ // newly merged cluster a shadow of the restored cluster and
+ // retry the same steps similar to retry_configuration. As
+ // we redo these steps, we consult the shadow cluster and if
+ // the dependent/dependency entry is there, then instead of
+ // adding it to another (new/exsiting) cluster that would
+ // later be merged into this non-shadow cluster, we add it
+ // directly to the non-shadow cluster (potentially merging
+ // other cluster which it feels like by definition should
+ // all be already fully negotiated). The end result is that
+ // once we reach this point again, there will be nothing to
+ // merge.
+ //
+ // The shadow check is part of postponed_configs::add().
+ //
+ // @@ Add assert() that shadow-based merge always results in
+ // fully negotiated cluster.
+ //
+ throw retry_configuration {cfg.depth};
+ }
+ }
+ }
}
return true;
@@ -4374,6 +4471,9 @@ namespace bpkg
postponed_configurations postponed_cfgs_;
};
+ // @@ Is this logic correct: can we end up revisiting the same depth
+ // (seems so) and if so, is it harmless (likely so).
+ //
size_t depth (pcfg != nullptr ? pcfg->depth : 0);
string t ("collect_build_postponed (" + to_string (depth) + ")");
@@ -4511,6 +4611,13 @@ namespace bpkg
// @@ Negotiate configuration.
+ /*
+ for (...)
+ {
+ // up_negotiate (...);
+ }
+ */
+
// Being negotiated (so can only be up-negotiated).
//
pcfg->negotiated = false;
@@ -4708,6 +4815,8 @@ namespace bpkg
if (e.depth != pcd)
throw;
+ // Restore the state from snapshot.
+ //
// Note: postponed_cfgs is re-assigned.
//
s.restore (*this,
@@ -4722,21 +4831,49 @@ namespace bpkg
<< "dependent " << e.dependent
<< ", re-negotiating";});
+ // Record the dependent/dependencies/position that triggered this
+ // so that we don't repeat this up-negotiate/throw/catch dance
+ // when we retry collect_build_postponed().
+ //
+ // 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, maybe we
+ // need to prune the part of the configuration that was added
+ // by them?
+ //
pc->add_shadow (move (e.dependent),
e.position,
move (e.dependencies));
+ }
+ catch (merge_configuration& e)
+ {
+ // If this is not "our problem", then keep looking.
+ //
+ if (e.depth != pcd)
+ throw;
- // @@ TODO: we need to somehow record the dependent/dependencies
- // that triggered this (included in the exception, presumably)
- // so that we don't repeat this up-negotiate/throw/catch dance.
- // Note clear if we should just add it to the cluster (we could
- // do it with proper depends position, etc) or somewhere on the
- // side. We will also need to have a corresponding check in the
- // throw side when we re-visit.
+ postponed_configuration shadow (move (*pc));
+
+ // Restore the state from snapshot.
+ //
+ // Note: postponed_cfgs is re-assigned.
//
- // But 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).
+ s.restore (*this,
+ postponed_repo,
+ postponed_alts,
+ postponed_deps,
+ postponed_cfgs);
+
+ pc = &postponed_cfgs[i];
+
+ // @@ TODO
+ /*
+ l5 ([&]{trace << "cfg-negotiation of " << *pc << " failed due to "
+ << "dependent " << e.dependent
+ << ", re-negotiating";});
+ */
+
+ pc->shadow = move (shadow); // @@ Try optional<>.
}
}