Add support for preserving old package configuration on up/downgrade and reconfiguration

6 files changed, 323 insertions, 178 deletions

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<config_variable>* css,
                     optional<dir_path> src_root,
                     optional<dir_path> out_root,
-                    bool load_old_dependent_config)
+                    optional<dir_path> old_src_root,
+                    optional<dir_path> 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<strings, vector<config_variable>> 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
     // <config-dir>/<package-name>), 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_variable>* config_srcs,
                       optional<dir_path> src_root,
                       optional<dir_path> out_root,
-                      bool load_old_dependent_config);
-
+                      optional<dir_path> old_src_root,
+                      optional<dir_path> old_out_root,
+                      uint16_t load_config_flags);
 
     package_key package;
     bool system;
     shared_ptr<const available_package> 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_source> 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<available_package>& override,
-                 optional<dir_path> src_root,
-                 optional<dir_path> out_root,
-                 bool load_old_dependent_config)
+                 bool load_old_dependent_config,
+                 const shared_ptr<available_package>& override)
   {
     shared_ptr<available_package> ap (override != nullptr
                                       ? override
@@ -390,12 +388,59 @@ namespace bpkg
         ap = nullptr;
     }
 
-    if (!src_root && ap != nullptr)
+    optional<dir_path> src_root;
+    optional<dir_path> out_root;
+
+    optional<dir_path> old_src_root;
+    optional<dir_path> 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<dir_path> ());
+      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<package_skeleton>& 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<package_version_key> 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<available_package>& override = nullptr,
-                   optional<dir_path> src_root = nullopt,
-                   optional<dir_path> out_root = nullopt,
-                   bool load_old_dependent_config = false);
+                   bool load_old_dependent_config = true,
+                   const shared_ptr<available_package>& override = nullptr);
   };
 
   using build_package_list = std::list<reference_wrapper<build_package>>;
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<selected_package>& 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<package_name>& 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 */);