Fix test and install rules to handle see-through groups correctly

11 files changed, 297 insertions, 132 deletions

diff --git a/build2/cc/install-rule.cxx b/build2/cc/install-rule.cxx
index 4e232ff..0fdb98b 100644
--- a/build2/cc/install-rule.cxx
+++ b/build2/cc/install-rule.cxx
@@ -26,36 +26,11 @@ namespace build2
         : common (move (d)), link_ (l) {}
 
     const target* install_rule::
-    filter (action a, const target& t, prerequisite_member p) const
+    filter (action a, const target& t, prerequisite_iterator& i) const
     {
       // NOTE: see libux_install_rule::filter() if changing anything here.
 
-      otype ot (link_type (t).type);
-
-      // Don't install executable's prerequisite headers.
-      //
-      if (t.is_a<exe> () && x_header (p))
-        return nullptr;
-
-      // Here is a problem: if the user spells the obj*/bmi*{} targets
-      // explicitly, then the source files, including headers/modules may be
-      // specified as preprequisites of those targets and not of this target.
-      // While this can be worked around for headers by also listing them as
-      // prerequisites of this target, this won't work for modules (since they
-      // are compiled). So what we are going to do here is detect bmi*{} and
-      // translate them to their mxx{} (this doesn't quite work for headers
-      // since there would normally be several of them).
-      //
-      if (p.is_a<bmi> () || p.is_a (compile_types (ot).bmi))
-      {
-        const target& mt (p.search (t));
-
-        for (prerequisite_member mp: group_prerequisite_members (a, mt))
-        {
-          if (mp.is_a (*x_mod))
-            return t.is_a<exe> () ? nullptr : file_rule::filter (a, mt, mp);
-        }
-      }
+      const prerequisite& p (i->prerequisite);
 
       // If this is a shared library prerequisite, install it as long as it
       // is in the same amalgamation as we are.
@@ -63,13 +38,18 @@ namespace build2
       // Less obvious: we also want to install a static library prerequisite
       // of a library (since it could be referenced from its .pc file, etc).
       //
+      // Note: for now we assume these prerequisites never come from see-
+      // through groups.
+      //
+      otype ot (link_type (t).type);
+
       bool st (t.is_a<exe>  () || t.is_a<libs> ()); // Target needs shared.
       bool at (t.is_a<liba> () || t.is_a<libs> ()); // Target needs static.
 
       if ((st && (p.is_a<libx> () || p.is_a<libs> ())) ||
           (at && (p.is_a<libx> () || p.is_a<liba> ())))
       {
-        const target* pt (&p.search (t));
+        const target* pt (&search (t, p));
 
         // If this is the lib{}/libu{} group, pick a member which we would
         // link. For libu{} we want the "see through" logic.
@@ -87,7 +67,68 @@ namespace build2
           return pt;
       }
 
-      return file_rule::filter (a, t, p);
+      // The rest of the tests only succeed if the base filter() succeeds.
+      //
+      const target* pt (file_rule::filter (a, t, p));
+      if (pt == nullptr)
+        return pt;
+
+      // Don't install executable's prerequisite headers and module
+      // interfaces.
+      //
+      // Note that if they come from a group, then we assume the entire
+      // group is not to be installed.
+      //
+      if (t.is_a<exe> ())
+      {
+        if (x_header (p))
+          pt = nullptr;
+        else if (p.type.see_through)
+        {
+          for (i.enter_group (); i.group (); )
+          {
+            if (x_header (*++i))
+              pt = nullptr;
+          }
+        }
+
+        if (pt == nullptr)
+          return pt;
+      }
+
+      // Here is a problem: if the user spells the obj*/bmi*{} targets
+      // explicitly, then the source files, including headers/modules may be
+      // specified as preprequisites of those targets and not of this target.
+      // While this can be worked around for headers by also listing them as
+      // prerequisites of this target, this won't work for modules (since they
+      // are compiled). So what we are going to do here is detect bmi*{} and
+      // translate them to their mxx{} (this doesn't quite work for headers
+      // since there would normally be many of them).
+      //
+      // Note: for now we assume bmi*{} never come from see-through groups.
+      //
+      if (p.is_a<bmi> () || p.is_a (compile_types (ot).bmi))
+      {
+        // This is tricky: we need to "look" inside groups for mxx{} but if
+        // found, remap to the group, not member.
+        //
+        for (prerequisite_member pm:
+               group_prerequisite_members (a, *pt, members_mode::maybe))
+        {
+          if (pm.is_a (*x_mod))
+          {
+            pt = t.is_a<exe> ()
+              ? nullptr
+              : file_rule::filter (a, *pt, pm.prerequisite);
+            break;
+          }
+        }
+
+        if (pt == nullptr)
+          return pt;
+      }
+
+      return pt;
     }
 
     bool install_rule::
@@ -214,35 +255,23 @@ namespace build2
         : common (move (d)), link_ (l) {}
 
     const target* libux_install_rule::
-    filter (action a, const target& t, prerequisite_member p) const
+    filter (action a, const target& t, prerequisite_iterator& i) const
     {
+      const prerequisite& p (i->prerequisite);
+
       // The "see through" semantics that should be parallel to install_rule
       // above. In particular, here we use libue/libua/libus{} as proxies for
       // exe/liba/libs{} there.
-
+      //
       otype ot (link_type (t).type);
 
-      if (t.is_a<libue> () && x_header (p))
-        return nullptr;
-
-      if (p.is_a<bmi> () || p.is_a (compile_types (ot).bmi))
-      {
-        const target& mt (p.search (t));
-
-        for (prerequisite_member mp: group_prerequisite_members (a, mt))
-        {
-          if (mp.is_a (*x_mod))
-            return t.is_a<libue> () ? nullptr : alias_rule::filter (a, mt, mp);
-        }
-      }
-
       bool st (t.is_a<libue> () || t.is_a<libus> ()); // Target needs shared.
       bool at (t.is_a<libua> () || t.is_a<libus> ()); // Target needs static.
 
       if ((st && (p.is_a<libx> () || p.is_a<libs> ())) ||
           (at && (p.is_a<libx> () || p.is_a<liba> ())))
       {
-        const target* pt (&p.search (t));
+        const target* pt (&search (t, p));
 
         if (const libx* l = pt->is_a<libx> ())
           pt = &link_member (*l, a, link_info (t.base_scope (), ot));
@@ -254,7 +283,46 @@ namespace build2
           return pt;
       }
 
-      return alias_rule::filter (a, t, p);
+      const target* pt (install::file_rule::instance.filter (a, t, p));
+      if (pt == nullptr)
+        return pt;
+
+      if (t.is_a<libue> ())
+      {
+        if (x_header (p))
+          pt = nullptr;
+        else if (p.type.see_through)
+        {
+          for (i.enter_group (); i.group (); )
+          {
+            if (x_header (*++i))
+              pt = nullptr;
+          }
+        }
+
+        if (pt == nullptr)
+          return pt;
+      }
+
+      if (p.is_a<bmi> () || p.is_a (compile_types (ot).bmi))
+      {
+        for (prerequisite_member pm:
+               group_prerequisite_members (a, *pt, members_mode::maybe))
+        {
+          if (pm.is_a (*x_mod))
+          {
+            pt = t.is_a<libue> ()
+              ? nullptr
+              : install::file_rule::instance.filter (a, *pt, pm.prerequisite);
+            break;
+          }
+        }
+
+        if (pt == nullptr)
+          return pt;
+      }
+
+      return pt;
     }
 
     bool libux_install_rule::
diff --git a/build2/cc/install-rule.hxx b/build2/cc/install-rule.hxx
index ac2f93a..dbbe4e5 100644
--- a/build2/cc/install-rule.hxx
+++ b/build2/cc/install-rule.hxx
@@ -33,7 +33,7 @@ namespace build2
       install_rule (data&&, const link_rule&);
 
       virtual const target*
-      filter (action, const target&, prerequisite_member) const override;
+      filter (action, const target&, prerequisite_iterator&) const override;
 
       virtual bool
       match (action, target&, const string&) const override;
@@ -63,7 +63,7 @@ namespace build2
       libux_install_rule (data&&, const link_rule&);
 
       virtual const target*
-      filter (action, const target&, prerequisite_member) const override;
+      filter (action, const target&, prerequisite_iterator&) const override;
 
       virtual bool
       match (action, target&, const string&) const override;
diff --git a/build2/install/init.cxx b/build2/install/init.cxx
index a11f3e5..259fb07 100644
--- a/build2/install/init.cxx
+++ b/build2/install/init.cxx
@@ -165,6 +165,8 @@ namespace build2
     static const dir_path dir_man  (dir_path (dir_data) /= "man");
     static const dir_path dir_man1 (dir_path ("man") /= "man1");
 
+    static const group_rule group_rule_ (true /* see_through_only */);
+
     bool
     init (scope& rs,
           scope& bs,
@@ -207,18 +209,24 @@ namespace build2
         vp.insert<bool>   ("install.subdirs", variable_visibility::project);
       }
 
-      // Register our alias and file rules.
+      // Register our rules.
       //
       {
+        auto& r (bs.rules);
+
         const auto& ar (alias_rule::instance);
         const auto& fr (file_rule::instance);
+        const auto& gr (group_rule_);
 
-        bs.rules.insert<alias> (perform_install_id,   "install.alias", ar);
-        bs.rules.insert<alias> (perform_uninstall_id, "uninstall.alias", ar);
+        r.insert<alias> (perform_install_id,   "install.alias", ar);
+        r.insert<alias> (perform_uninstall_id, "uninstall.alias", ar);
 
-        bs.rules.insert<file> (perform_install_id,   "install.file", fr);
-        bs.rules.insert<file> (perform_uninstall_id, "uninstall.file", fr);
-      }
+        r.insert<file> (perform_install_id,   "install.file", fr);
+        r.insert<file> (perform_uninstall_id, "uninstall.file", fr);
+
+        r.insert<target> (perform_install_id,   "install.file", gr);
+        r.insert<target> (perform_uninstall_id, "uninstall.file", gr);
+     }
 
       // Configuration.
       //
diff --git a/build2/install/rule.cxx b/build2/install/rule.cxx
index 4d4cb51..2b5d048 100644
--- a/build2/install/rule.cxx
+++ b/build2/install/rule.cxx
@@ -53,9 +53,16 @@ namespace build2
     }
 
     const target* alias_rule::
-    filter (action, const target& t, prerequisite_member p) const
+    filter (action a, const target& t, prerequisite_iterator& i) const
     {
-      return &p.search (t);
+      assert (i->target == nullptr);
+      return filter (a, t, i->prerequisite);
+    }
+
+    const target* alias_rule::
+    filter (action, const target& t, const prerequisite& p) const
+    {
+      return &search (t, p);
     }
 
     recipe alias_rule::
@@ -68,17 +75,24 @@ namespace build2
       // @@ Shouldn't we do match in parallel (here and below)?
       //
       auto& pts (t.prerequisite_targets[a]);
-      for (prerequisite_member p: group_prerequisite_members (a, t))
+
+      auto pms (group_prerequisite_members (a, t, members_mode::never));
+      for (auto i (pms.begin ()), e (pms.end ()); i != e; ++i)
       {
+        const prerequisite& p (i->prerequisite);
+
         // Ignore unresolved targets that are imported from other projects.
         // We are definitely not installing those.
         //
-        if (p.proj ())
+        if (p.proj)
           continue;
 
         // Let a customized rule have its say.
         //
-        const target* pt (filter (a, t, p));
+        // Note: we assume that if the filter enters the group, then it
+        // iterates over all its members.
+        //
+        const target* pt (filter (a, t, i));
         if (pt == nullptr)
         {
           l5 ([&]{trace << "ignoring " << p << " (filtered out)";});
@@ -112,7 +126,14 @@ namespace build2
 
     // group_rule
     //
-    const group_rule group_rule::instance;
+    const group_rule group_rule::instance (false /* see_through_only */);
+
+    bool group_rule::
+    match (action a, target& t, const string& h) const
+    {
+      return (!see_through || t.type ().see_through) &&
+        alias_rule::match (a, t, h);
+    }
 
     const target* group_rule::
     filter (action, const target&, const target& m) const
@@ -142,6 +163,7 @@ namespace build2
       if (gv.members != nullptr)
       {
         auto& pts (t.prerequisite_targets[a]);
+
         for (size_t i (0); i != gv.count; ++i)
         {
           const target* m (gv.members[i]);
@@ -194,9 +216,16 @@ namespace build2
     }
 
     const target* file_rule::
-    filter (action, const target& t, prerequisite_member p) const
+    filter (action a, const target& t, prerequisite_iterator& i) const
+    {
+      assert (i->target == nullptr);
+      return filter (a, t, i->prerequisite);
+    }
+
+    const target* file_rule::
+    filter (action, const target& t, const prerequisite& p) const
     {
-      const target& pt (p.search (t));
+      const target& pt (search (t, p));
       return pt.in (t.root_scope ()) ? &pt : nullptr;
     }
 
@@ -220,21 +249,25 @@ namespace build2
       // In both cases, the next step is to search, match, and collect all the
       // installable prerequisites.
       //
-      // @@ Unconditional group? How does it work for cli? Change to maybe
-      //    same like test? If so, also in alias_rule.
-      //
       auto& pts (t.prerequisite_targets[a]);
-      for (prerequisite_member p: group_prerequisite_members (a, t))
+
+      auto pms (group_prerequisite_members (a, t, members_mode::never));
+      for (auto i (pms.begin ()), e (pms.end ()); i != e; ++i)
       {
+        const prerequisite& p (i->prerequisite);
+
         // Ignore unresolved targets that are imported from other projects.
         // We are definitely not installing those.
         //
-        if (p.proj ())
+        if (p.proj)
           continue;
 
         // Let a customized rule have its say.
         //
-        const target* pt (filter (a, t, p));
+        // Note: we assume that if the filter enters the group, then it
+        // iterates over all its members.
+        //
+        const target* pt (filter (a, t, i));
         if (pt == nullptr)
         {
           l5 ([&]{trace << "ignoring " << p << " (filtered out)";});
diff --git a/build2/install/rule.hxx b/build2/install/rule.hxx
index b9699e3..2ad3d4b 100644
--- a/build2/install/rule.hxx
+++ b/build2/install/rule.hxx
@@ -25,8 +25,17 @@ namespace build2
       // Return NULL if this prerequisite should be ignored and pointer to its
       // target otherwise. The default implementation accepts all prerequsites.
       //
+      // The prerequisite it passed as an iterator allowing the filter to
+      // "see" inside groups.
+      //
+      using prerequisite_iterator =
+        prerequisite_members_range<group_prerequisites>::iterator;
+
+      virtual const target*
+      filter (action, const target&, prerequisite_iterator&) const;
+
       virtual const target*
-      filter (action, const target&, prerequisite_member) const;
+      filter (action, const target&, const prerequisite&) const;
 
       virtual recipe
       apply (action, target&) const override;
@@ -38,10 +47,19 @@ namespace build2
     // In addition to the alias rule's semantics, this rule sees through to
     // the group's members.
     //
+    // The default group_rule::instance matches any target for which it was
+    // registered. It is to be used for non-see-through groups that should
+    // exhibit the see-through behavior for install (see lib{} in the bin
+    // module for an example).
+    //
+    // We also register (for all targets) another instance of this rule that
+    // only matches see-through groups.
+    //
     class group_rule: public alias_rule
     {
     public:
-      using alias_rule::filter; // "Unhide" to make Clang happy.
+      virtual bool
+      match (action, target&, const string&) const override;
 
       // Return NULL if this group member should be ignored and pointer to its
       // target otherwise. The default implementation accepts all members.
@@ -49,11 +67,15 @@ namespace build2
       virtual const target*
       filter (action, const target&, const target& group_member) const;
 
+      using alias_rule::filter; // "Unhide" to make Clang happy.
+
       virtual recipe
       apply (action, target&) const override;
 
-      group_rule () {}
+      group_rule (bool see_through_only): see_through (see_through_only) {}
       static const group_rule instance;
+
+      bool see_through;
     };
 
     struct install_dir;
@@ -68,8 +90,17 @@ namespace build2
       // target otherwise. The default implementation ignores prerequsites
       // that are outside of this target's project.
       //
+      // The prerequisite it passed as an iterator allowing the filter to
+      // "see" inside groups.
+      //
+      using prerequisite_iterator =
+        prerequisite_members_range<group_prerequisites>::iterator;
+
+      virtual const target*
+      filter (action, const target&, prerequisite_iterator&) const;
+
       virtual const target*
-      filter (action, const target&, prerequisite_member) const;
+      filter (action, const target&, const prerequisite&) const;
 
       virtual recipe
       apply (action, target&) const override;
diff --git a/build2/target.hxx b/build2/target.hxx
index 105cc4b..5c8e7bd 100644
--- a/build2/target.hxx
+++ b/build2/target.hxx
@@ -697,8 +697,6 @@ namespace build2
   // also be traversed in reverse, but that's what you usually want,
   // anyway.
   //
-  // For constant iteration use const_group_prerequisites().
-  //
   class group_prerequisites
   {
   public:
@@ -1015,6 +1013,24 @@ namespace build2
       bool
       enter_group ();
 
+      // Return true if the next element is this group's members. Normally
+      // used to iterate over group members only, for example:
+      //
+      // for (...; ++i)
+      // {
+      //   if (i->prerequisite.type.see_through)
+      //   {
+      //     for (i.enter_group (); i.group (); )
+      //     {
+      //       ++i;
+      //       ...
+      //     }
+      //   }
+      // }
+      //
+      bool
+      group () const;
+
       value_type operator* () const
       {
         const target* t (k_ != nullptr ? k_:
diff --git a/build2/target.ixx b/build2/target.ixx
index 16dbf61..242e81d 100644
--- a/build2/target.ixx
+++ b/build2/target.ixx
@@ -212,7 +212,7 @@ namespace build2
 
     if (k_ == nullptr && g_.count != 0) // Iterating over a normal group.
     {
-      if (g_.members == nullptr || // leave_group()
+      if (g_.members == nullptr || // Special case, see leave_group().
           ++j_ > g_.count)
         g_.count = 0;
     }
@@ -248,7 +248,7 @@ namespace build2
     {
       // Otherwise assume it is a normal group.
       //
-      g_ = resolve_group_members (r_->a_, search (*i_));
+      g_ = resolve_group_members (r_->a_, search (r_->t_, *i_));
 
       if (g_.members == nullptr) // Members are not know.
       {
@@ -283,6 +283,16 @@ namespace build2
     }
   }
 
+  template <typename T>
+  inline bool prerequisite_members_range<T>::iterator::
+  group () const
+  {
+    return
+      k_ != nullptr ? k_->member != nullptr                  : /* ad hoc */
+      g_.count != 0 ? g_.members != nullptr && j_ < g_.count : /* normal */
+      false;
+  }
+
   // mtime_target
   //
   inline void mtime_target::
diff --git a/build2/test/rule.cxx b/build2/test/rule.cxx
index bc4060d..7c55445 100644
--- a/build2/test/rule.cxx
+++ b/build2/test/rule.cxx
@@ -56,11 +56,37 @@ namespace build2
       // ---------------------------------------+-------------+---------
       //            test     test     (& pass)  |    pass     |   noop
       //
+      auto& pts (t.prerequisite_targets[a]);
 
-      // If we are passing-through, then match our prerequisites.
+      // Resolve group members.
       //
-      // Note that we may already have stuff in prerequisite_targets (see
-      // group_rule).
+      if (!see_through || t.type ().see_through)
+      {
+        // Remember that we are called twice: first during update for test
+        // (pre-operation) and then during test. During the former, we rely on
+        // the normall update rule to resolve the group members. During the
+        // latter, there will be no rule to do this but the group will already
+        // have been resolved by the pre-operation.
+        //
+        // If the rule could not resolve the group, then we ignore it.
+        //
+        group_view gv (a.outer ()
+                       ? resolve_group_members (a, t)
+                       : t.group_members (a));
+
+        if (gv.members != nullptr)
+        {
+          for (size_t i (0); i != gv.count; ++i)
+          {
+            if (const target* m = gv.members[i])
+              pts.push_back (m);
+          }
+
+          match_members (a, t, pts);
+        }
+      }
+
+      // If we are passing-through, then match our prerequisites.
       //
       if (t.is_a<alias> () && pass (t))
       {
@@ -74,7 +100,6 @@ namespace build2
         match_prerequisites (a, t, t.root_scope ());
       }
 
-      auto& pts (t.prerequisite_targets[a]);
       size_t pass_n (pts.size ()); // Number of pass-through prerequisites.
 
       // See if it's testable and if so, what kind.
@@ -153,8 +178,8 @@ namespace build2
             test = false;
           else
           {
-            // Look for test input/stdin/stdout prerequisites. The same
-            // group logic as in the testscript case above.
+            // Look for test input/stdin/stdout prerequisites. The same group
+            // reasoning as in the testscript case above.
             //
             for (prerequisite_member p:
                    group_prerequisite_members (a, t, members_mode::maybe))
@@ -274,40 +299,6 @@ namespace build2
       }
     }
 
-    recipe group_rule::
-    apply (action a, target& t) const
-    {
-      // Resolve group members.
-      //
-      // Remember that we are called twice: first during update for test
-      // (pre-operation) and then during test. During the former, we rely on
-      // the normall update rule to resolve the group members. During the
-      // latter, there will be no rule to do this but the group will already
-      // have been resolved by the pre-operation.
-      //
-      // If the rule could not resolve the group, then we ignore it.
-      //
-      group_view gv (a.outer ()
-                     ? resolve_group_members (a, t)
-                     : t.group_members (a));
-
-      if (gv.members != nullptr)
-      {
-        auto& pts (t.prerequisite_targets[a]);
-        for (size_t i (0); i != gv.count; ++i)
-        {
-          if (const target* m = gv.members[i])
-            pts.push_back (m);
-        }
-
-        match_members (a, t, pts);
-      }
-
-      // Delegate to the base rule.
-      //
-      return rule::apply (a, t);
-    }
-
     target_state rule::
     perform_update (action a, const target& t)
     {
diff --git a/build2/test/rule.hxx b/build2/test/rule.hxx
index b331263..4f02390 100644
--- a/build2/test/rule.hxx
+++ b/build2/test/rule.hxx
@@ -20,9 +20,6 @@ namespace build2
     class rule: public build2::rule, protected virtual common
     {
     public:
-      explicit
-      rule (common_data&& d): common (move (d)) {}
-
       virtual bool
       match (action, target&, const string&) const override;
 
@@ -37,26 +34,32 @@ namespace build2
 
       target_state
       perform_script (action, const target&, size_t) const;
+
+      rule (common_data&& d, bool see_through_only)
+          : common (move (d)), see_through (see_through_only) {}
+
+      bool see_through;
     };
 
-    class default_rule: public rule // For disambiguation in module.
+    class default_rule: public rule
     {
     public:
       explicit
-      default_rule (common_data&& d): common (move (d)), rule (move (d)) {}
+      default_rule (common_data&& d)
+          : common (move (d)),
+            rule (move (d), true /* see_through_only */) {}
     };
 
-    // In addition to the above rule's semantics, this rule sees through to
-    // the group's members.
+    // To be used for non-see-through groups that should exhibit the see-
+    // through behavior for install (see lib{} in the bin module for an
+    // example).
     //
     class group_rule: public rule
     {
     public:
       explicit
-      group_rule (common_data&& d): common (move (d)), rule (move (d)) {}
-
-      virtual recipe
-      apply (action, target&) const override;
+      group_rule (common_data&& d)
+          : common (move (d)), rule (move (d), false /* see_through_only */) {}
     };
   }
 }
diff --git a/old-tests/cli/simple/build/bootstrap.build b/old-tests/cli/simple/build/bootstrap.build
index 2c116c9..40e30ac 100644
--- a/old-tests/cli/simple/build/bootstrap.build
+++ b/old-tests/cli/simple/build/bootstrap.build
@@ -1,3 +1,6 @@
 project = cli-simple
+version = 1.0.0
 amalgamation = # Disabled.
 using config
+using install
+using test
diff --git a/old-tests/cli/simple/buildfile b/old-tests/cli/simple/buildfile
index ddbf27c..5aa86a9 100644
--- a/old-tests/cli/simple/buildfile
+++ b/old-tests/cli/simple/buildfile
@@ -8,8 +8,10 @@ cxx.poptions += "-I$out_root"
 
 using cli
 
-exe{driver}: cxx{driver} cxx{test}
-cxx{test} hxx{test}: cli{test}
+#exe{driver}: cxx{driver} cxx{test}
+#cxx{test} hxx{test}: cli{test}
 
-#exe{driver}: cxx{driver} cli.cxx{test}
-#cli.cxx{test}: cli{test}
+exe{driver}: cxx{driver} cli.cxx{test}
+#exe{driver}: cxx{driver} lib{test}
+#lib{test}: cli.cxx{test}
+cli.cxx{test}: cli{test}