aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorKaren Arutyunov <karen@codesynthesis.com>2022-05-31 22:10:57 +0300
committerKaren Arutyunov <karen@codesynthesis.com>2022-06-02 15:25:09 +0300
commit3ce11cb9ae8caec98f0530181395307307eebf4e (patch)
treee68f7473bf5b5f0e04f8b8eb7eccfbf4d945a9a8
parent75241c122c96163d81811f3f3189093f79b0dbe9 (diff)
Review/complete documentation plus make some minor code cleanups
-rw-r--r--bpkg/pkg-build.cxx509
1 files changed, 266 insertions, 243 deletions
diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index c1cbffe..fcf6cac 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -532,10 +532,12 @@ namespace bpkg
// any version constraints. Specifically, during this step, we may
// "upgrade" or "downgrade" a package that is already in a map as a
// result of another package depending on it and, for example, requiring
- // a different version. One notable side-effect of this process is that
- // we may end up with a lot more packages in the map (but not in the list)
- // than we will have on the list. This is because some of the prerequisites
- // of "upgraded" or "downgraded" packages may no longer need to be built.
+ // a different version. If that happens, we make sure that the replaced
+ // package version doesn't apply constraints and/or configuration to its
+ // own dependencies anymore and also its non-shared dependencies are
+ // gone from the map, recursively (see replaced_versions for more details).
+ // One notable side-effect of this process is that all the packages in the
+ // map end up in the list.
//
// Note that we don't try to do exhaustive constraint satisfaction (i.e.,
// there is no backtracking). Specifically, if we have two candidate
@@ -609,7 +611,7 @@ namespace bpkg
//
// Initially nullopt. Can be filled partially if the package prerequisite
// builds collection is postponed for any reason (see postponed_packages
- // for possible reasons).
+ // and postponed_configurations for possible reasons).
//
optional<bpkg::dependencies> dependencies;
@@ -630,9 +632,10 @@ namespace bpkg
// If the package prerequisite builds collection is postponed, then this
// member stores the references to the enabled alternatives (in available
- // package) of a dependency being the cause of the postponement. This, in
- // particular, allows not to re-evaluate conditions multiple times on the
- // re-collection attempts.
+ // package) of a dependency being the cause of the postponement together
+ // with their original indexes in the respective dependency alternatives
+ // list. This, in particular, allows not to re-evaluate conditions
+ // multiple times on the re-collection attempts.
//
// Note: it shouldn't be very common for a dependency to contain more than
// two true alternatives.
@@ -744,8 +747,9 @@ namespace bpkg
// recursively.
//
// This is required if it is being built as a source package and needs to
- // be up/down-graded and/or reconfigured, it is a repointed dependent, or
- // it is already in the process of being collected.
+ // be up/down-graded and/or reconfigured and has some buildfile clauses,
+ // it is a repointed dependent, or it is already in the process of being
+ // collected.
//
bool
recollect_recursively (const repointed_dependents& rpt_depts) const
@@ -757,14 +761,12 @@ namespace bpkg
selected->state == package_state::configured &&
selected->substate != package_substate::system);
- package_key pk (db, name ());
-
return !system &&
(dependencies ||
selected->version != available_version () ||
(!config_vars.empty () &&
has_buildfile_clause (available->dependencies)) ||
- rpt_depts.find (pk) != rpt_depts.end ());
+ rpt_depts.find (package_key (db, name ())) != rpt_depts.end ());
}
// State flags.
@@ -1051,6 +1053,8 @@ namespace bpkg
// order not prevent, for example, system to non-system upgrade.
}
+ // Initialize the skeleton of a being built package.
+ //
package_skeleton&
init_skeleton (const common_options& options)
{
@@ -1119,6 +1123,16 @@ namespace bpkg
// same) and replacing the package drop with a package version build can
// always been handled in-place.
//
+ // On the first glance, the map entries which have not been used for
+ // replacement during the package collection (bogus entries) are harmless
+ // and can be ignored. However, the dependency configuration negotiation
+ // machinery refers to this map and skips existing dependents with
+ // configuration clause which belong to it (see query_existing_dependents()
+ // for details). Thus, if after the package collection some bogus entries
+ // are present in the map, then it means that we could have erroneously
+ // skipped some existing dependents because of them and so need to erase
+ // these entries and re-collect.
+ //
struct replaced_version
{
// Desired package version, repository fragment, and system flag.
@@ -1209,9 +1223,9 @@ namespace bpkg
// End up in the following clusters (see string() below for the cluster
// representation):
//
- // {foo bar | libfoo->{foo/1 bar/1}}
- // {bar | libbar->{bar/2}}
- // {baz | libbaz->{baz/1}}
+ // {foo bar | libfoo->{foo/1,1 bar/1,1}}
+ // {bar | libbar->{bar/2,1}}
+ // {baz | libbaz->{baz/1,1}}
//
// Or, another example:
//
@@ -1219,11 +1233,14 @@ namespace bpkg
// bar: depends: libfoo libbar
// baz: depends: libbaz
//
- // {foo bar | libfoo->{foo/1 bar/1} libbar->{bar/1}}
- // {baz | libbaz->{baz/1}}
+ // {foo bar | libfoo->{foo/1,1 bar/1,1} libbar->{bar/1,1}}
+ // {baz | libbaz->{baz/1,1}}
//
- // Note that a dependent can belong to any given cluster with only one
- // `depends` position.
+ // Note that a dependent can belong to any given non-negotiated cluster with
+ // only one `depends` position. However, if some dependency configuration is
+ // up-negotiated for a dependent, then multiple `depends` positions will
+ // correspond to this dependent in the same cluster. Naturally, such
+ // clusters are always (being) negotiated.
//
// Note that adding new dependent/dependencies to the postponed
// configurations can result in merging some of the existing clusters if the
@@ -1235,8 +1252,8 @@ namespace bpkg
// to the clusters in the second example will merge them into a single
// cluster:
//
- // {foo bar baz fox | libfoo->{foo/1 bar/1} libbar->{bar/1 fox/1}
- // libbaz->{baz/1 fox/1}}
+ // {foo bar baz fox | libfoo->{foo/1,1 bar/1,1} libbar->{bar/1,1 fox/1,1}
+ // libbaz->{baz/1,1 fox/1,1}}
//
// Also note that we keep track of packages which turn out to be
// dependencies of existing (configured) dependents with configuration
@@ -1361,6 +1378,8 @@ namespace bpkg
packages ({move (dep)}));
}
+ // Add dependencies of a dependent.
+ //
// Note: adds the specified dependencies to the end of the configuration
// dependencies list suppressing duplicates.
//
@@ -1400,7 +1419,7 @@ namespace bpkg
}
}
- // Return true if any of the new or existing dependents depend on the
+ // Return true if any of the configuration's dependents depend on the
// specified package.
//
bool
@@ -1625,7 +1644,7 @@ namespace bpkg
//
// For example:
//
- // {foo^ bar | libfoo->{foo/1 bar/1} libbar->{bar/1}}!
+ // {foo^ bar | libfoo->{foo/2,3 bar/1,1} libbar->{bar/1,1}}!
//
std::string
string () const
@@ -1725,8 +1744,8 @@ namespace bpkg
public:
// Return the configuration the dependent is added to (after all the
// potential configuration merges, etc). Also return in second absent if
- // the merge happenned due to the shadow cluster logic (in which case the
- // cluster was/is being negotitated) and true/false indicating whether all
+ // the merge happened due to the shadow cluster logic (in which case the
+ // cluster was/is being negotiated) and true/false indicating whether all
// the merge-involved configurations are negotiated (the negotiated member
// is true for all of them).
//
@@ -2052,7 +2071,9 @@ namespace bpkg
size_t next_id_ = 1;
};
- // @@ TODO: describe.
+ // The following exceptions are used by the dependency configuration
+ // up-negotiation/refinement machinery. See the collect lambda in
+ // collect_build_prerequisites() for details.
//
struct retry_configuration
{
@@ -2093,8 +2114,8 @@ namespace bpkg
//
// Note that the latter kind of dependent is what eventually causes
// recursive processing of the dependency packages. Which means we must
- // watch out for bogus entries in this map which feels like we may still end
- // up with (e.g., because postponement caused cross-talk between dependency
+ // watch out for bogus entries in this map which we may still end up with
+ // (e.g., because postponement caused cross-talk between dependency
// alternatives). Thus we keep flags that indicate whether we have seen each
// type of dependent and then just process dependencies that have the first
// (without config) but not the second (with config). We also need to track
@@ -2167,15 +2188,22 @@ namespace bpkg
}
};
- // @@ TODO Describe.
+ // Map of existing dependents which need to be re-evaluated to the position
+ // with the dependency index not greater than the specified one.
//
- // This mechanism only applies to handling of existing dependents where we
- // may re-evaluate them to a certain position but later realize we've gone
- // too far.
+ // This mechanism applies when we re-evaluate an existing dependent to a
+ // certain position but later realize we've gone too far. In this case we
+ // note the earlier position information and re-collect from scratch. On the
+ // re-collection any re-evaluation of the dependent to a greater position
+ // will be either skipped or performed but to this earlier position (see the
+ // replace member for details).
//
// We consider the postponement bogus if some dependent re-evaluation was
- // skipped due its presence but no re-evaluation to this (or earlier)
- // dependency index was performed.
+ // skipped due to its presence but no re-evaluation to this (or earlier)
+ // dependency index was performed. Thus, if after the package collection
+ // some bogus entries are present in the map, then it means that we have
+ // skipped the respective re-evaluations erroneously and so need to erase
+ // these entries and re-collect.
//
// Note that if no re-evaluation is skipped due to this postponement then it
// is harmless and we don't consider it bogus.
@@ -2187,8 +2215,8 @@ namespace bpkg
// position is encountered while processing the same cluster as what
// contains the later position. In this case, if we merely skip, then we
// will never naturally encounter the earlier position. So we have to
- // force the issue (even if things change enough for us to never see
- // the later position again).
+ // force the issue (even if things change enough for us to never see the
+ // later position again).
//
bool replace;
@@ -2213,8 +2241,7 @@ namespace bpkg
//
bool replace = false;
- // Erase the bogus postponements and, if any, throw cancel_postponement,
- // if requested.
+ // Erase the bogus postponements and throw cancel_postponement, if any.
//
struct cancel_postponement: scratch_collection
{
@@ -2257,11 +2284,6 @@ namespace bpkg
}
};
- struct postpone_position: scratch_collection
- {
- postpone_position (): scratch_collection ("earlier dependency position") {}
- };
-
struct build_packages: build_package_list
{
build_packages () = default;
@@ -2362,7 +2384,8 @@ namespace bpkg
// Collect the package being built. Return its pointer if this package
// version was, in fact, added to the map and NULL if it was already there
- // or the existing version was preferred. So can be used as bool.
+ // and the existing version was preferred or if the package build has been
+ // replaced with the drop. So can be used as bool.
//
// Consult replaced_vers for an existing version replacement entry and
// follow it, if present, potentially collecting the package drop
@@ -2504,16 +2527,11 @@ namespace bpkg
{
build_package& bp (i->second.package);
- // Can't think of the scenario when this happens. We would start
- // collecting from scratch (see below).
- //
-
// Note that we used to think that the scenario when the build could
// replace drop could never happen since we would start collecting
// from scratch. This has changed when we introduced replaced_versions
// for collecting drops.
//
- //
if (bp.action && *bp.action == build_package::drop) // Drop.
{
bp = move (pkg);
@@ -2601,10 +2619,9 @@ namespace bpkg
<< " over " << p2->available_name_version_db ();});
}
- // See if we are replacing the object. If not, then we don't
- // need to collect its prerequisites since that should have
- // already been done. Remember, p1 points to the object we
- // want to keep.
+ // See if we are replacing the object. If not, then we don't need to
+ // collect its prerequisites since that should have already been
+ // done. Remember, p1 points to the object we want to keep.
//
bool replace (p1 != &i->second.package);
@@ -2771,6 +2788,9 @@ namespace bpkg
// and, potentially, for its reconfigured existing prerequisites,
// recursively.
//
+ // - For an existing dependent being re-evaluated to the specific
+ // dependency position.
+ //
// Note that for these cases, as it was said above, we can potentially
// fail if the dependent is an orphan, but this is exactly what we need to
// do in that case, since we won't be able to re-collect its dependencies.
@@ -2793,13 +2813,15 @@ namespace bpkg
// postponed_dependencies for details) and also recording the dependent in
// postponed_cfgs (see postponed_configurations for details). If it turns
// out that some dependency of such a dependent has already been collected
- // (via some other dependent without configuration clauses) or
- // configuration for a dependency has already been negotiated (between
- // some other dependents), then throw the postpone_dependency
- // exception. The caller normally handles this exception by rolling back
- // to some previous collection state and recollecting packages, but now
- // with the knowledge about premature dependency collection or premature
- // configuration negotiation.
+ // via some other dependent without configuration clauses, then throw the
+ // postpone_dependency exception. This exception is handled by
+ // re-collecting packages from scratch, but now with the knowledge about
+ // premature dependency collection. If it turns out that some dependency
+ // configuration has already been negotiated between some other
+ // dependents, then up-negotiate the configuration and throw
+ // retry_configuration exception so that the configuration refinement can
+ // be performed (see the collect lambda implementation for details on the
+ // configuration refinement machinery).
//
// If the package is a dependency of a configured dependent with
// configuration clause and needs to be reconfigured (being upgraded, has
@@ -2808,13 +2830,9 @@ namespace bpkg
// an existing dependent (see postponed_configurations for details). If
// this dependent already belongs to some (being) negotiated configuration
// cluster with a greater dependency position then record this dependency
- // position in postponed_poss and throw postpone_position.
- //
- // If a dependency of a dependent with configuration clause is being
- // negotiated (the negotiated member of the respective cluster in
- // postponed_cfgs is false), then it is not collected recursively (being
- // already collected) and if the specified dependent didn't participate in
- // the negotiation, then the dependency configuration is up-negotiated.
+ // position in postponed_poss and throw postpone_position. This exception
+ // is handled by re-collecting packages from scratch, but now with the
+ // knowledge about position this dependent needs to be re-evaluated to.
//
struct postpone_dependency: scratch_collection
{
@@ -2829,6 +2847,12 @@ namespace bpkg
}
};
+ struct postpone_position: scratch_collection
+ {
+ postpone_position ()
+ : scratch_collection ("earlier dependency position") {}
+ };
+
void
collect_build_prerequisites (const pkg_build_options& options,
build_package& pkg,
@@ -2993,13 +3017,14 @@ namespace bpkg
pk = package_key (bp->db, bp->name ());
- // Note that here we side-step the bogus logic (by not setting
- // the skipped flag) because in this case (replace=true) our
- // choices are either (potentially) bogus or pathological
- // (where we have evaluated too far). In other words, the
- // postponed entry may cause the depends entry that triggered
- // it to disappear (and thus, strictly speaking, to become
- // bogus) but if we cancel it, we will be back to square one.
+ // Note that here we side-step the bogus logic (by not
+ // setting the skipped flag) because in this case
+ // (replace=true) our choices are either (potentially) bogus
+ // or pathological (where we have evaluated too far). In
+ // other words, the postponed entry may cause the depends
+ // entry that triggered it to disappear (and thus, strictly
+ // speaking, to become bogus) but if we cancel it, we will
+ // be back to square one.
}
}
}
@@ -3367,9 +3392,10 @@ namespace bpkg
bool system (false);
bool specified (false);
- // If the user specified the desired dependency version constraint,
- // then we will use it to overwrite the constraint imposed by the
- // dependent package, checking that it is still satisfied.
+ // If the user specified the desired dependency version
+ // constraint, then we will use it to overwrite the constraint
+ // imposed by the dependent package, checking that it is still
+ // satisfied.
//
// Note that we can't just rely on the execution plan refinement
// that will pick up the proper dependency version at the end of
@@ -3770,7 +3796,7 @@ namespace bpkg
// for this package version. Consider this scenario as an
// example: hello/1.0.0 and libhello/1.0.0 in stable and
// libhello/2.0.0 in testing. As a prerequisite of hello, which
- // version should libhello resolve to? While one can probably
+ // version should libhello resolve to? While one can probably
// argue either way, resolving it to 1.0.0 is the conservative
// choice and the user can always override it by explicitly
// building libhello.
@@ -4195,7 +4221,8 @@ namespace bpkg
}
}
- // Don't print the "while satisfying..." chain.
+ // Don't print the "while satisfying..." chain if we are about
+ // to re-collect the packages.
//
if (scratch)
dep_chain.clear ();
@@ -4224,7 +4251,7 @@ namespace bpkg
// Do not collect prerequisites recursively for dependent
// re-evaluation. Instead, if the re-evaluation position is
// reached, collect the dependency packages to add them to the
- // existing dependent's cluster
+ // existing dependent's cluster.
//
if (reeval)
{
@@ -4395,8 +4422,8 @@ namespace bpkg
{
l5 ([&]{trace << "re-evaluating dependent "
<< pkg.available_name_version_db ()
- << " involves non-negotiated configurations "
- << "and results in " << cfg << ", throwing "
+ << " involves negotiated configurations and "
+ << "results in " << cfg << ", throwing "
<< "merge_configuration";});
// Don't print the "while satisfying..." chain.
@@ -4438,10 +4465,6 @@ namespace bpkg
pair<postponed_configuration&, optional<bool>> r (
postponed_cfgs.add (pk, false /* existing */, dp, cfg_deps));
- // Up-negotiate this dependent and re-negotiate (refine) postponed
- // if any (being) negotiated configurations were involved into the
- // configuration addition/merge. @@ Stray/old comment?
- //
postponed_configuration& cfg (r.first);
if (cfg.depth == 0)
@@ -4512,7 +4535,7 @@ namespace bpkg
<< pkg.available_name_version_db ()
<< " is negotiated";});
- // Note that we may still add extra dependenncies to this
+ // Note that we may still add extra dependencies to this
// cluster which we still need to configure and recursively
// collect before indicating to the caller (returning true)
// that we are done with this depends value and the
@@ -4705,7 +4728,7 @@ namespace bpkg
// satisfies the constraint. On the other hand, it could be that
// it's other dependent's constraints that we cannot satisfy
// together with others. And in this case we may want some other
- // alternative. Consider, as an example, something like this:
+ // alternative. Consider, as an example, something like this:
//
// depends: libfoo >= 2.0.0 | libfoo >= 1.0.0 libbar
//
@@ -4728,9 +4751,9 @@ namespace bpkg
// Note that when we see the first satisfactory alternative, we
// don't know yet if it is a single alternative or the first of
// the (multiple) true alternatives (those are handled
- // differently). Thus, we postpone its processing until the
- // second satisfactory alternative is encountered or the end of
- // the alternatives list is reached.
+ // differently). Thus, we postpone its processing until the second
+ // satisfactory alternative is encountered or the end of the
+ // alternatives list is reached.
//
if (!first_alt)
{
@@ -4955,6 +4978,43 @@ namespace bpkg
<< pkg.available_name_version_db ();});
}
+ void
+ collect_build_prerequisites (const pkg_build_options& o,
+ database& db,
+ const package_name& name,
+ const function<find_database_function>& fdb,
+ const repointed_dependents& rpt_depts,
+ const function<add_priv_cfg_function>& apc,
+ bool initial_collection,
+ replaced_versions& replaced_vers,
+ postponed_packages& postponed_repo,
+ postponed_packages& postponed_alts,
+ size_t max_alt_index,
+ postponed_dependencies& postponed_deps,
+ postponed_configurations& postponed_cfgs,
+ postponed_positions& postponed_poss)
+ {
+ auto mi (map_.find (db, name));
+ assert (mi != map_.end ());
+
+ build_package_refs dep_chain;
+
+ collect_build_prerequisites (o,
+ mi->second.package,
+ fdb,
+ rpt_depts,
+ apc,
+ initial_collection,
+ replaced_vers,
+ dep_chain,
+ &postponed_repo,
+ &postponed_alts,
+ max_alt_index,
+ postponed_deps,
+ postponed_cfgs,
+ postponed_poss);
+ }
+
// Collect the repointed dependents and their replaced prerequisites,
// recursively.
//
@@ -5230,45 +5290,6 @@ namespace bpkg
}
void
- collect_build_prerequisites (const pkg_build_options& o,
- database& db,
- const package_name& name,
- const function<find_database_function>& fdb,
- const repointed_dependents& rpt_depts,
- const function<add_priv_cfg_function>& apc,
- bool initial_collection,
- replaced_versions& replaced_vers,
- postponed_packages& postponed_repo,
- postponed_packages& postponed_alts,
- size_t max_alt_index,
- postponed_dependencies& postponed_deps,
- postponed_configurations& postponed_cfgs,
- postponed_positions& postponed_poss)
- {
- auto mi (map_.find (db, name));
- assert (mi != map_.end ());
-
- build_package_refs dep_chain;
-
- collect_build_prerequisites (o,
- mi->second.package,
- fdb,
- rpt_depts,
- apc,
- initial_collection,
- replaced_vers,
- dep_chain,
- &postponed_repo,
- &postponed_alts,
- max_alt_index,
- postponed_deps,
- postponed_cfgs,
- postponed_poss);
- }
-
- // Note: depth is only used for tracing.
- //
- void
collect_build_postponed (const pkg_build_options& o,
replaced_versions& replaced_vers,
postponed_packages& postponed_repo,
@@ -5348,7 +5369,16 @@ namespace bpkg
postponed_configurations postponed_cfgs_;
};
- // @@ TODO Describe.
+ // This exception is thrown if negotiation of the current cluster needs
+ // to be postponed. This is normally required if this cluster contains
+ // some existing dependent which needs to be re-evaluated to a
+ // dependency position greater than some other not yet negotiated
+ // cluster will re-evaluate this dependent to. Sometimes this another
+ // cluster yet needs to be created in which case the exception carries
+ // the information required for such a creation (see
+ // postponed_position's replace flag for details).
+ //
+ // @@ Maybe it's better to call it postpone_configuration or similar?
//
struct skip_configuration
{
@@ -5378,39 +5408,18 @@ namespace bpkg
// @@ TODO Negotiate the config.
//
- // Notes:
- //
- // - While re-collecting the existing (already configured)
- // dependents we need to handle a possible situation when the
- // postponed dependency is resolved from a dependency alternative
- // without configuration clause (see
- // collect_build_prerequisites() implementation for details).
- //
- // - When re-evaluate an existing dependent we need to realize that
- // some of it configured dependencies can be in some other
- // clusters.
- //
assert (!pcfg->negotiated);
- // @@ Before negotiating the cluster we should look if there is a
- // shadow cluster which fully contains this cluster. (Question: can
- // we end up with multiple shadow clusters? Probably we don't
- // expect this, so assert.) If that's the case it actually needs to
- // be force-merged into the cluster containing the shadow instead
- // of negotiating it here. Should we just merge-force on the
- // merge_configuration catch site all those non-negotiated clusters
- // which are fully contained in the shadow cluster.
- //
-
- // Re-evaluate existing dependents with configuration clause of this
- // config dependencies up to these dependencies. Omit dependents which
- // are already being built or dropped. Add these dependents to this
- // cluster with the 'existing' flag.
+ // Re-evaluate existing dependents with configuration clause for
+ // dependencies in this configuration cluster up to these
+ // dependencies. Omit dependents which are already being built or
+ // dropped. Note that these dependents, potentially with additional
+ // dependencies, will be added to this cluster with the 'existing'
+ // flag as a part of the dependents' re-evaluation (see the collect
+ // lambda in collect_build_prerequisites() for details).
//
- // @@ Also note that we need to watch carefully if the re-evaluation
- // may end up with merge of pcfg into some other cluster. If this
- // case pcfg pointer will be invalidated which we will need to
- // handle somehow.
+ // After being re-evaluated the existing dependents are recursively
+ // collected in the same way as the new dependents.
//
{
// Map existing dependents to the dependencies they apply a
@@ -5418,6 +5427,14 @@ namespace bpkg
// for a dependent re-evaluation and its subsequent recursive
// collection (selected package, etc).
//
+ // As it was said, we may end up adding additional dependencies to
+ // pcfg->dependencies which in turn may have additional existing
+ // dependents which we need to process. Feels like doing this
+ // iteratively is the best option.
+ //
+ // Note that we need to make sure we don't re-process the same
+ // existing dependents.
+ //
struct existing_dependent_ex: existing_dependent
{
packages dependencies;
@@ -5428,20 +5445,12 @@ namespace bpkg
};
map<package_key, existing_dependent_ex> dependents;
- // Looks like we may end up adding additional dependencies to
- // pcfg->dependencies which in turn may have additional existing
- // dependents which we need to process. Feels like doing this
- // iteratively is the best option.
- //
- // Note that we need to make sure we don't re-process the same
- // existing dependents.
- //
const packages& deps (pcfg->dependencies);
- // Note that the below collect_build_prerequisites() call can add
- // new dependencies to the end of the cluster's dependencies list.
- // Thus on each iteration we will only add existing dependents of
- // unprocessed/new dependencies. We will also skip the already
+ // Note that the below collect_build_prerequisites() call can only
+ // add new dependencies to the end of the cluster's dependencies
+ // list. Thus on each iteration we will only add existing dependents
+ // of unprocessed/new dependencies. We will also skip the already
// re-evaluated existing dependents.
//
for (size_t i (0); i != deps.size (); )
@@ -5495,8 +5504,8 @@ namespace bpkg
// If this dependent is present in postponed_deps, then it
// means someone depends on it with configuration and it's no
// longer considered an existing dependent (it will be
- // reconfigured). However, this fact may not be reflected
- // yet. And it can actually turn out bogus.
+ // reconfigured). However, this fact may not be reflected yet.
+ // And it can actually turn out bogus.
//
auto pi (postponed_deps.find (pk));
if (pi != postponed_deps.end ())
@@ -5558,7 +5567,7 @@ namespace bpkg
// that one in the existing map entry then skip it (this
// position will be up-negotiated, if it's still present).
// Otherwise, if the position is less then overwrite the
- // existing entry. Otherwise (the position is equal), just
+ // existing entry. Otherwise (the position is equal), just
// add the dependency to the existing entry.
//
// Note that we want to re-evaluate the dependent up to the
@@ -5666,8 +5675,16 @@ namespace bpkg
skip = true;
}
}
- else if (di < ei)
+ else
{
+ // Otherwise, this dependent wouldn't be considered as
+ // an existing by query_existing_dependents() since as
+ // it is (being) negotiated then it is already
+ // re-evaluated and so is being built (see the verify
+ // lambda above).
+ //
+ assert (ei > di);
+
// Feels like there cannot be an earlier position.
//
postponed_position pp (ed.dependency_position,
@@ -5694,6 +5711,8 @@ namespace bpkg
if (skip)
throw skip_configuration ();
+ // Finally, re-evaluate the dependent.
+ //
packages& ds (ed.dependencies);
pair<shared_ptr<available_package>,
@@ -5906,9 +5925,9 @@ namespace bpkg
// collect_build_prerequisites() for details).
//
{
- const bpkg::dependencies& deps (b->available->dependencies);
- bpkg::dependencies& sdeps (*b->dependencies);
- vector<size_t>& salts (*b->alternatives);
+ const bpkg::dependencies& deps (b->available->dependencies);
+ bpkg::dependencies& sdeps (*b->dependencies);
+ vector<size_t>& salts (*b->alternatives);
size_t di (sdeps.size ());
@@ -6065,7 +6084,7 @@ namespace bpkg
//
// Note that since the potential snapshot restore replaces all the
// list entries we cannot iterate using the iterator here. Also note
- // that the list size may not change during iterating.
+ // that the list size may change during iterating.
//
for (size_t ci (0); ci != postponed_cfgs.size (); ++ci)
{
@@ -6151,7 +6170,7 @@ namespace bpkg
replace_existing_dependent_dependency (
trace,
o,
- ed,
+ ed, // Note: modified.
pos,
fdb,
rpt_depts,
@@ -6411,15 +6430,6 @@ namespace bpkg
assert (!prog);
- // Finally, erase the bogus postponements and re-collect from scratch,
- // if any (see postponed_dependencies for details).
- //
- // Note that we used to re-collect such postponements in-place but
- // re-doing from scratch feels more correct (i.e., we may end up doing
- // it earlier which will affect dependency alternatives).
- //
- postponed_deps.cancel_bogus (trace, false /* initial_collection */);
-
// If we still have any non-negotiated clusters and non-replace
// postponed positions, then it's possible one of them is the cross-
// dependent pathological case where we will never hit it unless we
@@ -6454,10 +6464,19 @@ namespace bpkg
prog = true;
continue; // Go back to negotiating skipped cluster.
}
+
+ // Finally, erase the bogus postponements and re-collect from scratch,
+ // if any (see postponed_dependencies for details).
+ //
+ // Note that we used to re-collect such postponements in-place but
+ // re-doing from scratch feels more correct (i.e., we may end up doing
+ // it earlier which will affect dependency alternatives).
+ //
+ postponed_deps.cancel_bogus (trace, false /* initial_collection */);
}
// If any postponed_{repo,alts} builds remained, then perform the
- // diagnostics run. Naturally we chouldn't have any postponed_cfgs
+ // diagnostics run. Naturally we shouldn't have any postponed_cfgs
// without one of the former.
//
if (!postponed_repo.empty ())
@@ -6854,11 +6873,11 @@ namespace bpkg
// Return the list of existing dependents that has a configuration clause
// for the specified dependency. Skip dependents which are being built and
// require recursive recollection or dropped (present in the map) or
- // expected to be built or dropped (present in replaced_vers).
+ // expected to be built or dropped (present in rpt_depts or replaced_vers).
//
// Optionally, specify the function which can verify the dependent build
// and decide whether to override the default behavior and still add the
- // dependent package to the resulting list, returning true in this case.
+ // dependent package to the resulting list, returning true in that's case.
//
struct existing_dependent
{
@@ -6899,6 +6918,13 @@ namespace bpkg
{
package_key pk (ddb, pd.name);
+ if (rpt_depts.find (pk) != rpt_depts.end ())
+ {
+ l5 ([&]{trace << "skip repointed existing dependent " << pk
+ << " of dependency " << name << db;});
+ continue;
+ }
+
// Ignore dependent which is already being built or dropped.
//
const build_package* p (entered_build (pk));
@@ -6944,9 +6970,10 @@ namespace bpkg
return r;
}
- // Update the existing dependent object with the new dependency position
- // and collect the dependency referred by this position. Return the
- // pointer to the collected build package object.
+ // Update the existing dependent object (previously obtained with the
+ // query_existing_dependents() call) with the new dependency position and
+ // collect the dependency referred by this position. Return the pointer to
+ // the collected build package object.
//
const build_package*
replace_existing_dependent_dependency (
@@ -6961,6 +6988,13 @@ namespace bpkg
replaced_versions& replaced_vers,
postponed_configurations& postponed_cfgs)
{
+ // The repointed dependent cannot be returned by
+ // query_existing_dependents(). If that's wasn't the case, the below
+ // would likely fall apart for it.
+ //
+ assert (rpt_depts.find (package_key (ed.db, ed.selected->name)) ==
+ rpt_depts.end ());
+
shared_ptr<selected_package> dsp;
database* pdb (nullptr);
const version_constraint* vc (nullptr);
@@ -10241,11 +10275,13 @@ namespace bpkg
// Note that we should not clean the deps list on scratch_col (scratch
// during the package collection) because we want to enter them before
// collect_build_postponed() and they could be the dependents that have
- // the config clauses. In a sense, change to postponed_deps map should
- // not affect the deps list. But not the other way around: a dependency
- // erased from the deps list could have caused an entry in the
- // postponed_deps map. And so we clean postponed_deps on scratch_exe
- // (scratch during the plan execution).
+ // the config clauses. In a sense, change to replaced_vers,
+ // postponed_deps, or postponed_poss maps should not affect the deps
+ // list. But not the other way around: a dependency erased from the deps
+ // list could have caused an entry in the replaced_vers, postponed_deps,
+ // and/or postponed_poss maps. And so we clean replaced_vers,
+ // postponed_deps, and postponed_poss on scratch_exe (scratch during the
+ // plan execution).
//
for (bool refine (true), scratch_exe (true), scratch_col (false);
refine; )
@@ -10274,15 +10310,13 @@ namespace bpkg
// Temporarily add the replacement prerequisites to the repointed
// dependent prerequisites sets and persist the changes.
//
- // Note that we don't copy the prerequisite information into the
- // replacements, since it is unused in the collecting/ordering logic.
- //
for (auto& rd: rpt_depts)
{
database& db (rd.first.db);
const package_name& nm (rd.first.name);
shared_ptr<selected_package> sp (db.load<selected_package> (nm));
+ package_prerequisites& prereqs (sp->prerequisites);
for (const auto& prq: rd.second)
{
@@ -10290,16 +10324,27 @@ namespace bpkg
{
const package_key& p (prq.first);
- auto i (sp->prerequisites.emplace (
+ // Find the being replaced prerequisite to copy it's information
+ // into the replacement.
+ //
+ auto i (find_if (prereqs.begin (), prereqs.end (),
+ [&p] (const auto& pr)
+ {
+ return pr.first.object_id () == p.name;
+ }));
+
+ assert (i != prereqs.end ());
+
+ auto j (prereqs.emplace (
lazy_shared_ptr<selected_package> (p.db.get (),
p.name),
- prerequisite_info {nullopt, make_pair (0, 0)}));
+ i->second));
// The selected package should only contain the old
// prerequisites at this time, so adding a replacement should
// always succeed.
//
- assert (i.second);
+ assert (j.second);
}
}
@@ -10727,12 +10772,6 @@ namespace bpkg
// Erase the bogus replacements and re-collect from scratch, if any
// (see replaced_versions for details).
//
- // Note that we refer replaced_vers to skip existing dependents with
- // configuration clause while negotiate configuration for their
- // dependencies. Thus, if the replacement is bogus we could have
- // erroneously skipped the dependent because of it and so need to
- // re-collect.
- //
replaced_vers.cancel_bogus (trace, true /* scratch */);
// Erase the bogus existing dependent re-evaluation postponements
@@ -10759,21 +10798,6 @@ namespace bpkg
//
replaced_vers.cancel_bogus (trace, false /* scratch */);
- for (auto i (replaced_vers.begin ()); i != replaced_vers.end (); )
- {
- const replaced_version& v (i->second);
-
- if (!v.replaced)
- {
- l5 ([&]{trace << "erase bogus version replacement "
- << i->first;});
-
- i = replaced_vers.erase (i);
- }
- else
- ++i;
- }
-
restore_repointed_dependents ();
// Commit linking of private configurations that were potentially
@@ -11194,15 +11218,15 @@ namespace bpkg
//
// Note, however, that we cannot easily determine if the
// prerequisite corresponds to the runtime or build-time
- // dependency, since we only store its version constraint. The
- // current implementation relies on the fact that the build-time
- // dependency configuration type (host or build2) differs from the
- // dependent configuration type (target is a common case) and
- // doesn't work well, for example, for the self-hosted
- // configurations. For them it can fail erroneously. We can
- // potentially fix that by additionally storing the build-time
- // flag besides the version constraint. However, let's first see
- // if it ever becomes a problem.
+ // dependency, since we don't store this information for
+ // prerequisites. The current implementation relies on the fact
+ // that the build-time dependency configuration type (host or
+ // build2) differs from the dependent configuration type (target
+ // is a common case) and doesn't work well, for example, for the
+ // self-hosted configurations. For them it can fail
+ // erroneously. We can potentially fix that by additionally
+ // storing the build-time flag for a prerequisite. However, let's
+ // first see if it ever becomes a problem.
//
prerequisites r;
const package_prerequisites& prereqs (sp->prerequisites);
@@ -11899,7 +11923,6 @@ namespace bpkg
assert (sp->state == package_state::unpacked ||
sp->state == package_state::transient);
-
if (result || progress)
{
const char* what (sp->state == package_state::transient
@@ -12301,8 +12324,8 @@ namespace bpkg
else if (ap != nullptr)
{
// If the package prerequisites builds are collected, then use the
- // resulting package skeleton and dependency list for optimization
- // (not to re-evaluate enable conditions, etc).
+ // resulting package skeleton and the pre-selected dependency
+ // alternatives.
//
// Note that we may not collect the package prerequisites builds if
// the package is already configured but we still need to reconfigure
@@ -12336,7 +12359,7 @@ namespace bpkg
}
else
{
- assert (sp != nullptr); // See above.
+ assert (sp != nullptr && !p.skeleton); // See above.
optional<dir_path> src_root (p.external_dir ());