Deal with order dependence in dist rule

6 files changed, 191 insertions, 60 deletions

diff --git a/libbuild2/dist/init.cxx b/libbuild2/dist/init.cxx
index 26ff86d..9de84ce 100644
--- a/libbuild2/dist/init.cxx
+++ b/libbuild2/dist/init.cxx
@@ -21,8 +21,6 @@ namespace build2
 {
   namespace dist
   {
-    static const rule rule_;
-
     void
     boot (scope& rs, const location&, module_boot_extra& extra)
     {
@@ -196,7 +194,7 @@ namespace build2
           const location& l,
           bool first,
           bool,
-          module_init_extra&)
+          module_init_extra& extra)
     {
       tracer trace ("dist::init");
 
@@ -208,13 +206,19 @@ namespace build2
 
       l5 ([&]{trace << "for " << rs;});
 
+      module& mod (extra.module_as<module> ());
+
       auto& vp (rs.var_pool (true /* public */)); // All qualified.
 
       // Register our wildcard rule. Do it explicitly for the alias to prevent
       // something like insert<target>(dist_id, test_id) taking precedence.
       //
-      rs.insert_rule<target> (dist_id, 0, "dist",       rule_);
-      rs.insert_rule<alias>  (dist_id, 0, "dist.alias", rule_);
+      {
+        const rule& r (mod);
+
+        rs.insert_rule<target> (dist_id, 0, "dist",       r);
+        rs.insert_rule<alias>  (dist_id, 0, "dist.alias", r);
+      }
 
       // We need this rule for out-of-any-project dependencies (for example,
       // executables imported from /usr/bin, etc). We are registering it on
diff --git a/libbuild2/dist/module.hxx b/libbuild2/dist/module.hxx
index 9c682d0..dbe2a3e 100644
--- a/libbuild2/dist/module.hxx
+++ b/libbuild2/dist/module.hxx
@@ -10,14 +10,19 @@
 #include <libbuild2/module.hxx>
 #include <libbuild2/variable.hxx>
 
+#include <libbuild2/dist/types.hxx>
+#include <libbuild2/dist/rule.hxx>
+
 #include <libbuild2/export.hxx>
 
 namespace build2
 {
   namespace dist
   {
-    struct LIBBUILD2_SYMEXPORT module: build2::module
+    class LIBBUILD2_SYMEXPORT module: public build2::module,
+                                      public rule
     {
+    public:
       static const string name;
 
       const variable& var_dist_package;
@@ -38,6 +43,10 @@ namespace build2
         adhoc.push_back (move (f));
       }
 
+      // List of postponed prerequisites.
+      //
+      postponed_prerequisites postponed;
+
       // Distribution post-processing callbacks.
       //
       // Only the last component in the pattern may contain wildcards. If the
@@ -69,8 +78,9 @@ namespace build2
 
       // Implementation details.
       //
+    public:
       module (const variable& v_d_p)
-          : var_dist_package (v_d_p) {}
+        : rule (postponed), var_dist_package (v_d_p) {}
 
     public:
       bool distributed = false; // True if this project is being distributed.
diff --git a/libbuild2/dist/operation.cxx b/libbuild2/dist/operation.cxx
index c7ce7f2..e5c9307 100644
--- a/libbuild2/dist/operation.cxx
+++ b/libbuild2/dist/operation.cxx
@@ -15,6 +15,7 @@
 #include <libbuild2/filesystem.hxx>
 #include <libbuild2/diagnostics.hxx>
 
+#include <libbuild2/dist/types.hxx>
 #include <libbuild2/dist/module.hxx>
 
 using namespace std;
@@ -236,6 +237,11 @@ namespace build2
         fail << "unknown distribution package name" <<
           info << "did you forget to set dist.package?";
 
+      // Need non-const in order to clear postponed. This is safe since only
+      // doing it when running serially.
+      //
+      module& mod (const_cast<module&> (*rs.find_module<module> (module::name)));
+
       const string& dist_package (cast<string> (l));
       const process_path& dist_cmd (cast<process_path> (rs.vars["dist.cmd"]));
 
@@ -272,60 +278,91 @@ namespace build2
         values params;
         path_name pn ("<dist>");
         const location loc (pn); // Dummy location.
+        action_targets ts {tgt};
+
+        auto process_postponed = [&ctx, &mod] ()
         {
-          action_targets ts {tgt};
+          if (!mod.postponed.list.empty ())
+          {
+            // Re-grab the phase lock similar to perform_match().
+            //
+            phase_lock l (ctx, run_phase::match);
+
+            // Note that we don't need to bother with the mutex since we do
+            // all of this serially. But we can end up with new elements at
+            // the end.
+            //
+            // Strictly speaking, to handle this correctly we would need to do
+            // multiple passes over this list and only give up when we cannot
+            // make any progress since earlier entries that we cannot resolve
+            // could be "fixed" by later entries. But this feels far-fetched
+            // and so let's wait for a real example before complicating this.
+            //
+            for (auto i (mod.postponed.list.begin ());
+                 i != mod.postponed.list.end ();
+                 ++i)
+            {
+              rule::match_postponed (i->action, i->target, i->prereq);
+            }
+          }
+        };
 
-          auto mog = make_guard ([&ctx] () {ctx.match_only = false;});
-          ctx.match_only = true;
+        auto mog = make_guard ([&ctx] () {ctx.match_only = false;});
+        ctx.match_only = true;
 
-          const operations& ops (rs.root_extra->operations);
-          for (operations::size_type id (default_id + 1); // Skip default_id.
-               id < ops.size ();
-               ++id)
+        const operations& ops (rs.root_extra->operations);
+        for (operations::size_type id (default_id + 1); // Skip default_id.
+             id < ops.size ();
+             ++id)
+        {
+          if (const operation_info* oif = ops[id])
           {
-            if (const operation_info* oif = ops[id])
-            {
-              // Skip aliases (e.g., update-for-install). In fact, one can
-              // argue the default update should be sufficient since it is
-              // assumed to update all prerequisites and we no longer support
-              // ad hoc stuff like test.input. Though here we are using the
-              // dist meta-operation, not perform.
-              //
-              if (oif->id != id)
-                continue;
+            // Skip aliases (e.g., update-for-install). In fact, one can argue
+            // the default update should be sufficient since it is assumed to
+            // update all prerequisites and we no longer support ad hoc stuff
+            // like test.input. Though here we are using the dist
+            // meta-operation, not perform.
+            //
+            if (oif->id != id)
+              continue;
 
-              // Use standard (perform) match.
-              //
-              if (auto pp = oif->pre_operation)
+            // Use standard (perform) match.
+            //
+            if (auto pp = oif->pre_operation)
+            {
+              if (operation_id pid = pp (ctx, params, dist_id, loc))
               {
-                if (operation_id pid = pp (ctx, params, dist_id, loc))
-                {
-                  const operation_info* poif (ops[pid]);
-                  ctx.current_operation (*poif, oif, false /* diag_noise */);
-                  action a (dist_id, poif->id, oif->id);
-                  perform_match (params, a, ts,
-                                 1     /* diag (failures only) */,
-                                 false /* progress */);
-                }
+                const operation_info* poif (ops[pid]);
+                ctx.current_operation (*poif, oif, false /* diag_noise */);
+                action a (dist_id, poif->id, oif->id);
+                mod.postponed.list.clear ();
+                perform_match (params, a, ts,
+                               1     /* diag (failures only) */,
+                               false /* progress */);
+                process_postponed ();
               }
+            }
 
-              ctx.current_operation (*oif, nullptr, false /* diag_noise */);
-              action a (dist_id, oif->id);
-              perform_match (params, a, ts,
-                             1     /* diag (failures only) */,
-                             false /* progress */);
+            ctx.current_operation (*oif, nullptr, false /* diag_noise */);
+            action a (dist_id, oif->id);
+            mod.postponed.list.clear ();
+            perform_match (params, a, ts,
+                           1     /* diag (failures only) */,
+                           false /* progress */);
+            process_postponed ();
 
-              if (auto po = oif->post_operation)
+            if (auto po = oif->post_operation)
+            {
+              if (operation_id pid = po (ctx, params, dist_id))
               {
-                if (operation_id pid = po (ctx, params, dist_id))
-                {
-                  const operation_info* poif (ops[pid]);
-                  ctx.current_operation (*poif, oif, false /* diag_noise */);
-                  action a (dist_id, poif->id, oif->id);
-                  perform_match (params, a, ts,
-                                 1     /* diag (failures only) */,
-                                 false /* progress */);
-                }
+                const operation_info* poif (ops[pid]);
+                ctx.current_operation (*poif, oif, false /* diag_noise */);
+                action a (dist_id, poif->id, oif->id);
+                mod.postponed.list.clear ();
+                perform_match (params, a, ts,
+                               1     /* diag (failures only) */,
+                               false /* progress */);
+                process_postponed ();
               }
             }
           }
@@ -381,7 +418,7 @@ namespace build2
             dir_path out_nroot (out_root / pd);
             const scope& nrs (ctx.scopes.find_out (out_nroot));
 
-            if (nrs.out_path () != out_nroot) // This subproject not loaded.
+            if (nrs.out_path () != out_nroot) // This subproject is not loaded.
               continue;
 
             if (!nrs.src_path ().sub (src_root)) // Not a strong amalgamation.
@@ -508,8 +545,6 @@ namespace build2
 
       // Copy over all the files. Apply post-processing callbacks.
       //
-      const module& mod (*rs.find_module<module> (module::name));
-
       prog = prog && show_progress (1 /* max_verb */);
       size_t prog_percent (0);
 
diff --git a/libbuild2/dist/rule.cxx b/libbuild2/dist/rule.cxx
index 0c72ff5..7233eba 100644
--- a/libbuild2/dist/rule.cxx
+++ b/libbuild2/dist/rule.cxx
@@ -72,6 +72,8 @@ namespace build2
 
             // Search for an existing target or existing file in src.
             //
+            // Note: see also similar code in match_postponed() below.
+            //
             const prerequisite_key& k (p.key ());
             pt = k.tk.type->search (t, k);
 
@@ -85,12 +87,13 @@ namespace build2
                   !p.dir.sub (out_root))
                 continue;
 
-              // @@ TODO: this can actually be order-dependent: for example
-              //    libs{} prerequisite may be unknown because we haven't
-              //    matched the lib{} group yet.
+              // This can be order-dependent: for example libs{} prerequisite
+              // may be unknown because we haven't matched the lib{} group
+              // yet. So we postpone this for later (see match_postponed()).
               //
-              fail << "prerequisite " << k << " is not existing source file "
-                   << "nor known output target" << endf;
+              mlock l (postponed_.mutex);
+              postponed_.list.push_back (postponed_prerequisite {a, t, p,});
+              continue;
             }
 
             search_custom (p, *pt); // Cache.
@@ -107,5 +110,35 @@ namespace build2
 
       return noop_recipe; // We will never be executed.
     }
+
+    void rule::
+    match_postponed (action a, const target& t, const prerequisite& p)
+    {
+      const prerequisite_key& k (p.key ());
+      const target* pt (k.tk.type->search (t, k));
+
+      if (pt == nullptr)
+      {
+        // Note that we do loose the diag frame that we normally get when
+        // failing during match. So let's mention the target manually.
+        //
+        fail << "prerequisite " << k << " is not existing source file nor "
+             << "known output target" <<
+          info << "while applying rule dist to " << diag_do (a, t);
+      }
+
+      search_custom (p, *pt); // Cache.
+
+      // It's theoretically possible that the target gets entered but nobody
+      // else depends on it but us. So we need to make sure it's matched
+      // (since it, in turns, can pull in other targets). Note that this could
+      // potentially add new postponed prerequisites to the list.
+      //
+      if (!pt->matched (a))
+      {
+        if (pt->dir.sub (t.root_scope ().out_path ()))
+          match_direct_sync (a, *pt);
+      }
+    }
   }
 }
diff --git a/libbuild2/dist/rule.hxx b/libbuild2/dist/rule.hxx
index a864015..ec6c41a 100644
--- a/libbuild2/dist/rule.hxx
+++ b/libbuild2/dist/rule.hxx
@@ -11,6 +11,8 @@
 #include <libbuild2/action.hxx>
 #include <libbuild2/target.hxx>
 
+#include <libbuild2/dist/types.hxx>
+
 namespace build2
 {
   namespace dist
@@ -26,13 +28,20 @@ namespace build2
     class rule: public simple_rule
     {
     public:
-      rule () {}
+      explicit
+      rule (postponed_prerequisites& p): postponed_ (p) {}
 
       virtual bool
       match (action, target&) const override;
 
       virtual recipe
       apply (action, target&) const override;
+
+      static void
+      match_postponed (action, const target&, const prerequisite&);
+
+    private:
+      postponed_prerequisites& postponed_;
     };
   }
 }
diff --git a/libbuild2/dist/types.hxx b/libbuild2/dist/types.hxx
new file mode 100644
index 0000000..f3b1009
--- /dev/null
+++ b/libbuild2/dist/types.hxx
@@ -0,0 +1,40 @@
+// file      : libbuild2/dist/types.hxx -*- C++ -*-
+// license   : MIT; see accompanying LICENSE file
+
+#ifndef LIBBUILD2_DIST_TYPES_HXX
+#define LIBBUILD2_DIST_TYPES_HXX
+
+#include <libbuild2/types.hxx>
+#include <libbuild2/forward.hxx>
+
+#include <libbuild2/prerequisite-key.hxx>
+
+namespace build2
+{
+  namespace dist
+  {
+    // List of prerequisites that could not be searched to a target and were
+    // postponed for later re-search. This can happen, for example, because a
+    // prerequisite would resolve to a member of a group that hasn't been
+    // matched yet (for example, libs{} of lib{}). See rule::apply() for
+    // details.
+    //
+    // Note that we are using list instead of vector because new elements can
+    // be added at the end while we are iterating over the list.
+    //
+    struct postponed_prerequisite
+    {
+      build2::action                          action;
+      reference_wrapper<const build2::target> target;
+      reference_wrapper<const prerequisite>   prereq;
+    };
+
+    struct postponed_prerequisites
+    {
+      build2::mutex                        mutex;
+      build2::list<postponed_prerequisite> list;
+    };
+  }
+}
+
+#endif // LIBBUILD2_DIST_TYPES_HXX