From da825584c23df8574afc0ec1a40403125396ee6d Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Mon, 18 Sep 2023 20:18:00 +0300 Subject: Don't create config cluster for existing dependents in simple cases --- bpkg/pkg-build-collect.cxx | 377 ++++++++++++++++++++++++++++----------------- bpkg/pkg-build-collect.hxx | 74 ++++++--- 2 files changed, 287 insertions(+), 164 deletions(-) (limited to 'bpkg') diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx index 159871b..3581a6b 100644 --- a/bpkg/pkg-build-collect.cxx +++ b/bpkg/pkg-build-collect.cxx @@ -1503,7 +1503,7 @@ namespace bpkg return &p; } - optional> build_packages:: + optional build_packages:: collect_build_prerequisites (const pkg_build_options& options, build_package& pkg, build_package_refs& dep_chain, @@ -1519,7 +1519,8 @@ namespace bpkg postponed_dependencies& postponed_deps, postponed_configurations& postponed_cfgs, unacceptable_alternatives& unacceptable_alts, - optional> reeval_pos) + optional> reeval_pos, + const optional& orig_dep) { // NOTE: don't forget to update collect_build_postponed() if changing // anything in this function. Also enable and run the tests with the @@ -1536,7 +1537,11 @@ namespace bpkg bool pre_reeval (reeval_pos && reeval_pos->first == 0); assert (!pre_reeval || reeval_pos->second == 0); - bool reeval (reeval_pos && reeval_pos->first != 0); + // Must only be specified in the pre-reevaluation mode. + // + assert (orig_dep.has_value () == pre_reeval); + + bool reeval (reeval_pos && reeval_pos->first != 0); assert (!reeval || reeval_pos->second != 0); // The being (pre-)re-evaluated dependent cannot be recursively collected @@ -1569,12 +1574,16 @@ namespace bpkg postponed_edeps.find (pk) == postponed_edeps.end ()) { // Note that there can be multiple existing dependents for a dependency. + // Also note that we skip the existing dependents for which re- + // evaluation is optional not to initiate any negotiation in a simple + // case (see collect_build_prerequisites() description for details). // vector eds ( query_existing_dependents (trace, options, pk.db, pk.name, + true /* exclude_optional */, fdb, rpt_depts, replaced_vers)); @@ -1693,15 +1702,14 @@ namespace bpkg postponed_cfgs); } - // Postpone the original dependency recursive collection if the - // existing dependent has deviated or the dependency belongs to the - // earliest depends clause with configuration clause or to some - // later depends clause. It is supposed that it will be collected - // during its existing dependent re-collection. + // Postpone the recursive collection of a dependency if the existing + // dependent has deviated or the dependency belongs to the earliest + // depends clause with configuration clause or to some later depends + // clause. It is supposed that it will be collected during its + // existing dependent re-collection. // - if (!ed.dependency || // Dependent has deviated. - !ed.orig_dependency_position || // Later depends clause. - *ed.orig_dependency_position == ed.dependency_position) + if (!ed.dependency || // Dependent has deviated. + ed.originating_dependency_position >= ed.dependency_position) { postpone = true; postponed_edeps[pk].emplace_back (ed.db, ed.selected->name); @@ -1873,7 +1881,23 @@ namespace bpkg bool postponed (false); bool reevaluated (false); - vector r; + // In the pre-reevaluation mode keep track of configuration variable + // prefixes similar to what we do in pkg_configure_prerequisites(). Stop + // tracking if we discovered that the dependent re-evaluation is not + // optional. + // + vector banned_var_prefixes; + + auto references_banned_var = [&banned_var_prefixes] (const string& clause) + { + for (const string& p: banned_var_prefixes) + { + if (clause.find (p) != string::npos) + return true; + } + + return false; + }; if (pre_reeval) { @@ -1896,10 +1920,12 @@ namespace bpkg << " deviated: number of depends clauses changed to " << nn << " from " << on;}); - throw reeval_deviated (); + throw reevaluation_deviated (); } } + pre_reevaluate_result r; + for (size_t di (sdeps.size ()); di != deps.size (); ++di) { // Fail if we missed the re-evaluation target position for any reason. @@ -1932,7 +1958,7 @@ namespace bpkg << ": toolchain buildtime dependency replaced the " << " regular one with selected alternative " << oi;}); - throw reeval_deviated (); + throw reevaluation_deviated (); } } @@ -1958,8 +1984,23 @@ namespace bpkg { const dependency_alternative& da (das[i]); - if (!da.enable || - skel.evaluate_enable (*da.enable, make_pair (di, i))) + bool enabled; + + if (da.enable) + { + if (pre_reeval && + r.reevaluation_optional && + references_banned_var (*da.enable)) + { + r.reevaluation_optional = false; + } + + enabled = skel.evaluate_enable (*da.enable, make_pair (di, i)); + } + else + enabled = true; + + if (enabled) edas.push_back (make_pair (ref (da), i)); } } @@ -1978,7 +2019,7 @@ namespace bpkg << ": dependency with previously selected " << "alternative " << oi << " is now disabled";}); - throw reeval_deviated (); + throw reevaluation_deviated (); } } @@ -2106,7 +2147,7 @@ namespace bpkg << " is now in build system module " << "configuration";}); - throw reeval_deviated (); + throw reevaluation_deviated (); } assert (dr == nullptr); // Should fail on the "silent" run. @@ -2173,7 +2214,7 @@ namespace bpkg if (!wildcard (*dep_constr) && !satisfies (*dep_constr, dp.constraint)) { - // We should end up throwing reeval_deviated exception + // We should end up throwing reevaluation_deviated exception // before the diagnostics run in the pre-reevaluation mode. // assert (!pre_reeval || dr == nullptr); @@ -2279,7 +2320,7 @@ namespace bpkg << " deviated: package " << dn << *ddb << " is broken";}); - throw reeval_deviated (); + throw reevaluation_deviated (); } assert (dr == nullptr); // Should fail on the "silent" run. @@ -2414,7 +2455,7 @@ namespace bpkg << db->config_orig << ", " << ldb.config_orig;}); - throw reeval_deviated (); + throw reevaluation_deviated (); } assert (dr == nullptr); // Should fail on the "silent" run. @@ -2448,7 +2489,7 @@ namespace bpkg << "is found for build-time dependency (" << dp << ')';}); - throw reeval_deviated (); + throw reevaluation_deviated (); } // The private config should be created on the "silent" run @@ -2560,7 +2601,7 @@ namespace bpkg << "module " << dn << " in its dependent " << "package configuration " << pdb.config_orig;}); - throw reeval_deviated (); + throw reevaluation_deviated (); } assert (dr == nullptr); // Should fail on the "silent" run. @@ -2596,7 +2637,7 @@ namespace bpkg << pkg.available_name_version_db () << " deviated: is now orphaned";}); - throw reeval_deviated (); + throw reevaluation_deviated (); } assert (dr == nullptr); // Should fail on the "silent" run. @@ -2675,7 +2716,7 @@ namespace bpkg << (!dep_constr ? "" : "user-specified ") << "dependency constraint (" << d << ')';}); - throw reeval_deviated (); + throw reevaluation_deviated (); } if (dep_constr && !system && postponed_repo != nullptr) @@ -2774,7 +2815,7 @@ namespace bpkg // if (dap->system_version (*ddb) == nullptr) { - // We should end up throwing reeval_deviated exception + // We should end up throwing reevaluation_deviated exception // before the diagnostics run in the pre-reevaluation mode. // assert (!pre_reeval || dr == nullptr); @@ -2790,7 +2831,7 @@ namespace bpkg if (!satisfies (*dap->system_version (*ddb), d.constraint)) { - // We should end up throwing reeval_deviated exception + // We should end up throwing reevaluation_deviated exception // before the diagnostics run in the pre-reevaluation mode. // assert (!pre_reeval || dr == nullptr); @@ -2880,7 +2921,7 @@ namespace bpkg { if (!satisfies (v1, c2.value)) { - // We should end up throwing reeval_deviated exception + // We should end up throwing reevaluation_deviated exception // before the diagnostics run in the pre-reevaluation // mode. // @@ -3691,6 +3732,7 @@ namespace bpkg options, d.db, d.name, + false /* exclude_optional */, fdb, rpt_depts, replaced_vers)) @@ -3944,31 +3986,101 @@ namespace bpkg }; // Select a dependency alternative, copying it alone into the resulting - // dependencies list and evaluating its reflect clause, if present. + // dependencies list and evaluating its reflect clause, if present. In + // the pre-reevaluation mode update the variable prefixes list, if the + // selected alternative has config clause, and the pre-reevaluation + // resulting information (re-evaluation position, etc). + // + // Note that prebuilds are only used in the pre-reevaluation mode. // bool selected (false); - auto select = [&sdeps, &salts, &sdas, &skel, di, &selected] - (const dependency_alternative& da, size_t dai) + auto select = [&sdeps, &salts, &sdas, + &skel, + di, + &selected, + pre_reeval, &banned_var_prefixes, &references_banned_var, + &orig_dep, + &r] (const dependency_alternative& da, + size_t dai, + prebuilds&& pbs) + { + assert (sdas.empty ()); + + if (pre_reeval) { - assert (sdas.empty ()); + pair pos (di + 1, dai + 1); - // Avoid copying enable/reflect not to evaluate them repeatedly. + bool contains_orig_dep ( + find_if (pbs.begin (), pbs.end (), + [&orig_dep] (const prebuild& pb) + { + return pb.dependency.name == orig_dep->name && + pb.db == orig_dep->db; + }) != pbs.end ()); + + // If the selected alternative contains the originating dependency, + // then set the originating dependency position, unless it is + // already set (note that the same dependency package may + // potentially be specified in multiple depends clauses). // - sdas.emplace_back (nullopt /* enable */, - nullopt /* reflect */, - da.prefer, - da.accept, - da.require, - da /* dependencies */); + if (contains_orig_dep && r.originating_dependency_position.first == 0) + r.originating_dependency_position = pos; - sdeps.push_back (move (sdas)); - salts.push_back (dai); + if (da.prefer || da.require) + { + if (contains_orig_dep) + r.reevaluation_optional = false; - if (da.reflect) - skel.evaluate_reflect (*da.reflect, make_pair (di, dai)); + // If this is the first selected alternative with the config + // clauses, then save its position and the dependency packages. + // + if (r.reevaluation_position.first == 0) + { + r.reevaluation_position = pos; - selected = true; - }; + for (prebuild& pb: pbs) + r.reevaluation_dependencies.emplace_back ( + pb.db, move (pb.dependency.name)); + } + + // Save the variable prefixes for the selected alternative + // dependencies, if we still track them. + // + if (r.reevaluation_optional) + { + for (const dependency& d: da) + banned_var_prefixes.push_back ( + "config." + d.name.variable () + '.'); + } + } + } + + // Avoid copying enable/reflect not to evaluate them repeatedly. + // + sdas.emplace_back (nullopt /* enable */, + nullopt /* reflect */, + da.prefer, + da.accept, + da.require, + da /* dependencies */); + + sdeps.push_back (move (sdas)); + salts.push_back (dai); + + if (da.reflect) + { + if (pre_reeval && + r.reevaluation_optional && + references_banned_var (*da.reflect)) + { + r.reevaluation_optional = false; + } + + skel.evaluate_reflect (*da.reflect, make_pair (di, dai)); + } + + selected = true; + }; // Postpone the prerequisite builds collection, optionally inserting the // package to the postponements set (can potentially already be there) @@ -4039,18 +4151,6 @@ namespace bpkg // bool reused_only (false); - auto pre_reeval_append_result = [&r] (pair pos, - prebuilds&& builds) - { - postponed_configuration::packages deps; - deps.reserve (builds.size ()); - - for (prebuild& b: builds) - deps.emplace_back (b.db, move (b.dependency.name)); - - r.emplace_back (pos, move (deps), nullopt /* has_alternative */); - }; - for (size_t i (0); i != edas.size (); ++i) { // Skip the unacceptable alternatives. @@ -4142,9 +4242,7 @@ namespace bpkg &trace, &postpone, &collect, - &select, - &pre_reeval_append_result] - (size_t index, precollect_result&& pcr) + &select] (size_t index, precollect_result&& pcr) { const auto& eda (edas[index]); const dependency_alternative& da (eda.first); @@ -4192,16 +4290,7 @@ namespace bpkg << ": selected alternative changed to " << ni << " from " << oi;}); - throw reeval_deviated (); - } - - pre_reeval_append_result (make_pair (di + 1, ni), - move (*pcr.builds)); - - if (da.prefer || da.require) - { - postpone (nullptr); - return true; + throw reevaluation_deviated (); } } else if (!collect (da, dai, move (*pcr.builds), prereqs)) @@ -4210,7 +4299,7 @@ namespace bpkg return true; } - select (da, dai); + select (da, dai, move (*pcr.builds)); // Make sure no more true alternatives are selected during this // function call unless we are (pre-)reevaluating a dependent. @@ -4284,16 +4373,7 @@ namespace bpkg << ": selected alternative (single) changed to " << ni << " from " << oi;}); - throw reeval_deviated (); - } - - pre_reeval_append_result (make_pair (di + 1, ni), - move (*pcr.builds)); - - if (da.prefer || da.require) - { - postpone (nullptr); - break; + throw reevaluation_deviated (); } } else if (!collect (da, dai, move (*pcr.builds), prereqs)) @@ -4302,7 +4382,7 @@ namespace bpkg break; } - select (da, dai); + select (da, dai, move (*pcr.builds)); } } @@ -4329,7 +4409,7 @@ namespace bpkg << ": now can't select alternative, previously " << oi << " was selected";}); - throw reeval_deviated (); + throw reevaluation_deviated (); } if (reeval) @@ -4445,7 +4525,15 @@ namespace bpkg } } - if (postponed) + // Bail out if the collection is postponed or we are in the + // pre-reevaluation mode and have already collected all the required + // information. + // + if (postponed || + (pre_reeval && + r.reevaluation_position.first != 0 && + r.originating_dependency_position.first != 0 && + !r.reevaluation_optional)) break; } @@ -4459,18 +4547,34 @@ namespace bpkg if (pre_reeval) { + // It doesn't feel like it may happen in the pre-reevaluation mode. If + // it still happens, maybe due to some manual tampering, let's assume + // this as a deviation case. + // + if (r.originating_dependency_position.first == 0) + { + l5 ([&]{trace << "re-evaluation of dependent " + << pkg.available_name_version_db () + << " deviated: previously selected dependency " + << *orig_dep << " is not selected anymore";}); + + throw reevaluation_deviated (); + } + l5 ([&] { diag_record dr (trace); dr << "pre-reevaluated " << pkg.available_name_version_db () << ": "; - if (postponed) + pair pos (r.reevaluation_position); + + if (pos.first != 0) { - assert (!r.empty ()); + dr << pos.first << ',' << pos.second; - dr << r.back ().position.first << ',' - << r.back ().position.second; + if (r.reevaluation_optional) + dr << " re-evaluation is optional"; } else dr << "end reached"; @@ -4486,9 +4590,9 @@ namespace bpkg << pkg.available_name_version_db ();}); } - return pre_reeval && postponed - ? optional> (move (r)) - : nullopt; + return pre_reeval && r.reevaluation_position.first != 0 + ? move (r) + : optional (); } void build_packages:: @@ -5002,6 +5106,7 @@ namespace bpkg o, p.db, p.name, + false /* exclude_optional */, fdb, rpt_depts, replaced_vers)) @@ -6104,6 +6209,7 @@ namespace bpkg o, pk.db, pk.name, + false /* exclude_optional */, fdb, rpt_depts, replaced_vers)) @@ -6712,6 +6818,7 @@ namespace bpkg const pkg_build_options& o, database& db, const package_name& name, + bool exclude_optional, const function& fdb, const repointed_dependents& rpt_depts, const replaced_versions& replaced_vers) @@ -6828,6 +6935,8 @@ namespace bpkg lazy_shared_ptr> rp ( find_available_fragment (o, ddb, dsp)); + optional orig_dep (package_key {db, name}); + try { build_package p { @@ -6865,7 +6974,7 @@ namespace bpkg unacceptable_alternatives unacceptable_alts; replaced_versions replaced_vers; - optional> deps ( + optional pr ( collect_build_prerequisites (o, p, dep_chain, @@ -6881,7 +6990,8 @@ namespace bpkg postponed_deps, postponed_cfgs, unacceptable_alts, - pair (0, 0))); + pair (0, 0), + orig_dep)); // Must be read-only. // @@ -6894,49 +7004,36 @@ namespace bpkg unacceptable_alts.empty () && replaced_vers.empty ()); - if (deps) + if (pr && (!pr->reevaluation_optional || !exclude_optional)) { - package_key pk {db, name}; - - assert (!deps->empty ()); - - // Try to retrieve the original dependency position. If we fail, - // then this dependency belongs to the depends clause which comes - // after the re-evaluation target position. - // - optional> odp; - - for (const postponed_configuration::dependency& d: *deps) - { - if (find (d.begin (), d.end (), pk) != d.end ()) - { - odp = d.position; - break; - } - } - - // Try to preserve the name of the original dependency as the one - // which brings the existing dependent to the config cluster. + // Try to preserve the name of the originating dependency as the + // one which brings the existing dependent to the config cluster. // Failed that, use the first dependency in the alternative which // we will be re-evaluating to. // - postponed_configuration::dependency& d (deps->back ()); + package_key dep (*orig_dep); + + pre_reevaluate_result::packages& deps ( + pr->reevaluation_dependencies); + + assert (!deps.empty ()); - if (find (d.begin (), d.end (), pk) == d.end ()) - pk = move (d.front ()); + if (find (deps.begin (), deps.end (), dep) == deps.end ()) + dep = move (deps.front ()); r.push_back ( - existing_dependent {ddb, move (dsp), - move (pk), d.position, - package_key {db, name}, odp}); + existing_dependent { + ddb, move (dsp), + move (dep), pr->reevaluation_position, + move (*orig_dep), pr->originating_dependency_position}); } } - catch (const reeval_deviated&) + catch (const reevaluation_deviated&) { r.push_back ( existing_dependent {ddb, move (dsp), nullopt, {}, - package_key {db, name}, nullopt}); + move (*orig_dep), {}}); } } } @@ -7080,23 +7177,23 @@ namespace bpkg ed.selected, move (rp.first), move (rp.second), - nullopt, // Dependencies. - nullopt, // Dependencies alternatives. - nullopt, // Package skeleton. - nullopt, // Postponed dependency alternatives. - false, // Recursive collection. - nullopt, // Hold package. - nullopt, // Hold version. - {}, // Constraints. - false, // System. - false, // Keep output directory. - false, // Disfigure (from-scratch reconf). - false, // Configure-only. - nullopt, // Checkout root. - false, // Checkout purge. - strings (), // Configuration variables. - {ed.orig_dependency}, // Required by (dependency). - false, // Required by dependents. + nullopt, // Dependencies. + nullopt, // Dependencies alternatives. + nullopt, // Package skeleton. + nullopt, // Postponed dependency alternatives. + false, // Recursive collection. + nullopt, // Hold package. + nullopt, // Hold version. + {}, // Constraints. + false, // System. + false, // Keep output directory. + false, // Disfigure (from-scratch reconf). + false, // Configure-only. + nullopt, // Checkout root. + false, // Checkout purge. + strings (), // Configuration variables. + {ed.originating_dependency}, // Required by (dependency). + false, // Required by dependents. flags}; // Note: not recursive. diff --git a/bpkg/pkg-build-collect.hxx b/bpkg/pkg-build-collect.hxx index 1c3bc09..d3bcb79 100644 --- a/bpkg/pkg-build-collect.hxx +++ b/bpkg/pkg-build-collect.hxx @@ -1248,15 +1248,31 @@ namespace bpkg // exception if such a cycle is detected. // // If {0,0} is specified as the reeval_pos argument, then perform the - // pre-reevaluation. In this read-only mode perform the regular dependency - // alternative selection but not the actual dependency collection and stop - // when all the depends clauses are processed or an alternative with the - // configuration clause is encountered. In the latter case return the list - // of the selected alternative dependencies/positions, where the last - // entry corresponds to the alternative with the encountered configuration - // clause. Return nullopt otherwise. Also look for any deviation in the - // dependency alternatives selection and throw reeval_deviated exception - // if such a deviation is detected. + // pre-reevaluation of an existing dependent, requested due to the + // specific dependency up/down-grade or reconfiguration (must be passed as + // the orig_dep; we call it originating dependency). The main purpose of + // this read-only mode is to obtain the position of the earliest selected + // dependency alternative with the config clause, if any, which the + // re-evaluation needs to be performed to and to determine if such a + // re-evaluation is optional (see pre_reevaluate_result for the full + // information being retrieved). The re-evaluation is considered to be + // optional if the existing dependent has no config clause for the + // originating dependency and the enable and reflect clauses do not refer + // to any of the dependency configuration variables (which can only be + // those which the dependent has the configuration clauses for; see the + // bpkg manual for details). The thinking here is that such an existing + // dependent may not change any configuration it applies to its + // dependencies and thus it doesn't call for any negotiations (note: if + // there are config clauses for the upgraded originating dependency, then + // the potentially different defaults for its config variables may affect + // the configuration this dependent applies to its dependencies). Such a + // dependent can also be reconfigured without pre-selection of its + // dependency alternatives since pkg-configure is capable of doing that on + // its own for such a simple case (see pkg_configure_prerequisites() for + // details). Also look for any deviation in the dependency alternatives + // selection and throw reevaluation_deviated exception if such a deviation + // is detected. Return nullopt if no dependency alternative with the + // config clause is selected. // // If the package is a dependency of configured dependents and needs to be // reconfigured (being upgraded, has configuration specified, etc), then @@ -1319,9 +1335,19 @@ namespace bpkg size_t depth; }; - struct reeval_deviated {}; + struct reevaluation_deviated {}; - optional> + struct pre_reevaluate_result + { + using packages = postponed_configuration::packages; + + pair reevaluation_position; + packages reevaluation_dependencies; + bool reevaluation_optional = true; + pair originating_dependency_position; + }; + + optional collect_build_prerequisites (const pkg_build_options&, build_package&, build_package_refs& dep_chain, @@ -1337,7 +1363,8 @@ namespace bpkg postponed_dependencies&, postponed_configurations&, unacceptable_alternatives&, - optional> reeval_pos = nullopt); + optional> reeval_pos = nullopt, + const optional& orig_dep = nullopt); void collect_build_prerequisites (const pkg_build_options&, @@ -1476,28 +1503,26 @@ namespace bpkg // in the map) or expected to be built or dropped (present in rpt_depts or // replaced_vers). Also skip dependents which impose the version // constraint on this dependency and the dependency doesn't satisfy this - // constraint. + // constraint. Optionally, skip the existing dependents for which + // re-evaluation is considered optional (exclude_optional argument; see + // pre-reevaluation mode of collect_build_prerequisites() for details). // struct existing_dependent { // Dependent. // - reference_wrapper db; - shared_ptr selected; + reference_wrapper db; + shared_ptr selected; // Earliest dependency with config clause. // - optional dependency; - pair dependency_position; + optional dependency; + pair dependency_position; - // Original dependency. - // - // Note that we always know the original dependency but may not be able - // to obtain its position if it comes after the earliest dependency with - // config clause or the dependent deviates. + // Originating dependency passed to the function call. // - package_key orig_dependency; - optional> orig_dependency_position; + package_key originating_dependency; + pair originating_dependency_position; }; // This exception is thrown by collect_build_prerequisites() and @@ -1517,6 +1542,7 @@ namespace bpkg const pkg_build_options&, database&, const package_name&, + bool exclude_optional, const function&, const repointed_dependents&, const replaced_versions&); -- cgit v1.1