From 0370038a2c2e5fc575a543e2fbcf85a7c254283d Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Fri, 27 Oct 2023 17:32:11 +0300 Subject: Add support for preserving old package configuration on up/downgrade and reconfiguration --- bpkg/package-skeleton.cxx | 238 +++++++++++++++++++++++++++------------------ bpkg/package-skeleton.hxx | 64 +++++++++--- bpkg/pkg-build-collect.cxx | 119 ++++++++++++++++------- bpkg/pkg-build-collect.hxx | 8 +- bpkg/pkg-build.cxx | 67 +++++++------ bpkg/pkg-configure.cxx | 5 +- 6 files changed, 323 insertions(+), 178 deletions(-) (limited to 'bpkg') diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx index 7effca0..78635e7 100644 --- a/bpkg/package-skeleton.cxx +++ b/bpkg/package-skeleton.cxx @@ -141,7 +141,7 @@ namespace bpkg // (build2 stuff is only forward-declared in the header). // static build2::scope_map::iterator - bootstrap (package_skeleton&, const strings&); + bootstrap (package_skeleton&, const strings&, bool old = false); package_skeleton:: ~package_skeleton () @@ -153,6 +153,7 @@ namespace bpkg : package (move (v.package)), system (v.system), available (move (v.available)), + load_config_flags (v.load_config_flags), co_ (v.co_), db_ (v.db_), var_prefix_ (move (v.var_prefix_)), @@ -162,7 +163,9 @@ namespace bpkg config_srcs_ (v.config_srcs_), src_root_ (move (v.src_root_)), out_root_ (move (v.out_root_)), - load_old_dependent_config_ (v.load_old_dependent_config_), + src_root_specified_ (v.src_root_specified_), + old_src_root_ (move (v.old_src_root_)), + old_out_root_ (move (v.old_out_root_)), created_ (v.created_), verified_ (v.verified_), loaded_old_config_ (v.loaded_old_config_), @@ -192,6 +195,7 @@ namespace bpkg package = move (v.package); system = v.system; available = move (v.available); + load_config_flags = v.load_config_flags; co_ = v.co_; db_ = v.db_; var_prefix_ = move (v.var_prefix_); @@ -201,7 +205,9 @@ namespace bpkg config_srcs_ = v.config_srcs_; src_root_ = move (v.src_root_); out_root_ = move (v.out_root_); - load_old_dependent_config_ = v.load_old_dependent_config_; + src_root_specified_ = v.src_root_specified_; + old_src_root_ = move (v.old_src_root_); + old_out_root_ = move (v.old_out_root_); created_ = v.created_; verified_ = v.verified_; loaded_old_config_ = v.loaded_old_config_; @@ -231,6 +237,7 @@ namespace bpkg : package (v.package), system (v.system), available (v.available), + load_config_flags (v.load_config_flags), co_ (v.co_), db_ (v.db_), var_prefix_ (v.var_prefix_), @@ -240,7 +247,9 @@ namespace bpkg config_srcs_ (v.config_srcs_), src_root_ (v.src_root_), out_root_ (v.out_root_), - load_old_dependent_config_ (v.load_old_dependent_config_), + src_root_specified_ (v.src_root_specified_), + old_src_root_ (v.old_src_root_), + old_out_root_ (v.old_out_root_), created_ (v.created_), verified_ (v.verified_), loaded_old_config_ (v.loaded_old_config_), @@ -303,17 +312,19 @@ namespace bpkg const vector* css, optional src_root, optional out_root, - bool load_old_dependent_config) + optional old_src_root, + optional old_out_root, + uint16_t lcf) : package (move (pk)), system (sys), available (move (ap)), + load_config_flags (lcf), co_ (&co), db_ (&package.db.get ()), var_prefix_ ("config." + package.name.variable ()), config_vars_ (move (cvs)), disfigure_ (df), - config_srcs_ (df ? nullptr : css), - load_old_dependent_config_ (load_old_dependent_config) + config_srcs_ (df ? nullptr : css) { if (available != nullptr) assert (available->bootstrap_build); // Should have skeleton info. @@ -331,8 +342,9 @@ namespace bpkg if (find_if (config_srcs_->begin (), config_srcs_->end (), [this] (const config_variable& v) { - return v.source == config_source::user || - (load_old_dependent_config_ && + return ((load_config_flags & load_config_user) != 0 && + v.source == config_source::user) || + ((load_config_flags & load_config_dependent) != 0 && v.source == config_source::dependent); }) == config_srcs_->end ()) config_srcs_ = nullptr; @@ -355,8 +367,8 @@ namespace bpkg { // For now tighten it even further so that we can continue // using repositories without package skeleton information - // (bootstrap.build, root.build). See load_old_config() for - // details. + // (bootstrap.build, root.build). See + // load_old_config_impl() for details. // #if 0 return project_override (v, var_prefix_); @@ -372,11 +384,27 @@ namespace bpkg { src_root_ = move (*src_root); + assert (!src_root_.empty ()); // Must exist. + + src_root_specified_ = true; + if (out_root) out_root_ = move (*out_root); } else assert (!out_root); + + if (old_src_root) + { + old_src_root_ = move (*old_src_root); + + assert (!old_src_root_.empty ()); // Must exist. + + if (old_out_root) + old_out_root_ = move (*old_out_root); + } + else + assert (!old_out_root); } // Serialize a variable assignment for a command line override. @@ -466,7 +494,7 @@ namespace bpkg ctx_ == nullptr); if (!loaded_old_config_) - load_old_config (); + load_old_config_impl (); try { @@ -708,7 +736,7 @@ namespace bpkg ctx_ == nullptr); if (!loaded_old_config_) - load_old_config (); + load_old_config_impl (); try { @@ -721,10 +749,10 @@ namespace bpkg // (e.g., either via an attribute or via special config.assert directive // or some such). // - // For now we rely on load_defaults() and load_old_config() to "flush" - // out any unrelated errors (e.g., one of the modules configuration is - // bad, etc). However, if that did not happen naturally, then we must do - // it ourselves. + // For now we rely on load_defaults() and load_old_config_impl() to + // "flush" out any unrelated errors (e.g., one of the modules + // configuration is bad, etc). However, if that did not happen + // naturally, then we must do it ourselves. // if (!verified_) { @@ -787,10 +815,12 @@ namespace bpkg // Print the location of a depends value in the specified manifest file. // - // Note that currently we only use this function for the external packages. - // We could also do something similar for normal packages by pointing to the - // manifest we have serialized. In this case we would also need to make sure - // the temp directory is not cleaned in case of an error. Maybe one day. + // Note that currently we only use this function for the being reconfigured + // and external packages (i.e. when the existing source directory is + // specified). We could also do something similar for the remaining cases by + // pointing to the manifest we have serialized. In this case we would also + // need to make sure the temp directory is not cleaned in case of an error. + // Maybe one day. // static void depends_location (const build2::diag_record& dr, @@ -863,20 +893,19 @@ namespace bpkg uint64_t il (1); auto df = build2::make_diag_frame ( - [this, &cond, &rs, depends_index] (const build2::diag_record& dr) + [this, &cond, depends_index] (const build2::diag_record& dr) { dr << info << "enable condition: (" << cond << ")"; - // For external packages we have the manifest so print the location - // of the depends value in questions. + // If an existing source directory has been specified, then we have + // the manifest and so print the location of the depends value in + // questions. // - if (rs.out_eq_src ()) + if (src_root_specified_) + depends_location (dr, src_root_ / manifest_file, depends_index); + else dr << info << "in depends manifest value of package " << package.name; - else - depends_location (dr, - rs.src_path () / manifest_file, - depends_index); }); lexer l (is, in, il /* start line */); @@ -932,9 +961,9 @@ namespace bpkg // values that are derived from them in root.build). It seems like we have // two options here: either enter them as true overrides similar to // config_vars_ or just evaluate them similar to loading config.build - // (which, BTW, we might have, in case of an external package). The big - // problem with the former approach is that it will then prevent any - // further reflect clauses from modifying the same values. + // (which, BTW, we might have, in case of a being reconfigured or external + // package). The big problem with the former approach is that it will then + // prevent any further reflect clauses from modifying the same values. // // So overall it feels like we have iterative/compartmentalized // configuration process. A feedback loop, in a sense. And it's the @@ -1060,7 +1089,7 @@ namespace bpkg // Note: keep it active until the end (see the override detection). // auto df = build2::make_diag_frame ( - [this, &refl, &rs, depends_index] (const build2::diag_record& dr) + [this, &refl, depends_index] (const build2::diag_record& dr) { // Probably safe to assume a one-line fragment contains a variable // assignment. @@ -1071,16 +1100,15 @@ namespace bpkg dr << info << "reflect clause:\n" << trim_right (string (refl)); - // For external packages we have the manifest so print the - // location of the depends value in questions. + // If an existing source directory has been specified, then we have + // the manifest and so print the location of the depends value in + // questions. // - if (rs.out_eq_src ()) + if (src_root_specified_) + depends_location (dr, src_root_ / manifest_file, depends_index); + else dr << info << "in depends manifest value of package " << package.name; - else - depends_location (dr, - rs.src_path () / manifest_file, - depends_index); }); lexer l (is, in, il /* start line */); @@ -1278,21 +1306,20 @@ namespace bpkg uint64_t il (1); auto df = build2::make_diag_frame ( - [this, &prefer, &rs, depends_index] (const build2::diag_record& dr) + [this, &prefer, depends_index] (const build2::diag_record& dr) { dr << info << "prefer clause:\n" << trim_right (string (prefer)); - // For external packages we have the manifest so print the - // location of the depends value in questions. + // If an existing source directory has been specified, then we + // have the manifest and so print the location of the depends + // value in questions. // - if (rs.out_eq_src ()) + if (src_root_specified_) + depends_location (dr, src_root_ / manifest_file, depends_index); + else dr << info << "in depends manifest value of package " << package.name; - else - depends_location (dr, - rs.src_path () / manifest_file, - depends_index); }); lexer l (is, in, il /* start line */); @@ -1340,20 +1367,19 @@ namespace bpkg uint64_t il (1); auto df = build2::make_diag_frame ( - [this, &accept, &rs, depends_index] (const build2::diag_record& dr) + [this, &accept, depends_index] (const build2::diag_record& dr) { dr << info << "accept condition: (" << accept << ")"; - // For external packages we have the manifest so print the - // location of the depends value in questions. + // If an existing source directory has been specified, then we + // have the manifest and so print the location of the depends + // value in questions. // - if (rs.out_eq_src ()) + if (src_root_specified_) + depends_location (dr, src_root_ / manifest_file, depends_index); + else dr << info << "in depends manifest value of package " << package.name; - else - depends_location (dr, - rs.src_path () / manifest_file, - depends_index); }); lexer l (is, in, il /* start line */); @@ -1573,21 +1599,20 @@ namespace bpkg uint64_t il (1); auto df = build2::make_diag_frame ( - [this, &require, &rs, depends_index] (const build2::diag_record& dr) + [this, &require, depends_index] (const build2::diag_record& dr) { dr << info << "require clause:\n" << trim_right (string (require)); - // For external packages we have the manifest so print the - // location of the depends value in questions. + // If an existing source directory has been specified, then we + // have the manifest and so print the location of the depends + // value in questions. // - if (rs.out_eq_src ()) + if (src_root_specified_) + depends_location (dr, src_root_ / manifest_file, depends_index); + else dr << info << "in depends manifest value of package " << package.name; - else - depends_location (dr, - rs.src_path () / manifest_file, - depends_index); }); lexer l (is, in, il /* start line */); @@ -1854,7 +1879,7 @@ namespace bpkg empty_print () { if (!loaded_old_config_) - load_old_config (); + load_old_config_impl (); return (dependent_vars_.empty () && reflect_.empty () && @@ -1885,7 +1910,7 @@ namespace bpkg using build2::config::variable_origin; if (!loaded_old_config_) - load_old_config (); + load_old_config_impl (); auto print = [&os, indent, @@ -1965,6 +1990,13 @@ namespace bpkg } } + void package_skeleton:: + load_old_config () + { + if (!loaded_old_config_) + load_old_config_impl (); + } + pair> package_skeleton:: collect_config () && { @@ -1975,7 +2007,7 @@ namespace bpkg using build2::config::variable_origin; if (!loaded_old_config_) - load_old_config (); + load_old_config_impl (); // Merge all the variables into a single list in the correct order // and assign their sources while at it. @@ -2098,7 +2130,7 @@ namespace bpkg assert (db_ != nullptr); // Must be called before collect_config(). if (!loaded_old_config_) - load_old_config (); + load_old_config_impl (); sha256 cs; @@ -2144,7 +2176,7 @@ namespace bpkg bool cache) { // Merge variable overrides (note that the order is important). See also a - // custom/optimized version in load_old_config(). + // custom/optimized version in load_old_config_impl(). // if (!cache || !cmd_vars_cache_) { @@ -2192,7 +2224,7 @@ namespace bpkg } void package_skeleton:: - load_old_config () + load_old_config_impl () { assert (!loaded_old_config_ && ctx_ == nullptr); @@ -2245,7 +2277,7 @@ namespace bpkg << package.name; }); - rs = bootstrap (*this, *cmd_vars)->second.front (); + rs = bootstrap (*this, *cmd_vars, true /* old */)->second.front (); // Load project's root.build. // @@ -2268,11 +2300,12 @@ namespace bpkg // // Also, build2 warns about unused variables being dropped. // - // Note that currently load_old_config() is disabled unless there is - // a config.*.develop variable; see package_skeleton ctor. + // Note that currently load_old_config_impl() is disabled unless + // there is a config.*.develop variable or we were asked to load + // dependent configuration; see package_skeleton ctor. - // Extract and merge old user configuration variables from config.build - // (or equivalent) into config_vars. + // Extract and merge old user and/or dependent configuration variables + // from config.build (or equivalent) into config_vars. // if (config_srcs_ != nullptr) { @@ -2284,8 +2317,9 @@ namespace bpkg names storage; for (const config_variable& v: *config_srcs_) { - if (!(v.source == config_source::user || - (load_old_dependent_config_ && + if (!(((load_config_flags & load_config_user) != 0 && + v.source == config_source::user) || + ((load_config_flags & load_config_dependent) != 0 && v.source == config_source::dependent))) continue; @@ -2335,7 +2369,10 @@ namespace bpkg } loaded_old_config_ = true; - verified_ = true; // Managed to load without errors. + + if (old_src_root_.empty ()) + verified_ = true; // Managed to load without errors. + ctx_ = nullptr; } catch (const build2::failed&) @@ -2358,7 +2395,7 @@ namespace bpkg } if (!loaded_old_config_) - load_old_config (); + load_old_config_impl (); try { @@ -2578,7 +2615,7 @@ namespace bpkg // Bootstrap the package skeleton. // static build2::scope_map::iterator - bootstrap (package_skeleton& skl, const strings& cmd_vars) + bootstrap (package_skeleton& skl, const strings& cmd_vars, bool old) { assert (skl.db_ != nullptr && skl.ctx_ == nullptr && @@ -2600,6 +2637,12 @@ namespace bpkg // Create the skeleton filesystem state, if it doesn't exist yet. // + if (old && skl.old_src_root_.empty ()) + old = false; + + dir_path& skl_src_root (old ? skl.old_src_root_ : skl.src_root_); + dir_path& skl_out_root (old ? skl.old_out_root_ : skl.out_root_); + if (!skl.created_) { const available_package& ap (*skl.available); @@ -2609,11 +2652,13 @@ namespace bpkg // they never clash with other temporary subdirectories (git // repositories, etc). // - if (skl.src_root_.empty () || skl.out_root_.empty ()) + // Note: for old src/out, everything should already exist. + // + if (!old && (skl_src_root.empty () || skl_out_root.empty ())) { // Cannot be specified if src_root_ is unspecified. // - assert (skl.out_root_.empty ()); + assert (skl_out_root.empty ()); // Note that only configurations which can be used as repository // information sources has the temporary directory facility @@ -2641,14 +2686,16 @@ namespace bpkg d /= "skeletons"; d /= skl.package.name.string () + '-' + ap.version.string (); - if (skl.src_root_.empty ()) - skl.src_root_ = move (d); // out_root_ is the same. + if (skl_src_root.empty ()) + skl_src_root = move (d); // out_root_ is the same. else - skl.out_root_ = move (d); // Don't even need to create it. + skl_out_root = move (d); // Don't even need to create it. } - if (!exists (skl.src_root_)) + if (!exists (skl_src_root)) { + assert (!old); // An old package version cannot not exist. + // Create the buildfiles. // // Note that it's probably doesn't matter which naming scheme to use @@ -2658,7 +2705,7 @@ namespace bpkg { bool an (*ap.alt_naming); - path bf (skl.src_root_ / + path bf (skl_src_root / (an ? alt_bootstrap_file : std_bootstrap_file)); mk_p (bf.directory ()); @@ -2683,11 +2730,11 @@ namespace bpkg if (ap.root_build) save (*ap.root_build, - skl.src_root_ / (an ? alt_root_file : std_root_file)); + skl_src_root / (an ? alt_root_file : std_root_file)); for (const buildfile& f: ap.buildfiles) { - path p (skl.src_root_ / + path p (skl_src_root / (an ? alt_build_dir : std_build_dir) / f.path); @@ -2728,7 +2775,7 @@ namespace bpkg m.dependencies.push_back (das); } - path mf (skl.src_root_ / manifest_file); + path mf (skl_src_root / manifest_file); try { @@ -2753,7 +2800,8 @@ namespace bpkg } } - skl.created_ = true; + if (!old) + skl.created_ = true; } try @@ -2786,10 +2834,10 @@ namespace bpkg // Note that it's ok for out_root to not exist (external package). // - const dir_path& src_root (skl.src_root_); - const dir_path& out_root (skl.out_root_.empty () - ? skl.src_root_ - : skl.out_root_); + const dir_path& src_root (skl_src_root); + const dir_path& out_root (skl_out_root.empty () + ? skl_src_root + : skl_out_root); auto rsi (create_root (ctx, out_root, src_root)); scope& rs (*rsi->second.front ()); diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx index 04abc0e..947522e 100644 --- a/bpkg/package-skeleton.hxx +++ b/bpkg/package-skeleton.hxx @@ -27,14 +27,27 @@ namespace bpkg // is NULL, then the skeleton can only be used to print and collect the // configuration information. // - // If the package is external, then the existing package source root + // If the package is being reconfigured (rather than up/downgraded), then + // the existing package source and output root directories (src_root and + // out_root) need to be specified (as absolute and normalized). Otherwise, + // if the package is external, then the existing package source root // directory needs to be specified (as absolute and normalized). In this // case, if output root is specified (as absolute and normalized; normally // /), then it's used as is. Otherwise, an empty // skeleton directory is used as output root. // - // If the package is not external, then none of the root directories - // should be specified. + // If the package is neither being reconfigured nor is external, then none + // of the root directories should be specified. + // + // If the package is configured as source and the user and/or dependent + // configuration is requested to be loaded from config.build, then the + // existing package old source and output root directories (old_src_root + // and old_out_root) need to be specified (as absolute and normalized). If + // specified, they are used instead of package source and output root + // directories to load the current user and/or dependent configuration. + // The idea here is that during package upgrade/downgrade, we want to load + // the old configuration from the old version's src/out but then continue + // evaluating clauses using the new version's src/out. // // The disfigure argument should indicate whether the package is being // reconfigured from scratch (--disfigure). @@ -69,13 +82,21 @@ namespace bpkg const vector* config_srcs, optional src_root, optional out_root, - bool load_old_dependent_config); - + optional old_src_root, + optional old_out_root, + uint16_t load_config_flags); package_key package; bool system; shared_ptr available; + // Load package (old) configuration flags. + // + uint16_t load_config_flags; + + static const uint16_t load_config_user = 0x1; + static const uint16_t load_config_dependent = 0x2; + // The following functions should be called in the following sequence // (* -- zero or more, ? -- zero or one): // @@ -86,6 +107,10 @@ namespace bpkg // * config_checksum() // collect_config() // + // Note that the load_old_config() function can be called at eny point + // before collect_config() (and is called implicitly by most other + // functions). + // // Note that a copy of the skeleton is expected to continue with the // sequence rather than starting from scratch, unless reset() is called. // @@ -170,6 +195,11 @@ namespace bpkg void print_config (ostream&, const char* indent); + // Load the package's old configuration, unless it is already loaded. + // + void + load_old_config (); + // Return the accumulated configuration variables (first) and project // configuration variable sources (second). Note that the arrays are not // necessarily parallel (config_vars may contain non-project variables). @@ -203,9 +233,10 @@ namespace bpkg package_skeleton& operator= (const package_skeleton&) = delete; private: - // Load old user configuration variables from config.build (or equivalent) - // and merge them into config_vars_. Also verify new user configuration - // already in config_vars_ makes sense. + // Load old user and/or dependent configuration variables from + // config.build (or equivalent) and merge them into config_vars_ and + // config_var_srcs_. Also verify new user configuration already in + // config_vars_ makes sense. // // This should be done before any attempt to load the configuration with // config.config.disfigure and, if this did not happen, inside @@ -213,7 +244,7 @@ namespace bpkg // config.config.disfigure). // void - load_old_config (); + load_old_config_impl (); // (Re)load the build system state. // @@ -257,7 +288,7 @@ namespace bpkg // Configuration sources for variables in config_vars_ (parallel). Can // only contain config_source::{user,dependent} entries (see - // load_old_config() for details). + // load_old_config_impl() for details). // vector config_var_srcs_; @@ -268,7 +299,18 @@ namespace bpkg dir_path src_root_; // Must be absolute and normalized. dir_path out_root_; // If empty, the same as src_root_. - bool load_old_dependent_config_; + // True if the existing source root directory has been specified. + // + // Note that if that's the case, we can use the manifest file this + // directory contains for diagnostics. + // + bool src_root_specified_ = false; + + // If specified, are used instead of {src,out}_root_ for loading of the + // project configuration variables. + // + dir_path old_src_root_; + dir_path old_out_root_; bool created_ = false; bool verified_ = false; diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx index c83645b..cb7ea9a 100644 --- a/bpkg/pkg-build-collect.cxx +++ b/bpkg/pkg-build-collect.cxx @@ -364,10 +364,8 @@ namespace bpkg package_skeleton& build_package:: init_skeleton (const common_options& options, - const shared_ptr& override, - optional src_root, - optional out_root, - bool load_old_dependent_config) + bool load_old_dependent_config, + const shared_ptr& override) { shared_ptr ap (override != nullptr ? override @@ -390,12 +388,59 @@ namespace bpkg ap = nullptr; } - if (!src_root && ap != nullptr) + optional src_root; + optional out_root; + + optional old_src_root; + optional old_out_root; + uint16_t load_config_flags (0); + + if (ap != nullptr) { - src_root = external_dir (); - out_root = (src_root && !disfigure - ? dir_path (db.get ().config) /= name ().string () - : optional ()); + bool src_conf (selected != nullptr && + selected->state == package_state::configured && + selected->substate != package_substate::system); + + database& pdb (db); + + // If the package is being reconfigured, then specify {src,out}_root as + // the existing source and output root directories not to create the + // skeleton directory needlessly. Otherwise, if the being built package + // is external, then specify src_root as its existing source directory + // and out_root as its potentially non-existing output directory. + // + // Can we actually use the existing output root directory if the package + // is being reconfigured but we are requested to ignore the current + // configuration? Yes we can, since load_config_flags stays 0 in this + // case and all the variables in config.build will be ignored. + // + if (src_conf && ap->version == selected->version) + { + src_root = selected->effective_src_root (pdb.config); + out_root = selected->effective_out_root (pdb.config); + } + else + { + src_root = external_dir (); + + if (src_root) + out_root = dir_path (pdb.config) /= name ().string (); + } + + // Specify old_{src,out}_root paths and set load_config_flags if the old + // configuration is present and is requested to be loaded. + // + if (src_conf && (!disfigure || load_old_dependent_config)) + { + old_src_root = selected->effective_src_root (pdb.config); + old_out_root = selected->effective_out_root (pdb.config); + + if (!disfigure) + load_config_flags |= package_skeleton::load_config_user; + + if (load_old_dependent_config) + load_config_flags |= package_skeleton::load_config_dependent; + } } skeleton = package_skeleton ( @@ -408,7 +453,9 @@ namespace bpkg (selected != nullptr ? &selected->config_variables : nullptr), move (src_root), move (out_root), - load_old_dependent_config); + move (old_src_root), + move (old_out_root), + load_config_flags); return *skeleton; } @@ -2049,7 +2096,7 @@ namespace bpkg // Bail out if this is a configured non-system package and no recursive // collection is required. // - bool src_conf (sp != nullptr && + bool src_conf (sp != nullptr && sp->state == package_state::configured && sp->substate != package_substate::system); @@ -2109,21 +2156,7 @@ namespace bpkg } if (!pkg.skeleton) - { - // In the (pre-)reevaluation mode make sure that the user-specified - // and the dependent configurations are both loaded by the skeleton. - // - if (pre_reeval || reeval) - { - pkg.init_skeleton (options, - nullptr /* override */, - sp->effective_src_root (pdb.config), - sp->effective_out_root (pdb.config), - true /* load_old_dependent_config */); - } - else - pkg.init_skeleton (options); - } + pkg.init_skeleton (options); } else l5 ([&]{trace << "resume " << pkg.available_name_version_db ();}); @@ -4219,19 +4252,33 @@ namespace bpkg build_package* b (entered_build (pk)); assert (b != nullptr); + optional& ps (b->skeleton); + + // If the dependency's skeleton is already present, then + // this dependency's configuration has already been + // initially negotiated (see collect_build_postponed() for + // details) and will now be be up-negotiated. Thus, in + // particular, the skeleton must not have the old + // configuration dependent variables be loaded. + // + assert (!ps || + (ps->load_config_flags & + package_skeleton::load_config_dependent) == 0); + package_skeleton* depc; if (b->recursive_collection) { - assert (b->skeleton); + assert (ps); - depcs_storage.push_front (*b->skeleton); + depcs_storage.push_front (*ps); depc = &depcs_storage.front (); depc->reset (); } else - depc = &(b->skeleton - ? *b->skeleton - : b->init_skeleton (options)); + depc = &(ps + ? *ps + : b->init_skeleton (options, + false /* load_old_dependent_config */)); depcs.push_back (*depc); } @@ -5890,9 +5937,11 @@ namespace bpkg // assert (b != nullptr && !b->recursive_collection); - package_skeleton* depc (&(b->skeleton - ? *b->skeleton - : b->init_skeleton (o /* options */))); + package_skeleton* depc ( + &(b->skeleton + ? *b->skeleton + : b->init_skeleton (o, + false /* load_old_dependent_config */))); depcs.push_back (*depc); } @@ -5984,7 +6033,7 @@ namespace bpkg // throwing the retry_configuration exception). // if (!b->skeleton) - b->init_skeleton (o /* options */); + b->init_skeleton (o, false /* load_old_dependent_config */); package_skeleton& ps (*b->skeleton); diff --git a/bpkg/pkg-build-collect.hxx b/bpkg/pkg-build-collect.hxx index d64abc5..f1dc18e 100644 --- a/bpkg/pkg-build-collect.hxx +++ b/bpkg/pkg-build-collect.hxx @@ -310,7 +310,7 @@ namespace bpkg // std::set required_by; - // If this flags is true, then required_by contains dependents. + // If this flag is true, then required_by contains dependents. // // We need this because required_by packages have different semantics for // different actions: the dependent for regular builds and dependency for @@ -453,10 +453,8 @@ namespace bpkg // package_skeleton& init_skeleton (const common_options&, - const shared_ptr& override = nullptr, - optional src_root = nullopt, - optional out_root = nullopt, - bool load_old_dependent_config = false); + bool load_old_dependent_config = true, + const shared_ptr& override = nullptr); }; using build_package_list = std::list>; diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx index 4ff013e..857e66e 100644 --- a/bpkg/pkg-build.cxx +++ b/bpkg/pkg-build.cxx @@ -5511,7 +5511,9 @@ namespace bpkg // Since there is no available package specified we need to // find it (or create a transient one). // - cfg = &p.init_skeleton (o, find_available (o, pdb, sp)); + cfg = &p.init_skeleton (o, + true /* load_old_dependent_config */, + find_available (o, pdb, sp)); } } else @@ -5902,6 +5904,8 @@ namespace bpkg database& pdb (p.db); shared_ptr& sp (p.selected); + assert (sp != nullptr); // Shouldn't be here otherwise. + // Each package is disfigured in its own transaction, so that we // always leave the configuration in a valid state. // @@ -5913,7 +5917,7 @@ namespace bpkg bool external (false); if (!simulate) { - external = (sp != nullptr && sp->external () && p.external ()); + external = (sp->external () && p.external ()); // Reset the keep_out flag if the package being unpacked is not // external. @@ -5934,8 +5938,6 @@ namespace bpkg { vector& ps (previous_prerequisites[&p]); - assert (sp != nullptr); // Shouldn't be here otherwise. - if (!sp->prerequisites.empty ()) { ps.reserve (sp->prerequisites.size ()); @@ -5948,16 +5950,38 @@ namespace bpkg // For an external package being replaced with another external, keep // the configuration unless requested not to with --disfigure. // - // Note that for other cases the preservation of the configuration is - // still a @@ TODO (the idea is to use our config.config.{save,load} - // machinery). Also see "parallel" logic in package_skeleton. + bool disfigure (p.disfigure || !external); + + // If the skeleton was not initialized yet (this is an existing package + // reconfiguration and no configuration was printed as a part of the + // plan, etc), then initialize it now. Whether the skeleton is newly + // initialized or not, make sure that the current configuration is + // loaded, unless the package project is not being disfigured. // + if (*p.action != build_package::drop && !p.system) + { + if (!p.skeleton) + { + // If there is no available package specified for the build package + // object, then we need to find it (or create a transient one). + // + p.init_skeleton (o, + true /* load_old_dependent_config */, + (p.available == nullptr + ? find_available (o, pdb, sp) + : nullptr)); + } + + if (disfigure) + p.skeleton->load_old_config (); + } + // Commits the transaction. // pkg_disfigure (o, pdb, t, sp, !p.keep_out /* clean */, - p.disfigure || !external /* disfigure */, + disfigure, simulate); r = true; @@ -6498,14 +6522,7 @@ namespace bpkg } 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); + assert (p.skeleton); // Must be initialized before disfiguring. cpr = pkg_configure_prerequisites (o, pdb, @@ -6533,22 +6550,10 @@ namespace bpkg // assert (sp->state == package_state::unpacked); - // Initialize the skeleton if it is not initialized yet. + // The skeleton must be initialized before disfiguring and the + // package can't be system. // - // 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, find_available (o, pdb, sp)); - - assert (p.skeleton->available != nullptr); // Can't be system. + assert (p.skeleton && p.skeleton->available != nullptr); const dependencies& deps (p.skeleton->available->dependencies); diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx index 0850001..eb5b85b 100644 --- a/bpkg/pkg-configure.cxx +++ b/bpkg/pkg-configure.cxx @@ -1109,7 +1109,10 @@ namespace bpkg &p->config_variables, move (src_root), move (out_root), - true /* load_old_dependent_config */), + nullopt /* old_src_root */, + nullopt /* old_out_root */, + package_skeleton::load_config_user | + package_skeleton::load_config_dependent), nullptr /* prerequisites */, false /* disfigured */, false /* simulate */); -- cgit v1.1