Increment version iteration number for selected non-external package regardless of manifest/subprojects checksum

10 files changed, 115 insertions, 162 deletions

diff --git a/bpkg/package.cxx b/bpkg/package.cxx
index fe04248..c98e7a4 100644
--- a/bpkg/package.cxx
+++ b/bpkg/package.cxx
@@ -389,45 +389,54 @@ namespace bpkg
                             false /* iteration */))
       return nullopt;
 
-    string mc (package_checksum (o, d, pi));
+    bool changed (!p->external ());
 
-    // The selected package must not be "simulated" (see pkg-build for
-    // details).
+    // If the selected package is not external, then increment the iteration
+    // number to make the external package preferable. Note that for such
+    // packages the manifest/subprojects and buildfiles checksums are absent.
     //
-    assert (p->manifest_checksum);
-
-    bool changed (mc != *p->manifest_checksum);
-
-    // If the manifest hasn't changed and the package has buildfile clauses in
-    // the dependencies, then check if the buildfiles haven't changed either.
-    //
-    if (!changed && p->buildfiles_checksum)
+    if (!changed)
     {
-      // Always calculate the checksum over the buildfiles since the package
-      // is external.
+      // The selected package must not be "simulated" (see pkg-build for
+      // details).
       //
-      changed = package_buildfiles_checksum (
-        nullopt /* bootstrap_build */,
-        nullopt /* root_build */,
-        {}      /* buildfiles */,
-        d) != *p->buildfiles_checksum;
-    }
+      assert (p->manifest_checksum);
 
-    // If the manifest hasn't changed but the selected package points to an
-    // external source directory, then we also check if the directory have
-    // moved.
-    //
-    if (!changed && p->external ())
-    {
-      dir_path src_root (p->effective_src_root (db.config));
+      changed = (package_checksum (o, d, pi) != *p->manifest_checksum);
 
-      // We need to complete and normalize the source directory as it may
-      // generally be completed against the configuration directory (unlikely
-      // but possible), that can be relative and/or not normalized.
+      // If the manifest hasn't changed and the package has buildfile clauses
+      // in the dependencies, then check if the buildfiles haven't changed
+      // either.
       //
-      normalize (src_root, "package source");
+      if (!changed && p->buildfiles_checksum)
+      {
+        // Always calculate the checksum over the buildfiles since the package
+        // is external.
+        //
+        changed = package_buildfiles_checksum (
+          nullopt /* bootstrap_build */,
+          nullopt /* root_build */,
+          {}      /* buildfiles */,
+          d) != *p->buildfiles_checksum;
+      }
+
+      // If the manifest hasn't changed but the selected package points to an
+      // external source directory, then we also check if the directory have
+      // moved.
+      //
+      if (!changed)
+      {
+        dir_path src_root (p->effective_src_root (db.config));
 
-      changed = src_root != normalize (d, "package source");
+        // We need to complete and normalize the source directory as it may
+        // generally be completed against the configuration directory
+        // (unlikely but possible), that can be relative and/or not
+        // normalized.
+        //
+        normalize (src_root, "package source");
+
+        changed = src_root != normalize (d, "package source");
+      }
     }
 
     return !changed
diff --git a/bpkg/package.hxx b/bpkg/package.hxx
index 3283be5..060f13a 100644
--- a/bpkg/package.hxx
+++ b/bpkg/package.hxx
@@ -1237,9 +1237,9 @@ namespace bpkg
     // package version revision increment. In particular, new subprojects
     // should trigger the package reconfiguration.
     //
-    // Must be present if the source directory is present, unless the object
-    // is created/updated during the package build simulation (see pkg-build
-    // for details). Note that during the simulation the manifest may not be
+    // Only present for external packages, unless the objects are
+    // created/updated during the package build simulation (see pkg-build for
+    // details). Note that during the simulation the manifest may not be
     // available.
     //
     // @@ Currently we don't consider subprojects recursively (would most
@@ -1251,13 +1251,13 @@ namespace bpkg
     //
     optional<std::string> manifest_checksum;
 
-    // Absent if the package has no buildfile clauses in the dependencies.
-    // Otherwise, the checksum of the buildfiles calculated over the *-build
-    // manifest values or, if unspecified, the files in the package source
-    // directory.
+    // Only present for external packages which have buildfile clauses in the
+    // dependencies, unless the objects are created/updated during the package
+    // build simulation (see pkg-build for details).
     //
-    // Note that for external packages the checksum is always calculated over
-    // the files. This is "parallel" to the package skeleton logic.
+    // Note that the checksum is always calculated over the files rather than
+    // the *-build manifest values. This is "parallel" to the package skeleton
+    // logic.
     //
     optional<std::string> buildfiles_checksum;
 
@@ -1429,15 +1429,17 @@ namespace bpkg
   //
   // - The package directory is considered an iteration of the package if this
   //   upstream version and revision is already present (selected) in the
-  //   configuration and has a source directory. If that's the case, then the
-  //   specified directory path and the package checksum (see
-  //   package_checksum() for details) are compared to the ones of the package
-  //   present in the configuration. If both match, then the present package
-  //   version (including its iteration, if any) is returned.  Otherwise (the
-  //   package has moved and/or the package information has changed), the
-  //   present package version with the incremented iteration number is
-  //   returned. Note that the directory path is matched only for the external
-  //   selected packages.
+  //   configuration and has a source directory. If that's the case and if the
+  //   present version is not external (the package is being switched to a
+  //   local potentially amended version), then the present package version
+  //   with the incremented iteration number is returned. Otherwise (the
+  //   present package is external), the specified directory path and the
+  //   package checksum (see package_checksum() for details) are compared to
+  //   the ones of the package present in the configuration. If both match,
+  //   then the present package version (including its iteration, if any) is
+  //   returned. Otherwise (the package has moved and/or the package
+  //   information has changed), the present package version with the
+  //   incremented iteration number is returned.
   //
   // - Only a single package iteration is valid per version in the
   //   configuration. This, in particular, means that a package of the
diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx
index 95e4eee..69c0eea 100644
--- a/bpkg/pkg-build-collect.cxx
+++ b/bpkg/pkg-build-collect.cxx
@@ -1411,7 +1411,6 @@ namespace bpkg
           (pkg.selected->version != pkg.available_version () ||
            pkg.system))
       {
-
         for (const postponed_configuration& cfg: postponed_cfgs)
         {
           auto i (cfg.dependents.find (pk));
diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index 55fae96..2796550 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -5370,7 +5370,7 @@ namespace bpkg
 
             // Commits the transaction.
             //
-            sp = pkg_unpack (o, pdb, af.database (), t, ap->id.name, simulate);
+            sp = pkg_unpack (o, pdb, t, ap->id.name, simulate);
 
             if (result)
               text << "unpacked " << *sp << pdb;
diff --git a/bpkg/pkg-checkout.cxx b/bpkg/pkg-checkout.cxx
index 87579cc..9793c41 100644
--- a/bpkg/pkg-checkout.cxx
+++ b/bpkg/pkg-checkout.cxx
@@ -170,9 +170,6 @@ namespace bpkg
     const repository_location& rl (pl->repository_fragment->location);
 
     auto_rmdir rmd;
-    optional<string> mc;
-    optional<string> bc;
-
     const dir_path& ord (output_root ? *output_root : c);
     dir_path d (ord / dir_path (n.string () + '-' + v.string ()));
 
@@ -312,21 +309,6 @@ namespace bpkg
              "!config.dist.bootstrap=true",
              "config.dist.root='" + ord.representation () + '\'',
              bspec);
-
-      mc = package_checksum (o, d, nullptr /* package_info */);
-
-      // Calculate the buildfiles checksum if the package has any buildfile
-      // clauses in the dependencies.
-      //
-      // Note that the available package already has all the buildfiles
-      // loaded.
-      //
-      if ((p != nullptr && p->manifest_checksum == mc)
-          ? p->buildfiles_checksum.has_value ()
-          : has_buildfile_clause (ap->dependencies))
-        bc = package_buildfiles_checksum (ap->bootstrap_build,
-                                          ap->root_build,
-                                          ap->buildfiles);
     }
 
     if (p != nullptr)
@@ -360,13 +342,16 @@ namespace bpkg
 
     if (p != nullptr)
     {
+      // Note: we can be replacing an external package and thus we reset the
+      // manifest/subprojects and buildfiles checksums.
+      //
       p->version = move (v);
       p->state = package_state::unpacked;
       p->repository_fragment = rl;
       p->src_root = move (d);
       p->purge_src = purge;
-      p->manifest_checksum = move (mc);
-      p->buildfiles_checksum = move (bc);
+      p->manifest_checksum = nullopt;
+      p->buildfiles_checksum = nullopt;
 
       pdb.update (p);
     }
@@ -386,8 +371,8 @@ namespace bpkg
         false,
         move (d),  // Source root.
         purge,     // Purge directory.
-        move (mc),
-        move (bc),
+        nullopt,   // No manifest/subprojects checksum.
+        nullopt,   // No buildfiles checksum.
         nullopt,   // No output directory yet.
         {}});      // No prerequisites captured yet.
 
diff --git a/bpkg/pkg-purge.cxx b/bpkg/pkg-purge.cxx
index 4fe040e..07951a6 100644
--- a/bpkg/pkg-purge.cxx
+++ b/bpkg/pkg-purge.cxx
@@ -50,6 +50,7 @@ namespace bpkg
       //
       p->src_root = nullopt;
       p->manifest_checksum = nullopt;
+      p->buildfiles_checksum = nullopt;
 
       if (archive)
       {
diff --git a/bpkg/pkg-unpack.cxx b/bpkg/pkg-unpack.cxx
index 8f01a8a..90ed331 100644
--- a/bpkg/pkg-unpack.cxx
+++ b/bpkg/pkg-unpack.cxx
@@ -162,7 +162,10 @@ namespace bpkg
     optional<string> mc;
     optional<string> bc;
 
-    if (!simulate)
+    // Only calculate the manifest/subprojects and buildfiles checksums for
+    // external packages (see selected_package::external() for details).
+    //
+    if (!simulate && (rl.empty () || rl.directory_based ()))
     {
       mc = package_checksum (o, d, pi);
 
@@ -334,7 +337,6 @@ namespace bpkg
   shared_ptr<selected_package>
   pkg_unpack (const common_options& co,
               database& db,
-              database& rdb,
               transaction& t,
               const package_name& name,
               bool simulate)
@@ -371,8 +373,6 @@ namespace bpkg
       fail << "package directory " << d << " already exists";
 
     auto_rmdir arm;
-    optional<string> mc;
-    optional<string> bc;
 
     if (!simulate)
     {
@@ -402,62 +402,11 @@ namespace bpkg
       {
         fail << "unable to extract " << a << " to " << c << ": " << e;
       }
-
-      mc = package_checksum (co, d, nullptr /* package_info */);
-
-      // Calculate the buildfiles checksum if the package has any buildfile
-      // clauses in the dependencies.
-      //
-      // Note that we may not have the available package (e.g., fetched as an
-      // existing package archive rather than from an archive repository), in
-      // which case we need to parse the manifest to retrieve the
-      // dependencies. This is unfortunate, but is probably not a big deal
-      // performance-wise given that this is not too common and we are running
-      // an archive unpacking process anyway.
-      //
-      shared_ptr<available_package> ap (
-        rdb.find<available_package> (available_package_id (n, v)));
-
-      if (ap != nullptr)
-      {
-        // Note that the available package already has all the buildfiles
-        // loaded.
-        //
-        if (has_buildfile_clause (ap->dependencies))
-          bc = package_buildfiles_checksum (ap->bootstrap_build,
-                                            ap->root_build,
-                                            ap->buildfiles);
-      }
-      else
-      {
-        // Note that we don't need to translate the package version here since
-        // the manifest comes from an archive and so has a proper version
-        // already.
-        //
-        package_manifest m (
-          pkg_verify (co,
-                      d,
-                      true  /* ignore_unknown */,
-                      false /* ignore_toolchain */,
-                      false /* load_buildfiles */,
-                      function<package_manifest::translate_function> ()));
-
-        if (has_buildfile_clause (m.dependencies))
-          bc = package_buildfiles_checksum (m.bootstrap_build,
-                                            m.root_build,
-                                            m.buildfiles,
-                                            d,
-                                            m.buildfile_paths,
-                                            m.alt_naming);
-      }
     }
 
     p->src_root = d.leaf (); // For now assuming to be in configuration.
     p->purge_src = true;
 
-    p->manifest_checksum = move (mc);
-    p->buildfiles_checksum = move (bc);
-
     p->state = package_state::unpacked;
 
     db.update (p);
@@ -522,7 +471,6 @@ namespace bpkg
       p = v.empty ()
         ? pkg_unpack (o,
                       db /* pdb */,
-                      db /* rdb */,
                       t,
                       n,
                       false /* simulate */)
diff --git a/bpkg/pkg-unpack.hxx b/bpkg/pkg-unpack.hxx
index 99d74e0..7394732 100644
--- a/bpkg/pkg-unpack.hxx
+++ b/bpkg/pkg-unpack.hxx
@@ -32,13 +32,9 @@ namespace bpkg
 
   // Unpack the fetched package and commit the transaction.
   //
-  // Note that both package and repository information configurations need to
-  // be passed.
-  //
   shared_ptr<selected_package>
   pkg_unpack (const common_options&,
-              database& pdb,
-              database& rdb,
+              database&,
               transaction&,
               const package_name&,
               bool simulate);
diff --git a/tests/pkg-build.testscript b/tests/pkg-build.testscript
index fc89df5..d30186a 100644
--- a/tests/pkg-build.testscript
+++ b/tests/pkg-build.testscript
@@ -4625,33 +4625,36 @@ test.arguments += --sys-no-query
                 $backend_configured
               EOO
 
-            # No upgrade after turning a package from the archive-based repo
-            # into an external package.
+            # Iteration increment and upgrade after turning a package from the
+            # archive-based repo into an external package.
             #
             $* fax/ 2>>~%EOE%;
-              %info: .+fax-1.0.0.+ is up to date%
-              updated fax/1.0.0
+              disfigured fax/1.0.0
+              using fax/1.0.0#1 (external)
+              configured fax/1.0.0#1
+              %info: .+dir.fax.+ is up to date%
+              updated fax/1.0.0#1
               EOE
 
             $pkg_status -r >>"EOO";
-              !fax configured !1.0.0
+              !fax configured !1.0.0#1
                 $backend_configured
               EOO
 
-            # Upgrade after the package' buildfile is edited.
+            # Further upgrade after the package' buildfile is edited.
             #
             echo '' >+fax/build/root.build;
 
             $* fax/ 2>>~%EOE%;
-              disfigured fax/1.0.0
-              using fax/1.0.0#1 (external)
-              configured fax/1.0.0#1
+              disfigured fax/1.0.0#1
+              using fax/1.0.0#2 (external)
+              configured fax/1.0.0#2
               %info: .+fax.+ is up to date%
-              updated fax/1.0.0#1
+              updated fax/1.0.0#2
               EOE
 
             $pkg_status -r >>"EOO";
-              !fax configured !1.0.0#1
+              !fax configured !1.0.0#2
                 $backend_configured
               EOO
 
@@ -4659,11 +4662,11 @@ test.arguments += --sys-no-query
             #
             $* fax/ 2>>~%EOE%;
               %info: .+fax.+ is up to date%
-              updated fax/1.0.0#1
+              updated fax/1.0.0#2
               EOE
 
             $pkg_status -r >>"EOO";
-              !fax configured !1.0.0#1
+              !fax configured !1.0.0#2
                 $backend_configured
               EOO
 
@@ -4692,37 +4695,40 @@ test.arguments += --sys-no-query
                 $backend_configured
               EOO
 
-            # No upgrade after turning a package from the archive-based repo
-            # into an external package.
+            # Iteration increment and upgrade after turning a package from the
+            # archive-based repo into an external package.
             #
             $rep_add --type dir fax/ && $rep_fetch;
 
             $* fax 2>>~%EOE%;
-              %info: .+fax-1.0.0.+ is up to date%
-              updated fax/1.0.0
+              disfigured fax/1.0.0
+              using fax/1.0.0#1 (external)
+              configured fax/1.0.0#1
+              %info: .+dir.fax.+ is up to date%
+              updated fax/1.0.0#1
               EOE
 
             $pkg_status -r >>"EOO";
-              !fax configured 1.0.0
+              !fax configured 1.0.0#1
                 $backend_configured
               EOO
 
-            # Upgrade after the package' buildfile is edited.
+            # Further upgrade after the package' buildfile is edited.
             #
             echo '' >+fax/build/root.build;
 
             $rep_fetch;
 
             $* fax 2>>~%EOE%;
-              disfigured fax/1.0.0
-              using fax/1.0.0#1 (external)
-              configured fax/1.0.0#1
+              disfigured fax/1.0.0#1
+              using fax/1.0.0#2 (external)
+              configured fax/1.0.0#2
               %info: .+fax.+ is up to date%
-              updated fax/1.0.0#1
+              updated fax/1.0.0#2
               EOE
 
             $pkg_status -r >>"EOO";
-              !fax configured 1.0.0#1
+              !fax configured 1.0.0#2
                 $backend_configured
               EOO
 
@@ -4732,11 +4738,11 @@ test.arguments += --sys-no-query
 
             $* fax 2>>~%EOE%;
               %info: .+fax.+ is up to date%
-              updated fax/1.0.0#1
+              updated fax/1.0.0#2
               EOE
 
             $pkg_status -r >>"EOO";
-              !fax configured 1.0.0#1
+              !fax configured 1.0.0#2
                 $backend_configured
               EOO
 
diff --git a/tests/rep-fetch.testscript b/tests/rep-fetch.testscript
index b9b9e05..b713c0c 100644
--- a/tests/rep-fetch.testscript
+++ b/tests/rep-fetch.testscript
@@ -631,11 +631,15 @@ if! $remote
 
       : unchanged-external
       :
+      : Test that iteration is still incremented when a non-external package
+      : from a pkg repository is switched to the same unedited external
+      : package.
+      :
       {
         $clone_cfg && $rep_add $src/libhello-1.0.0;
         $* 2>!;
 
-        $pkg_status libhello >'libhello unpacked 1.0.0'
+        $pkg_status libhello >'libhello unpacked 1.0.0 available 1.0.0#1'
       }
 
       : changed-external
@@ -655,6 +659,9 @@ if! $remote
 
     : git-rep
     :
+    : Test that iteration is still incremented when a non-external package
+    : from a git repository is switched to the same unedited external package.
+    :
     if ($git_supported && !$remote)
     {
       rep = $canonicalize([dir_path] $out_git/state0);
@@ -668,7 +675,7 @@ if! $remote
       $rep_add $rep/style.git;
       $* 2>!;
 
-      $pkg_status style >"style unpacked 1.0.0";
+      $pkg_status style >"style unpacked 1.0.0 available 1.0.0#1";
 
       $pkg_purge style 2>"purged style/1.0.0"
     }