Switch back to single available system package version

3 files changed, 113 insertions, 88 deletions

diff --git a/bpkg/system-package-manager-debian.cxx b/bpkg/system-package-manager-debian.cxx
index f30cfd6..cf15a46 100644
--- a/bpkg/system-package-manager-debian.cxx
+++ b/bpkg/system-package-manager-debian.cxx
@@ -203,6 +203,8 @@ namespace bpkg
       {
         unique_ptr<package_status_debian> g (parse_group (gs[i]));
 
+        // @@ Shouldn't we filter some based on what we are installing?
+
         if (!g->main.empty ())   r->extras.push_back (move (g->main));
         if (!g->dev.empty ())    r->extras.push_back (move (g->dev));
         if (!g->doc.empty ())    r->extras.push_back (move (g->doc));
@@ -218,7 +220,7 @@ namespace bpkg
     return r;
   }
 
-  const vector<unique_ptr<package_status>>*
+  optional<const system_package_status*>
   system_package_manager_debian::
   pkg_status (const package_name& pn,
               const available_packages* aps,
@@ -231,13 +233,13 @@ namespace bpkg
       auto i (status_cache_.find (pn));
 
       if (i != status_cache_.end ())
-        return &i->second;
+        return i->second.get ();
 
       if (aps == nullptr)
         return nullptr;
     }
 
-    vector<unique_ptr<package_status>> r;
+    vector<unique_ptr<package_status_debian>> rs;
 
     // Translate our package name to the Debian package names.
     //
@@ -276,7 +278,7 @@ namespace bpkg
         else
           s.reset (new package_status_debian (n));
 
-        r.push_back (move (s));
+        rs.push_back (move (s));
       }
       else
       {
@@ -289,16 +291,15 @@ namespace bpkg
           // Suppress duplicates for good measure based on the main package
           // name (and falling back to -dev if empty).
           //
-          auto i (find_if (r.begin (), r.end (),
-                           [&s] (const unique_ptr<package_status>& x)
+          auto i (find_if (rs.begin (), rs.end (),
+                           [&s] (const unique_ptr<package_status_debian>& x)
                            {
-                             const package_status_debian& d (as_debian (x));
                              return s->main.empty ()
-                               ? s->dev == d.dev
-                               : s->main == d.main;
+                               ? s->dev == x->dev
+                               : s->main == x->main;
                            }));
-          if (i == r.end ())
-            r.push_back (move (s));
+          if (i == rs.end ())
+            rs.push_back (move (s));
           else
           {
             // @@ Should we verify the rest matches for good measure?
@@ -309,42 +310,12 @@ namespace bpkg
 
     // First look for an already installed package.
     //
+    unique_ptr<package_status_debian> r;
 
 
     // Next look for available versions if we are allowed to install.
     //
-    // We only do this if we don't have a package already installed. This is
-    // because while Debian generally supports installing multiple versions in
-    // parallel, this is unlikely to be supported for the packages we are
-    // interested in due to the underlying limitations.
-    //
-    // Specifically, the packages that we are primarily interested in are
-    // libraries with headers and executables (tools). While Debian is able to
-    // install multiple libraries in parallel, it normally can only install a
-    // single set of headers, static libraries, pkg-config files, etc., (i.e.,
-    // the -dev package) at a time due to them being installed into the same
-    // location (e.g., /usr/include). The same holds for executables, which
-    // are installed into the same location (e.g., /usr/bin).
-    //
-    // It is possible that a certain library has made arrangements for
-    // multiple of its versions to co-exist. For example, hypothetically, our
-    // libssl package could be mapped to both libssl1.1 libssl1.1-dev and
-    // libssl3 libssl3-dev which could be installed at the same time (note
-    // that it is not the case in reality; there is only libssl-dev). However,
-    // in this case, we should probably also have two packages with separate
-    // names (e.g., libssl and libssl3) that can also co-exist. An example of
-    // this would be libQt5Core and libQt6Core. (Note that strictly speaking
-    // there could be different degrees of co-existence: for the system
-    // package manager it is sufficient for different versions not to clobber
-    // each other's files while for us we may also need the ability to use
-    // different versions in the base build).
-    //
-    // Note also that the above reasoning is quite C/C++-centric and it's
-    // possible that multiple versions of libraries (or equivalent) for other
-    // languages (e.g., Rust) can always co-exist. In this case we may need to
-    // revise this decision.
-    //
-    if (install && r.empty ())
+    if (r == nullptr && install)
     {
       if (fetch && !fetched_)
       {
@@ -356,12 +327,13 @@ namespace bpkg
 
     // Cache.
     //
-    return &status_cache_.emplace (pn, move (r)).first->second;
+    return status_cache_.emplace (pn, move (r)).first->second.get ();
   }
 
-  bool system_package_manager_debian::
-  pkg_install (const package_name&, const version&)
+  void system_package_manager_debian::
+  pkg_install (const vector<package_name>&)
   {
-    return false;
+    assert (!installed_);
+    installed_ = true;
   }
 }
diff --git a/bpkg/system-package-manager-debian.hxx b/bpkg/system-package-manager-debian.hxx
index a2f56a2..74d9867 100644
--- a/bpkg/system-package-manager-debian.hxx
+++ b/bpkg/system-package-manager-debian.hxx
@@ -4,6 +4,8 @@
 #ifndef BPKG_SYSTEM_PACKAGE_MANAGER_DEBIAN_HXX
 #define BPKG_SYSTEM_PACKAGE_MANAGER_DEBIAN_HXX
 
+#include <map>
+
 #include <bpkg/types.hxx>
 #include <bpkg/utility.hxx>
 
@@ -17,14 +19,14 @@ namespace bpkg
   class system_package_manager_debian: public system_package_manager
   {
   public:
-    virtual const vector<unique_ptr<system_package_status>>*
+    virtual optional<const system_package_status*>
     pkg_status (const package_name&,
                 const available_packages*,
                 bool install,
                 bool fetch) override;
 
-    virtual bool
-    pkg_install (const package_name&, const version&) override;
+    virtual void
+    pkg_install (const vector<package_name>&) override;
 
   public:
     // Note: expects os_release::name_id to be "debian" or os_release::like_id
@@ -35,7 +37,10 @@ namespace bpkg
         : system_package_manager (move (osr)) {}
 
   protected:
-    bool fetched_ = false; // True if already fetched metadata.
+    bool fetched_ = false;   // True if already fetched metadata.
+    bool installed_ = false; // True if already installed.
+
+    std::map<package_name, unique_ptr<system_package_status>> status_cache_;
   };
 }
 
diff --git a/bpkg/system-package-manager.hxx b/bpkg/system-package-manager.hxx
index 7d960f4..2d782b3 100644
--- a/bpkg/system-package-manager.hxx
+++ b/bpkg/system-package-manager.hxx
@@ -4,8 +4,6 @@
 #ifndef BPKG_SYSTEM_PACKAGE_MANAGER_HXX
 #define BPKG_SYSTEM_PACKAGE_MANAGER_HXX
 
-#include <map>
-
 #include <libbpkg/manifest.hxx>     // version
 #include <libbpkg/package-name.hxx>
 
@@ -20,6 +18,65 @@ namespace bpkg
   // The system package manager interface. Used by both pkg-build (to query
   // and install system packages) and by pkg-bindist (to build them).
   //
+  // Note that currently the result of a query is a single available version.
+  // While some package managers may support having multiple available
+  // versions and may even allow installing multiple versions in parallel,
+  // supporting this on our side will complicate things quite a bit. While we
+  // can probably plug multiple available versions into our constraint
+  // satisfaction machinery, the rabbit hole goes deeper than that since, for
+  // example, different bpkg packages can be mapped to the same system
+  // package, as is the case for libcrypto/libssl which are both mapped to
+  // libssl on Debian.  This means we will need to somehow coordinate (and
+  // likely backtrack) version selection between unrelated bpkg packages
+  // because only one underlying system version can be selected. (One
+  // simplified way to handle this would be to detect that different versions
+  // we selected and fail asking the user to resolve this manually.)
+  //
+  // Additionally, parallel installation is unlikely to be suppored for the
+  // packages we are interested in due to the underlying limitations.
+  // Specifically, the packages that we are primarily interested in are
+  // libraries with headers and executables (tools). While most package
+  // managers (e.g., Debian, Fedora) are able to install multiple libraries in
+  // parallel, they normally can only install a single set of headers, static
+  // libraries, pkg-config files, etc., (e.g., -dev/-devel package) at a time
+  // due to them being installed into the same location (e.g.,
+  // /usr/include). The same holds for executables, which are installed into
+  // the same location (e.g., /usr/bin).
+  //
+  // It is possible that a certain library has made arrangements for
+  // multiple of its versions to co-exist. For example, hypothetically, our
+  // libssl package could be mapped to both libssl1.1 libssl1.1-dev and
+  // libssl3 libssl3-dev which could be installed at the same time (note
+  // that it is not the case in reality; there is only libssl-dev). However,
+  // in this case, we should probably also have two packages with separate
+  // names (e.g., libssl and libssl3) that can also co-exist. An example of
+  // this would be libQt5Core and libQt6Core. (Note that strictly speaking
+  // there could be different degrees of co-existence: for the system
+  // package manager it is sufficient for different versions not to clobber
+  // each other's files while for us we may also need the ability to use
+  // different versions in the base build).
+  //
+  // Note also that the above reasoning is quite C/C++-centric and it's
+  // possible that multiple versions of libraries (or equivalent) for other
+  // languages (e.g., Rust) can always co-exist. Plus, even in the case of
+  // C/C++ libraries, there is still the plausible case of picking one of
+  // the multiple available version.
+  //
+  // On the other hand, the ultimate goal of system package managers, at least
+  // traditional ones like Debian and Fedora, is to end up with a single,
+  // usually the latest available, version of the package that is used by
+  // everyone. In fact, if one looks at a stable distributions of Debian and
+  // Fedora, they normally provide only a single version of each package. This
+  // decision will also likely simplify the implementation. For example, on
+  // Debian, it's straightforward to get the installed and candidate versions
+  // (e.g., from apt-cache policy). But getting all the possible versions that
+  // can be installed without having to specify the release explicitly is a
+  // lot less straightforward (see the apt-cache command documentation in The
+  // Debian Administrator's Handbook for background).
+  //
+  // So for now we keep it simple and pick a single available version but can
+  // probably revise this decision later.
+  //
   class system_package_status
   {
   public:
@@ -27,27 +84,18 @@ namespace bpkg
     //
     bpkg::version version;
 
+    // System (as in, distribution package) version.
+    //
+    string system_version;
+
     // The system package can be either "available already installed",
     // "available partially installed" (for example, libfoo but not
     // libfoo-dev is installed) or "available not yet installed".
     //
-    // Whether not_installed versions can be returned along with installed
-    // or partially_installed depends on whether the packager manager can
-    // install multiple versions side-by-side.
-    //
     enum status_type {installed, partially_installed, not_installed};
 
     status_type status = not_installed;
 
-    // System (as in, distribution package) name and version.
-    //
-    // @@ But these could be multiple. Do we really need this?
-    // @@ Can now probably provide as virtual functions.
-    /*
-    string system_name;
-    string system_version;
-    */
-
   public:
     virtual
     ~system_package_status ();
@@ -63,41 +111,43 @@ namespace bpkg
     // This function has two modes: cache-only (available_packages is NULL)
     // and full (available_packages is not NULL). In the cache-only mode this
     // function returns the status of this package if it has already been
-    // queried and NULL otherwise. This allows the caller to only collect all
-    // the available packages (for the name/version mapping information) if
-    // really necessary.
+    // queried and nullopt otherwise. This allows the caller to only collect
+    // all the available packages (for the name/version mapping information)
+    // if really necessary.
     //
-    // The returned list should be arranged in the preference order with the
-    // first entry having the highest preference. Normally this will be in the
-    // descending version order but can also be something more elaborate, such
-    // as the already installed or partially installed version coming first
-    // with the descending version order after that.
-    //
-    // The returned list can be empty, which indicates that no such package
+    // The returned status can be NULL, which indicates that no such package
     // is available from the system package manager. Note that empty is also
     // returned if no fully installed package is available from the system and
     // the install argument is false.
     //
     // If fetch is false, then do not re-fetch the system package repository
-    // metadata (that is, available packages/versions) before querying for
-    // the available version of the not yet installed or partially installed
+    // metadata (that is, available packages/versions) before querying for the
+    // available version of the not yet installed or partially installed
     // packages.
     //
-    virtual const vector<unique_ptr<system_package_status>>*
+    virtual optional<const system_package_status*>
     pkg_status (const package_name&,
                 const available_packages*,
                 bool install,
                 bool fetch) = 0;
 
-    // Install the previously-queried package that is not installed or
-    // partially installed.
+    // Install the specified subset of the previously-queried packages.
+    //
+    // Note that this function should be called only once after the final set
+    // of the necessary system packages has been determined. And the specified
+    // subset should contain all the selected packages, including the already
+    // fully installed. This allows the implementation to merge and de-
+    // duplicate the system package set to be installed (since some bpkg
+    // packages may be mapped to the same system package), perform post-
+    // installation verifications (such as making sure the versions of already
+    // installed packages have not changed due to upgrades), implement version
+    // holds, etc.
     //
-    // Return false if the installation was aborted by the user (for example,
-    // the user answered 'N' to the prompt). @@ Do we really need this? We
-    // may not always be able to distinguish.
+    // Note also that the implementation is expected to issue appropriate
+    // progress and diagnostics.
     //
-    virtual bool
-    pkg_install (const package_name&, const version&) = 0;
+    virtual void
+    pkg_install (const vector<package_name>&) = 0;
 
   public:
     virtual
@@ -162,8 +212,6 @@ namespace bpkg
                                 const vector<string>& like_ids);
   protected:
     os_release os_release_;
-    std::map<package_name,
-             vector<unique_ptr<system_package_status>>> status_cache_;
   };
 
   // Create a package manager instance corresponding to the specified host