Fix 'unordered build' assertion failure due to bug in build_packages::order()

6 files changed, 139 insertions, 103 deletions

diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx
index 67357f8..16d0b0b 100644
--- a/bpkg/pkg-build-collect.cxx
+++ b/bpkg/pkg-build-collect.cxx
@@ -6780,12 +6780,11 @@ namespace bpkg
   build_packages::iterator build_packages::
   order (database& db,
          const package_name& name,
-         optional<bool> buildtime,
          const function<find_database_function>& fdb,
          bool reorder)
   {
     package_refs chain;
-    return order (db, name, buildtime, chain, fdb, reorder);
+    return order (db, name, chain, fdb, reorder);
   }
 
   void build_packages::
@@ -7581,23 +7580,11 @@ namespace bpkg
   build_packages::iterator build_packages::
   order (database& db,
          const package_name& name,
-         optional<bool> buildtime,
          package_refs& chain,
          const function<find_database_function>& fdb,
          bool reorder)
   {
-    package_map::iterator mi;
-
-    if (buildtime)
-    {
-      database* ddb (fdb (db, name, *buildtime));
-
-      mi = ddb != nullptr
-        ? map_.find (*ddb, name)
-        : map_.find_dependency (db, name, *buildtime);
-    }
-    else
-      mi = map_.find (db, name);
+    package_map::iterator mi (map_.find (db, name));
 
     // Every package that we order should have already been collected.
     //
@@ -7607,18 +7594,16 @@ namespace bpkg
 
     assert (p.action); // Can't order just a pre-entered package.
 
-    database& pdb (p.db);
-
     // Make sure there is no dependency cycle.
     //
-    package_ref cp {pdb, name};
+    package_ref cp {db, name};
     {
       auto i (find (chain.begin (), chain.end (), cp));
 
       if (i != chain.end ())
       {
         diag_record dr (fail);
-        dr << "dependency cycle detected involving package " << name << pdb;
+        dr << "dependency cycle detected involving package " << name << db;
 
         auto nv = [this] (const package_ref& cp)
         {
@@ -7687,20 +7672,21 @@ namespace bpkg
           i = j;
     };
 
-    // Similar to collect_build(), we can prune if the package is already
-    // configured, right? While in collect_build() we didn't need to add
-    // prerequisites of such a package, it doesn't mean that they actually
-    // never ended up in the map via another dependency path. For example,
-    // some can be a part of the initial selection. And in that case we must
-    // order things properly.
+    // Similar to collect_build_prerequisites(), we can prune if the package
+    // is already configured, right? While in collect_build_prerequisites() we
+    // didn't need to add prerequisites of such a package, it doesn't mean
+    // that they actually never ended up in the map via another dependency
+    // path. For example, some can be a part of the initial selection. And in
+    // that case we must order things properly.
     //
     // Also, if the package we are ordering is not a system one and needs to
     // be disfigured during the plan execution, then we must order its
     // (current) dependencies that also need to be disfigured.
     //
     // And yet, if the package we are ordering is a repointed dependent, then
-    // we must order not only its unamended and new prerequisites but also its
-    // replaced prerequisites, which can also be disfigured.
+    // we must order not only its unamended and new prerequisites
+    // (prerequisite replacements) but also its replaced prerequisites, which
+    // can also be disfigured.
     //
     bool src_conf (sp != nullptr &&
                    sp->state == package_state::configured &&
@@ -7720,34 +7706,38 @@ namespace bpkg
     if (build && !p.system)
     {
       // So here we are going to do things differently depending on whether
-      // the package is already configured or not. If it is and not as a
-      // system package, then that means we can use its prerequisites
-      // list. Otherwise, we use the manifest data.
+      // the package prerequisites builds are collected or not. If they are
+      // not, then the package is being reconfigured and we use its configured
+      // prerequisites list. Otherwise, we use its collected prerequisites
+      // builds.
       //
-      if (src_conf                              &&
-          sp->version == p.available_version () &&
-          (p.config_vars.empty () ||
-           !has_buildfile_clause (ap->dependencies)))
+      if (!p.dependencies)
       {
+        assert (src_conf); // Shouldn't be here otherwise.
+
+        // A repointed dependent have always its prerequisite replacements
+        // collected, so p.dependencies must always be present for them.
+        //
+        assert ((p.flags & build_package::build_repoint) == 0);
+
         for (const auto& p: sp->prerequisites)
         {
           database& db (p.first.database ());
           const package_name& name (p.first.object_id ());
 
-          // The prerequisites may not necessarily be in the map.
-          //
-          // Note that for the repointed dependent we also order its new and
-          // replaced prerequisites here, since they all are in the selected
-          // package prerequisites set.
+          // The prerequisites may not necessarily be in the map or have an
+          // action be present, but they can never be dropped.
           //
           auto i (map_.find (db, name));
-          if (i != map_.end () && i->second.package.action)
-            update (order (db,
-                           name,
-                           nullopt /* buildtime */,
-                           chain,
-                           fdb,
-                           false   /* reorder */));
+          if (i != map_.end ())
+          {
+            optional<build_package::action_type> a (i->second.package.action);
+
+            assert (!a || *a != build_package::drop); // See above.
+
+            if (a)
+              update (order (db, name, chain, fdb, false /* reorder */));
+          }
         }
 
         // We just ordered them among other prerequisites.
@@ -7756,11 +7746,10 @@ namespace bpkg
       }
       else
       {
-        // The package prerequisites builds must already be collected and
-        // thus the resulting dependency list is complete.
+        // If the package prerequisites builds are collected, then the
+        // resulting dependency list must be complete.
         //
-        assert (p.dependencies &&
-                p.dependencies->size () == ap->dependencies.size ());
+        assert (p.dependencies->size () == ap->dependencies.size ());
 
         // We are iterating in reverse so that when we iterate over the
         // dependency list (also in reverse), prerequisites will be built in
@@ -7779,18 +7768,44 @@ namespace bpkg
 
           assert (das.size () == 1);
 
+          bool buildtime (das.buildtime);
+
           for (const dependency& d: das.front ())
           {
+            const package_name& n (d.name);
+
+            // Use the custom search function to find the dependency's build
+            // configuration. Failed that, search for it recursively.
+            //
+            database* ddb (fdb (db, n, buildtime));
+
+            auto i (ddb != nullptr
+                    ? map_.find (*ddb, n)
+                    : map_.find_dependency (db, n, buildtime));
+
             // Note that for the repointed dependent we only order its new and
-            // unamended prerequisites here. Its replaced prerequisites will
-            // be ordered below.
+            // potentially unamended prerequisites here (see
+            // collect_build_prerequisites() for details). Thus its
+            // (unamended) prerequisites may not necessarily be in the map or
+            // have an action be present, but they can never be dropped. Its
+            // replaced prerequisites will be ordered below.
             //
-            update (order (pdb,
-                           d.name,
-                           das.buildtime,
-                           chain,
-                           fdb,
-                           false /* reorder */));
+            if (i != map_.end ())
+            {
+              optional<build_package::action_type> a (
+                i->second.package.action);
+
+              assert (!a || *a != build_package::drop); // See above.
+
+              if (a)
+              {
+                update (order (i->first.db,
+                               n,
+                               chain,
+                               fdb,
+                               false /* reorder */));
+              }
+            }
           }
         }
       }
@@ -7815,12 +7830,7 @@ namespace bpkg
         // since we do not reorder.
         //
         if (i != map_.end () && disfigure (i->second.package))
-          update (order (db,
-                         name,
-                         nullopt /* buildtime */,
-                         chain,
-                         fdb,
-                         false   /* reorder */));
+          update (order (db, name, chain, fdb, false /* reorder */));
       }
     }
 
diff --git a/bpkg/pkg-build-collect.hxx b/bpkg/pkg-build-collect.hxx
index 181a86e..5362c65 100644
--- a/bpkg/pkg-build-collect.hxx
+++ b/bpkg/pkg-build-collect.hxx
@@ -1491,23 +1491,16 @@ namespace bpkg
                              const function<add_priv_cfg_function>&,
                              postponed_configuration* = nullptr);
 
-    // Order the previously-collected package with the specified name
-    // returning its positions.
+    // Order the previously-collected package with the specified name and
+    // configuration returning its position.
     //
-    // If buildtime is nullopt, then search for the specified package build in
-    // only the specified configuration. Otherwise, treat the package as a
-    // dependency and use the custom search function to find its build
-    // configuration. Failed that, search for it recursively (see
-    // package_map::find_dependency() for details).
-    //
-    // Recursively order the package dependencies being ordered failing if a
+    // Recursively order the collected package dependencies, failing if a
     // dependency cycle is detected. If reorder is true, then reorder this
     // package to be considered as "early" as possible.
     //
     iterator
     order (database&,
            const package_name&,
-           optional<bool> buildtime,
            const function<find_database_function>&,
            bool reorder = true);
 
@@ -1674,7 +1667,6 @@ namespace bpkg
     iterator
     order (database&,
            const package_name&,
-           optional<bool> buildtime,
            package_refs& chain,
            const function<find_database_function>&,
            bool reorder);
diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index 9cf466c..1aaed99 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -4551,24 +4551,16 @@ namespace bpkg
         // appear (e.g., on the plan) last.
         //
         for (const dep& d: deps)
-          pkgs.order (d.db,
-                      d.name,
-                      nullopt               /* buildtime */,
-                      find_prereq_database,
-                      false                 /* reorder */);
+          pkgs.order (d.db, d.name, find_prereq_database, false /* reorder */);
 
         for (const build_package& p: reverse_iterate (hold_pkgs))
-          pkgs.order (p.db,
-                      p.name (),
-                      nullopt /* buildtime */,
-                      find_prereq_database);
+          pkgs.order (p.db, p.name (), find_prereq_database);
 
         for (const auto& rd: rpt_depts)
           pkgs.order (rd.first.db,
                       rd.first.name,
-                      nullopt               /* buildtime */,
                       find_prereq_database,
-                      false                 /* reorder */);
+                      false /* reorder */);
 
         for (const postponed_configuration& cfg: postponed_cfgs)
         {
@@ -4577,11 +4569,7 @@ namespace bpkg
             if (d.second.existing)
             {
               const package_key& p (d.first);
-
-              pkgs.order (p.db,
-                          p.name,
-                          nullopt /* buildtime */,
-                          find_prereq_database);
+              pkgs.order (p.db, p.name, find_prereq_database);
             }
           }
         }
@@ -4590,10 +4578,7 @@ namespace bpkg
         {
           assert (p->recursive_collection);
 
-          pkgs.order (p->db,
-                      p->name (),
-                      nullopt /* buildtime */,
-                      find_prereq_database);
+          pkgs.order (p->db, p->name (), find_prereq_database);
         }
 
         // Collect and order all the dependents that we will need to
@@ -4617,9 +4602,8 @@ namespace bpkg
             if (sp != nullptr && sp->hold_package)
               pkgs.order (db,
                           p.name,
-                          nullopt               /* buildtime */,
                           find_prereq_database,
-                          false                 /* reorder */);
+                          false /* reorder */);
           };
 
           if (p.db != nullptr)
diff --git a/tests/common/dependency-alternatives/t8a/dax-1.0.0.tar.gz b/tests/common/dependency-alternatives/t8a/dax-1.0.0.tar.gz
new file mode 100644
index 0000000..34c3aae
--- /dev/null
+++ b/tests/common/dependency-alternatives/t8a/dax-1.0.0.tar.gz
diff --git a/tests/common/dependency-alternatives/t8a/dix-1.0.0.tar.gz b/tests/common/dependency-alternatives/t8a/dix-1.0.0.tar.gz
new file mode 100644
index 0000000..764e530
--- /dev/null
+++ b/tests/common/dependency-alternatives/t8a/dix-1.0.0.tar.gz
diff --git a/tests/pkg-build.testscript b/tests/pkg-build.testscript
index 010f703..006dde3 100644
--- a/tests/pkg-build.testscript
+++ b/tests/pkg-build.testscript
@@ -158,6 +158,8 @@
 # |   |                            libbox >= 0.1.1 config.box.backend=libbox,
 # |   |                            libbaz
 # |   |-- bux-1.0.0.tar.gz      -> bix
+# |   |-- dax-1.0.0.tar.gz      -> libbar ? ($config.dax.extras)
+# |   |-- dix-1.0.0.tar.gz      -> dax require {config.dax.extras=true}
 # |   |-- fax-1.0.0.tar.gz      -> libbar ^1.0.0 ? ($cxx.target.class == 'windows') config.fax.backend=libbar |
 # |   |                            libbaz ^1.0.0 ? ($cxx.target.class != 'windows') config.fax.backend=libbaz,
 # |   |                            libbiz ? ($config.fax.libbiz) config.fax.extras='[b\i$z]',
@@ -4648,6 +4650,54 @@ test.arguments += --sys-no-query
         $pkg_drop fax
       }
 
+      : enable-indirect-dependency
+      :
+      {
+        $clone_cfg;
+
+        test.arguments += --plan "";
+
+        $* dax 2>>~%EOE%;
+            new dax/1.0.0
+          fetched dax/1.0.0
+          unpacked dax/1.0.0
+          configured dax/1.0.0
+          %info: .+dax-1.0.0.+ is up to date%
+          updated dax/1.0.0
+          EOE
+
+        $pkg_status -r >>EOO;
+          !dax configured 1.0.0
+          EOO
+
+        $* dix 2>>~%EOE%;
+            new libbar/1.0.0 (required by dax)
+            reconfigure/update dax/1.0.0 (required by dix)
+              config.dax.extras=true (set by dix)
+            new dix/1.0.0
+          disfigured dax/1.0.0
+          fetched libbar/1.0.0
+          unpacked libbar/1.0.0
+          fetched dix/1.0.0
+          unpacked dix/1.0.0
+          configured libbar/1.0.0
+          configured dax/1.0.0
+          configured dix/1.0.0
+          %info: .+dix-1.0.0.+ is up to date%
+          updated dix/1.0.0
+          EOE
+
+        $pkg_status -r >>EOO;
+          !dax configured 1.0.0
+            libbar configured 1.0.0
+          !dix configured 1.0.0
+            !dax configured 1.0.0
+              libbar configured 1.0.0
+          EOO
+
+        $pkg_drop dax dix
+      }
+
       : reevaluate-alternatives
       :
       {
@@ -17568,9 +17618,9 @@ test.arguments += --sys-no-query
         build plan:
           upgrade libbar/1.0.0
             config.libbar.extras=true (set by bax)
+          upgrade libbiz/1.0.0
           reconfigure/update bax/1.0.0
             config.bax.libfoo_extras=true (set by bax)
-          upgrade libbiz/1.0.0
         trace: execute_plan: simulate: no
         %.*
         EOE
@@ -24494,14 +24544,14 @@ else
         %  upgrade libbaz/1.1.0 \[h1.\] \(required by foo \[h1.\]\)%
         %  drop libbuild2-bar/1.0.0 \[h1..bpkg.build2.\] \(unused\)%
         %  upgrade foo/1.1.0 \[h1.\]%
-        %  reconfigure libbox \(dependent of foo \[h1.\]\)%
         %  reconfigure libbar \(dependent of foo \[h1.\]\)%
+        %  reconfigure libbox \(dependent of foo \[h1.\]\)%
         %  drop libfax/1.0.0 \(unused\)%
           reconfigure/update libfix/1.0.0
         continue? [Y/n] update dependent packages? [Y/n] disfigured libfix/1.0.0
         disfigured libfax/1.0.0
-        disfigured libbar/1.0.0
         disfigured libbox/1.0.0
+        disfigured libbar/1.0.0
         %disfigured foo/1.0.0 \[h1.\]%
         %disfigured libbuild2-bar/1.0.0 \[h1..bpkg.build2.\]%
         %disfigured libbaz/1.0.0 \[h1.\]%
@@ -24516,19 +24566,19 @@ else
         %configured libfax/1.0.0 \[t2.\]%
         %configured libbaz/1.1.0 \[h1.\]%
         %configured foo/1.1.0 \[h1.\]%
-        configured libbox/1.0.0
         configured libbar/1.0.0
+        configured libbox/1.0.0
         configured libfix/1.0.0
         %info: t2.+libfax-1.0.0.+ is up to date%
         %info: h1.+foo-1.1.0.+ is up to date%
         %info: t1.+libfix-1.0.0.+ is up to date%
-        %info: t1.+libbox-1.0.0.+ is up to date%
         %info: t1.+libbar-1.0.0.+ is up to date%
+        %info: t1.+libbox-1.0.0.+ is up to date%
         %updated libfax/1.0.0 \[t2.\]%
         %updated foo/1.1.0 \[h1.\]%
         updated libfix/1.0.0
-        updated libbox/1.0.0
         updated libbar/1.0.0
+        updated libbox/1.0.0
         EOE
 
       $pkg_status -d t1 --link -r >>/EOO