From f7786c1a87a374a33a9810626a6f5d26b61b0a23 Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Mon, 23 Jan 2023 13:22:05 +0200
Subject: Review

---
 bpkg/pkg-build-collect.hxx             |  6 ++++++
 bpkg/pkg-build.cxx                     | 24 +++++++++++-------------
 bpkg/system-package-manager-debian.cxx |  2 +-
 bpkg/system-package-manager-debian.hxx |  3 ++-
 bpkg/system-package-manager.hxx        |  2 +-
 5 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/bpkg/pkg-build-collect.hxx b/bpkg/pkg-build-collect.hxx
index 5f64f31..6c79abe 100644
--- a/bpkg/pkg-build-collect.hxx
+++ b/bpkg/pkg-build-collect.hxx
@@ -216,6 +216,12 @@ namespace bpkg
     // system package manager (as opposed to user/fallback). Otherwise, return
     // NULL (so can be used as bool).
     //
+    // Note on terminology: We call the bpkg package that is being configured
+    // as available from the system as "system package" and we call the
+    // underlying package managed by the system/distribution package manager
+    // as "system/distribution package". See system-package-manager.hxx for
+    // background.
+    //
     const system_package_status*
     system_status () const;
 
diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index 1af6c7a..5afc1a5 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -4408,9 +4408,9 @@ namespace bpkg
       system_map sys_map;
 
       // Iterate in the reverse order as we will do for printing the action
-      // lines. This way a sys-install action line will be printed right
+      // lines. This way a system-install action line will be printed right
       // before the bpkg action line of a package which appears first in the
-      // sys-install action's 'required by' list.
+      // system-install action's 'required by' list.
       //
       for (const build_package& p: reverse_iterate (pkgs))
       {
@@ -4479,6 +4479,9 @@ namespace bpkg
           // Make sure that we print this system-install action just once.
           //
           sys_map.erase (j);
+
+          // Note that we don't increment i in order to re-iterate this pkgs
+          // entry.
         }
         else
         {
@@ -4859,33 +4862,28 @@ namespace bpkg
     {
       // Collect the names of all the system packages being managed by the
       // system package manager (as opposed to user/fallback), suppressing
-      // duplicates. Check if any of the system/distribution packages need to
-      // be installed.
+      // duplicates.
       //
-      vector<package_name> ips;
-      bool install (false);
+      vector<package_name> ps;
 
       for (build_package& p: build_pkgs)
       {
         if (p.system_status () &&
-            find (ips.begin (), ips.end (), p.name ()) == ips.end ())
+            find (ps.begin (), ps.end (), p.name ()) == ps.end ())
         {
-          ips.push_back (p.name ());
-
-          if (!install && p.system_install ())
-            install = true;
+          ps.push_back (p.name ());
         }
       }
 
       // Install the system/distribution packages.
       //
-      if (install)
+      if (!ps.empty ())
       {
         // Otherwise, we wouldn't get any package statuses.
         //
         assert (sys_pkg_mgr && *sys_pkg_mgr != nullptr);
 
-        (*sys_pkg_mgr)->pkg_install (ips);
+        (*sys_pkg_mgr)->pkg_install (ps, /*&& ops.sys_install ()*/ false);
       }
     }
 
diff --git a/bpkg/system-package-manager-debian.cxx b/bpkg/system-package-manager-debian.cxx
index d294416..1f187c8 100644
--- a/bpkg/system-package-manager-debian.cxx
+++ b/bpkg/system-package-manager-debian.cxx
@@ -973,7 +973,7 @@ namespace bpkg
   }
 
   void system_package_manager_debian::
-  pkg_install (const vector<package_name>&)
+  pkg_install (const vector<package_name>&, bool)
   {
     assert (!installed_);
     installed_ = true;
diff --git a/bpkg/system-package-manager-debian.hxx b/bpkg/system-package-manager-debian.hxx
index 74d9867..6fd1f2e 100644
--- a/bpkg/system-package-manager-debian.hxx
+++ b/bpkg/system-package-manager-debian.hxx
@@ -26,7 +26,8 @@ namespace bpkg
                 bool fetch) override;
 
     virtual void
-    pkg_install (const vector<package_name>&) override;
+    pkg_install (const vector<package_name>&,
+                 bool install) override;
 
   public:
     // Note: expects os_release::name_id to be "debian" or os_release::like_id
diff --git a/bpkg/system-package-manager.hxx b/bpkg/system-package-manager.hxx
index 6cf90e5..f4ce37e 100644
--- a/bpkg/system-package-manager.hxx
+++ b/bpkg/system-package-manager.hxx
@@ -154,7 +154,7 @@ namespace bpkg
     // progress and diagnostics.
     //
     virtual void
-    pkg_install (const vector<package_name>&) = 0;
+    pkg_install (const vector<package_name>&, bool install) = 0;
 
   public:
     virtual
-- 
cgit v1.1