Change inner rule/prerequisites match order in install::file_rule

The old order messed up the for-install signaling logic. See the long comment
in install::file_rule::apply_impl() for background and details.

7 files changed, 82 insertions, 4 deletions

diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx
index 4db3d72..ad4c406 100644
--- a/libbuild2/algorithm.cxx
+++ b/libbuild2/algorithm.cxx
@@ -1265,6 +1265,20 @@ namespace build2
     return ct.try_matched_state (a, false);
   }
 
+  void
+  match_only_sync (action a, const target& t)
+  {
+    assert (t.ctx.phase == run_phase::match);
+
+    target_lock l (lock_impl (a, t, scheduler::work_none));
+
+    if (l.target != nullptr && l.offset < target::offset_matched)
+    {
+      if (match_impl (l, true /* step */).second == target_state::failed)
+        throw failed ();
+    }
+  }
+
   // Note: lock is a reference to avoid the stacking overhead.
   //
   static group_view
diff --git a/libbuild2/algorithm.hxx b/libbuild2/algorithm.hxx
index 8bdf737..7cc42c2 100644
--- a/libbuild2/algorithm.hxx
+++ b/libbuild2/algorithm.hxx
@@ -376,6 +376,13 @@ namespace build2
   pair<bool, target_state>
   match_sync (action, const target&, unmatch);
 
+  // As above but only match the target (unless already matched) without
+  // applying the match (which is normally done with match_sync()). You will
+  // most likely regret using this function.
+  //
+  LIBBUILD2_SYMEXPORT void
+  match_only_sync (action, const target&);
+
   // As above but without incrementing the target's dependents count. Should
   // be executed with execute_direct_*().
   //
diff --git a/libbuild2/bash/rule.cxx b/libbuild2/bash/rule.cxx
index 29c6a2a..502a206 100644
--- a/libbuild2/bash/rule.cxx
+++ b/libbuild2/bash/rule.cxx
@@ -63,10 +63,12 @@ namespace build2
     // in_rule
     //
     bool in_rule::
-    match (action a, target& t, const string& hint, match_extra&) const
+    match (action a, target& xt, const string& hint, match_extra&) const
     {
       tracer trace ("bash::in_rule::match");
 
+      file& t (xt.as<file> ()); // Only registered for exe{} and bash{}.
+
       // Note that for bash{} and for exe{} with hint we match even if the
       // target does not depend on any modules (while it could have been
       // handled by the in module, that would require loading it).
@@ -89,6 +91,12 @@ namespace build2
         l4 ([&]{trace << "no bash module prerequisite or hint for target "
                       << t;});
 
+      // If we match, derive the file name early as recommended by the in
+      // rule.
+      //
+      if (fi && fm)
+        t.derive_path ();
+
       return fi && fm;
     }
 
diff --git a/libbuild2/in/rule.cxx b/libbuild2/in/rule.cxx
index 74bc2a7..31a9d94 100644
--- a/libbuild2/in/rule.cxx
+++ b/libbuild2/in/rule.cxx
@@ -47,6 +47,13 @@ namespace build2
       if (!fi)
         l5 ([&]{trace << "no in file prerequisite for target " << t;});
 
+      // If we match, derive the file name here instead of in apply() to make
+      // it available early for the in{} prerequisite search (see
+      // install::file_rule::apply_impl() for background).
+      //
+      if (fi)
+        t.derive_path ();
+
       return fi;
     }
 
@@ -55,9 +62,9 @@ namespace build2
     {
       file& t (xt.as<file> ());
 
-      // Derive the file name.
+      // Make sure derived rules assign the path in match().
       //
-      t.derive_path ();
+      assert (!t.path ().empty ());
 
       // Inject dependency on the output directory.
       //
diff --git a/libbuild2/in/rule.hxx b/libbuild2/in/rule.hxx
index 369fd93..67c2509 100644
--- a/libbuild2/in/rule.hxx
+++ b/libbuild2/in/rule.hxx
@@ -22,6 +22,11 @@ namespace build2
     // cache data (e.g., in match() or apply()) to be used in substitute() and
     // lookup() calls.
     //
+    // A derived rule is also required to derive the target file name in
+    // match() instead of apply() to make it available early for the in{}
+    // prerequisite search (see install::file_rule::apply_impl() for
+    // background).
+    //
     // Note also that currently this rule ignores the dry-run mode (see
     // perform_update() for the rationale).
     //
diff --git a/libbuild2/install/rule.cxx b/libbuild2/install/rule.cxx
index 20a4bc3..4dd11e8 100644
--- a/libbuild2/install/rule.cxx
+++ b/libbuild2/install/rule.cxx
@@ -378,11 +378,36 @@ namespace build2
       // (actual update). We used to do this after matching the prerequisites
       // but the inner rule may provide some rule-specific information (like
       // the target extension for exe{}) that may be required during the
-      // prerequisite search (like the base name for in{}).
+      // prerequisite search (like the base name for in{}; this no longer
+      // reproduces likely due to the changes to exe{} extension derivation
+      // but a contrived arrangement can still be made to trigger this).
       //
+      // But then we discovered that doing this before the prerequisites messes
+      // up with the for-install signaling. Specifically, matching the
+      // prerequisites may signal that they are being updated for install,
+      // for example, for a library via a metadata library used in a moc
+      // recipe. While matching the inner rule may trigger updating during
+      // match of such prerequisites, for example, a source file generated by
+      // that moc recipe that depends on this metadata library. If we match
+      // prerequisites before, then the library that is pulled by the metadata
+      // library will be updated before we had a chance to signal that it
+      // should be updated for install.
+      //
+      // To try to accommodate both cases (as best as we can) we now split the
+      // inner rule match into two steps: we do the match before and apply
+      // after. This allows rules that deal with tricky prerequisites like
+      // in{} to assign the target path in match() instead of apply() (see
+      // in::rule, for example).
+      //
+#if 0
       optional<bool> unchanged;
       if (a.operation () == update_id)
         unchanged = match_inner (a, t, unmatch::unchanged).first;
+#else
+      action ia (a.inner_action ());
+      if (a.operation () == update_id)
+        match_only_sync (ia, t);
+#endif
 
       optional<const scope*> is; // Installation scope (resolve lazily).
 
@@ -454,6 +479,12 @@ namespace build2
           pts.push_back (prerequisite_target (pt, pi));
       }
 
+#if 1
+      optional<bool> unchanged;
+      if (a.operation () == update_id)
+        unchanged = match_sync (ia, t, unmatch::unchanged).first;
+#endif
+
       if (a.operation () == update_id)
       {
         return *unchanged
diff --git a/libbuild2/version/rule.cxx b/libbuild2/version/rule.cxx
index 98dc2da..65c1117 100644
--- a/libbuild2/version/rule.cxx
+++ b/libbuild2/version/rule.cxx
@@ -93,6 +93,12 @@ namespace build2
       if (!fi)
         l5 ([&]{trace << "no in file prerequisite for target " << t;});
 
+      // If we match, derive the file name early as recommended by the in
+      // rule.
+      //
+      if (fm && fi)
+        t.derive_path ();
+
       return fm && fi;
     }