From 48c9bb1534b17f2e5a9182dc76d2bd1b93e32574 Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Wed, 26 Jul 2023 19:55:52 +0300 Subject: Fix unexpected 'manual configuration of dependents with prefer or require clauses is not yet supported' error (GH issue #302) --- bpkg/pkg-build.cxx | 47 +++++++++----- bpkg/pkg-configure.cxx | 168 +++++++++++++++++++++++++++++++++++++++++-------- bpkg/pkg-configure.hxx | 46 +++++++++++--- 3 files changed, 210 insertions(+), 51 deletions(-) (limited to 'bpkg') diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx index ed23ea5..9a73ea6 100644 --- a/bpkg/pkg-build.cxx +++ b/bpkg/pkg-build.cxx @@ -5699,10 +5699,9 @@ namespace bpkg // On the package reconfiguration we will try to resolve dependencies to // the same prerequisites (see pkg_configure() for details). For that, we - // will save prerequisites before disfiguring the dependents. Note, - // though, that this is not required for dependents with the collected - // prerequisites builds since the dependency alternatives are already - // selected for them. + // will save prerequisites before disfiguring a package. Note, though, + // that this is not required for the recursively collected packages since + // the dependency alternatives are already selected for them. // map> previous_prerequisites; @@ -5727,7 +5726,7 @@ namespace bpkg bool external (false); if (!simulate) { - external = sp != nullptr && sp->external () && p.external (); + external = (sp != nullptr && sp->external () && p.external ()); // Reset the keep_out flag if the package being unpacked is not // external. @@ -5736,16 +5735,27 @@ namespace bpkg p.keep_out = false; } - if (*p.action != build_package::drop && - !p.dependencies && - !sp->prerequisites.empty ()) + // Save prerequisites before disfiguring the package. + // + // Note that we add the prerequisites list to the map regardless if + // there are any prerequisites or not to, in particular, indicate the + // package reconfiguration mode to the subsequent + // pkg_configure_prerequisites() call (see the function documentation + // for details). + // + if (*p.action != build_package::drop && !p.dependencies && !p.system) { vector& ps (previous_prerequisites[&p]); - ps.reserve (sp->prerequisites.size ()); + assert (sp != nullptr); // Shouldn't be here otherwise. + + if (!sp->prerequisites.empty ()) + { + ps.reserve (sp->prerequisites.size ()); - for (const auto& pp: sp->prerequisites) - ps.push_back (pp.first.object_id ()); + for (const auto& pp: sp->prerequisites) + ps.push_back (pp.first.object_id ()); + } } // For an external package being replaced with another external, keep @@ -6195,12 +6205,6 @@ namespace bpkg info << "while configuring " << p.name () << p.db; })); - auto prereqs = [&p, &previous_prerequisites] () - { - auto i (previous_prerequisites.find (&p)); - return i != previous_prerequisites.end () ? &i->second : nullptr; - }; - configure_prerequisites_result cpr; if (p.system) { @@ -6219,6 +6223,15 @@ namespace bpkg } else { + // Should only be called for packages whose prerequisites are saved. + // + auto prereqs = [&p, &previous_prerequisites] () + { + auto i (previous_prerequisites.find (&p)); + assert (i != previous_prerequisites.end ()); + return &i->second; + }; + if (ap != nullptr) { assert (*p.action == build_package::build); diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx index 291929b..5bb7dfa 100644 --- a/bpkg/pkg-configure.cxx +++ b/bpkg/pkg-configure.cxx @@ -51,11 +51,109 @@ namespace bpkg // assert (unconstrain_deps == nullptr || simulate); + // No use case for both being specified. + // + assert (alts == nullptr || prev_prereqs == nullptr); + tracer_guard tg (db, trace); package_prerequisites prereqs; strings vars; + // Notes on the buildfile clauses evaluation: + // + // - In the manual configuration mode (alts == NULL, prev_prereqs == NULL) + // we always evaluate the enable and reflect clauses. We, however, fail + // if any of the prefer or require clauses are specified in any of the + // enabled dependency alternatives, assuming that this package didn't + // negotiate its preferences/requirements for the dependency + // configurations. + // + // Note that evaluating the require and prefer clauses in this case is + // meaningless since we don't reconfigure the dependencies nor negotiate + // configurations with other dependents. What we should probably do is + // load configurations of the dependencies and use them while evaluating + // the dependent's enable and reflect clauses as we go along. Probably + // we should still evaluate the accept clauses to make sure that the + // dependency is configured acceptably for the dependent. + // + // - In the pre-selected alternatives mode (alts != NULL, prev_prereqs == + // NULL) we don't evaluate the enable, prefer, and require clauses since + // they have already been evaluated as a part of the dependency + // alternatives selection and the dependency configurations negotiation. + // We, however always evaluate the reflect clauses. + // + // - In the reconfiguration mode (prev_prereqs != NULL, alts == NULL) we + // don't evaluate the prefer and require clauses, assuming that was done + // on some previous pkg-build run when this package and its dependencies + // have been configured. But because of this we may not evaluate the + // enable and reflect clauses which refer to dependency configuration + // variables. Thus, for now we fail if such an enable or reflect clause + // is encountered and evaluate all the enable and reflect clauses + // otherwise. In the future it feels like such cases should somehow + // be handled at the pkg-build level (e.g., detect such situations and + // cause the dependent re-evaluation instead of re-configuration). Note + // that if/when this is implemented, the current logic could still be + // used as an optimization (reflection of dependency configuration is + // not very common). + // + // @@ It seems there is a hole in the reconfiguration mode: + // + // foo -> libfoo {require {config.libfoo.proto=1} + // reflect {config.foo.reflect=1}} | + // libfoo {require {config.libfoo.proto=2} + // reflect {config.foo.reflect=2}} + // -> libbar {require {config.libfoo.proto=2}} + // -> libbaz + // + // foo is configured with config.foo.reflect=2 + // + // If afterwords libbaz is upgraded, the foo is + // reconfigured with config.foo.reflect=1 + // + // It feels more and more that the proper solution here is to go + // "nuclear" and to always "upgrade" re-configuration for a package + // with any prefer/require into a complete re-evaluation. The most + // likely drawback of this approach would be unnecessary + // re-evaluations if we use the "any prefer/require in any + // alternative" condition. So we may need to exclude some common + // simple cases where such an upgrade is not needed. For example: + // + // foo -> libfoo require {config.libfoo.proto=1} + // foo -> libbar require {config.libbar.proto=1} + // foo -> libbaz + // + // Feels like the condition will have to be based on the presence + // of clauses plus their textual analysis. + // + bool manual (alts == nullptr && prev_prereqs == nullptr); + + // In the reconfiguration mode keep track of configuration variable + // prefixes (in the 'config..' form) for dependencies in the + // selected alternatives with the prefer or require clauses specified and + // fail if any enable or reflect clause refers to them. + // + // Note that the enable and reflect clauses may only refer to dependency + // configuration variables of already selected alternatives with the + // prefer or require clauses specified. + // + vector banned_var_prefixes; + + auto verify_banned_vars = [&ps, + &banned_var_prefixes] (const string& clause, + const char* what) + { + for (const string& p: banned_var_prefixes) + { + if (clause.find (p) != string::npos) + { + fail << "unable to reconfigure dependent " << ps.package.name + << " with " << what << " clause that refers to dependency " + << "configuration variables"; + } + } + }; + // Alternatives argument must be parallel to the dependencies argument if // specified. // @@ -75,18 +173,6 @@ namespace bpkg size_t>, 2> edas; - // If the dependency alternatives are not pre-selected, then evaluate - // the enable clauses. - // - // Note that evaluating the require and prefer clauses in this case is - // meaningless since we don't reconfigure the dependencies nor negotiate - // configurations with other dependents. What we should probably do is - // load configurations of the dependencies and use them while evaluating - // the dependent's enable and reflect clauses as we go along. Probably - // we should still evaluate the accept clauses to make sure that the - // dependency is configured acceptably for the dependent. For now we - // fail and will support this maybe later. - // if (alts == nullptr) { if (toolchain_buildtime_dependency (o, das, &ps.package.name)) @@ -96,14 +182,23 @@ namespace bpkg { const dependency_alternative& da (das[i]); - if (!da.enable || ps.evaluate_enable (*da.enable, make_pair (di, i))) + // Evaluate the dependency alternative enable clause, if present, + // unless it refers to any banned variables in which case we fail. + // + if (da.enable) { - if (da.prefer || da.require) - fail << "manual configuration of dependents with prefer or " - << "require clauses is not yet supported"; + if (!banned_var_prefixes.empty ()) + verify_banned_vars (*da.enable, "enable"); - edas.push_back (make_pair (ref (da), i)); + if (!ps.evaluate_enable (*da.enable, make_pair (di, i))) + continue; } + + if (manual && (da.prefer || da.require)) + fail << "manual configuration of dependents with prefer or " + << "require clauses is not yet supported"; + + edas.push_back (make_pair (ref (da), i)); } if (edas.empty ()) @@ -130,7 +225,9 @@ namespace bpkg // for (const vector* pps (prev_prereqs);;) { - bool satisfied (false); + const pair, + size_t>* selected_alt (nullptr); + for (const auto& eda: edas) { const dependency_alternative& da (eda.first); @@ -335,12 +432,7 @@ namespace bpkg } } - // Evaluate the dependency alternative reflect clause, if present. - // - if (da.reflect) - ps.evaluate_reflect (*da.reflect, make_pair (di, dai)); - - satisfied = true; + selected_alt = &eda; break; } @@ -348,7 +440,7 @@ namespace bpkg // "recreate dependency decisions" mode. In the latter case fall back // to the "make dependency decisions" mode and retry. // - if (!satisfied) + if (selected_alt == nullptr) { if (pps != nullptr) { @@ -359,6 +451,32 @@ namespace bpkg fail << "unable to satisfy dependency on " << das; } + const dependency_alternative& da (selected_alt->first); + + // In the reconfiguration mode ban the usage of the selected + // alternative dependency configuration variables in the subsequent + // enable and reflect clauses. + // + if (alts == nullptr && !manual && (da.prefer || da.require)) + { + for (const dependency& d: da) + banned_var_prefixes.push_back ( + "config." + d.name.variable () + '.'); + } + + // Evaluate the selected dependency alternative reflect clause, if + // present, unless it refers to any banned variables in which case we + // fail. + // + if (da.reflect) + { + if (!banned_var_prefixes.empty ()) + verify_banned_vars (*da.reflect, "reflect"); + + ps.evaluate_reflect (*da.reflect, + make_pair (di, selected_alt->second)); + } + // The dependency alternative is selected and its dependencies are // resolved to the selected packages. So proceed to the next depends // value. diff --git a/bpkg/pkg-configure.hxx b/bpkg/pkg-configure.hxx index 24e06a3..876d11a 100644 --- a/bpkg/pkg-configure.hxx +++ b/bpkg/pkg-configure.hxx @@ -45,11 +45,10 @@ namespace bpkg // Given dependencies of a package, return its prerequisite packages, // configuration variables that resulted from selection of these // prerequisites (import, reflection, etc), and sources of the configuration - // variables resulted from evaluating the reflect clauses. See - // pkg_configure() for the semantics of the dependency list. Fail if for - // some of the dependency alternative lists there is no satisfactory - // alternative (all its dependencies are configured, satisfy the respective - // constraints, etc). + // variables resulted from evaluating the reflect clauses. Fail if for some + // of the dependency alternative lists there is no satisfactory alternative + // (all its dependencies are configured, satisfy the respective constraints, + // etc). // // The package dependency constraints are expected to be complete. // @@ -62,12 +61,41 @@ namespace bpkg // be parallel to the dependencies argument and specify indexes of the // selected alternatives. // - // If prerequisites corresponding to the previous configured state of the - // package are specified, then for each depends value try to select an - // alternative where dependencies all belong to this list (the "recreate + // If the dependency alternatives are not pre-selected (alternatives == + // NULL), then for each depends value select the first satisfactory + // alternative encountered. If, however, prerequisites corresponding to the + // previous configured state of the package are specified + // (prev_prerequisites != NULL), then for each depends value try to select + // an alternative where dependencies all belong to this list (the "recreate // dependency decisions" mode). Failed that, select an alternative as if no // prerequisites are specified (the "make dependency decisions" mode). // + // Note that there are actually 3 possible use cases for + // pkg_configure_prerequisites(): + // + // - The package is being configured manually. In this case its dependency + // alternatives are not pre-selected and there is no information about its + // previous configured state (alternatives == NULL, prev_prerequisites == + // NULL). + // + // - The package is being built, upgraded, or re-evaluated. In this case its + // dependency alternatives are pre-selected, their enable, prefer, and + // require clauses are evaluated, and there is no need in the previous + // configured state information (alternatives != NULL, prev_prerequisites + // == NULL). + // + // - The package is being reconfigured for a reason other than any of the + // mentioned above (dependency up/down-grade/reconfiguration, deorphaning, + // pkg-build --disfigure is specified, etc). In this case its dependency + // alternatives are not pre-selected but the previous configured state + // information is provided (alternatives == NULL, prev_prerequisites != + // NULL). + // + // - There are no use cases when both dependency alternatives are + // pre-selected and the previous configured state information needs to be + // provided. Thus, alternatives and prev_prerequisites must never be both + // NULL. + // // Optionally, remove constraints from the specified dependencies // (unconstrain_deps). Only allowed in the simulation mode. // @@ -122,7 +150,7 @@ namespace bpkg // required. // // Note: expects all the non-external packages to be configured to be - // already unpackged (for subproject discovery). + // already unpacked (for subproject discovery). // void pkg_configure (const common_options&, -- cgit v1.1