From 306ad185e8d8f50923316bdd1b68ef9a3b1e50e6 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Tue, 11 Apr 2023 14:47:42 +0200 Subject: Split package configuration into two passes in pkg-build --- bpkg/pkg-build.cxx | 309 ++++++++++++++++++++++++++++++++----------------- bpkg/pkg-configure.cxx | 178 ++++++++++++++-------------- bpkg/pkg-configure.hxx | 75 ++++++++++-- 3 files changed, 359 insertions(+), 203 deletions(-) diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx index d30555f..2484880 100644 --- a/bpkg/pkg-build.cxx +++ b/bpkg/pkg-build.cxx @@ -5350,14 +5350,37 @@ namespace bpkg return true; }; - if (progress) + // On the first pass collect all the build_package's to be configured and + // calculate their configure_prerequisites_result's. + // + struct configure_package { - prog_i = 0; - prog_n = static_cast (count_if (build_pkgs.begin (), - build_pkgs.end (), - configure_pred)); - prog_percent = 100; - } + reference_wrapper pkg; + configure_prerequisites_result res; // Unused for system package. + }; + vector configure_packages; + + configure_packages.reserve (build_pkgs.size ()); + + // Return the "would be" state of packages that would be configured + // by this stage. + // + function configured_state ( + [&configure_packages] (const shared_ptr& sp) + -> optional> + { + for (const configure_package& cp: configure_packages) + { + const build_package& p (cp.pkg); + + if (p.selected == sp) + return make_pair ( + package_state::configured, + p.system ? package_substate::system : package_substate::none); + } + + return nullopt; + }); for (build_package& p: reverse_iterate (build_pkgs)) { @@ -5369,7 +5392,7 @@ namespace bpkg shared_ptr& sp (p.selected); const shared_ptr& ap (p.available); - // Configure the package. + // Collect the package. // // At this stage the package is either selected, in which case it's a // source code one, or just available, in which case it is a system @@ -5381,7 +5404,6 @@ namespace bpkg assert (sp != nullptr || p.system); database& pdb (p.db); - transaction t (pdb, !simulate /* start */); // Show how we got here if things go wrong, for example selecting a @@ -5401,131 +5423,202 @@ namespace bpkg return i != previous_prerequisites.end () ? &i->second : nullptr; }; - // Note that pkg_configure() commits the transaction. - // + configure_prerequisites_result cpr; if (p.system) { + // We have no choice but to configure system packages on the first + // pass since otherwise there will be no selected package for + // pkg_configure_prerequisites() to find. Luckily they have no + // dependencies and so can be configured in any order. We will print + // their progress/result on the second pass in the proper order. + // + // Note: commits the transaction. + // sp = pkg_configure_system (ap->id.name, p.available_version (), pdb, t); } - else if (ap != nullptr) + else { - assert (*p.action == build_package::build); - - // If the package prerequisites builds are collected, then use the - // 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 - // it due, for example, to an upgrade of its dependency. In this case - // we pass to pkg_configure() the newly created package skeleton which - // contains the package configuration variables specified on the - // command line but (naturally) no reflection configuration variables. - // Note, however, that in this case pkg_configure() call will evaluate - // the reflect clauses itself and so the proper reflection variables - // will still end up in the package configuration. - // - // @@ Note that if we ever allow the user to override the alternative - // selection, this will break (and also if the user re-configures - // the package manually). Maybe that a good reason not to allow - // this? Or we could store this information in the database. - // - if (p.dependencies) + if (ap != nullptr) { - assert (p.skeleton); + assert (*p.action == build_package::build); - pkg_configure (o, - pdb, - t, - sp, - *p.dependencies, - &*p.alternatives, - move (*p.skeleton), - nullptr /* previous_prerequisites */, - p.disfigure, - simulate, - fdb); + // If the package prerequisites builds are collected, then use the + // 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 it due, for example, to an upgrade of its dependency. + // In this case we pass to pkg_configure() the newly created package + // skeleton which contains the package configuration variables + // specified on the command line but (naturally) no reflection + // configuration variables. Note, however, that in this case + // pkg_configure() call will evaluate the reflect clauses itself and + // so the proper reflection variables will still end up in the + // package configuration. + // + // @@ Note that if we ever allow the user to override the + // alternative selection, this will break (and also if the user + // re-configures the package manually). Maybe that a good reason + // not to allow this? Or we could store this information in the + // database. + // + if (p.dependencies) + { + assert (p.skeleton); + + cpr = pkg_configure_prerequisites (o, + pdb, + t, + *p.dependencies, + &*p.alternatives, + move (*p.skeleton), + nullptr /* prev_prerequisites */, + simulate, + fdb, + configured_state); + } + else + { + assert (sp != nullptr); // See above. + + // Note that the skeleton can be present if, for example, this is + // a dependency which configuration has been negotiated but it is + // not collected recursively since it has no buildfile clauses. + // + if (!p.skeleton) + p.init_skeleton (o); + + cpr = pkg_configure_prerequisites (o, + pdb, + t, + ap->dependencies, + nullptr /* alternatives */, + move (*p.skeleton), + prereqs (), + simulate, + fdb, + configured_state); + } } - else + else // Dependent. { - assert (sp != nullptr); // See above. + // This is an adjustment of a dependent which cannot be system + // (otherwise it wouldn't be a dependent) and cannot become system + // (otherwise it would be a build). + // + assert (*p.action == build_package::adjust && !sp->system ()); - // Note that the skeleton can be present if, for example, this is a - // dependency which configuration has been negotiated but it is not - // collected recursively since it has no buildfile clauses. + // Must be in the unpacked state since it was disfigured on the + // first pass (see above). // + assert (sp->state == package_state::unpacked); + + // Initialize the skeleton if it is not initialized yet. + // + // Note that the skeleton can only be present here if it was + // initialized during the preparation of the plan and so this plan + // execution is not simulated (see above for details). + // + // Also note that there is no available package specified for the + // build package object here and so we need to find it (or create a + // transient one). + // + assert (p.available == nullptr && (!p.skeleton || !simulate)); + if (!p.skeleton) - p.init_skeleton (o); + p.init_skeleton (o, find_available (o, pdb, sp)); - pkg_configure (o, - pdb, - t, - sp, - ap->dependencies, - nullptr /* alternatives */, - move (*p.skeleton), - prereqs (), - p.disfigure, - simulate, - fdb); + assert (p.skeleton->available != nullptr); // Can't be system. + + const dependencies& deps (p.skeleton->available->dependencies); + + // @@ Note that on reconfiguration the dependent looses the + // potential configuration variables specified by the user on + // some previous build, which can be quite surprising. Should we + // store this information in the database? + // + // Note: this now works for external packages via package + // skeleton (which extracts user configuration). + // + cpr = pkg_configure_prerequisites (o, + pdb, + t, + deps, + nullptr /* alternatives */, + move (*p.skeleton), + prereqs (), + simulate, + fdb, + configured_state); } + + t.commit (); } - else // Dependent. - { - // This is an adjustment of a dependent which cannot be system - // (otherwise it wouldn't be a dependent) and cannot become system - // (otherwise it would be a build). - // - assert (*p.action == build_package::adjust && - !p.system && - !sp->system ()); - // Must be in the unpacked state since it was disfigured on the first - // pass (see above). - // - assert (sp->state == package_state::unpacked); + configure_packages.push_back (configure_package {p, move (cpr)}); + } - // Initialize the skeleton if it is not initialized yet. - // - // Note that the skeleton can only be present here if it was - // initialized during the preparation of the plan and so this plan - // execution is not simulated (see above for details). - // - // Also note that there is no available package specified for the - // build package object here and so we need to find it (or create a - // transient one). - // - assert (p.available == nullptr && (!p.skeleton || !simulate)); + if (progress) + { + prog_i = 0; + prog_n = configure_packages.size (); + prog_percent = 100; + } - if (!p.skeleton) - p.init_skeleton (o, find_available (o, pdb, sp)); + for (configure_package& cp: configure_packages) + { + build_package& p (cp.pkg); - assert (p.skeleton->available != nullptr); // Can't be system. + const shared_ptr& sp (p.selected); - const dependencies& deps (p.skeleton->available->dependencies); + // Configure the package (system already configured). + // + // NOTE: remember to update the preparation of the plan to be presented + // to the user if changing anything here. + // + database& pdb (p.db); + + if (!p.system) + { + const shared_ptr& ap (p.available); + + transaction t (pdb, !simulate /* start */); - // @@ Note that on reconfiguration the dependent looses the potential - // configuration variables specified by the user on some previous - // build, which can be quite surprising. Should we store this - // information in the database? + // Show how we got here if things go wrong. // - // I believe this now works for external packages via package - // skeleton (which extracts user configuration). + auto g ( + make_exception_guard ( + [&p] () + { + info << "while configuring " << p.name () << p.db; + })); + + // Note that pkg_configure() commits the transaction. // - pkg_configure (o, - pdb, - t, - sp, - deps, - nullptr /* alternatives */, - move (*p.skeleton), - prereqs (), - false /* disfigured */, - simulate, - fdb); + if (ap != nullptr) + { + pkg_configure (o, + pdb, + t, + sp, + move (cp.res), + p.disfigure, + simulate); + } + else // Dependent. + { + pkg_configure (o, + pdb, + t, + sp, + move (cp.res), + false /* disfigured */, + simulate); + } } r = true; diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx index 20a2567..59a6af0 100644 --- a/bpkg/pkg-configure.cxx +++ b/bpkg/pkg-configure.cxx @@ -19,32 +19,7 @@ using namespace butl; 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). - // - struct configure_prerequisites_result - { - package_prerequisites prerequisites; - strings config_variables; // Note: name and value. - - // Only contains sources of configuration variables collected using the - // package skeleton, excluding those user-specified variables which are - // not the project variables for the specified package (module - // configuration variables, etc). Thus, it is not parallel to the - // config_variables member. - // - vector config_sources; // Note: name and source. - }; - - // Note: loads selected packages. - // - static configure_prerequisites_result + configure_prerequisites_result pkg_configure_prerequisites (const common_options& o, database& db, transaction&, @@ -53,8 +28,13 @@ namespace bpkg package_skeleton&& ps, const vector* prev_prereqs, bool simulate, - const function& fdb) + const function& fdb, + const function& fps) { + tracer trace ("pkg_configure_prerequisites"); + + tracer_guard tg (db, trace); + package_prerequisites prereqs; strings vars; @@ -167,9 +147,15 @@ namespace bpkg const shared_ptr& dp (spd.first); - if (dp == nullptr || - dp->state != package_state::configured || - !satisfies (dp->version, d.constraint) || + if (dp == nullptr) + break; + + optional> dps; + if (fps != nullptr) + dps = fps (dp); + + if ((dps ? dps->first : dp->state) != package_state::configured || + !satisfies (dp->version, d.constraint) || (pps != nullptr && find (pps->begin (), pps->end (), dp->name) == pps->end ())) break; @@ -244,7 +230,13 @@ namespace bpkg { shared_ptr sp (pr.first.load ()); - if (!sp->system ()) + optional> ps; + if (fps != nullptr) + ps = fps (sp); + + if (ps + ? ps->second != package_substate::system + : !sp->system ()) { // @@ Note that this doesn't work for build2 modules that // require bootstrap. For their dependents we need to @@ -263,7 +255,26 @@ namespace bpkg // Also note that such modules are marked with `requires: // bootstrap` in their manifest. // - dir_path od (sp->effective_out_root (pdb.config)); + // Note that we currently don't support global overrides + // in the shared build2 context (but could probably do, + // if necessary). + // + + dir_path od; + if (ps) + { + // There is no out_root for a would-be configured package. + // So we calculate it like in pkg_configure() below (yeah, + // it's an ugly hack). + // + od = sp->external () + ? pdb.config / dir_path (sp->name.string ()) + : pdb.config / dir_path (sp->name.string () + '-' + + sp->version.string ()); + } + else + od = sp->effective_out_root (pdb.config); + vars.push_back ("config.import." + sp->name.variable () + "='" + od.representation () + '\''); } @@ -339,13 +350,9 @@ namespace bpkg database& db, transaction& t, const shared_ptr& p, - const dependencies& deps, - const vector* alts, - package_skeleton&& ps, - const vector* pps, + configure_prerequisites_result&& cpr, bool disfigured, - bool simulate, - const function& fdb) + bool simulate) { tracer trace ("pkg_configure"); @@ -359,6 +366,8 @@ namespace bpkg // Calculate package's out_root. // + // Note: see a version of this in pkg_configure_prerequisites(). + // dir_path out_root ( p->external () ? c / dir_path (p->name.string ()) @@ -367,41 +376,13 @@ namespace bpkg l4 ([&]{trace << "src_root: " << src_root << ", " << "out_root: " << out_root;}); - // Verify all our prerequisites are configured and populate the - // prerequisites list. - // assert (p->prerequisites.empty ()); - - configure_prerequisites_result cpr ( - pkg_configure_prerequisites (o, - db, - t, - deps, - alts, - move (ps), - pps, - simulate, - fdb)); - p->prerequisites = move (cpr.prerequisites); + // Configure. + // if (!simulate) { - // Form the buildspec. - // - string bspec; - - // Use path representation to get canonical trailing slash. - // - if (src_root == out_root) - bspec = "configure('" + out_root.representation () + "')"; - else - bspec = "configure('" + - src_root.representation () + "'@'" + - out_root.representation () + "')"; - - l4 ([&]{trace << "buildspec: " << bspec;}); - // Unless this package has been completely disfigured, disfigure all the // package configuration variables to reset all the old values to // defaults (all the new user/dependent/reflec values, including old @@ -424,8 +405,21 @@ namespace bpkg dvar += "**'"; } - // Configure. + // Form the buildspec. + // + string bspec; + + // Use path representation to get canonical trailing slash. // + if (src_root == out_root) + bspec = "configure('" + out_root.representation () + "')"; + else + bspec = "configure('" + + src_root.representation () + "'@'" + + out_root.representation () + "')"; + + l4 ([&]{trace << "buildspec: " << bspec;}); + try { run_b (o, @@ -436,25 +430,11 @@ namespace bpkg } catch (const failed&) { - // If we failed to configure the package, make sure we revert - // it back to the unpacked state by running disfigure (it is - // valid to run disfigure on an un-configured build). And if - // disfigure fails as well, then the package will be set into - // the broken state. - // - - // Indicate to pkg_disfigure() we are partially configured. + // See below for comments. // p->out_root = out_root.leaf (); p->state = package_state::broken; - - // Commits the transaction. - // - pkg_disfigure (o, db, t, - p, - true /* clean */, - true /* disfigure */, - false /* simulate */); + pkg_disfigure (o, db, t, p, true, true, false); throw; } @@ -468,6 +448,34 @@ namespace bpkg t.commit (); } + void + pkg_configure (const common_options& o, + database& db, + transaction& t, + const shared_ptr& p, + const dependencies& deps, + const vector* alts, + package_skeleton&& ps, + const vector* pps, + bool disfigured, + bool simulate, + const function& fdb) + { + configure_prerequisites_result cpr ( + pkg_configure_prerequisites (o, + db, + t, + deps, + alts, + move (ps), + pps, + simulate, + fdb, + nullptr)); + + pkg_configure (o, db, t, p, move (cpr), disfigured, simulate); + } + shared_ptr pkg_configure_system (const package_name& n, const version& v, diff --git a/bpkg/pkg-configure.hxx b/bpkg/pkg-configure.hxx index 16ed96f..a7409b9 100644 --- a/bpkg/pkg-configure.hxx +++ b/bpkg/pkg-configure.hxx @@ -21,6 +21,14 @@ namespace bpkg int pkg_configure (const pkg_configure_options&, cli::scanner& args); + // Configure a system package and commit the transaction. + // + shared_ptr + pkg_configure_system (const package_name&, + const version&, + database&, + transaction&); + // The custom search function. If specified, it is called by pkg_configure() // to obtain the database to search for the prerequisite in, instead of // searching for it in the linked databases, recursively. If the function @@ -31,7 +39,14 @@ namespace bpkg const package_name&, bool buildtime); - // Configure the package, update its state, and commit the transaction. + // 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). // // The package dependency constraints are expected to be complete. // @@ -50,6 +65,54 @@ namespace bpkg // dependency decisions" mode). Failed that, select an alternative as if no // prerequisites are specified (the "make dependency decisions" mode). // + struct configure_prerequisites_result + { + package_prerequisites prerequisites; + strings config_variables; // Note: name and value. + + // Only contains sources of configuration variables collected using the + // package skeleton, excluding those user-specified variables which are + // not the project variables for the specified package (module + // configuration variables, etc). Thus, it is not parallel to the + // config_variables member. + // + vector config_sources; // Note: name and source. + }; + + // Return the "would be" state for packages that would be configured + // by this stage. + // + using find_package_state_function = + optional> ( + const shared_ptr&); + + // Note: loads selected packages. + // + configure_prerequisites_result + pkg_configure_prerequisites (const common_options&, + database&, + transaction&, + const dependencies&, + const vector* alternatives, + package_skeleton&&, + const vector* prev_prerequisites, + bool simulate, + const function&, + const function&); + + // Configure the package, update its state, and commit the transaction. + // + void + pkg_configure (const common_options&, + database&, + transaction&, + const shared_ptr&, + configure_prerequisites_result&&, + bool disfigured, + bool simulate); + + // Note: loads selected packages. + // void pkg_configure (const common_options&, database&, @@ -58,18 +121,10 @@ namespace bpkg const dependencies&, const vector* alternatives, package_skeleton&&, - const vector* prerequisites, + const vector* prev_prerequisites, bool disfigured, bool simulate, const function& = {}); - - // Configure a system package and commit the transaction. - // - shared_ptr - pkg_configure_system (const package_name&, - const version&, - database&, - transaction&); } #endif // BPKG_PKG_CONFIGURE_HXX -- cgit v1.1