From 2f39c94e04007837b57b119a365fdae3254b36cf Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Mon, 23 May 2022 16:33:54 +0300 Subject: Treat replacement of existing dependent as version replacement as well --- bpkg/pkg-build.cxx | 113 +++++++++++++++++++++--------- tests/pkg-build.testscript | 170 ++++++++++++++++++++++++++++++++++++++------- 2 files changed, 224 insertions(+), 59 deletions(-) diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx index 91f32c3..fc049a6 100644 --- a/bpkg/pkg-build.cxx +++ b/bpkg/pkg-build.cxx @@ -1354,11 +1354,14 @@ namespace bpkg ddi.add (dependency (position, move (deps))); - // Conceptually we can only move from existing to non-existing (e.g., - // due to a upgrade/downgrade later). But that case is handled via - // the version replacement rollback. - // - assert (ddi.existing == existing); + // Conceptually, on the first glance, we can only move from existing + // to non-existing (e.g., due to a upgrade/downgrade later) and that + // case is handled via the version replacement rollback. However, + // after re-evaluation the existing dependent is handled similar to + // the new dependent and we can potentially up-negotiate the + // dependency configuration for it. + // + assert (ddi.existing || !existing); } else { @@ -1845,9 +1848,14 @@ namespace bpkg if (i != end ()) { // Note that the cluster with a shadow cluster is by definition - // either being negotiated or has been negotiated. + // either being negotiated or has been negotiated. Actually, there + // is also a special case when we didn't negotiate the configuration + // yet and are in the process of re-evaluating existing dependents. + // Note though, that in this case we have already got the try/catch + // frame corresponding to the cluster negotiation (see + // collect_build_postponed() for details). // - assert (i->negotiated); + assert (i->depth != 0); ri = i; @@ -2216,6 +2224,8 @@ namespace bpkg // instead. Add entry to replaced_vers and throw replace_version if the // existing version needs to be replaced but the new version cannot be // re-collected recursively in-place (see replaced_versions for details). + // Also add entry and throw if the existing dependent needs to be + // replaced. // // Optionally, pass the function which verifies the chosen package // version. It is called before replace_version is potentially thrown or @@ -2255,11 +2265,11 @@ namespace bpkg const function& apc, bool initial_collection, replaced_versions& replaced_vers, + postponed_configurations& postponed_cfgs, build_package_refs* dep_chain = nullptr, postponed_packages* postponed_repo = nullptr, postponed_packages* postponed_alts = nullptr, postponed_dependencies* postponed_deps = nullptr, - postponed_configurations* postponed_cfgs = nullptr, const function& vpb = nullptr) { using std::swap; // ...and not list::swap(). @@ -2271,8 +2281,7 @@ namespace bpkg bool recursive (dep_chain != nullptr); assert ((postponed_repo != nullptr) == recursive && (postponed_alts != nullptr) == recursive && - (postponed_deps != nullptr) == recursive && - (postponed_cfgs != nullptr) == recursive); + (postponed_deps != nullptr) == recursive); // Only builds are allowed here. // @@ -2315,6 +2324,25 @@ namespace bpkg } } + // Add the version replacement entry, call the verification function if + // specified, and throw replace_version. + // + auto replace_ver = [&cp, &vpb, &vi, &replaced_vers] + (const build_package& p) + { + replaced_version rv (p.available, p.repository_fragment, p.system); + + if (vi != replaced_vers.end ()) + vi->second = move (rv); + else + replaced_vers.emplace (move (cp), move (rv)); + + if (vpb) + vpb (p, true /* scratch */); + + throw replace_version (); + }; + auto i (map_.find (cp)); // If we already have an entry for this package name, then we have to @@ -2481,21 +2509,7 @@ namespace bpkg << p1->available_name_version_db ();}); if (scratch) - { - replaced_version rv (p1->available, - p1->repository_fragment, - p1->system); - - if (vi != replaced_vers.end ()) - vi->second = move (rv); - else - replaced_vers.emplace (move (cp), move (rv)); - - if (vpb) - vpb (*p1, true /* scratch */); - - throw replace_version (); - } + replace_ver (*p1); } else { @@ -2514,6 +2528,27 @@ namespace bpkg } else { + // Treat the replacement of the existing dependent as a version + // replacement as well. This way we will not be treating the dependent + // as an existing on the re-collection (see + // query_existing_dependents() for details). + // + // Note: an existing dependent may not be configured as system. + // + if (pkg.selected != nullptr && + (pkg.selected->version != pkg.available_version () || + pkg.system)) + { + + for (const postponed_configuration& cfg: postponed_cfgs) + { + auto i (cfg.dependents.find (cp)); + + if (i != cfg.dependents.end () && i->second.existing) + replace_ver (pkg); + } + } + // This is the first time we are adding this package name to the map. // l4 ([&]{trace << "add " << pkg.available_name_version_db ();}); @@ -2559,7 +2594,7 @@ namespace bpkg postponed_alts, 0 /* max_alt_index */, *postponed_deps, - *postponed_cfgs); + postponed_cfgs); return &p; } @@ -3836,7 +3871,15 @@ namespace bpkg bool f (prq.selected->hold_version); bool w (!f && prq.selected->hold_package); - if (f || w || verb >= 2) + // Note that there is no sense to warn or inform the user if + // we are about to start re-collection from scratch. + // + // @@ It seems that we may still warn/inform multiple times + // about the same package if we start from scratch. The + // intermediate diagnostics can probably be irrelevant to + // the final result. + // + if (f || ((w || verb >= 2) && !scratch)) { const version& av (p.available_version ()); @@ -3888,11 +3931,11 @@ namespace bpkg apc, initial_collection, replaced_vers, + postponed_cfgs, nullptr /* dep_chain */, nullptr /* postponed_repo */, nullptr /* postponed_alts */, nullptr /* postponed_deps */, - nullptr /* postponed_cfgs */, verify)); config_package dcp (b.db, b.available->id.name); @@ -4723,11 +4766,11 @@ namespace bpkg apc, true /* initial_collection */, replaced_vers, + postponed_cfgs, &dep_chain, &postponed_repo, &postponed_alts, - &postponed_deps, - &postponed_cfgs); + &postponed_deps); } } @@ -5234,7 +5277,8 @@ namespace bpkg rpt_depts, apc, true /* initial_collection */, - replaced_vers); + replaced_vers, + postponed_cfgs); build_package* b (entered_build (cp)); assert (b != nullptr); @@ -9738,7 +9782,8 @@ namespace bpkg rpt_depts, add_priv_cfg, true /* initial_collection */, - replaced_vers); + replaced_vers, + postponed_cfgs); // Collect all the prerequisites of the user selection. // @@ -9894,11 +9939,11 @@ namespace bpkg add_priv_cfg, true /* initial_collection */, replaced_vers, + postponed_cfgs, &dep_chain, &postponed_repo, &postponed_alts, - &postponed_deps, - &postponed_cfgs); + &postponed_deps); } } diff --git a/tests/pkg-build.testscript b/tests/pkg-build.testscript index 5e70055..80f940c 100644 --- a/tests/pkg-build.testscript +++ b/tests/pkg-build.testscript @@ -5473,7 +5473,6 @@ test.options += --no-progress : postpone-existing : - if false { $clone_cfg; @@ -5487,17 +5486,7 @@ test.options += --no-progress # $* fix 2>!; - # @@ So here we should have been in a situation that after libfoo is - # postponed with {foo^ | libfoo->{foo/1,1}} creation and we got - # to negotiating it, by that time foo has stopped to be an - # "existing" dependent since it was downgraded via fix/0.1.0. The - # natural fix would be adding a record somewhere and start from - # scratch (maybe add into postponed_poss with position {0,0}). We - # however crash on assertion failure earlier (see below). Sounds - # like we need to scratch earlier or maybe get rid of assertion - # and deal with the existing flag there. - # - $* libfoo/0.1.0 fix/0.1.0 2>>~%EOE% + $* libfoo/0.1.0 fix/0.1.0 2>>~%EOE%; %.* trace: pkg_build: refine package collection/plan execution from scratch %.* @@ -5509,6 +5498,40 @@ test.options += --no-progress %.* trace: collect_build_prerequisites: begin fix/0.1.0 %.* + trace: pkg_build: refine package collection/plan execution from scratch + %.* + trace: collect_build: add libfoo/0.1.0 + trace: collect_build: add fix/0.1.0 + %.* + trace: collect_build_prerequisites: skip expected to be built existing dependent foo of dependency libfoo + trace: collect_build_prerequisites: begin libfoo/0.1.0 + trace: collect_build_prerequisites: end libfoo/0.1.0 + %.* + trace: collect_build_prerequisites: begin fix/0.1.0 + %.* + trace: collect_build: apply version replacement for foo/0.1.0 + trace: collect_build: replacement: foo/0.1.0 + trace: collect_build: add foo/0.1.0 + info: package fix dependency on (foo == 0.1.0) is forcing downgrade of foo/1.0.0 to 0.1.0 + trace: collect_build_prerequisites: no cfg-clause for dependency foo/0.1.0 of dependent fix/0.1.0 + %.* + trace: collect_build_prerequisites: skip being built existing dependent fix of dependency foo + trace: collect_build_prerequisites: begin foo/0.1.0 + %.* + trace: collect_build: pick libfoo/0.1.0 over libfoo/1.0.0 + trace: collect_build_prerequisites: cannot cfg-postpone dependency libfoo/0.1.0 of dependent foo/0.1.0 (collected prematurely), throwing postpone_dependency + trace: pkg_build: collection failed due to prematurely collected dependency (libfoo), retry from scratch + %.* + trace: pkg_build: refine package collection/plan execution from scratch + %.* + trace: collect_build: add libfoo/0.1.0 + trace: collect_build: add fix/0.1.0 + trace: pkg_build: dep-postpone user-specified libfoo + %.* + trace: collect_build_prerequisites: begin fix/0.1.0 + %.* + trace: collect_build: apply version replacement for foo/0.1.0 + trace: collect_build: replacement: foo/0.1.0 trace: collect_build: add foo/0.1.0 info: package fix dependency on (foo == 0.1.0) is forcing downgrade of foo/1.0.0 to 0.1.0 trace: collect_build_prerequisites: no cfg-clause for dependency foo/0.1.0 of dependent fix/0.1.0 @@ -5518,9 +5541,37 @@ test.options += --no-progress %.* trace: collect_build: pick libfoo/0.1.0 over libfoo/1.0.0 trace: collect_build_prerequisites: cfg-postpone dependency libfoo/0.1.0 of dependent foo/0.1.0 - trace: postponed_configurations::add: add {foo 1,1: libfoo} to {foo^ | libfoo->{foo/1,1}} - bpkg: /home/karen/work/build2/bpkg/bpkg/pkg-build.cxx:1361: void bpkg::postponed_configuration::add(bpkg::config_package&&, bool, std::pair, bpkg::postponed_configuration::packages&&): Assertion 'ddi.existing == existing' failed. + trace: postponed_configurations::add: create {foo | libfoo->{foo/1,1}} + trace: collect_build_prerequisites: postpone foo/0.1.0 + trace: collect_build_prerequisites: end fix/0.1.0 + trace: collect_build_postponed (0): begin + trace: collect_build_postponed (1): begin {foo | libfoo->{foo/1,1}} + %.* + trace: collect_build_postponed (1): skip being built existing dependent foo of dependency libfoo + trace: collect_build_postponed (1): cfg-negotiate begin {foo | libfoo->{foo/1,1}} + trace: collect_build_postponed (1): recursively collect cfg-negotiated dependencies + trace: collect_build_prerequisites: begin libfoo/0.1.0 + trace: collect_build_prerequisites: end libfoo/0.1.0 + trace: collect_build_postponed (1): recursively collect cfg-negotiated dependents + trace: collect_build_postponed (1): select cfg-negotiated dependency alternative for dependent foo/0.1.0 + trace: collect_build_prerequisites: resume foo/0.1.0 + trace: collect_build_prerequisites: end foo/0.1.0 + trace: collect_build_postponed (1): cfg-negotiate end {foo | libfoo->{foo/1,1}}! + trace: collect_build_postponed (1): end {foo | libfoo->{foo/1,1}} + trace: collect_build_postponed (0): end + %.* + trace: execute_plan: simulate: yes + %.* EOE + + $pkg_status -r >>EOO; + !libfoo configured !0.1.0 available 1.0.0 + !fix configured !0.1.0 available 1.0.0 + foo configured 0.1.0 available 1.0.0 + !libfoo configured !0.1.0 available 1.0.0 + EOO + + $pkg_drop fix libfoo --drop-dependent } } @@ -5562,22 +5613,14 @@ test.options += --no-progress EOE } - : re-evaluating-dependent + : up-negotiate : - if false { $clone_cfg; $* tex 2>!; - # @@ Seems we should get rid of this assertion (as suggested above) - # since the situation is pretty valid here: we just should - # up-negotiate 'tex: depends: libfoo(c)' after existing dependent - # tex was re-evaluated and become a regular dependent in a sence - # (we still need to keep the existing flag for it; see - # collect_build_prerequisites()). - # - $* baz/0.1.0 2>>~%EOE% + $* baz/0.1.0 2>>~%EOE%; %.* trace: pkg_build: refine package collection/plan execution from scratch %.* @@ -5620,8 +5663,85 @@ test.options += --no-progress trace: collect_build: pick libfoo/0.1.0 over libfoo/1.0.0 trace: collect_build_prerequisites: cfg-postpone dependency libfoo/0.1.0 of dependent tex/1.0.0 trace: postponed_configurations::add: add {tex 2,1: libfoo} to {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1}}? - bpkg: /home/karen/work/build2/bpkg/bpkg/pkg-build.cxx:1361: void bpkg::postponed_configuration::add(bpkg::config_package&&, bool, std::pair, bpkg::postponed_configuration::packages&&): Assertion 'ddi.existing == existing' failed. + trace: collect_build_prerequisites: cfg-postponing dependent tex/1.0.0 involves negotiated configurations and results in {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1 tex/2,1}}?, throwing retry_configuration + trace: collect_build_postponed (0): cfg-negotiation of {baz | libbar->{baz/1,1} libfoo->{baz/1,1}} failed due to dependent tex, adding shadow dependent and re-negotiating + trace: collect_build_postponed (1): begin {baz | libbar->{baz/1,1} libfoo->{baz/1,1}} + %.* + trace: collect_build_postponed (1): re-evaluate existing dependents for {baz | libbar->{baz/1,1} libfoo->{baz/1,1}} + trace: collect_build: add tex/1.0.0 + trace: collect_build_prerequisites: reeval tex/1.0.0 + %.* + trace: collect_build: pick libbar/0.1.0 over libbar/1.0.0 + trace: postponed_configurations::add: add {tex^ 1,1: libbar} to {baz | libbar->{baz/1,1} libfoo->{baz/1,1}} + trace: collect_build_prerequisites: re-evaluating dependent tex/1.0.0 results in {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1}} + trace: collect_build_prerequisites: re-evaluated tex/1.0.0 + trace: collect_build_postponed (1): cfg-negotiate begin {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1}} + trace: collect_build_postponed (1): recursively collect cfg-negotiated dependencies + trace: collect_build_prerequisites: begin libbar/0.1.0 + trace: collect_build_prerequisites: end libbar/0.1.0 + trace: collect_build_prerequisites: begin libfoo/0.1.0 + trace: collect_build_prerequisites: end libfoo/0.1.0 + trace: collect_build_postponed (1): recursively collect cfg-negotiated dependents + trace: collect_build_postponed (1): select cfg-negotiated dependency alternative for dependent baz/0.1.0 + trace: collect_build_prerequisites: resume baz/0.1.0 + trace: collect_build_prerequisites: end baz/0.1.0 + trace: collect_build_postponed (1): select cfg-negotiated dependency alternative for dependent tex/1.0.0 + trace: collect_build_prerequisites: resume tex/1.0.0 + %.* + trace: collect_build: pick libfoo/0.1.0 over libfoo/1.0.0 + trace: collect_build_prerequisites: cfg-postpone dependency libfoo/0.1.0 of dependent tex/1.0.0 + trace: postponed_configurations::add: add {tex 2,1: libfoo} to {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1}}? + trace: collect_build_prerequisites: dependent tex/1.0.0 is a shadow dependent for {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1 tex/2,1}}? + trace: collect_build_prerequisites: cfg-postponing dependent tex/1.0.0 involves non-negotiated configurations and results in {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1 tex/2,1}}?, throwing merge_configuration + trace: collect_build_postponed (0): cfg-negotiation of {baz | libbar->{baz/1,1} libfoo->{baz/1,1}} failed due to non-negotiated clusters, force-merging based on shadow cluster {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1 tex/2,1}}? + trace: collect_build_postponed (1): begin {baz | libbar->{baz/1,1} libfoo->{baz/1,1}} + %.* + trace: collect_build_postponed (1): re-evaluate existing dependents for {baz | libbar->{baz/1,1} libfoo->{baz/1,1}} + trace: collect_build: add tex/1.0.0 + trace: collect_build_prerequisites: reeval tex/1.0.0 + %.* + trace: collect_build: pick libbar/0.1.0 over libbar/1.0.0 + trace: postponed_configurations::add: add {tex^ 1,1: libbar} to {baz | libbar->{baz/1,1} libfoo->{baz/1,1}} (shadow cluster-based) + trace: collect_build_prerequisites: re-evaluating dependent tex/1.0.0 results in {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1}} + trace: collect_build_prerequisites: re-evaluated tex/1.0.0 + trace: collect_build_postponed (1): cfg-negotiate begin {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1}} + trace: collect_build_postponed (1): recursively collect cfg-negotiated dependencies + trace: collect_build_prerequisites: begin libbar/0.1.0 + trace: collect_build_prerequisites: end libbar/0.1.0 + trace: collect_build_prerequisites: begin libfoo/0.1.0 + trace: collect_build_prerequisites: end libfoo/0.1.0 + trace: collect_build_postponed (1): recursively collect cfg-negotiated dependents + trace: collect_build_postponed (1): select cfg-negotiated dependency alternative for dependent baz/0.1.0 + trace: collect_build_prerequisites: resume baz/0.1.0 + trace: collect_build_prerequisites: end baz/0.1.0 + trace: collect_build_postponed (1): select cfg-negotiated dependency alternative for dependent tex/1.0.0 + trace: collect_build_prerequisites: resume tex/1.0.0 + %.* + trace: collect_build: pick libfoo/0.1.0 over libfoo/1.0.0 + trace: collect_build_prerequisites: cfg-postpone dependency libfoo/0.1.0 of dependent tex/1.0.0 + trace: postponed_configurations::add: add {tex 2,1: libfoo} to {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1}}? (shadow cluster-based) + trace: collect_build_prerequisites: dependent tex/1.0.0 is a shadow dependent for {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1 tex/2,1}}? + trace: collect_build_prerequisites: configuration for cfg-postponed dependencies of dependent tex/1.0.0 is negotiated + trace: collect_build_prerequisites: dependency libfoo/0.1.0 of dependent tex/1.0.0 is already (being) recursively collected, skipping + trace: collect_build_prerequisites: end tex/1.0.0 + trace: collect_build_postponed (1): cfg-negotiate end {baz tex^ | libbar->{baz/1,1 tex/1,1} libfoo->{baz/1,1 tex/2,1}}! + trace: collect_build_postponed (1): end {baz | libbar->{baz/1,1} libfoo->{baz/1,1}} + trace: collect_build_postponed (0): end + %.* + trace: execute_plan: simulate: yes + %.* EOE + + $pkg_status -r >>EOO; + !tex configured 1.0.0 + libbar configured 0.1.0 available 1.0.0 + libfoo configured 0.1.0 available 1.0.0 + !baz configured !0.1.0 available 1.0.0 + libbar configured 0.1.0 available 1.0.0 + libfoo configured 0.1.0 available 1.0.0 + EOO + + $pkg_drop tex baz } } } -- cgit v1.1