Minor tweaks to package skeleton implementation

4 files changed, 44 insertions, 45 deletions

diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx
index a0fd419..d789f60 100644
--- a/bpkg/package-skeleton.cxx
+++ b/bpkg/package-skeleton.cxx
@@ -83,17 +83,17 @@ namespace bpkg
                     const available_package& ap,
                     const strings& cvs,
                     optional<dir_path> src_root)
-      : db_ (db),
-        available_ (ap),
-        config_vars_ (cvs),
-        src_root_ (move (src_root))
+      : db_ (db), available_ (ap), config_vars_ (cvs)
   {
     // Should not be created for stubs.
     //
     assert (available_.get ().bootstrap_build);
 
-    if (src_root_)
+    if (src_root)
+    {
+      src_root_ = move (*src_root);
       out_root_ = dir_path (db_.get ().config_orig) /= name ().string ();
+    }
   }
 
   void package_skeleton::
@@ -123,7 +123,7 @@ namespace bpkg
       // they never clash with other temporary subdirectories (git
       // repositories, etc).
       //
-      if (!src_root_)
+      if (src_root_.empty ())
       {
         auto i (temp_dir.find (db_.get ().config_orig));
         assert (i != temp_dir.end ());
@@ -132,11 +132,10 @@ namespace bpkg
         d /= "skeletons";
         d /= name ().string () + '-' + ap.version.string ();
 
-        src_root_ = d;
-        out_root_ = move (d);
+        src_root_ = move (d); // out_root_ is the same.
       }
 
-      if (!exists (*src_root_))
+      if (!exists (src_root_))
       {
         // Create the buildfiles.
         //
@@ -145,7 +144,7 @@ namespace bpkg
         // additional files.
         //
         {
-          path bf (*src_root_ / std_bootstrap_file);
+          path bf (src_root_ / std_bootstrap_file);
 
           mk_p (bf.directory ());
 
@@ -168,11 +167,12 @@ namespace bpkg
           save (*ap.bootstrap_build, bf);
 
           if (ap.root_build)
-            save (*ap.root_build, *src_root_ / std_root_file);
+            save (*ap.root_build, src_root_ / std_root_file);
         }
 
         // Create the manifest file containing the bare minimum of values
-        // which can potentially be required to load the build system state.
+        // which can potentially be required to load the build system state
+        // (i.e., either via the version module or manual version extraction).
         //
         {
           package_manifest m;
@@ -181,11 +181,8 @@ namespace bpkg
 
           // Note that there is no guarantee that the potential build2
           // constraint has already been verified. Thus, we also serialize the
-          // depends value, delegating the constraint verification to the
-          // version module. Also note that normally the toolchain build-time
-          // dependencies are specified first and, if that's the case, their
-          // constraints are already verified at this point and so build2 will
-          // not fail due to the constraint violation.
+          // build2 dependency value, letting the version module verify the
+          // constraint.
           //
           // Also note that the resulting file is not quite a valid package
           // manifest, since it doesn't contain all the required values
@@ -201,7 +198,7 @@ namespace bpkg
               m.dependencies.push_back (das);
           }
 
-          path mf (*src_root_ / manifest_file);
+          path mf (src_root_ / manifest_file);
 
           try
           {
@@ -210,12 +207,14 @@ namespace bpkg
             m.serialize (s);
             os.close ();
           }
-          catch (const manifest_serialization&)
+          catch (const manifest_serialization& e)
           {
             // We shouldn't be creating a non-serializable manifest, since
             // it's crafted from the parsed values.
             //
             assert (false);
+
+            fail << "unable to serialize " << mf << ": " << e.description;
           }
           catch (const io_error& e)
           {
diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx
index 00be6a3..b26c7b9 100644
--- a/bpkg/package-skeleton.hxx
+++ b/bpkg/package-skeleton.hxx
@@ -22,10 +22,11 @@ namespace bpkg
     // If the package is external and will not be disfigured, then the
     // existing package source root directory needs to be specified. In this
     // case this source directory and the automatically deduced potentially
-    // non-existing out root directory will be used for build2 state loading
+    // non-existent out root directory will be used for build2 state loading
     // instead of the newly created skeleton directory. This, in particular,
-    // allows to consider existing configuration variables while evaluating
-    // the dependency clauses.
+    // makes sure we take into account the existing configuration variables
+    // while evaluating the dependency clauses (this logic is "parallel" to
+    // the configuration preservation in pkg-build.cxx).
     //
     // Note that the database and available_package are expected to outlive
     // this object.
@@ -125,8 +126,8 @@ namespace bpkg
     reference_wrapper<const available_package> available_;
     strings config_vars_;
 
-    optional<dir_path> src_root_;
-    optional<dir_path> out_root_;
+    dir_path src_root_;
+    dir_path out_root_; // If empty, the same as src_root_.
 
     unique_ptr<build2::context> ctx_;
     bool created_ = false;
diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index f4432b6..73d2284 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -1267,14 +1267,12 @@ namespace bpkg
         if (size_t n = deps.size ())
           pkg.dependencies->reserve (n);
 
-        optional<dir_path> src_root;
-        if (pkg.external () && !pkg.disfigure)
-          src_root = sp->src_root;
-
         pkg.skeleton = package_skeleton (pdb,
                                          *ap,
                                          pkg.config_vars,
-                                         move (src_root));
+                                         (pkg.external () && !pkg.disfigure
+                                          ? sp->src_root
+                                          : nullopt));
       }
 
       dependencies& sdeps (*pkg.dependencies);
@@ -7663,7 +7661,7 @@ namespace bpkg
       //
       // 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).
+      // machinery). Also see "parallel" logic in package_skeleton.
       //
       // Commits the transaction.
       //
@@ -8099,11 +8097,12 @@ namespace bpkg
         }
         else
         {
-          optional<dir_path> src_root;
-          if (p.external () && !p.disfigure)
-            src_root = sp->src_root;
-
-          package_skeleton ps (pdb, *ap, p.config_vars, move (src_root));
+          package_skeleton ps (pdb,
+                               *ap,
+                               p.config_vars,
+                               (p.external () && !p.disfigure
+                                ? sp->src_root
+                                : nullopt));
 
           pkg_configure (o,
                          pdb,
@@ -8134,11 +8133,12 @@ namespace bpkg
         if (dap == nullptr)
           dap = make_available (o, pdb, sp);
 
-        optional<dir_path> src_root;
-        if (p.external () && !p.disfigure)
-          src_root = sp->src_root;
-
-        package_skeleton ps (pdb, *dap, p.config_vars, move (src_root));
+        package_skeleton ps (pdb,
+                             *dap,
+                             p.config_vars,
+                             (p.external () && !p.disfigure
+                              ? sp->src_root
+                              : nullopt));
 
         // @@ Note that on reconfiguration the dependent looses the potential
         //    configuration variables specified by the user on some previous
diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx
index ff6fde6..9339f75 100644
--- a/bpkg/pkg-configure.cxx
+++ b/bpkg/pkg-configure.cxx
@@ -429,11 +429,10 @@ namespace bpkg
       //
       shared_ptr<available_package> ap (make_available (o, db, p));
 
-      optional<dir_path> src_root;
-      if (p->external ())
-        src_root = p->src_root;
-
-      package_skeleton ps (db, *ap, vars, move (src_root));
+      package_skeleton ps (db,
+                           *ap,
+                           vars,
+                           p->external () ? p->src_root : nullopt);
 
       pkg_configure (o,
                      db,