Add support for multiple system package versionssys-install-multiple-versions

8 files changed, 191 insertions, 71 deletions

diff --git a/bpkg/package.cxx b/bpkg/package.cxx
index fe04248..03416be 100644
--- a/bpkg/package.cxx
+++ b/bpkg/package.cxx
@@ -71,10 +71,10 @@ namespace bpkg
 
   // available_package
   //
-  const version* available_package::
-  system_version (database& db) const
+  const system_package_versions* available_package::
+  system_versions (database& db) const
   {
-    if (!system_version_)
+    if (system_versions_.empty ())
     {
       assert (db.system_repository);
 
@@ -83,36 +83,36 @@ namespace bpkg
         // Only cache if it is authoritative.
         //
         if (sp->authoritative)
-          system_version_ = sp->version;
+          system_versions_ = sp->versions;
         else
-          return &sp->version;
+          return &sp->versions;
       }
     }
 
-    return system_version_ ? &*system_version_ : nullptr;
+    return !system_versions_.empty () ? &system_versions_ : nullptr;
   }
 
-  pair<const version*, bool> available_package::
-  system_version_authoritative (database& db) const
+  pair<const system_package_versions*, bool> available_package::
+  system_versions_authoritative (database& db) const
   {
     assert (db.system_repository);
 
     const system_package* sp (db.system_repository->find (id.name));
 
-    if (!system_version_)
+    if (system_versions_.empty ())
     {
       if (sp != nullptr)
       {
         // Only cache if it is authoritative.
         //
         if (sp->authoritative)
-          system_version_ = sp->version;
+          system_versions_ = sp->versions;
         else
-          return make_pair (&sp->version, false);
+          return make_pair (&sp->versions, false);
       }
     }
 
-    return make_pair (system_version_ ?  &*system_version_ : nullptr,
+    return make_pair (!system_versions_.empty () ? &system_versions_ : nullptr,
                       sp != nullptr ? sp->authoritative : false);
   }
 
diff --git a/bpkg/package.hxx b/bpkg/package.hxx
index 6afc624..8754534 100644
--- a/bpkg/package.hxx
+++ b/bpkg/package.hxx
@@ -22,6 +22,7 @@
 #include <bpkg/utility.hxx>
 
 #include <bpkg/diagnostics.hxx>
+#include <bpkg/system-repository.hxx>
 
 // Used by the data migration entries.
 //
@@ -729,7 +730,7 @@ namespace bpkg
 
   private:
     #pragma db transient
-    mutable optional<version_type> system_version_;
+    mutable system_package_versions system_versions_;
 
   public:
     // Note: version constraints must be complete and the bootstrap build must
@@ -766,23 +767,48 @@ namespace bpkg
     //
     available_package (package_name n, version_type sysv)
         : id (move (n), wildcard_version),
-          version (wildcard_version),
-          system_version_ (sysv) {}
+          version (wildcard_version)
+    {
+      system_versions_.push_back (move (sysv));
+    }
 
     bool
     stub () const {return version.compare (wildcard_version, true) == 0;}
 
-    // Return package system version if one has been discovered. Note that
+    // Return package system versions if any has been discovered. Note that
     // we do not implicitly assume a wildcard version.
     //
-    const version_type*
-    system_version (database&) const;
+    const system_package_versions*
+    system_versions (database&) const;
 
     // As above but also return an indication if the version information is
     // authoritative.
     //
+    pair<const system_package_versions*, bool>
+    system_versions_authoritative (database&) const;
+
+#if 1
+    const version_type*
+    system_version (database& db) const
+    {
+      const system_package_versions* vs (system_versions (db));
+      assert (vs == nullptr || vs->size () == 1);
+      return vs != nullptr ? &vs->front () : nullptr;
+    }
+
+
     pair<const version_type*, bool>
-    system_version_authoritative (database&) const;
+    system_version_authoritative (database& db) const
+    {
+      pair<const system_package_versions*, bool> r (
+        system_versions_authoritative (db));
+
+      assert (r.first == nullptr || r.first->size () == 1);
+      return r.first != nullptr
+             ? make_pair (&r.first->front (), r.second)
+             : make_pair (nullptr, r.second);
+    }
+#endif
 
     // Database mapping.
     //
diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx
index 12db3ba..506897f 100644
--- a/bpkg/pkg-build-collect.cxx
+++ b/bpkg/pkg-build-collect.cxx
@@ -100,6 +100,19 @@ namespace bpkg
       (*action == build && (flags & (build_repoint | build_reevaluate)) != 0);
   }
 
+  // @@ MULT-SYS Note that the semantics of this function to return the
+  //    package version which this build should result with (should we rename
+  //    it to just version()?).
+  //
+  //    We should probably make `bool system` member as
+  //    `optional<version> system` and, if present, return it's value here.
+  //
+  //    The creator of build_package object should provide the version for the
+  //    system package build. It can, for example, be the latest version from
+  //    the system repository when this is a build-to-hold object (specified
+  //    on the command line). Or it can be a satisfying version from
+  //    the system repository for a dependency build.
+  //
   const version& build_package::
   available_version () const
   {
@@ -107,7 +120,7 @@ namespace bpkg
     //
     assert (available != nullptr &&
             (system
-             ? available->system_version (db) != nullptr
+             ? available->system_versions (db) != nullptr
              : !available->stub ()));
 
     return system ? *available->system_version (db) : available->version;
@@ -1809,7 +1822,12 @@ namespace bpkg
         shared_ptr<selected_package>               selected;
         shared_ptr<available_package>              available;
         lazy_shared_ptr<bpkg::repository_fragment> repository_fragment;
+
+        // @@ MULT-SYS Make it of optional<version> type as
+        //    build_package::system (see above for details).
+        //
         bool                                       system;
+
         bool                                       specified_dependency;
         bool                                       force;
 
@@ -2115,6 +2133,11 @@ namespace bpkg
                 // A stub satisfies any version constraint so we weed them out
                 // (returning stub as an available package feels wrong).
                 //
+                // For a system selected package we still create a stub, but
+                // with a fixed system version (see
+                // available_package(package_name, version_type) constructor
+                // for details).
+                //
                 if (dap == nullptr || dap->stub ())
                   rp = make_available_fragment (options, *ddb, dsp);
               }
@@ -2457,7 +2480,7 @@ namespace bpkg
                 // version constraint). If it were, then the system version
                 // wouldn't be NULL and would satisfy itself.
                 //
-                if (dap->system_version (*ddb) == nullptr)
+                if (dap->system_versions (*ddb) == nullptr)
                 {
                   if (dr != nullptr)
                     *dr << error << "dependency " << d << " of package "
@@ -2468,6 +2491,10 @@ namespace bpkg
                   return precollect_result (false /* postpone */);
                 }
 
+                // @@ MULT-SYS Go through all sys versions and only if none
+                //    satisfying found, only then fail. Otherwise save the
+                //    satisfying version to the `system` variable.
+                //
                 if (!satisfies (*dap->system_version (*ddb), d.constraint))
                 {
                   if (dr != nullptr)
@@ -2485,6 +2512,9 @@ namespace bpkg
               }
               else
               {
+                // @@ MULT-SYS Go through all sys versions and if there is a
+                //    satisfying one, the save it into the `system` variable.
+                //
                 auto p (dap->system_version_authoritative (*ddb));
 
                 if (p.first != nullptr &&
@@ -2500,14 +2530,14 @@ namespace bpkg
               reused = false;
 
             r.push_back (prebuild {d,
-                  *ddb,
-                  move (dsp),
-                  move (dap),
-                  move (rp.second),
-                  system,
-                  specified,
-                  force,
-                  ru});
+                                   *ddb,
+                                   move (dsp),
+                                   move (dap),
+                                   move (rp.second),
+                                   system,
+                                   specified,
+                                   force,
+                                   ru});
           }
 
           // Now, as we have pre-collected the dependency builds, go through
@@ -2537,6 +2567,10 @@ namespace bpkg
 
               if (bp.action && *bp.action == build_package::build)
               {
+                // @@ MULT-SYS:
+                //
+                // const version& v1 (b.system ? *b.system : dap->version);
+                //
                 const version& v1 (b.system
                                    ? *dap->system_version (b.db)
                                    : dap->version);
@@ -3985,7 +4019,7 @@ namespace bpkg
     package_key pk (db, sp->name);
 
     // If there is an entry for building specific version of the package (the
-    // available member is not NULL), then it wasn't created to prevent out
+    // available member is not NULL), then it wasn't created to prevent our
     // drop (see replaced_versions for details). This rather mean that the
     // replacement version is not being built anymore due to the plan
     // refinement. Thus, just erase the entry in this case and continue.
@@ -4000,6 +4034,11 @@ namespace bpkg
       {
         if (verb >= 5)
         {
+          // @@ MULT-SYS:
+          //
+          // const optional<version>& s (v.system);
+          // const version& av (s ? *s : ap->version);
+          //
           bool s (v.system);
           const version& av (s ? *ap->system_version (db) : ap->version);
 
diff --git a/bpkg/pkg-build-collect.hxx b/bpkg/pkg-build-collect.hxx
index e47d9fa..ff7735c 100644
--- a/bpkg/pkg-build-collect.hxx
+++ b/bpkg/pkg-build-collect.hxx
@@ -673,6 +673,10 @@ namespace bpkg
     //
     shared_ptr<available_package> available;
     lazy_shared_ptr<bpkg::repository_fragment> repository_fragment;
+
+    // @@ MULT-SYS Make it of optional<version> type as build_package::system
+    //    (see available_version() implementation for details).
+    //
     bool system; // Meaningless for the drop.
 
     // True if the entry has been inserted or used for the replacement during
diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index f452d50..32a6fee 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -520,10 +520,8 @@ namespace bpkg
     //
     optional<version_constraint> c;
 
-    if (!dvc)
+    if (!dsys && !dvc) // @@ For now we don't patch system packages.
     {
-      assert (!dsys); // The version can't be empty for the system package.
-
       if (patch)
       {
         c = patch_constraint (sp, ignore_unsatisfiable);
@@ -535,7 +533,7 @@ namespace bpkg
         }
       }
     }
-    else if (!dsys || !wildcard (*dvc))
+    else if (!dsys || (dvc && !wildcard (*dvc)))
       c = dvc;
 
     available_packages afs (find_available (nm, c, rfs));
@@ -568,10 +566,8 @@ namespace bpkg
       //
       // Note that we also handle a package stub here.
       //
-      if (!dvc && dsp != nullptr && av < dsp->version)
+      if (!dsys && !dvc && dsp != nullptr && av < dsp->version)
       {
-        assert (!dsys); // Version can't be empty for the system package.
-
         // For the selected system package we still need to pick a source
         // package version to downgrade to.
         //
@@ -1533,7 +1529,14 @@ namespace bpkg
 
       package_scheme               scheme;
       package_name                 name;
+
+      // Note that currently a system package without constraint is always
+      // configured not to hold the version and can implicitly be
+      // up/downgraded. But in the future we can add support for holding the
+      // resolved version, for example, with the sys:libfoo/? syntax.
+      //
       optional<version_constraint> constraint;
+
       string                       value;
       pkg_options                  options;
       strings                      config_vars;
@@ -1652,19 +1655,39 @@ namespace bpkg
       return r;
     };
 
-    // Figure out the system package version unless explicitly specified and
-    // add the system package authoritative information to the database's
+    // Figure out the system package version(s) unless explicitly specified
+    // and add the system package authoritative information to the database's
     // system repository unless the database is NULL or it already contains
-    // authoritative information for this package. Return the figured out
-    // system package version as constraint.
+    // authoritative information for this package.
+    //
+    // Specifically, if the system package version is not specified its
+    // deduction is performed as follows:
+    //
+    // - If no system manager is available or --sys-no-query option is
+    //   specified, then the version is the wildcard version.
+    //
+    // - Otherwise, the system manager is queried for available versions and
+    //   the resulting versions are mapped to the downstream package
+    //   versions. The system versions which cannot be mapped are not
+    //   considered.
+    //
+    // - Fail if no version were deduced.
     //
     // Note that it is assumed that all the possible duplicates are handled
     // elsewhere/later.
     //
+    // @@ Since the only outcome of the function is adding the system package
+    //    versions to the system repository, calling with the NULL database
+    //    doesn't make much sense. This, however, can be plausable if we
+    //    implement more accurate diagnostics (see below). So let's keep
+    //    `database*` in favor of `database&` for now.
+    //
     auto add_system_package = [] (database* db,
                                   const package_name& nm,
-                                  optional<version_constraint> vc)
+                                  const optional<version_constraint>& vc)
     {
+      system_package_versions vs;
+
       if (!vc)
       {
         // @@ Where do we check that this package should have available
@@ -1713,15 +1736,23 @@ namespace bpkg
           //@@ TODO: if we extracted a version, then we need to add an entry
           //   to the imaginary stubs (probably checking for duplicated), just
           //   as if the user specified it explicitly.
+
+          if (db != nullptr)
+            vs.push_back (wildcard_version); // @@ TMP
         }
-        else
-          vc = version_constraint (wildcard_version);
+        else if (db != nullptr)
+          vs.push_back (wildcard_version);
       }
       else
+      {
         // The system package may only have an exact/wildcard version
         // specified.
         //
-        assert (vc->min_version == vc->max_version);
+        assert (vc->min_version && vc->min_version == vc->max_version);
+
+        if (db != nullptr)
+          vs.push_back (*vc->min_version);
+      }
 
       if (db != nullptr)
       {
@@ -1730,9 +1761,7 @@ namespace bpkg
         const system_package* sp (db->system_repository->find (nm));
 
         if (sp == nullptr || !sp->authoritative)
-          db->system_repository->insert (nm,
-                                         *vc->min_version,
-                                         true /* authoritative */);
+          db->system_repository->insert (nm, vs, true /* authoritative */);
 
         // @@ If it is authoritative, wouldn't it be a good idea to check that
         //    the versions match?
@@ -1766,8 +1795,6 @@ namespace bpkg
         //       source of authoritative information is system package manager
         //       and it caches its results.
       }
-
-      return vc;
     };
 
     // Create the parsed package argument. Issue diagnostics and fail if the
@@ -1806,7 +1833,7 @@ namespace bpkg
       {
       case package_scheme::sys:
         {
-          r.constraint = add_system_package (db, r.name, move (r.constraint));
+          add_system_package (db, r.name, r.constraint);
           break;
         }
       case package_scheme::none: break; // Nothing to do.
@@ -2221,7 +2248,7 @@ namespace bpkg
              !compare_options (a.options, pa.options) ||
              a.config_vars != pa.config_vars))
           fail << "duplicate package " << pa.name <<
-            info << "first mentioned as " << arg_string (r.first->second) <<
+            info << "first mentioned as " << arg_string (a) <<
             info << "second mentioned as " << arg_string (pa);
 
         return !r.second;
@@ -2464,10 +2491,8 @@ namespace bpkg
 
               bool sys (arg_sys (pa));
 
-              if (!pa.constraint)
+              if (!sys && !pa.constraint)
               {
-                assert (!sys);
-
                 if (pa.options.patch () &&
                     (sp = pdb->find<selected_package> (pa.name)) != nullptr)
                 {
@@ -2485,7 +2510,7 @@ namespace bpkg
                   patch = true;
                 }
               }
-              else if (!sys || !wildcard (*pa.constraint))
+              else if (!sys || (pa.constraint && !wildcard (*pa.constraint)))
                 c = pa.constraint;
 
               auto rp (find_available_one (pa.name, c, root));
@@ -2652,7 +2677,7 @@ namespace bpkg
           //
           if (pa.constraint)
           {
-            for (;;)
+            for (;;) // Breakout loop.
             {
               if (ap != nullptr) // Must be that version, see above.
                 break;
diff --git a/bpkg/system-repository.cxx b/bpkg/system-repository.cxx
index d7a47b7..229ebd3 100644
--- a/bpkg/system-repository.cxx
+++ b/bpkg/system-repository.cxx
@@ -5,10 +5,14 @@
 
 namespace bpkg
 {
-  const version& system_repository::
-  insert (const package_name& name, const version& v, bool authoritative)
+  const system_package_versions& system_repository::
+  insert (const package_name& name,
+          const system_package_versions& vs,
+          bool authoritative)
   {
-    auto p (map_.emplace (name, system_package {v, authoritative}));
+    assert (!vs.empty ());
+
+    auto p (map_.emplace (name, system_package {vs, authoritative}));
 
     if (!p.second)
     {
@@ -21,10 +25,10 @@ namespace bpkg
       if (authoritative >= sp.authoritative)
       {
         sp.authoritative = authoritative;
-        sp.version = v;
+        sp.versions = vs;
       }
     }
 
-    return p.first->second.version;
+    return p.first->second.versions;
   }
 }
diff --git a/bpkg/system-repository.hxx b/bpkg/system-repository.hxx
index f33d622..d070c49 100644
--- a/bpkg/system-repository.hxx
+++ b/bpkg/system-repository.hxx
@@ -20,23 +20,38 @@ namespace bpkg
   // that are present in the database; in a sence it was authoritative but
   // on some previous run.
   //
-  // Note that in our model we assume that once an authoritative version has
-  // been discovered, it does not change (on this run; see caching logic in
+  // Note that in our model we assume that once an authoritative versions have
+  // been discovered, they does not change (on this run; see caching logic in
   // available package).
   //
+  using system_package_versions = small_vector<version, 2>;
+
   struct system_package
   {
-    using version_type = bpkg::version;
-
-    version_type version;
+    system_package_versions versions;
     bool authoritative;
   };
 
   class system_repository
   {
   public:
-    const version&
-    insert (const package_name& name, const version&, bool authoritative);
+    // @@ Add move-insertion overloads (system_package_versions&& and
+    //    version&&)?
+    //
+
+    // Note: the system package versions are assumed to be provided in the
+    // preference descending order.
+    //
+    const system_package_versions&
+    insert (const package_name& name,
+            const system_package_versions&,
+            bool authoritative);
+
+    const system_package_versions&
+    insert (const package_name& n, const version& v, bool a)
+    {
+      return insert (n, system_package_versions ({v}), a);
+    }
 
     const system_package*
     find (const package_name& name)
diff --git a/tests/pkg-system.testscript b/tests/pkg-system.testscript
index fc4f707..946a459 100644
--- a/tests/pkg-system.testscript
+++ b/tests/pkg-system.testscript
@@ -766,7 +766,7 @@ rep_remove += -d cfg 2>!
       EOE
 
     $pkg_status foo    >'!foo configured 2';
-    $pkg_status libbar >'libbar configured,system !*';
+    $pkg_status libbar >'libbar configured,system *';
 
     # Drop foo.
     #
@@ -938,7 +938,14 @@ rep_remove += -d cfg 2>!
       EOE
 
     $pkg_status foo    >'!foo configured 2';
-    $pkg_status libbar >'libbar configured,system !*';
+    $pkg_status libbar >'libbar configured,system *';
+
+    # Build ?sys:libbar/*.
+    #
+    # @@ Doesn't seems to reconfigure with the hold version flag. Need to fix.
+    #
+    #$pkg_build '?sys:libbar/*';
+    #$pkg_status libbar >'libbar configured,system !*';
 
     $pkg_drop foo
   }