From 3ce11cb9ae8caec98f0530181395307307eebf4e Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Tue, 31 May 2022 22:10:57 +0300 Subject: Review/complete documentation plus make some minor code cleanups --- bpkg/pkg-build.cxx | 509 ++++++++++++++++++++++++++++------------------------- 1 file 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 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> 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& fdb, + const repointed_dependents& rpt_depts, + const 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& fdb, - const repointed_dependents& rpt_depts, - const 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 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, @@ -5906,9 +5925,9 @@ namespace bpkg // collect_build_prerequisites() for details). // { - const bpkg::dependencies& deps (b->available->dependencies); - bpkg::dependencies& sdeps (*b->dependencies); - vector& salts (*b->alternatives); + const bpkg::dependencies& deps (b->available->dependencies); + bpkg::dependencies& sdeps (*b->dependencies); + vector& 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 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 sp (db.load (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 (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 src_root (p.external_dir ()); -- cgit v1.1