Explicit group: pattern members

8 files changed, 197 insertions, 71 deletions

diff --git a/libbuild2/adhoc-rule-buildscript.cxx b/libbuild2/adhoc-rule-buildscript.cxx
index 7c987f1..cd760b4 100644
--- a/libbuild2/adhoc-rule-buildscript.cxx
+++ b/libbuild2/adhoc-rule-buildscript.cxx
@@ -326,22 +326,11 @@ namespace build2
 
     group* g (t.is_a<group> ()); // Explicit group.
 
-    // Inject pattern's ad hoc group members, if any.
+    // Inject pattern's ad hoc group members, if any (explicit group members
+    // are injected after reset below).
     //
-    if (pattern != nullptr)
-    {
-      // @@ TODO: expl: pattern: if first must be file, we should probably add
-      //    them after the group's static_members. Suppress duplicates that
-      //    are already in group::static_members.
-
-      pattern->apply_adhoc_members (a, t, bs, me);
-
-      // A pattern rule that matches an explicit group should not inject any
-      // ad hoc members.
-      //
-      if (g != nullptr)
-        assert (t.adhoc_member == nullptr);
-    }
+    if (g == nullptr && pattern != nullptr)
+      pattern->apply_group_members (a, t, bs, me);
 
     // Derive file names for the target and its static/ad hoc group members,
     // if any.
@@ -357,27 +346,37 @@ namespace build2
         // is a file.
         //
         for (const target& m: g->static_members)
-        {
-          if (auto* p = m.is_a<path_target> ())
-            p->derive_path ();
-
           g->members.push_back (&m);
-        }
 
         g->members_static = g->members.size ();
 
+        if (pattern != nullptr)
+        {
+          pattern->apply_group_members (a, *g, bs, me);
+          g->members_static = g->members.size ();
+        }
+
         if (g->members_static == 0)
         {
           if (!script.depdb_dyndep_dyn_target)
             fail << "group " << *g << " has no static or dynamic members";
         }
-        else if (!g->members.front ()->is_a<file> ())
+        else
         {
-          // We use the first static member to derive depdb path, get mtime,
-          // etc. So it must be file-based.
+          if (!g->members.front ()->is_a<file> ())
+          {
+            // We use the first static member to derive depdb path, get mtime,
+            // etc. So it must be file-based.
+            //
+            fail << "first static member " << g->members.front ()
+                 << " of group " << *g << " is not a file";
+          }
+
+          // Derive paths for all the static members.
           //
-          fail << "first static member " << g->members.front () << " of group "
-               << *g << " is not a file";
+          for (const target* m: g->members)
+            if (auto* p = m->is_a<path_target> ())
+              p->derive_path ();
         }
       }
       else
@@ -404,6 +403,12 @@ namespace build2
           g->members.push_back (&m);
 
         g->members_static = g->members.size ();
+
+        if (pattern != nullptr)
+        {
+          pattern->apply_group_members (a, *g, bs, me);
+          g->members_static = g->members.size ();
+        }
       }
     }
 
@@ -680,7 +685,7 @@ namespace build2
                 what,
                 a, bs, *g,
                 move (f),
-                map_ext, def_tt, filter));
+                map_ext, def_tt, filter, true /* skip_match */));
 
             if (r.second)
               g->members.push_back (&r.first);
diff --git a/libbuild2/adhoc-rule-regex-pattern.cxx b/libbuild2/adhoc-rule-regex-pattern.cxx
index 354b859..0f2ecc3 100644
--- a/libbuild2/adhoc-rule-regex-pattern.cxx
+++ b/libbuild2/adhoc-rule-regex-pattern.cxx
@@ -319,8 +319,14 @@ namespace build2
   }
 
   void adhoc_rule_regex_pattern::
-  apply_adhoc_members (action a, target& t, const scope&, match_extra&) const
+  apply_group_members (action a, target& t, const scope& bs,
+                       match_extra&) const
   {
+    if (targets_.size () == 1) // The group/primary target is always present.
+      return;
+
+    group* g (t.is_a<group> ());
+
     const auto& mr (t.data<regex_match_results> (a));
 
     for (auto i (targets_.begin () + 1); i != targets_.end (); ++i)
@@ -348,14 +354,83 @@ namespace build2
         d.normalize ();
       }
 
-      // @@ TODO: currently this uses type as the ad hoc member identity.
-      //
-      add_adhoc_member (
-        t,
-        e.type,
-        move (d),
-        dir_path () /* out */,
-        substitute (t, mr, e.name.value, "ad hoc target group member"));
+      string n (substitute (
+                  t,
+                  mr,
+                  e.name.value,
+                  (g != nullptr
+                   ? "explicit target group member"
+                   : "ad hoc target group member")));
+
+      // @@ TODO: what if name contains extension? Shouldn't we call
+      //          split_name()?
+
+      if (g != nullptr)
+      {
+        auto& ms (g->members);
+
+        // These are conceptually static but they behave more like dynamic in
+        // that we likely need to insert the target, set its group, etc.
+        //
+        // Note: a custom version of the dyndep_rule::inject_group_member()
+        // logic.
+        //
+        auto l (search_new_locked (
+                  bs.ctx,
+                  e.type,
+                  move (d),
+                  dir_path (), // Always in out.
+                  move (n),
+                  nullptr /* ext */,
+                  &bs));
+
+        const target& t (l.first); // Note: non-const only if have lock.
+
+        // Note: we don't need to match the group recipe directy due to the
+        // special ad hoc recipe/rule semantics for explicit group members
+        // in match_rule().
+        //
+        if (l.second)
+        {
+          l.first.group = g;
+          l.second.unlock ();
+        }
+        else
+        {
+          if (find (ms.begin (), ms.end (), &t) != ms.end ())
+            continue;
+
+          // We can only update the group under lock.
+          //
+          target_lock tl (lock (a, t));
+
+          if (!tl)
+            fail << "group " << *g << " member " << t << " is already matched" <<
+              info << "static group members specified by pattern rules cannot "
+                   << "be used as prerequisites directly, only via group";
+
+          if (t.group == nullptr)
+            tl.target->group = g;
+          else if (t.group != g)
+          {
+            fail << "group " << *g << " member " << t
+                 << " is already member of group " << *t.group;
+          }
+        }
+
+        ms.push_back (&t);
+      }
+      else
+      {
+        // @@ TODO: currently this uses type as the ad hoc member identity.
+        //
+        add_adhoc_member (
+          t,
+          e.type,
+          move (d),
+          dir_path (), // Always in out.
+          move (n));
+      }
     }
   }
 
diff --git a/libbuild2/adhoc-rule-regex-pattern.hxx b/libbuild2/adhoc-rule-regex-pattern.hxx
index eb75ea0..9cb7874 100644
--- a/libbuild2/adhoc-rule-regex-pattern.hxx
+++ b/libbuild2/adhoc-rule-regex-pattern.hxx
@@ -35,7 +35,7 @@ namespace build2
     match (action, const target&, const string&, match_extra&) const override;
 
     virtual void
-    apply_adhoc_members (action, target&,
+    apply_group_members (action, target&,
                          const scope&,
                          match_extra&) const override;
 
diff --git a/libbuild2/build/script/parser.cxx b/libbuild2/build/script/parser.cxx
index d61b7d7..3b24802 100644
--- a/libbuild2/build/script/parser.cxx
+++ b/libbuild2/build/script/parser.cxx
@@ -2928,7 +2928,7 @@ namespace build2
                   what_tgt,
                   a, bs, *g,
                   f, // Can't move since need to return dyn_targets.
-                  map_ext, *def_tt, filter));
+                  map_ext, *def_tt, filter, true /* skip_match */));
 
               // Note: no target_decl shenanigans since reset the members on
               // each update.
diff --git a/libbuild2/dyndep.cxx b/libbuild2/dyndep.cxx
index d34834e..8781812 100644
--- a/libbuild2/dyndep.cxx
+++ b/libbuild2/dyndep.cxx
@@ -831,10 +831,11 @@ namespace build2
   inject_group_member_impl (action a, const scope& bs, mtime_target& g,
                             path f, string n, string e,
                             const target_type& tt,
-                            const function<dyndep_rule::group_filter_func>& fl)
+                            const function<dyndep_rule::group_filter_func>& fl,
+                            bool skip_match)
   {
-    // Assume nobody else can insert these members (seems reasonable seeing
-    // that their names are dynamically discovered).
+    // We expect that nobody else can insert these members (seems reasonable
+    // seeing that their names are dynamically discovered).
     //
     auto l (search_new_locked (
               bs.ctx,
@@ -847,29 +848,56 @@ namespace build2
 
     const file& t (l.first.as<file> ()); // Note: non-const only if have lock.
 
-    if (fl != nullptr && !fl (g, t))
-      return pair<const file&, bool> (t, false);
-
-    if (l.second)
+    bool locked (l.second);
+    if (locked)
     {
       l.first.group = &g;
       l.second.unlock ();
-      t.path (move (f)); // Only do this once.
+
+      // We don't need to match the group recipe directy from ad hoc
+      // recipes/rules due to the special semantics for explicit group members
+      // in match_rule(). This is what skip_match is for.
+      //
+      if (skip_match)
+      {
+        t.path (move (f));
+        return pair<const file&, bool> (t, true);
+      }
     }
     else
-      // Must have been already done (e.g., on previous operation in a
-      // batch).
-      //
-      assert (t.group == &g);
+    {
+      if (fl != nullptr && !fl (g, t))
+        return pair<const file&, bool> (t, false);
+    }
 
-    // This shouldn't fail since we are the only ones that should be matching
-    // this target.
+    // This shouldn't normally fail since we are the only ones that should
+    // know about this target (otherwise why is it dynamicaly discovered).
+    // However, nothing prevents the user from depending on such a target,
+    // however misguided.
     //
     target_lock tl (lock (a, t));
-    assert (tl);
 
-    match_inc_dependents (a, g);
-    match_recipe (tl, group_recipe);
+    if (!tl)
+      fail << "group " << g << " member " << t << " is already matched" <<
+        info << "dynamically extracted group members cannot be used as "
+             << "prerequisites directly, only via group";
+
+    if (!locked)
+    {
+      if (t.group == nullptr)
+        tl.target->group = &g;
+      else if (t.group != &g)
+        fail << "group " << g << " member " << t
+             << " is already member of group " << *t.group;
+    }
+
+    t.path (move (f));
+
+    if (!skip_match)
+    {
+      match_inc_dependents (a, g);
+      match_recipe (tl, group_recipe);
+    }
 
     return pair<const file&, bool> (t, true);
   }
@@ -885,7 +913,8 @@ namespace build2
     return inject_group_member_impl (a, bs, g,
                                      move (f), move (n).string (), move (e),
                                      tt,
-                                     nullptr /* filter */).first;
+                                     nullptr /* filter */,
+                                     false).first;
   }
 
   static const target_type&
@@ -931,7 +960,8 @@ namespace build2
                        path f,
                        const function<map_extension_func>& map_ext,
                        const target_type& fallback,
-                       const function<group_filter_func>& filter)
+                       const function<group_filter_func>& filter,
+                       bool skip_match)
   {
     path n (f.leaf ());
     string e (n.extension ());
@@ -945,7 +975,8 @@ namespace build2
     return inject_group_member_impl (a, bs, g,
                                      move (f), move (n).string (), move (e),
                                      tt,
-                                     filter);
+                                     filter,
+                                     skip_match);
   }
 
   pair<const file&, bool> dyndep_rule::
diff --git a/libbuild2/dyndep.hxx b/libbuild2/dyndep.hxx
index 0463215..07511ee 100644
--- a/libbuild2/dyndep.hxx
+++ b/libbuild2/dyndep.hxx
@@ -256,6 +256,11 @@ namespace build2
     //
     // If specified, the group_filter function is called on the target before
     // making it a group member, skipping it if this function returns false.
+    // Note that the filter is skipped if the target is newly inserted (the
+    // filter is meant to be used to skip duplicates).
+    //
+    // Note that skip_match is an implementation detail and should not be
+    // used.
     //
     using group_filter_func = bool (mtime_target& g, const file&);
 
@@ -265,7 +270,8 @@ namespace build2
                          path,
                          const function<map_extension_func>&,
                          const target_type& fallback,
-                         const function<group_filter_func>& = nullptr);
+                         const function<group_filter_func>& = nullptr,
+                         bool skip_match = false);
 
 
     // Find or insert a target file path as a target, make it a member of the
diff --git a/libbuild2/parser.cxx b/libbuild2/parser.cxx
index 692e284..1b8e2d3 100644
--- a/libbuild2/parser.cxx
+++ b/libbuild2/parser.cxx
@@ -1263,22 +1263,17 @@ namespace build2
 
           check_pattern (n, nloc);
 
-          // If we have group members, make sure it's for an ad hoc group. A
-          // rule for an explicit group that wishes to match based on some of
-          // its members feels far fetched.
+          // If we have group members, verify all the members are patterns or
+          // substitutions (ad hoc) or subsitutions (explicit) and of the
+          // correct pattern type. A rule for an explicit group that wishes to
+          // match based on some of its members feels far fetched.
           //
-          // @@ TODO: expl: pattern: this can be used to inject static members
-          //    (which otherwise would be tedious to repeat).
+          // For explicit groups the use-case is to inject static members
+          // which could otherwise be tedious to specify for each group.
           //
           const location& mloc (gns.empty () ? location () : gns[0].member_loc);
-
-          if (!gns.empty () && gns[0].expl)
-            fail (mloc) << "explicit group members in ad hoc pattern rule";
-
-          // Then verify all the ad hoc members are patterns or substitutions
-          // and of the correct type.
-          //
           names ns (gns.empty () ? names () : move (gns[0].ns));
+          bool expl (gns.empty () ? false : gns[0].expl);
 
           for (name& n: ns)
           {
@@ -1289,7 +1284,12 @@ namespace build2
             }
 
             if (*n.pattern != pattern_type::regex_substitution)
+            {
+              if (expl)
+                fail (mloc) << "explicit group member pattern " << n;
+
               check_pattern (n, mloc);
+            }
           }
 
           // The same for prerequisites except here we can have non-patterns.
@@ -1351,6 +1351,12 @@ namespace build2
             if (ttype == nullptr)
               fail (nloc) << "unknown target type " << n.type;
 
+            if (!gns.empty ())
+            {
+              if (ttype->is_a<group> () != expl)
+                fail (nloc) << "group type and target type mismatch";
+            }
+
             unique_ptr<adhoc_rule_pattern> rp;
             switch (pt)
             {
diff --git a/libbuild2/rule.hxx b/libbuild2/rule.hxx
index b89821b..4f77432 100644
--- a/libbuild2/rule.hxx
+++ b/libbuild2/rule.hxx
@@ -333,8 +333,11 @@ namespace build2
     virtual bool
     match (action, const target&, const string&, match_extra&) const = 0;
 
+    // Append additional group members. Note that this function should handle
+    // both ad hoc and explicit groups.
+    //
     virtual void
-    apply_adhoc_members (action, target&,
+    apply_group_members (action, target&,
                          const scope& base,
                          match_extra&) const = 0;