Do not treat primary ad hoc group member as group for variable lookup

Note that we started with this semantics but it was changed in a commit on
2021-09-16 for reasons not entirely unclear but most likely due to target-
specific variables specified for the group not being set on all the members.
Which we have now addressed (see the previous commit).

Note also that this new (old) semantics is not without its own drawbacks.
Specifically, there is a bit of waste when the target-specific variable is
really only meant for the recipe and thus setting it on all the members is
unnecessary. For example:

<{hxx ixx cxx}{options}>: cli{options}
{
  options = ...
}
{{
   # Use options.
}}

But this feels like a quality of implementation rather than conceptual
issue. For example, we could likely one day address it by synthesizing a
separate group target for ad hoc groups.

3 files changed, 38 insertions, 4 deletions

diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx
index d69ff95..76f7c4c 100644
--- a/libbuild2/algorithm.cxx
+++ b/libbuild2/algorithm.cxx
@@ -1853,6 +1853,7 @@ namespace build2
   {
     using mode = backlink_mode;
 
+    context& ctx (t.ctx);
     const scope& s (t.base_scope ());
 
     backlinks bls;
@@ -1895,11 +1896,12 @@ namespace build2
         // Note that we want to avoid group or tt/patter-spec lookup. And
         // since this is an ad hoc member (which means it was either declared
         // in the buildfile or added by the rule), we assume that the value,
-        // if any, will be set as a rule-specific variable (since setting it
-        // as a target-specific wouldn't be MT-safe). @@ Don't think this
-        // applies to declared ad hoc members.
+        // if any, will be set as a target or rule-specific variable.
         //
-        lookup l (mt->state[a].vars[t.ctx.var_backlink]);
+        lookup l (mt->state[a].vars[ctx.var_backlink]);
+
+        if (!l)
+          l = mt->vars[ctx.var_backlink];
 
         optional<mode> bm (l ? backlink_test (*mt, l) : m);
 
diff --git a/libbuild2/target.cxx b/libbuild2/target.cxx
index 78bc5ac..41a3273 100644
--- a/libbuild2/target.cxx
+++ b/libbuild2/target.cxx
@@ -158,6 +158,11 @@ namespace build2
     {
       ++r.second;
 
+      // While we went back to not treating the first member as a group for
+      // variable lookup, let's keep this logic in case one day we end up with
+      // a separate ad hoc group target.
+      //
+#if 0
       // In case of an ad hoc group, we may have to look in two groups.
       //
       if ((g1 = group) != nullptr)
@@ -175,6 +180,19 @@ namespace build2
           }
         }
       }
+#else
+      // Skip looking up in the ad hoc group, which is semantically the
+      // first/primary member.
+      //
+      if ((g1 = group == nullptr
+           ? nullptr
+           : group->adhoc_group () ? group->group : group))
+      {
+        auto p (g1->vars.lookup (var));
+        if (p.first != nullptr)
+          r.first = lookup_type (*p.first, p.second, g1->vars);
+      }
+#endif
     }
 
     // Delegate to scope's lookup_original().
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index 038552f..44e8538 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -408,6 +408,10 @@ namespace build2
     // - Ad hoc group cannot have sub-groups (of any kind) though an ad hoc
     //   group can be a sub-group of an explicit group.
     //
+    // - Member variable lookup skips the ad hoc group (since the group is the
+    //   first member, this is normally what we want). But special semantics
+    //   could be arranged; see var_backlink, for example.
+    //
     // Note that ad hoc groups can be part of explicit groups. In a sense, we
     // have a two-level grouping: an explicit group with its members each of
     // which can be an ad hoc group. For example, lib{} contains libs{} which
@@ -416,6 +420,16 @@ namespace build2
     // Use add_adhoc_member(), find_adhoc_member() from algorithms to manage
     // ad hoc members.
     //
+    // One conceptual issue we have with our ad hoc group implementation is
+    // that the behavior could be sensitive to the order in which the members
+    // are specified (due to the primary member logic). For example, unless we
+    // specify the header in the header/source group first, it will not be
+    // installed. Perhaps the solution is to synthesize a separate group
+    // target for the ad hoc members (with a special target type that rules
+    // like install could recognize). See also the variable lookup semantics.
+    // We could also probably support see_through via an attribute or some
+    // such.
+    //
     const_ptr<target> adhoc_member = nullptr;
 
     // Return true if this target is an ad hoc group (that is, its primary