From df8cc71e9245144656f349d90df598b4296ab10c 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 | 162 +++++++++++++++++++++++++++++++++---------------- bpkg/pkg-configure.cxx | 135 +++++++++++++++++------------------------ bpkg/pkg-configure.hxx | 60 +++++++++++++++--- 3 files changed, 219 insertions(+), 138 deletions(-) diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx index d30555f..adbcd8f 100644 --- a/bpkg/pkg-build.cxx +++ b/bpkg/pkg-build.cxx @@ -5350,14 +5350,17 @@ 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 ()); for (build_package& p: reverse_iterate (build_pkgs)) { @@ -5369,7 +5372,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 @@ -5380,8 +5383,13 @@ namespace bpkg // assert (sp != nullptr || p.system); - database& pdb (p.db); + if (p.system) + { + configure_packages.push_back (configure_package {p, {}}); + continue; + } + database& pdb (p.db); transaction t (pdb, !simulate /* start */); // Show how we got here if things go wrong, for example selecting a @@ -5401,16 +5409,8 @@ namespace bpkg return i != previous_prerequisites.end () ? &i->second : nullptr; }; - // Note that pkg_configure() commits the transaction. - // - if (p.system) - { - sp = pkg_configure_system (ap->id.name, - p.available_version (), - pdb, - t); - } - else if (ap != nullptr) + configure_prerequisites_result cpr; + if (ap != nullptr) { assert (*p.action == build_package::build); @@ -5437,17 +5437,15 @@ namespace bpkg { assert (p.skeleton); - pkg_configure (o, - pdb, - t, - sp, - *p.dependencies, - &*p.alternatives, - move (*p.skeleton), - nullptr /* previous_prerequisites */, - p.disfigure, - simulate, - fdb); + cpr = pkg_configure_prerequisites (o, + pdb, + t, + *p.dependencies, + &*p.alternatives, + move (*p.skeleton), + nullptr /* prev_prerequisites */, + simulate, + fdb); } else { @@ -5460,17 +5458,15 @@ namespace bpkg if (!p.skeleton) p.init_skeleton (o); - pkg_configure (o, - pdb, - t, - sp, - ap->dependencies, - nullptr /* alternatives */, - move (*p.skeleton), - prereqs (), - p.disfigure, - simulate, - fdb); + cpr = pkg_configure_prerequisites (o, + pdb, + t, + ap->dependencies, + nullptr /* alternatives */, + move (*p.skeleton), + prereqs (), + simulate, + fdb); } } else // Dependent. @@ -5479,9 +5475,7 @@ namespace bpkg // (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 ()); + assert (*p.action == build_package::adjust && !sp->system ()); // Must be in the unpacked state since it was disfigured on the first // pass (see above). @@ -5512,20 +5506,84 @@ namespace bpkg // build, which can be quite surprising. Should we store this // information in the database? // - // I believe this now works for external packages via package - // skeleton (which extracts user configuration). + // 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); + } + + configure_packages.push_back (configure_package {p, move (cpr)}); + + t.commit (); + } + + if (progress) + { + prog_i = 0; + prog_n = configure_packages.size (); + prog_percent = 100; + } + + for (configure_package& cp: configure_packages) + { + build_package& p (cp.pkg); + + shared_ptr& sp (p.selected); + const shared_ptr& ap (p.available); + + // Configure the package. + // + // NOTE: remember to update the preparation of the plan to be presented + // to the user if changing anything here. + // + database& pdb (p.db); + transaction t (pdb, !simulate /* start */); + + // Show how we got here if things go wrong. + // + auto g ( + make_exception_guard ( + [&p] () + { + info << "while configuring " << p.name () << p.db; + })); + + // Note that pkg_configure*() commits the transaction. + // + if (p.system) + { + sp = pkg_configure_system (ap->id.name, + p.available_version (), + pdb, + t); + } + else if (ap != nullptr) + { + pkg_configure (o, + pdb, + t, + sp, + move (cp.res), + p.disfigure, + simulate); + } + else // Dependent. + { pkg_configure (o, pdb, t, sp, - deps, - nullptr /* alternatives */, - move (*p.skeleton), - prereqs (), + move (cp.res), false /* disfigured */, - simulate, - fdb); + simulate); } r = true; diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx index 20a2567..c440877 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&, @@ -55,6 +30,10 @@ namespace bpkg bool simulate, const function& fdb) { + tracer trace ("pkg_configure_prerequisites"); + + tracer_guard tg (db, trace); + package_prerequisites prereqs; strings vars; @@ -263,6 +242,10 @@ namespace bpkg // Also note that such modules are marked with `requires: // bootstrap` in their manifest. // + // Note that we currently don't support global overrides + // in the shared build2 context (but could probably do, + // if necessary). + // dir_path od (sp->effective_out_root (pdb.config)); vars.push_back ("config.import." + sp->name.variable () + "='" + od.representation () + '\''); @@ -339,13 +322,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"); @@ -367,41 +346,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 +375,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 +400,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 +418,33 @@ 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)); + + 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..7587713 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 @@ -58,18 +66,56 @@ 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. + + // 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). // - shared_ptr - pkg_configure_system (const package_name&, - const version&, - database&, - transaction&); + 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. + // + 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&); + + void + pkg_configure (const common_options&, + database&, + transaction&, + const shared_ptr&, + configure_prerequisites_result&&, + bool disfigured, + bool simulate); } #endif // BPKG_PKG_CONFIGURE_HXX -- cgit v1.1