Ignore prerequisite mtimes that are not linker inputs

This makes sure, for example, that we don't unnecessarily re-link an
executable when its testscript prerequisite is changes.

9 files changed, 155 insertions, 151 deletions

diff --git a/build2/algorithm b/build2/algorithm
index 9401915..3dbef4c 100644
--- a/build2/algorithm
+++ b/build2/algorithm
@@ -180,15 +180,31 @@ namespace build2
   target_state
   reverse_execute_prerequisites (action, target&);
 
-  // A version of the above that also determines whether the action
-  // needs to be executed on the target based on the passed mtime
-  // timestamp.
+  // A version of the above that also determines whether the action needs to
+  // be executed on the target based on the passed timestamp and filter.
   //
-  // Note that because we use mtime, this function should normally
-  // only be used in the perform_update action.
+  // The filter is passed each prerequisite target and is expected to signal
+  // which ones should be used for timestamp comparison. If the filter is
+  // NULL, then all the prerequisites are used.
   //
-  bool
-  execute_prerequisites (action, target&, const timestamp&);
+  // Note that the return value is a pair with the second half indicating
+  // whether any prerequisites were updated. This is used to handle the
+  // situation where some prerequisites were updated but no update of the
+  // target is necessary. In this case we still signal that the target was
+  // (conceptually, but not physically) changed. This is important both to
+  // propagate the fact that some work has been done and to also allow our
+  // dependents to detect this case if they are up to something tricky (like
+  // recursively linking liba{} prerequisites).
+  //
+  // Note that because we use mtime, this function should normally only be
+  // used in the perform_update action.
+  //
+  using prerequisite_filter = function<bool (target&)>;
+
+  pair<bool, target_state>
+  execute_prerequisites (action, target&,
+                         const timestamp&,
+                         const prerequisite_filter& = nullptr);
 
   // Another version of the above that does two extra things for the caller:
   // it determines whether the action needs to be executed on the target based
@@ -198,17 +214,23 @@ namespace build2
   // prerequisites of the same type may get injected).
   //
   template <typename T>
-  T*
-  execute_prerequisites (action, target&, const timestamp&);
+  pair<T*, target_state>
+  execute_prerequisites (action, target&,
+                         const timestamp&,
+                         const prerequisite_filter& = nullptr);
 
-  target*
+  pair<target*, target_state>
   execute_prerequisites (const target_type&,
-                         action, target&, const timestamp&);
+                         action, target&,
+                         const timestamp&,
+                         const prerequisite_filter& = nullptr);
 
   template <typename T>
-  T*
+  pair<T*, target_state>
   execute_prerequisites (const target_type&,
-                         action, target&, const timestamp&);
+                         action, target&,
+                         const timestamp&,
+                         const prerequisite_filter& = nullptr);
 
   // Return noop_recipe instead of using this function directly.
   //
diff --git a/build2/algorithm.cxx b/build2/algorithm.cxx
index 638afd0..8874e96 100644
--- a/build2/algorithm.cxx
+++ b/build2/algorithm.cxx
@@ -413,54 +413,15 @@ namespace build2
     return r;
   }
 
-  bool
-  execute_prerequisites (action a, target& t, const timestamp& mt)
+  pair<target*, target_state>
+  execute_prerequisites (const target_type* tt,
+                         action a, target& t,
+                         const timestamp& mt, const prerequisite_filter& pf)
   {
     bool e (mt == timestamp_nonexistent);
 
-    for (target* pt: t.prerequisite_targets)
-    {
-      if (pt == nullptr) // Skipped.
-        continue;
-
-      target_state ts (execute (a, *pt));
-
-      if (!e)
-      {
-        // If this is an mtime-based target, then compare timestamps.
-        //
-        if (auto mpt = dynamic_cast<const mtime_target*> (pt))
-        {
-          timestamp mp (mpt->mtime ());
-
-          // What do we do if timestamps are equal? This can happen, for
-          // example, on filesystems that don't have subsecond resolution.
-          // There is not much we can do here except detect the case where
-          // the prerequisite was changed in this run which means the
-          // action must be executed on the target as well.
-          //
-          if (mt < mp || (mt == mp && ts == target_state::changed))
-            e = true;
-        }
-        else
-        {
-          // Otherwise we assume the prerequisite is newer if it was changed.
-          //
-          if (ts == target_state::changed)
-            e = true;
-        }
-      }
-    }
-
-    return e;
-  }
-
-  target*
-  execute_prerequisites (const target_type& tt,
-                         action a, target& t, const timestamp& mt)
-  {
-    bool e (mt == timestamp_nonexistent);
-    target* r (nullptr);
+    target* rt (tt != nullptr ? nullptr : &t);
+    target_state rs (target_state::unchanged);
 
     for (target* pt: t.prerequisite_targets)
     {
@@ -468,8 +429,11 @@ namespace build2
         continue;
 
       target_state ts (execute (a, *pt));
+      rs |= ts;
 
-      if (!e)
+      // Should we compare the timestamp to this target's?
+      //
+      if (!e && (!pf || pf (*pt)))
       {
         // If this is an mtime-based target, then compare timestamps.
         //
@@ -477,11 +441,8 @@ namespace build2
         {
           timestamp mp (mpt->mtime ());
 
-          // What do we do if timestamps are equal? This can happen, for
-          // example, on filesystems that don't have subsecond resolution.
-          // There is not much we can do here except detect the case where
-          // the prerequisite was changed in this run which means the
-          // action must be executed on the target as well.
+          // The same logic as in mtime_target::newer() (but avoids a call to
+          // state()).
           //
           if (mt < mp || (mt == mp && ts == target_state::changed))
             e = true;
@@ -495,12 +456,12 @@ namespace build2
         }
       }
 
-      if (r == nullptr && pt->is_a (tt))
-        r = pt;
+      if (rt == nullptr && pt->is_a (*tt))
+        rt = pt;
     }
 
-    assert (r != nullptr);
-    return e ? r : nullptr;
+    assert (rt != nullptr);
+    return make_pair (e ? rt : nullptr, rs);
   }
 
   target_state
diff --git a/build2/algorithm.ixx b/build2/algorithm.ixx
index 3128365..09d522f 100644
--- a/build2/algorithm.ixx
+++ b/build2/algorithm.ixx
@@ -217,18 +217,46 @@ namespace build2
     }
   }
 
+  // If the first argument is NULL, then the result is treated as a boolean
+  // value.
+  //
+  pair<target*, target_state>
+  execute_prerequisites (const target_type*,
+                         action, target&,
+                         const timestamp&, const prerequisite_filter&);
+
+  inline pair<bool, target_state>
+  execute_prerequisites (action a, target& t,
+                         const timestamp& mt, const prerequisite_filter& pf)
+  {
+    auto p (execute_prerequisites (nullptr, a, t, mt, pf));
+    return make_pair (p.first != nullptr, p.second);
+  }
+
   template <typename T>
-  inline T*
-  execute_prerequisites (action a, target& t, const timestamp& mt)
+  inline pair<T*, target_state>
+  execute_prerequisites (action a, target& t,
+                         const timestamp& mt, const prerequisite_filter& pf)
+  {
+    auto p (execute_prerequisites (T::static_type, a, t, mt, pf));
+    return make_pair (static_cast<T*> (p.first), p.second);
+  }
+
+  inline pair<target*, target_state>
+  execute_prerequisites (const target_type& tt,
+                         action a, target& t,
+                         const timestamp& mt, const prerequisite_filter& pf)
   {
-    return static_cast<T*> (execute_prerequisites (T::static_type, a, t, mt));
+    return execute_prerequisites (&tt, a, t, mt, pf);
   }
 
   template <typename T>
-  inline T*
+  inline pair<T*, target_state>
   execute_prerequisites (const target_type& tt,
-                         action a, target& t, const timestamp& mt)
+                         action a, target& t,
+                         const timestamp& mt, const prerequisite_filter& pf)
   {
-    return static_cast<T*> (execute_prerequisites (tt, a, t, mt));
+    auto p (execute_prerequisites (tt, a, t, mt, pf));
+    return make_pair (static_cast<T*> (p.first), p.second);
   }
 }
diff --git a/build2/cc/compile.cxx b/build2/cc/compile.cxx
index 6fd64c2..3318f21 100644
--- a/build2/cc/compile.cxx
+++ b/build2/cc/compile.cxx
@@ -888,16 +888,7 @@ namespace build2
           }
         }
 
-        if (ts != timestamp_unknown)
-        {
-          timestamp mt (pt.mtime ());
-
-          // See execute_prerequisites() for rationale behind the equal part.
-          //
-          return ts < mt || (ts == mt && pt.state () != target_state::changed);
-        }
-
-        return false;
+        return ts != timestamp_unknown ? pt.newer (ts) : false;
       };
 
       // Update and add a header file to the list of prerequisite targets.
@@ -1394,10 +1385,18 @@ namespace build2
     perform_update (action a, target& xt) const
     {
       file& t (static_cast<file&> (xt));
-      file* s (execute_prerequisites<file> (x_src, a, t, t.mtime ()));
 
-      if (s == nullptr)
-        return target_state::unchanged;
+      // Update prerequisites and determine if any relevant ones render us
+      // out-of-date. Note that currently we treat all the prerequisites
+      // as potentially affecting the result (for simplicity/performance).
+      //
+      file* s;
+      {
+        auto p (execute_prerequisites<file> (x_src, a, t, t.mtime ()));
+
+        if ((s = p.first) == nullptr)
+          return p.second;
+      }
 
       scope& bs (t.base_scope ());
       scope& rs (*bs.root_scope ());
diff --git a/build2/cc/link.cxx b/build2/cc/link.cxx
index 9dbfa36..a6b67d5 100644
--- a/build2/cc/link.cxx
+++ b/build2/cc/link.cxx
@@ -890,9 +890,12 @@ namespace build2
       otype lt (link_type (t));
       lorder lo (link_order (bs, lt));
 
-      // Update prerequisites.
+      // Update prerequisites. We determine if any relevant ones render us
+      // out-of-date manually below.
       //
-      bool update (execute_prerequisites (a, t, t.mtime ()));
+      bool update (false);
+      timestamp mt (t.mtime ());
+      target_state ts (execute_prerequisites (a, t));
 
       // If targeting Windows, take care of the manifest.
       //
@@ -913,7 +916,7 @@ namespace build2
             t,
             rpath_timestamp != timestamp_nonexistent));
 
-        timestamp mt (file_mtime (mf));
+        timestamp mf_mt (file_mtime (mf));
 
         if (tsys == "mingw32")
         {
@@ -923,7 +926,7 @@ namespace build2
           //
           manifest = mf + ".o";
 
-          if (mt > file_mtime (manifest))
+          if (mf_mt > file_mtime (manifest))
           {
             path of (relative (manifest));
 
@@ -998,7 +1001,7 @@ namespace build2
         {
           manifest = move (mf); // Save for link.exe's /MANIFESTINPUT.
 
-          if (mt > t.mtime ())
+          if (mf_mt > mt)
             update = true; // Manifest changed, force update.
         }
       }
@@ -1204,9 +1207,16 @@ namespace build2
             else
               cs.append (f->path ().string ());
           }
+          else
+            f = pt->is_a<exe> (); // Consider executable mtime (e.g., linker).
+
+          // Check if this input renders us out-of-date.
+          //
+          if (f != nullptr)
+            update = update || f->newer (mt);
         }
 
-        // Treat it as input for both MinGW and VC.
+        // Treat it as input for both MinGW and VC (mtime checked above).
         //
         if (!manifest.empty ())
           cs.append (manifest.string ());
@@ -1229,7 +1239,7 @@ namespace build2
       // this situation in the "from scratch" flag.
       //
       bool scratch (false);
-      if (dd.writing () || dd.mtime () > t.mtime ())
+      if (dd.writing () || dd.mtime () > mt)
         scratch = update = true;
 
       dd.close ();
@@ -1237,7 +1247,7 @@ namespace build2
       // If nothing changed, then we are done.
       //
       if (!update)
-        return target_state::unchanged;
+        return ts;
 
       // Ok, so we are updating. Finish building the command line.
       //
diff --git a/build2/cli/rule.cxx b/build2/cli/rule.cxx
index 5b78723..b97bea2 100644
--- a/build2/cli/rule.cxx
+++ b/build2/cli/rule.cxx
@@ -231,12 +231,18 @@ namespace build2
     {
       cli_cxx& t (static_cast<cli_cxx&> (xt));
 
-      // Execute our prerequsites and check if we are out of date.
+      // Update prerequisites and determine if any relevant ones render us
+      // out-of-date. Note that currently we treat all the prerequisites
+      // as potentially affecting the result (think prologues/epilogues,
+      // etc).
       //
-      cli* s (execute_prerequisites<cli> (a, t, t.mtime ()));
+      cli* s;
+      {
+        auto p (execute_prerequisites<cli> (a, t, t.mtime ()));
 
-      if (s == nullptr)
-        return target_state::unchanged;
+        if ((s = p.first) == nullptr)
+          return p.second;
+      }
 
       // Translate paths to relative (to working directory). This
       // results in easier to read diagnostics.
diff --git a/build2/rule b/build2/rule
index 8090fa0..015cf1a 100644
--- a/build2/rule
+++ b/build2/rule
@@ -39,8 +39,7 @@ namespace build2
     apply (action, target&) const = 0;
   };
 
-  // Fallback rule that on update verifies that the file exists and is
-  // not older than any of its prerequisites.
+  // Fallback rule that only matches if the file exists.
   //
   class file_rule: public rule
   {
@@ -51,9 +50,6 @@ namespace build2
     virtual recipe
     apply (action, target&) const override;
 
-    static target_state
-    perform_update (action, target&);
-
     static file_rule instance;
   };
 
diff --git a/build2/rule.cxx b/build2/rule.cxx
index a9b7c4e..1c0e08c 100644
--- a/build2/rule.cxx
+++ b/build2/rule.cxx
@@ -81,22 +81,18 @@ namespace build2
   recipe file_rule::
   apply (action a, target& t) const
   {
-    // Update triggers the update of this target's prerequisites
-    // so it would seem natural that we should also trigger their
-    // cleanup. However, this possibility is rather theoretical
-    // since such an update would render this target out of date
-    // which in turn would lead to an error. So until we see a
-    // real use-case for this functionality, we simply ignore
-    // the clean operation.
+    // Update triggers the update of this target's prerequisites so it would
+    // seem natural that we should also trigger their cleanup. However, this
+    // possibility is rather theoretical so until we see a real use-case for
+    // this functionality, we simply ignore the clean operation.
     //
     if (a.operation () == clean_id)
       return noop_recipe;
 
-    // If we have no prerequisites, then this means this file
-    // is up to date. Return noop_recipe which will also cause
-    // the target's state to be set to unchanged. This is an
-    // important optimization on which quite a few places that
-    // deal with predominantly static content rely.
+    // If we have no prerequisites, then this means this file is up to date.
+    // Return noop_recipe which will also cause the target's state to be set
+    // to unchanged. This is an important optimization on which quite a few
+    // places that deal with predominantly static content rely.
     //
     if (!t.has_prerequisites ())
       return noop_recipe;
@@ -104,43 +100,14 @@ namespace build2
     // Search and match all the prerequisites.
     //
     search_and_match_prerequisites (a, t);
-    return a == perform_update_id ? &perform_update : default_recipe;
-  }
 
-  target_state file_rule::
-  perform_update (action a, target& t)
-  {
-    // Make sure the target is not older than any of its prerequisites.
+    // Note that we used to provide perform_update() which checked that this
+    // target is not older than any of its prerequisites. However, later we
+    // realized this is probably wrong: consider a script with a testscript as
+    // a prerequisite; chances are the testscript will be newer than the
+    // script and there is nothing wrong with that.
     //
-    timestamp mt (dynamic_cast<path_target&> (t).mtime ());
-
-    for (target* pt: t.prerequisite_targets)
-    {
-      target_state ts (execute (a, *pt));
-
-      // If this is an mtime-based target, then compare timestamps.
-      //
-      if (auto mpt = dynamic_cast<const mtime_target*> (pt))
-      {
-        timestamp mp (mpt->mtime ());
-
-        if (mt < mp)
-          fail << "no recipe to " << diag_do (a, t) <<
-            info << "prerequisite " << *pt << " is ahead of " << t
-               << " by " << (mp - mt);
-      }
-      else
-      {
-        // Otherwise we assume the prerequisite is newer if it was changed.
-        //
-        if (ts == target_state::changed)
-          fail << "no recipe to " << diag_do (a, t) <<
-            info << "prerequisite " << *pt << " is ahead of " << t
-               << " because it was updated";
-      }
-    }
-
-    return target_state::unchanged;
+    return default_recipe;
   }
 
   file_rule file_rule::instance;
diff --git a/build2/target b/build2/target
index 1b75da3..dd9461c 100644
--- a/build2/target
+++ b/build2/target
@@ -1098,6 +1098,21 @@ namespace build2
       mtime_ = mt;
     }
 
+    // Return true if this target is newer than the specified timestamp.
+    //
+    bool
+    newer (timestamp mt)
+    {
+      timestamp mp (mtime ());
+
+      // What do we do if timestamps are equal? This can happen, for example,
+      // on filesystems that don't have subsecond resolution. There is not
+      // much we can do here except detect the case where the target was
+      // changed on this run.
+      //
+      return mt < mp || (mt == mp && state () == target_state::changed);
+    }
+
   protected:
     // Return timestamp_unknown if the mtime cannot be loaded.
     //