Clean up group state, mtime design

10 files changed, 124 insertions, 86 deletions

diff --git a/build/algorithm b/build/algorithm
index 1cfd15b..f108a8a 100644
--- a/build/algorithm
+++ b/build/algorithm
@@ -134,7 +134,11 @@ namespace build
   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 timestamp.
+  // needs to be executed on the target based on the passed mtime
+  // timestamp.
+  //
+  // Note that because we use mtime, this function should normally
+  // only be used in the perform_update action.
   //
   bool
   execute_prerequisites (action, target&, const timestamp&);
diff --git a/build/b.cxx b/build/b.cxx
index ef39446..d7fb9d3 100644
--- a/build/b.cxx
+++ b/build/b.cxx
@@ -212,10 +212,10 @@ main (int argc, char* argv[])
     rules[update_id][typeid (fsdir)].emplace ("fsdir", fsdir_r);
     rules[clean_id][typeid (fsdir)].emplace ("fsdir", fsdir_r);
 
-    path_rule path_r;
-    rules[default_id][typeid (path_target)].emplace ("path", path_r);
-    rules[update_id][typeid (path_target)].emplace ("path", path_r);
-    rules[clean_id][typeid (path_target)].emplace ("path", path_r);
+    file_rule file_r;
+    rules[default_id][typeid (file)].emplace ("file", file_r);
+    rules[update_id][typeid (file)].emplace ("file", file_r);
+    rules[clean_id][typeid (file)].emplace ("file", file_r);
 
     // Figure out work and home directories.
     //
diff --git a/build/cli/rule.cxx b/build/cli/rule.cxx
index 6377b29..3bcd074 100644
--- a/build/cli/rule.cxx
+++ b/build/cli/rule.cxx
@@ -210,78 +210,65 @@ namespace build
 
       // Execute our prerequsites and check if we are out of date.
       //
-      cli* s (execute_prerequisites<cli> (a, t, t.h ()->mtime ()));
-
-      target_state ts;
+      cli* s (execute_prerequisites<cli> (a, t, t.mtime ()));
 
       if (s == nullptr)
-        ts = target_state::unchanged;
-      else
-      {
-        // Translate source path to relative (to working directory). This
-        // results in easier to read diagnostics.
-        //
-        path relo (relative (t.dir));
-        path rels (relative (s->path ()));
+        return target_state::unchanged;
 
-        scope& rs (t.root_scope ());
-        const string& cli (rs["config.cli"].as<const string&> ());
-
-        vector<const char*> args {cli.c_str ()};
+      // Translate source path to relative (to working directory). This
+      // results in easier to read diagnostics.
+      //
+      path relo (relative (t.dir));
+      path rels (relative (s->path ()));
 
-        // See if we need to pass any --?xx-suffix options.
-        //
-        append_extension (args, *t.h (), "--hxx-suffix", "hxx");
-        append_extension (args, *t.c (), "--cxx-suffix", "cxx");
-        if (t.i () != nullptr)
-          append_extension (args, *t.i (), "--ixx-suffix", "ixx");
+      scope& rs (t.root_scope ());
+      const string& cli (rs["config.cli"].as<const string&> ());
 
-        append_options (args, t, "cli.options");
+      vector<const char*> args {cli.c_str ()};
 
-        if (!relo.empty ())
-        {
-          args.push_back ("-o");
-          args.push_back (relo.string ().c_str ());
-        }
+      // See if we need to pass any --?xx-suffix options.
+      //
+      append_extension (args, *t.h (), "--hxx-suffix", "hxx");
+      append_extension (args, *t.c (), "--cxx-suffix", "cxx");
+      if (t.i () != nullptr)
+        append_extension (args, *t.i (), "--ixx-suffix", "ixx");
 
-        args.push_back (rels.string ().c_str ());
-        args.push_back (nullptr);
+      append_options (args, t, "cli.options");
 
-        if (verb)
-          print_process (args);
-        else
-          text << "cli " << *s;
+      if (!relo.empty ())
+      {
+        args.push_back ("-o");
+        args.push_back (relo.string ().c_str ());
+      }
 
-        try
-        {
-          process pr (args.data ());
+      args.push_back (rels.string ().c_str ());
+      args.push_back (nullptr);
 
-          if (!pr.wait ())
-            throw failed ();
+      if (verb)
+        print_process (args);
+      else
+        text << "cli " << *s;
 
-          timestamp s (system_clock::now ());
+      try
+      {
+        process pr (args.data ());
 
-          // Update member timestamps.
-          //
-          t.h ()->mtime (s);
-          t.c ()->mtime (s);
-          if (t.i () != nullptr)
-            t.i ()->mtime (s);
+        if (!pr.wait ())
+          throw failed ();
 
-          ts = target_state::changed;
-        }
-        catch (const process_error& e)
-        {
-          error << "unable to execute " << args[0] << ": " << e.what ();
+        t.mtime (system_clock::now ());
+      }
+      catch (const process_error& e)
+      {
+        error << "unable to execute " << args[0] << ": " << e.what ();
 
-          if (e.child ())
-            exit (1);
+        if (e.child ())
+          exit (1);
 
-          throw failed ();
-        }
+        throw failed ();
       }
 
-      return ts;
+      return target_state::changed;
     }
 
     target_state compile::
@@ -297,16 +284,11 @@ namespace build
       bool r (false);
 
       if (t.i () != nullptr)
-      {
         r = rmfile (t.i ()->path (), *t.i ()) || r;
-        t.i ()->mtime (timestamp_nonexistent);
-      }
-
       r = rmfile (t.c ()->path (), *t.c ()) || r;
-      t.c ()->mtime (timestamp_nonexistent);
-
       r = rmfile (t.h ()->path (), *t.h ()) || r;
-      t.h ()->mtime (timestamp_nonexistent);
+
+      t.mtime (timestamp_nonexistent);
 
       // Clean prerequisites.
       //
diff --git a/build/cli/target b/build/cli/target
index e72d4e0..d733668 100644
--- a/build/cli/target
+++ b/build/cli/target
@@ -23,10 +23,10 @@ namespace build
       static const target_type static_type;
     };
 
-    class cli_cxx: public target
+    class cli_cxx: public mtime_target
     {
     public:
-      using target::target;
+      using mtime_target::mtime_target;
 
       target* m[3] {nullptr, nullptr, nullptr};
 
@@ -41,6 +41,9 @@ namespace build
       virtual group_view
       group_members (action) const;
 
+      virtual timestamp
+      load_mtime () const;
+
     public:
       virtual const target_type& type () const {return static_type;}
       static const target_type static_type;
diff --git a/build/cli/target.cxx b/build/cli/target.cxx
index aded1ff..2f8b54c 100644
--- a/build/cli/target.cxx
+++ b/build/cli/target.cxx
@@ -4,7 +4,10 @@
 
 #include <build/cli/target>
 
+#include <butl/filesystem>
+
 using namespace std;
+using namespace butl;
 
 namespace build
 {
@@ -34,11 +37,20 @@ namespace build
         : group_view {nullptr, 0};
     }
 
+    timestamp cli_cxx::
+    load_mtime () const
+    {
+      // The rule has been matched which means the members should
+      // be resolved and paths assigned.
+      //
+      return file_mtime (h ()->path ());
+    }
+
     const target_type cli_cxx::static_type
     {
       typeid (cli_cxx),
       "cli.cxx",
-      &target::static_type,
+      &mtime_target::static_type,
       &target_factory<cli_cxx>,
       nullptr,
       &search_target,
diff --git a/build/cxx/rule.cxx b/build/cxx/rule.cxx
index 08a9202..7547201 100644
--- a/build/cxx/rule.cxx
+++ b/build/cxx/rule.cxx
@@ -650,13 +650,13 @@ namespace build
               // expensive. At the same time, most of these headers are
               // existing files that we will never be updating (again,
               // system headers, for example) and the rule that will match
-              // them is fallback path_rule. So we are going to do a little
+              // them is fallback file_rule. So we are going to do a little
               // fast-path optimization by detecting this common case.
               //
               recipe_function* const* recipe (
                 pt.recipe (a).target<recipe_function*> ());
 
-              if (recipe == nullptr || *recipe != &path_rule::perform_update)
+              if (recipe == nullptr || *recipe != &file_rule::perform_update)
               {
                 // We only want to restart if our call to execute() actually
                 // caused an update. In particular, the target could already
diff --git a/build/rule b/build/rule
index 8c7e4c1..eda6ed3 100644
--- a/build/rule
+++ b/build/rule
@@ -64,10 +64,10 @@ namespace build
 
   extern operation_rule_map rules;
 
-  // Fallback rule that on update verifies that the path exists and is
+  // Fallback rule that on update verifies that the file exists and is
   // not older than any of its prerequisites.
   //
-  class path_rule: public rule
+  class file_rule: public rule
   {
   public:
     virtual match_result
diff --git a/build/rule.cxx b/build/rule.cxx
index ce8ef01..a8c77a8 100644
--- a/build/rule.cxx
+++ b/build/rule.cxx
@@ -21,7 +21,7 @@ namespace build
 {
   operation_rule_map rules;
 
-  // path_rule
+  // file_rule
   //
   // Note that this rule is special. It is the last, fallback rule. If
   // it doesn't match, then no other rule can possibly match and we have
@@ -30,9 +30,11 @@ namespace build
   // that normal implementations should follow. So you probably shouldn't
   // use it as a guide to implement your own, normal, rules.
   //
-  match_result path_rule::
+  match_result file_rule::
   match (action a, target& t, const string&) const
   {
+    tracer trace ("file_rule::match");
+
     // While strictly speaking we should check for the file's existence
     // for every action (because that's the condition for us matching),
     // for some actions this is clearly a waste. Say, perform_clean: we
@@ -57,14 +59,23 @@ namespace build
         if (pt.path ().empty ())
           pt.derive_path ();
 
-        return pt.mtime () != timestamp_nonexistent ? &t : nullptr;
+        // We cannot just call pt.mtime() since we haven't matched yet.
+        //
+        timestamp ts (file_mtime (pt.path ()));
+        pt.mtime (ts);
+
+        if (ts != timestamp_nonexistent)
+          return t;
+
+        level3 ([&]{trace << "no existing file for target " << t;});
+        return nullptr;
       }
     default:
       return t;
     }
   }
 
-  recipe path_rule::
+  recipe file_rule::
   apply (action a, target& t, const match_result&) const
   {
     // Update triggers the update of this target's prerequisites
@@ -87,7 +98,7 @@ namespace build
       : t.has_prerequisites () ? default_recipe : noop_recipe;
   }
 
-  target_state path_rule::
+  target_state file_rule::
   perform_update (action a, target& t)
   {
     // Make sure the target is not older than any of its prerequisites.
diff --git a/build/search.cxx b/build/search.cxx
index 872a0d6..dfb603d 100644
--- a/build/search.cxx
+++ b/build/search.cxx
@@ -116,9 +116,9 @@ namespace build
       //
       auto r (targets.insert (*tk.type, f.directory (), *tk.name, ext, trace));
 
-      // Has to be a path_target.
+      // Has to be a file_target.
       //
-      path_target& t (dynamic_cast<path_target&> (r.first));
+      file& t (dynamic_cast<file&> (r.first));
 
       level4 ([&]{trace << (r.second ? "new" : "existing") << " target "
                         << t << " for prerequisite " << pk;});
diff --git a/build/target b/build/target
index 000496c..39f6679 100644
--- a/build/target
+++ b/build/target
@@ -782,17 +782,40 @@ namespace build
   public:
     using target::target;
 
+    // Target mtime is only available after a rule has been matched
+    // (because this is when we know if we should get our mtime from
+    // the group and where the path which we need to load mtime is
+    // normally assigned). The mtime is also unavailable while the
+    // execution of the target is postponed (because we temporarily
+    // loose our group state).
+    //
+    // The rule for groups that utilize the group state is as follows:
+    // if it has any members that are mtime_targets, then the group
+    // should be mtime_target and the members get the mtime from it.
+    //
     timestamp
     mtime () const
     {
-      if (mtime_ == timestamp_unknown)
-        mtime_ = load_mtime ();
+      assert (raw_state != target_state::postponed);
+
+      const mtime_target* t (raw_state == target_state::group
+                             ? static_cast<const mtime_target*> (group)
+                             : this);
+
+      if (t->mtime_ == timestamp_unknown)
+        t->mtime_ = t->load_mtime ();
 
-      return mtime_;
+      return t->mtime_;
     }
 
     void
-    mtime (timestamp mt) {mtime_ = mt;}
+    mtime (timestamp mt)
+    {
+      // While we can cache the mtime at any time, it may be ignored
+      // if the target state is group (see the mtime() accessor).
+      //
+      mtime_ = mt;
+    }
 
   protected:
     virtual timestamp
@@ -853,8 +876,11 @@ namespace build
     using path_target::path_target;
 
   protected:
+    // Note that it is final in order to be consistent with file_rule,
+    // search_existing_file().
+    //
     virtual timestamp
-    load_mtime () const;
+    load_mtime () const final;
 
   public:
     virtual const target_type& type () const {return static_type;}