WIP: update documentation, deal with ad hoc group members

2 files changed, 97 insertions, 37 deletions

diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx
index bd89fc5..5b543cf 100644
--- a/libbuild2/algorithm.cxx
+++ b/libbuild2/algorithm.cxx
@@ -1111,10 +1111,6 @@ namespace build2
       //
       if (t.adhoc_group_member ())
       {
-        assert (!step);
-
-        const target& g (*t.group);
-
         // It feels natural to "convert" this call to the one for the group,
         // including the try_match part. Semantically, we want to achieve the
         // following:
@@ -1122,16 +1118,26 @@ namespace build2
         // [try_]match (a, g);
         // match_recipe (l, group_recipe);
         //
+        // Currently, ad hoc group members cannot have options. An alternative
+        // semantics could be to call the goup's rule to translate member
+        // options to group options and then (re)match the group with that.
+        // The implementation of this semantics could look like this:
+        //
+        // 1. Lock the group.
+        // 2. If not already offset_matched, do one step to get the rule.
+        // 3. Call the rule to translate options.
+        // 4. Continue matching the group passing the translated options.
+        // 5. Keep track of member options in member's cur_options to handle
+        //    member rematches (if already offset_{applied,executed}).
+
+        assert (!step && options == match_extra::all_options);
+
+        const target& g (*t.group);
+
         // What should we do with options? After some rumination it fells most
         // natural to treat options for the group and for its ad hoc member as
         // the same entity ... or not.
         //
-        // @@ But we cannot reapply them if options change since there is
-        //    no rule! Feels easiest to just assume no options for now? Or
-        //    member options are group options (won't then still need
-        //    reapply() support via member). Need to keep KISS for now so
-        //    that don't need to deal with _applied/_executed!
-        //
         auto df = make_diag_frame (
           [a, &t](const diag_record& dr)
           {
@@ -1139,15 +1145,17 @@ namespace build2
               dr << info << "while matching group rule to " << diag_do (a, t);
           });
 
-        // @@ TODO: unclear what to do about options here.
-        // @@ TODO: this could also be _applied/_executed, theoretically.
-
         pair<bool, target_state> r (match_impl (a, g, 0, nullptr, try_match));
 
         if (r.first)
         {
           if (r.second != target_state::failed)
           {
+            // Note: in particular, this makes sure we will never re-lock this
+            // member if already applied/executed.
+            //
+            s.match_extra.cur_options = match_extra::all_options;
+
             match_inc_dependents (a, g);
             match_recipe (l, group_recipe);
 
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index 32f717f..fc733b6 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -180,30 +180,82 @@ namespace build2
     bool locked;   // Normally true (see adhoc_rule::match() for background).
     bool fallback; // True if matching a fallback rule (see match_rule()).
 
-    // Match options.
-    //
-    // On initial match()/apply(), cur_options is initialized to ~0 (all
-    // options enabled) and the matching rule is expected to override it with
-    // new_options in apply() (note that match() should no base any decisions
-    // on new_options since they may change between match() and apply()). This
-    // way a rule that does not support any match options does not need to do
-    // anything. On rematch in the reapply() call, cur_options are the
-    // currently enabled options and new_options are the newly requested
-    // options. Here the rule is expected to factor new_options to cur_options
-    // as appropriate. Note also that on rematch, if current options already
-    // include new options, then no call to reapply() is made. This, in
-    // particular, means that a rule that does not adjust cur_options in
-    // match() will never get a reapply() call (because all the options are
-    // enabled from the start).
-    //
-    // Note: options are currently not supported in ad hoc recipes/rules.
-    //
-    // @@ We could use 0 new_options (which otherwise don't make sense) for
-    //    match for the purpose of resolving members?
-    //
-    // @@ TODO: clear already enabled options from new_options on rematch.
-    //
-    // @@ TODO doc
+    // When matching a rule, the caller may wish to request a subset of the
+    // full functionality of performing the operation on the target. This is
+    // achieved with match options.
+    //
+    // Since the match caller normally has no control over which rule will be
+    // matched, the options are not specific to a particular rule. Rather,
+    // options are defined for performing a specific operation on a specific
+    // target type and would normally be part of the target type semantics.
+    // To put it another way, when a rule matches a target of certain type for
+    // certain operation, there is an expectaion of certain semantics, some
+    // parts of which could be made optional.
+    //
+    // As a concrete example, consider installing libs{}, which traditionally
+    // has two parts: runtime (normally just the versioned shared library) and
+    // build-time (non-versioned symlinks, pkg-config files, headers, etc).
+    // The option to install only the runtime files is part of the bin::libs{}
+    // semantics, not of, say, cc::install_rule.
+    //
+    // The match options are specified as a uint64_t mask, which means there
+    // can be a maximum of 64 options per operation/target type. Options are
+    // opt-out rather than opt-in. That is, by default, all the options are
+    // enabled unless the match caller explicitly opted out of some
+    // functionality. Even if the caller opted out, there is no guarantee that
+    // the matching rule will honor this request (for example, because it is a
+    // user-provided ad hoc recipe). To put it another way, support for
+    // options is a quality of implementation matter.
+    //
+    // From the rule implementation's point view, match options are handled as
+    // follows: On initial match()/apply(), cur_options is initialized to ~0
+    // (all options enabled) and the matching rule is expected to override it
+    // with new_options in apply() (note that match() should no base any
+    // decisions on new_options since they may change between match() and
+    // apply()). This way a rule that does not support any match options does
+    // not need to do anything. Subsequent match calls may add new options
+    // which causes a rematch that manifests in the rule's reapply() call. In
+    // reapply(), cur_options are the currently enabled options and
+    // new_options are the newly requested options. Here the rule is expected
+    // to factor new_options to cur_options as appropriate. Note also that on
+    // rematch, if current options already include new options, then no call
+    // to reapply() is made. This, in particular, means that a rule that does
+    // not adjust cur_options in match() will never get a reapply() call
+    // (because all the options are enabled from the start). If a rematch is
+    // triggered after the rule has already been executed, an error is issued.
+    // This means that match options are not usable for operation/target types
+    // that could plausibly be executed during match. In particular, using
+    // match options for update and clean operations is a bad idea (update of
+    // pretty much any target can happen during match as a result of a tool
+    // update while clean might have to be performed during match to provide
+    // the mirro semantics).
+    //
+    // A rule that supports match options must also be prepared to handle the
+    // apply() call with new_options set to 0, for example, by using a
+    // minimally supported set of options instead. While 0 usually won't be
+    // passed by the match caller, this value is passed in the following
+    // circumstances:
+    //
+    //   - match to resolve group (resolve_group())
+    //   - match to resolve members (resolve_members())
+    //   - match of ad hoc group via one of its ad hoc members
+    //
+    // When it comes to match options specified for group members, the
+    // semantics differs between explicit and ad hoc groups. For explicit
+    // groups, the standard semantics described above applies and the group's
+    // reapply() function will be called both for the group itself as well as
+    // for its members and its the responsibility of the rule to decide what
+    // to do with the two sets of options (e.g., factor member's options into
+    // group's options, etc). For ad hoc groups, members are not matched to a
+    // rule but to the group_recipe directly (so there cannot be a call to
+    // reapply()). Currently, ad hoc group members cannot have options (more
+    // precisely, their options should always be ~0). An alternative semantics
+    // where the group rule is called to translate member options to group
+    // options may be implemented in the future (see match_impl_impl() for
+    // details).
+    //
+    // Note: match options are currently not exposed in Buildscript ad hoc
+    // recipes/rules (but are in C++).
     //
     uint64_t cur_options;
     uint64_t new_options;