From c279979af18d59d935512d91c7e75762b914bdfd Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Tue, 18 Jul 2023 18:14:07 +0300 Subject: Don't reconfigure dependency if negotiated configuration doesn't change --- bpkg/package-skeleton.cxx | 62 +++++++++++++++++++++++++++++++++++-- bpkg/package-skeleton.hxx | 7 +++++ bpkg/package.hxx | 52 ++++++++++++++++++------------- bpkg/package.xml | 6 ++++ bpkg/pkg-build-collect.cxx | 77 ++++++++++++++++++++++++++++------------------ bpkg/pkg-configure.cxx | 7 ++++- bpkg/pkg-configure.hxx | 5 +++ bpkg/pkg-disfigure.cxx | 3 ++ 8 files changed, 164 insertions(+), 55 deletions(-) diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx index a3cdcdc..567f2e7 100644 --- a/bpkg/package-skeleton.cxx +++ b/bpkg/package-skeleton.cxx @@ -1943,6 +1943,8 @@ namespace bpkg pair> package_skeleton:: collect_config () && { + // NOTE: remember to update config_checksum() if changing anything here. + assert (db_ != nullptr); // Must be called only once. using build2::config::variable_origin; @@ -1985,11 +1987,19 @@ namespace bpkg // variables which are project variables (i.e., names start with // config.). // + size_t pn (var_prefix_.size ()); for (const string& v: config_vars_) { - if (project_override (v, var_prefix_)) + size_t vn; + if (project_override (v, var_prefix_, &vn)) { - string n (var_name (v)); + // Skip config..develop (can potentially be passed by + // bdep-init) if the package doesn't use it. + // + if (!develop_ && v.compare (pn, vn - pn, ".develop") == 0) + continue; + + string n (v, 0, vn); // Check for a duplicate. // @@ -2055,6 +2065,54 @@ namespace bpkg return make_pair (move (vars), move (srcs)); } + string package_skeleton:: + config_checksum () + { + // Note: this is parallel to collect_config() logic but is not destructive. + + assert (db_ != nullptr); // Must be called before collect_config(). + + if (!loaded_old_config_) + load_old_config (); + + sha256 cs; + + if (!config_vars_.empty ()) + { + cstrings vs; + size_t pn (var_prefix_.size ()); + for (const string& v: config_vars_) + { + size_t vn; + if (project_override (v, var_prefix_, &vn)) + { + // Skip config..develop (can potentially be passed by + // bdep-init) if the package doesn't use it. + // + if (develop_ || v.compare (pn, vn - pn, ".develop") != 0) + cs.append (v); + } + } + } + + if (!dependent_vars_.empty ()) + { + for (const string& v: dependent_vars_) + cs.append (v); + } + + if (!reflect_.empty ()) + { + for (const reflect_variable_value& v: reflect_) + { + if (v.origin != build2::config::variable_origin::override_) + cs.append (serialize_cmdline (v.name, v.value)); + } + } + + return !cs.empty () ? cs.string () : string (); + } + const strings& package_skeleton:: merge_cmd_vars (const strings& dependent_vars, const strings& dependency_vars, diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx index b2ed752..bc5d25c 100644 --- a/bpkg/package-skeleton.hxx +++ b/bpkg/package-skeleton.hxx @@ -82,6 +82,7 @@ namespace bpkg // ? dependent_config() // * evaluate_*() // * empty() | print_config() + // * config_checksum() // collect_config() // // Note that a copy of the skeleton is expected to continue with the @@ -181,6 +182,12 @@ namespace bpkg pair> collect_config () &&; + // Return the checksum of the project configuration variables that will be + // returned by the collect_config() function call. + // + string + config_checksum (); + // Implementation details. // public: diff --git a/bpkg/package.hxx b/bpkg/package.hxx index 060f13a..02d2b07 100644 --- a/bpkg/package.hxx +++ b/bpkg/package.hxx @@ -26,9 +26,13 @@ // Used by the data migration entries. // +// NOTE: drop all the `#pragma db member(...) default(...)` pragmas when +// migration is no longer supported (i.e., the current and base schema +// versions are the same). +// #define DB_SCHEMA_VERSION_BASE 12 -#pragma db model version(DB_SCHEMA_VERSION_BASE, 23, closed) +#pragma db model version(DB_SCHEMA_VERSION_BASE, 24, closed) namespace bpkg { @@ -618,9 +622,6 @@ namespace bpkg // #pragma db value(test_dependency) definition - // @@ TMP Drop when database migration to the schema version 11 is no longer - // supported. - // #pragma db member(test_dependency::buildtime) default(false) using optional_test_dependency_type = optional; @@ -887,29 +888,21 @@ namespace bpkg // alt_naming // - // @@ TMP Drop when database migration to the schema version 20 is no - // longer supported. - // - // Note that since no real packages with alternative buildfile naming - // use conditional dependencies yet, we can just set alt_naming to - // false during migration to the database schema version 20. Also we - // never rely on alt_naming to be nullopt for the stub packages, so - // let's not complicate things and set alt_naming to false for them - // either. + // Note that since no real packages with alternative buildfile naming use + // conditional dependencies yet, we can just set alt_naming to false + // during migration to the database schema version 20. Also we never rely + // on alt_naming to be nullopt for the stub packages, so let's not + // complicate things and set alt_naming to false for them either. // #pragma db member(alt_naming) default(false) // *_build // - // @@ TMP Drop when database migration to the schema version 15 is no - // longer supported. - // - // Note that since no real packages use conditional dependencies yet, - // we can just set bootstrap_build to the empty string during migration - // to the database schema version 15. Also we never rely on - // bootstrap_build to be nullopt for the stub packages, so let's not - // complicate things and set bootstrap_build to the empty string for - // them either. + // Note that since no real packages use conditional dependencies yet, we + // can just set bootstrap_build to the empty string during migration to + // the database schema version 15. Also we never rely on bootstrap_build + // to be nullopt for the stub packages, so let's not complicate things and + // set bootstrap_build to the empty string for them either. // #pragma db member(bootstrap_build) default("") @@ -1272,8 +1265,15 @@ namespace bpkg package_prerequisites prerequisites; + // Project configuration variable names and their sources. + // vector config_variables; + // SHA256 checksum of variables (names and values) referred to by the + // config_variables member. + // + std::string config_checksum; + public: bool system () const @@ -1353,6 +1353,14 @@ namespace bpkg #pragma db member(config_variables) id_column("package") value_column("") + // For the sake of simplicity let's not calculate the checksum during + // migration. It seems that the only drawback of this approach is a + // (single) spurious reconfiguration of a dependency of a dependent with + // configuration clause previously configured by bpkg with the database + // schema version prior to 24. + // + #pragma db member(config_checksum) default("") + // Explicit aggregate initialization for C++20 (private default ctor). // selected_package (package_name n, diff --git a/bpkg/package.xml b/bpkg/package.xml index fd332bc..343f27a 100644 --- a/bpkg/package.xml +++ b/bpkg/package.xml @@ -1,4 +1,10 @@ + + + + + + diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx index 2e7cc42..c8173b2 100644 --- a/bpkg/pkg-build-collect.cxx +++ b/bpkg/pkg-build-collect.cxx @@ -3456,13 +3456,6 @@ namespace bpkg build_package* b (entered_build (p)); assert (b != nullptr); - // Reconfigure the configured dependencies (see - // collect_build_postponed() for details). - // - if (b->selected != nullptr && - b->selected->state == package_state::configured) - b->flags |= build_package::adjust_reconfigure; - if (!b->recursive_collection) { l5 ([&]{trace << "collecting cfg-postponed dependency " @@ -3470,17 +3463,19 @@ namespace bpkg << " of dependent " << pkg.available_name_version_db ();}); + assert (b->skeleton); // Should have been init'ed above. + + package_skeleton& ps (*b->skeleton); + // Similar to the inital negotiation case, verify and set // the dependent configuration for this dependency. // { - assert (b->skeleton); // Should have been init'ed above. - const package_configuration& pc ( cfg.dependency_configurations[p]); - pair pr (b->skeleton->available != nullptr - ? b->skeleton->verify_sensible (pc) + pair pr (ps.available != nullptr + ? ps.verify_sensible (pc) : make_pair (true, string ())); if (!pr.first) @@ -3494,7 +3489,7 @@ namespace bpkg pc.print (dr, " "); } - b->skeleton->dependent_config (pc); + ps.dependent_config (pc); } collect_build_prerequisites (options, @@ -3512,6 +3507,21 @@ namespace bpkg postponed_cfgs, postponed_poss, unacceptable_alts); + + // Unless the dependency is already being reconfigured, + // reconfigure it if its configuration changes. + // + if (!b->reconfigure ()) + { + const shared_ptr& sp (b->selected); + + if (sp != nullptr && + sp->state == package_state::configured && + sp->config_checksum != ps.config_checksum ()) + { + b->flags |= build_package::adjust_reconfigure; + } + } } else l5 ([&]{trace << "dependency " @@ -4933,38 +4943,27 @@ namespace bpkg build_package* b (entered_build (p)); assert (b != nullptr); - // Reconfigure the configured dependencies. - // - // Note that potentially this can be an overkill if the dependency - // configuration doesn't really change. Later we can implement some - // precise detection for that using configuration checksum or similar. - // - // Also note that for configured dependents which belong to the - // configuration cluster this flag is already set (see above). - // - if (b->selected != nullptr && - b->selected->state == package_state::configured) - b->flags |= build_package::adjust_reconfigure; - // Skip the dependencies which are already collected recursively. // if (!b->recursive_collection) { + assert (b->skeleton); // Should have been init'ed above. + + package_skeleton& ps (*b->skeleton); + // Verify and set the dependent configuration for this dependency. // // Note: see similar code for the up-negotiation case. // { - assert (b->skeleton); // Should have been init'ed above. - const package_configuration& pc ( pcfg->dependency_configurations[p]); // Skip the verification if this is a system package without // skeleton info. // - pair pr (b->skeleton->available != nullptr - ? b->skeleton->verify_sensible (pc) + pair pr (ps.available != nullptr + ? ps.verify_sensible (pc) : make_pair (true, string ())); if (!pr.first) @@ -4983,7 +4982,7 @@ namespace bpkg pc.print (dr, " "); // Note 4 spaces since in nested info. } - b->skeleton->dependent_config (pc); + ps.dependent_config (pc); } build_package_refs dep_chain; @@ -5002,6 +5001,24 @@ namespace bpkg postponed_cfgs, postponed_poss, unacceptable_alts); + + // Unless the dependency is already being reconfigured, reconfigure + // it if its configuration changes. + // + // Note that for configured dependents which belong to the + // configuration cluster this flag is already set (see above). + // + if (!b->reconfigure ()) + { + const shared_ptr& sp (b->selected); + + if (sp != nullptr && + sp->state == package_state::configured && + sp->config_checksum != ps.config_checksum ()) + { + b->flags |= build_package::adjust_reconfigure; + } + } } else l5 ([&]{trace << "dependency " << b->available_name_version_db () diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx index 2801539..291929b 100644 --- a/bpkg/pkg-configure.cxx +++ b/bpkg/pkg-configure.cxx @@ -371,9 +371,12 @@ namespace bpkg // etc) as well as their sources. // vector srcs; + string checksum; if (!simulate) { + checksum = ps.config_checksum (); + pair> rvs (move (ps).collect_config ()); strings& vs (rvs.first); @@ -395,7 +398,8 @@ namespace bpkg return configure_prerequisites_result {move (prereqs), move (vars), - move (srcs)}; + move (srcs), + move (checksum)}; } @@ -767,6 +771,7 @@ namespace bpkg #endif p->config_variables = move (cpr.config_sources); + p->config_checksum = move (cpr.config_checksum); } p->out_root = out_root.leaf (); diff --git a/bpkg/pkg-configure.hxx b/bpkg/pkg-configure.hxx index 099e1e8..24e06a3 100644 --- a/bpkg/pkg-configure.hxx +++ b/bpkg/pkg-configure.hxx @@ -83,6 +83,11 @@ namespace bpkg // config_variables member. // vector config_sources; // Note: name and source. + + // SHA256 checksum of variables (names and values) referred to by the + // config_sources member. + // + string config_checksum; }; // Return the "would be" state for packages that would be configured diff --git a/bpkg/pkg-disfigure.cxx b/bpkg/pkg-disfigure.cxx index 690c3e1..af2c4f1 100644 --- a/bpkg/pkg-disfigure.cxx +++ b/bpkg/pkg-disfigure.cxx @@ -200,7 +200,10 @@ namespace bpkg } if (disfigure) + { p->config_variables.clear (); + p->config_checksum.clear (); + } } p->out_root = nullopt; -- cgit v1.1