From f50a3a56b59698ffce3965711898a94e7849aa78 Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Fri, 18 Nov 2022 07:00:36 +0200
Subject: Complete low verbosity diagnostics rework

---
 libbuild2/adhoc-rule-buildscript.cxx   | 324 +++++++++++++++++++++++++++++++--
 libbuild2/adhoc-rule-buildscript.hxx   |   3 +
 libbuild2/adhoc-rule-cxx.cxx           |   2 +-
 libbuild2/algorithm.cxx                |  53 ++++--
 libbuild2/algorithm.hxx                |  46 +++--
 libbuild2/build/script/parser.cxx      |   6 +-
 libbuild2/build/script/parser.hxx      |  15 +-
 libbuild2/build/script/parser.test.cxx |   2 +-
 libbuild2/diagnostics.cxx              | 227 ++++++++++++++++++++++-
 libbuild2/diagnostics.hxx              |  50 ++++-
 libbuild2/diagnostics.ixx              |  19 ++
 libbuild2/filesystem.cxx               |  66 +++----
 libbuild2/filesystem.txx               |  22 ++-
 libbuild2/target-key.hxx               |  17 +-
 libbuild2/target-type.hxx              |   4 +-
 libbuild2/target.cxx                   |  90 +++++----
 libbuild2/target.hxx                   |   8 +-
 17 files changed, 801 insertions(+), 153 deletions(-)

(limited to 'libbuild2')

diff --git a/libbuild2/adhoc-rule-buildscript.cxx b/libbuild2/adhoc-rule-buildscript.cxx
index e4960c9..bd61a23 100644
--- a/libbuild2/adhoc-rule-buildscript.cxx
+++ b/libbuild2/adhoc-rule-buildscript.cxx
@@ -1473,7 +1473,7 @@ namespace build2
   //
   bool adhoc_buildscript_rule::
   execute_update_file (const scope& bs,
-                       action, const file& t,
+                       action a, const file& t,
                        build::script::environment& env,
                        build::script::default_runner& run,
                        bool deferred_failure) const
@@ -1496,16 +1496,46 @@ namespace build2
     {
       if (verb == 1)
       {
-        // @@ DIAG (and in default_action() below):
+        // By default we print the first non-ad hoc prerequisite target as the
+        // "main" prerequisite, unless there isn't any or it's not file-based,
+        // in which case we fallback to the second form without the
+        // prerequisite. Potential future improvements:
         //
-        // - we are printing target, not source (like in most other places)
+        // - Somehow detect that the first prerequisite target is a tool being
+        //   executed and fallback to the second form. It's tempting to just
+        //   exclude all exe{} targets, but this could be a rule for something
+        //   like strip.
         //
-        // - printing of ad hoc target group (the {hxx cxx}{foo} idea)
-        //
-        // - if we are printing prerequisites, should we print all of them
-        //   (including tools)?
-        //
-        text << *script.diag_name << ' ' << t;
+        const file* pt (nullptr);
+        for (const prerequisite_target& p: t.prerequisite_targets[a])
+        {
+          // See execute_update_prerequisites().
+          //
+          if (p.target != nullptr && !p.adhoc ())
+          {
+            pt = p.target->is_a<file> ();
+            break;
+          }
+        }
+
+        if (t.adhoc_member == nullptr)
+        {
+          if (pt != nullptr)
+            print_diag (script.diag_name->c_str (), *pt, t);
+          else
+            print_diag (script.diag_name->c_str (), t);
+        }
+        else
+        {
+          vector<target_key> ts;
+          for (const target* m (&t); m != nullptr; m = m->adhoc_member)
+            ts.push_back (m->key ());
+
+          if (pt != nullptr)
+            print_diag (script.diag_name->c_str (), pt->key (), move (ts));
+          else
+            print_diag (script.diag_name->c_str (), move (ts));
+        }
       }
     }
     else if (exec_diag)
@@ -1513,7 +1543,7 @@ namespace build2
       if (script.diag_preamble_temp_dir && !script.depdb_preamble_temp_dir)
         env.set_temp_dir_variable ();
 
-      names diag (
+      pair<names, location> diag (
         p.execute_diag_preamble (rs, bs,
                                  env, script, run,
                                  verb == 1   /* diag */,
@@ -1521,7 +1551,7 @@ namespace build2
                                  false       /* leave */));
 
       if (verb == 1)
-        text << diag; // @@ DIAG
+        print_custom_diag (bs, move (diag.first), diag.second);
     }
 
     if (exec_body)
@@ -1594,7 +1624,14 @@ namespace build2
     // temporary directory ($~) is that it's next to other output which makes
     // it easier to examine during recipe troubleshooting.
     //
-    return perform_clean_extra (a, t.as<file> (), {".d", ".t"});
+    // Finally, we print the entire ad hoc group at verbosity level 1, similar
+    // to the default update diagnostics.
+    //
+    return perform_clean_extra (a,
+                                t.as<file> (),
+                                {".d", ".t"},
+                                {},
+                                true /* show_adhoc_members */);
   }
 
   target_state adhoc_buildscript_rule::
@@ -1625,9 +1662,20 @@ namespace build2
       {
         if (verb == 1)
         {
-          // @@ DIAG: as above (execute_update_file()).
+          // For operations other than update (as well as for non-file
+          // targets), we default to the second form (without the
+          // prerequisite). Think test.
           //
-          text << *script.diag_name << ' ' << t;
+          if (t.adhoc_member == nullptr)
+            print_diag (script.diag_name->c_str (), t);
+          else
+          {
+            vector<target_key> ts;
+            for (const target* m (&t); m != nullptr; m = m->adhoc_member)
+              ts.push_back (m->key ());
+
+            print_diag (script.diag_name->c_str (), move (ts));
+          }
         }
       }
       else if (exec_diag)
@@ -1635,7 +1683,7 @@ namespace build2
         if (script.diag_preamble_temp_dir)
           e.set_temp_dir_variable ();
 
-        names diag (
+        pair<names, location> diag (
           p.execute_diag_preamble (rs, bs,
                                    e, script, r,
                                    verb == 1  /* diag */,
@@ -1643,7 +1691,7 @@ namespace build2
                                    !exec_body /* leave */));
 
         if (verb == 1)
-          text << diag; // @@ DIAG
+          print_custom_diag (bs, move (diag.first), diag.second);
       }
 
       if (exec_body)
@@ -1657,4 +1705,248 @@ namespace build2
 
     return target_state::changed;
   }
+
+  void adhoc_buildscript_rule::
+  print_custom_diag (const scope& bs, names&& ns, const location& l) const
+  {
+    // The straightforward thing to do would be to just print the diagnostics
+    // as specified by the user. But that will make some of the tidying up
+    // done by print_diag() unavailable to custom diagnostics. Things like
+    // omitting the out-qualification as well as compact printing of the
+    // groups. Also, in the future we may want to support colorization of the
+    // diagnostics, which will be difficult to achive with such a "just print"
+    // approach.
+    //
+    // So instead we are going to parse the custom diagnostics, translate
+    // names back to targets (where appropriate), and call one of the
+    // print_diag() functions. Specifically, we expect the custom diagnostics
+    // to be in one of the following two forms (which correspond to the two
+    // forms of pring_diag()):
+    //
+    // diag <prog> <l-target> <comb> <r-target>...
+    // diag <prog> <r-target>...
+    //
+    // And the way we are going to disambiguate this is by analyzing name
+    // types. Specifically, we expect <comb> to be a simple name that also
+    // does not contain any directory separators (so we can distinguish it
+    // from both target names as well as paths, which can be specified on
+    // either side).  We will also recognize `-` as the special stdout path
+    // name (so <comb> cannot be `-`). Finally, <l-target> (but not
+    // <r-target>) can be a string (e.g., an argument) but that should not
+    // pose an ambiguity.
+    //
+    // With this approach, the way to re-create the default diagnostics would
+    // be:
+    //
+    // diag <prog> ($>[0]) -> $<
+    // diag <prog> $<
+    //
+    auto i (ns.begin ()), e (ns.end ());
+
+    // <prog>
+    //
+    if (i == e)
+      fail (l) << "missing program name in diag builtin";
+
+    if (!i->simple () || i->empty ())
+      fail (l) << "expected simple name as program name in diag builtin";
+
+    const char* prog (i->value.c_str ());
+    ++i;
+
+    // <l-target>
+    //
+    const target* l_t (nullptr);
+    path l_p;
+    string l_s;
+
+    auto parse_target = [&bs, &l, &i, &e] () -> const target&
+    {
+      name& n (*i++);
+      name  o;
+
+      if (n.pair)
+      {
+        if (i == e)
+          fail (l) << "invalid target name pair in diag builtin";
+
+        o = move (*i++);
+      }
+
+      // Similar to to_target() in $target.*().
+      //
+      if (const target* r = search_existing (n, bs, o.dir))
+        return *r;
+
+      fail (l) << "target "
+               << (n.pair ? names {move (n), move (o)} : names {move (n)})
+               << " not found in diag builtin" << endf;
+    };
+
+    auto parse_first = [&l, &i, &e,
+                        &parse_target] (const target*& t, path& p, string& s,
+                                        const char* after)
+    {
+      if (i == e)
+        fail (l) << "missing target after " << after << " in diag builtin";
+
+      try
+      {
+        if (i->typed ())
+        {
+          t = &parse_target ();
+          return; // i is already incremented.
+        }
+        else if (!i->dir.empty ())
+        {
+          p = move (i->dir);
+          p /= i->value;
+        }
+        else if (path_traits::find_separator (i->value) != string::npos)
+        {
+          p = path (move (i->value));
+        }
+        else if (!i->value.empty ())
+        {
+          s = move (i->value);
+        }
+        else
+          fail (l) << "expected target, path, or argument after "
+                   << after << " in diag builtin";
+      }
+      catch (const invalid_path& e)
+      {
+        fail (l) << "invalid path '" << e.path <<  "' after "
+                 << after << " in diag builtin";
+      }
+
+      ++i;
+    };
+
+    parse_first (l_t, l_p, l_s, "program name");
+
+    // Now detect which form it is.
+    //
+    if (i != e &&
+        i->simple () &&
+        !i->empty () &&
+        path_traits::find_separator (i->value) == string::npos)
+    {
+      // The first form.
+
+      // <comb>
+      //
+      const char* comb (i->value.c_str ());
+      ++i;
+
+      // <r-target>
+      //
+      const target* r_t (nullptr);
+      path r_p;
+      string r_s;
+
+      parse_first (r_t, r_p, r_s, "combiner");
+
+      path_name r_pn;
+
+      if (r_t != nullptr)
+        ;
+      else if (!r_p.empty ())
+        r_pn = path_name (&r_p);
+      else
+      {
+        if (r_s != "-")
+          fail (l) << "expected target or path instead of '" << r_s
+                   << "' after combiner in diag builtin";
+
+        r_pn = path_name (move (r_s));
+      }
+
+      if (i == e)
+      {
+        if (r_t != nullptr)
+        {
+          if      (l_t != nullptr) print_diag (prog, *l_t, *r_t, comb);
+          else if (!l_p.empty ())  print_diag (prog,  l_p, *r_t, comb);
+          else                     print_diag (prog,  l_s, *r_t, comb);
+        }
+        else
+        {
+          if      (l_t != nullptr) print_diag (prog, *l_t, r_pn, comb);
+          else if (!l_p.empty ())  print_diag (prog,  l_p, r_pn, comb);
+          else                     print_diag (prog,  l_s, r_pn, comb);
+        }
+
+        return;
+      }
+
+      // We can only have multiple targets, not paths.
+      //
+      if (r_t == nullptr)
+        fail (l) << "unexpected name after path in diag builtin";
+
+      // <r-target>...
+      //
+      vector<target_key> r_ts {r_t->key ()};
+
+      do r_ts.push_back (parse_target ().key ()); while (i != e);
+
+      if      (l_t != nullptr) print_diag (prog,  l_t->key (), move (r_ts), comb);
+      else if (!l_p.empty ())  print_diag (prog,  l_p,         move (r_ts), comb);
+      else                     print_diag (prog,  l_s,         move (r_ts), comb);
+    }
+    else
+    {
+      // The second form.
+
+      // First "absorb" the l_* values as the first <r-target>.
+      //
+      const target* r_t (nullptr);
+      path_name r_pn;
+
+      if (l_t != nullptr)
+        r_t = l_t;
+      else if (!l_p.empty ())
+        r_pn = path_name (&l_p);
+      else
+      {
+        if (l_s != "-")
+        {
+          diag_record dr (fail (l));
+
+          dr << "expected target or path instead of '" << l_s
+             << "' after program name in diag builtin";
+
+          if (i != e)
+            dr << info << "alternatively, missing combiner after '"
+               << l_s << "'";
+        }
+
+        r_pn = path_name (move (l_s));
+      }
+
+      if (i == e)
+      {
+        if (r_t != nullptr)
+          print_diag (prog, *r_t);
+        else
+          print_diag (prog, r_pn);
+
+        return;
+      }
+
+      // We can only have multiple targets, not paths.
+      //
+      if (r_t == nullptr)
+        fail (l) << "unexpected name after path in diag builtin";
+
+      // <r-target>...
+      //
+      vector<target_key> r_ts {r_t->key ()};
+
+      do r_ts.push_back (parse_target ().key ()); while (i != e);
+
+      print_diag (prog, move (r_ts));
+    }
+  }
 }
diff --git a/libbuild2/adhoc-rule-buildscript.hxx b/libbuild2/adhoc-rule-buildscript.hxx
index 2334cdd..02939c1 100644
--- a/libbuild2/adhoc-rule-buildscript.hxx
+++ b/libbuild2/adhoc-rule-buildscript.hxx
@@ -79,6 +79,9 @@ namespace build2
     virtual void
     dump_text (ostream&, string&) const override;
 
+    void
+    print_custom_diag (const scope&, names&&, const location&) const;
+
   public:
     using script_type = build::script::script;
 
diff --git a/libbuild2/adhoc-rule-cxx.cxx b/libbuild2/adhoc-rule-cxx.cxx
index 5c08a09..fbe967e 100644
--- a/libbuild2/adhoc-rule-cxx.cxx
+++ b/libbuild2/adhoc-rule-cxx.cxx
@@ -304,7 +304,7 @@ namespace build2
       context& ctx (*t.ctx.module_context);
       scheduler::phase_guard pg (ctx.sched);
 
-      const uint16_t verbosity (3); // Project creation command verbosity.
+      uint16_t verbosity (3); // Project creation command verbosity.
 
       // Project and location signatures.
       //
diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx
index 045024b..889b4e9 100644
--- a/libbuild2/algorithm.cxx
+++ b/libbuild2/algorithm.cxx
@@ -3038,7 +3038,8 @@ namespace build2
   target_state
   perform_clean_extra (action a, const file& ft,
                        const clean_extras& extras,
-                       const clean_adhoc_extras& adhoc_extras)
+                       const clean_adhoc_extras& adhoc_extras,
+                       bool show_adhoc)
   {
     context& ctx (ft.ctx);
 
@@ -3070,6 +3071,12 @@ namespace build2
 
     // Now clean the ad hoc group file members, if any.
     //
+    // While at it, also collect the group target keys if we are showing
+    // the members. But only those that exist (since we don't want to
+    // print any diagnostics if none of them exist).
+    //
+    vector<target_key> tks;
+
     for (const target* m (ft.adhoc_member);
          m != nullptr;
          m = m->adhoc_member)
@@ -3110,21 +3117,38 @@ namespace build2
                         ? target_state::changed
                         : target_state::unchanged);
 
-        if (r == target_state::changed && ep.empty ())
-          ep = *mp;
-
-        er |= r;
+        if (r == target_state::changed)
+        {
+          if (show_adhoc && verb == 1)
+            tks.push_back (mf->key ());
+          else if (ep.empty ())
+          {
+            ep = *mp;
+            er |= r;
+          }
+        }
       }
     }
 
     // Now clean the primary target and its prerequisited in the reverse order
     // of update: first remove the file, then clean the prerequisites.
     //
-    // @@ DIAG: we print removal of individual ad hoc members above instead
-    //          of as group at once ({hxx, cxx}{...}).
-    //
-    if (clean && !fp.empty () && rmfile (fp, ft))
-      tr = target_state::changed;
+    if (clean && !fp.empty ())
+    {
+      if (show_adhoc && verb == 1 && !tks.empty ())
+      {
+        if (rmfile (fp, ft, 2 /* verbosity */))
+          tks.insert (tks.begin (), ft.key ());
+
+        print_diag ("rm", move (tks));
+        tr = target_state::changed;
+      }
+      else
+      {
+        if (rmfile (fp, ft))
+          tr = target_state::changed;
+      }
+    }
 
     // Update timestamp in case there are operations after us that could use
     // the information.
@@ -3190,12 +3214,17 @@ namespace build2
       {
         if (const target* m = gv.members[gv.count - 1])
         {
-          // @@ DIAG: do we want to show removal of group or each member?
+          // Note that at the verbosity level 1 we don't show the removal of
+          // each group member. This is consistent with what is normally shown
+          // during update.
           //
-          if (rmfile (m->as<file> ().path (), *m))
+          if (rmfile (m->as<file> ().path (), *m, 2 /* verbosity */))
             tr |= target_state::changed;
         }
       }
+
+      if (tr == target_state::changed && verb == 1)
+        print_diag ("rm", g);
     }
 
     g.mtime (timestamp_nonexistent);
diff --git a/libbuild2/algorithm.hxx b/libbuild2/algorithm.hxx
index e9245f4..aae59ca 100644
--- a/libbuild2/algorithm.hxx
+++ b/libbuild2/algorithm.hxx
@@ -845,8 +845,8 @@ namespace build2
   LIBBUILD2_SYMEXPORT target_state
   group_action (action, const target&);
 
-  // Standard perform(clean) action implementation for the file target
-  // (or derived).
+  // Standard perform(clean) action implementation for the file target (or
+  // derived). Note: also cleans ad hoc group members, if any.
   //
   LIBBUILD2_SYMEXPORT target_state
   perform_clean (action, const target&);
@@ -856,8 +856,8 @@ namespace build2
   LIBBUILD2_SYMEXPORT target_state
   perform_clean_depdb (action, const target&);
 
-  // As above but clean the target group. The group should be an mtime_target
-  // and members should be files.
+  // As above but clean the (non-ad hoc) target group. The group should be an
+  // mtime_target and members should be files.
   //
   LIBBUILD2_SYMEXPORT target_state
   perform_clean_group (action, const target&);
@@ -868,21 +868,22 @@ namespace build2
   LIBBUILD2_SYMEXPORT target_state
   perform_clean_group_depdb (action, const target&);
 
-  // Helper for custom perform(clean) implementations that cleans extra files
-  // and directories (recursively) specified as a list of either absolute
-  // paths or "path derivation directives". The directive string can be NULL,
-  // or empty in which case it is ignored. If the last character in a
-  // directive is '/', then the resulting path is treated as a directory
-  // rather than a file. The directive can start with zero or more '-'
-  // characters which indicate the number of extensions that should be
-  // stripped before the new extension (if any) is added (so if you want to
-  // strip the extension, specify just "-"). For example:
+  // Helpers for custom perform(clean) implementations that, besides the
+  // target and group members, can also clean extra files and directories
+  // (recursively) specified as a list of either absolute paths or "path
+  // derivation directives". The directive string can be NULL, or empty in
+  // which case it is ignored. If the last character in a directive is '/',
+  // then the resulting path is treated as a directory rather than a file. The
+  // directive can start with zero or more '-' characters which indicate the
+  // number of extensions that should be stripped before the new extension (if
+  // any) is added (so if you want to strip the extension, specify just
+  // "-"). For example:
   //
   // perform_clean_extra (a, t, {".d", ".dlls/", "-.dll"});
   //
   // The extra files/directories are removed first in the specified order
-  // followed by the ad hoc group member, then target itself, and, finally,
-  // the prerequisites in the reverse order.
+  // followed by the group member, then target itself, and, finally, the
+  // prerequisites in the reverse order.
   //
   // You can also clean extra files derived from ad hoc group members that are
   // "indexed" using their target types (see add/find_adhoc_member() for
@@ -901,16 +902,25 @@ namespace build2
 
   using clean_adhoc_extras = small_vector<clean_adhoc_extra, 2>;
 
+  // If show_adhoc_members is true, then print the entire ad hoc group instead
+  // of just the primary member at verbosity level 1 (see print_diag() for
+  // details). Note that the default is false because normally a rule
+  // implemented in C++ would only use an ad hoc group for subordiate members
+  // (.pdb, etc) and would use a dedicate target group type if the members
+  // are equal.
+  //
   LIBBUILD2_SYMEXPORT target_state
   perform_clean_extra (action, const file&,
                        const clean_extras&,
-                       const clean_adhoc_extras& = {});
+                       const clean_adhoc_extras& = {},
+                       bool show_adhoc_members = false);
 
   inline target_state
   perform_clean_extra (action a, const file& f,
-                       initializer_list<const char*> e)
+                       initializer_list<const char*> e,
+                       bool show_adhoc_members = false)
   {
-    return perform_clean_extra (a, f, clean_extras (e));
+    return perform_clean_extra (a, f, clean_extras (e), {}, show_adhoc_members);
   }
 
   // Similar to perform_clean_group() but with extras similar to
diff --git a/libbuild2/build/script/parser.cxx b/libbuild2/build/script/parser.cxx
index b406f12..5dd9179 100644
--- a/libbuild2/build/script/parser.cxx
+++ b/libbuild2/build/script/parser.cxx
@@ -1429,12 +1429,12 @@ namespace build2
         exec_lines (begin, end, exec_cmd);
       }
 
-      names parser::
+      pair<names, location> parser::
       execute_diag_preamble (const scope& rs, const scope& bs,
                              environment& e, const script& s, runner& r,
                              bool diag, bool enter, bool leave)
       {
-        tracer trace ("exec_diag_preamble");
+        tracer trace ("execute_diag_preamble");
 
         assert (!s.diag_preamble.empty ());
 
@@ -1499,7 +1499,7 @@ namespace build2
         if (leave)
           runner_->leave (e, s.end_loc);
 
-        return ns;
+        return make_pair (ns, dl.tokens.front ().location ());
       }
 
       void parser::
diff --git a/libbuild2/build/script/parser.hxx b/libbuild2/build/script/parser.hxx
index 1328bae..3121320 100644
--- a/libbuild2/build/script/parser.hxx
+++ b/libbuild2/build/script/parser.hxx
@@ -174,17 +174,18 @@ namespace build2
         }
 
         // If the diag argument is true, then execute the preamble including
-        // the (trailing) diagnostics line and return the resulting names (see
-        // exec_special() for the diagnostics line execution semantics).
-        // Otherwise, execute the preamble excluding the diagnostics line and
-        // return an empty names list. If requested, call the runner's enter()
-        // and leave() functions that initialize/clean up the environment
-        // before/after the preamble execution.
+        // the (trailing) diagnostics line and return the resulting names and
+        // its location (see exec_special() for the diagnostics line execution
+        // semantics). Otherwise, execute the preamble excluding the
+        // diagnostics line and return an empty names list and location. If
+        // requested, call the runner's enter() and leave() functions that
+        // initialize/clean up the environment before/after the preamble
+        // execution.
         //
         // Note: having both root and base scopes for testing (where we pass
         // global scope for both).
         //
-        names
+        pair<names, location>
         execute_diag_preamble (const scope& root, const scope& base,
                                environment&, const script&, runner&,
                                bool diag, bool enter, bool leave);
diff --git a/libbuild2/build/script/parser.test.cxx b/libbuild2/build/script/parser.test.cxx
index e1ee3ef..97eac22 100644
--- a/libbuild2/build/script/parser.test.cxx
+++ b/libbuild2/build/script/parser.test.cxx
@@ -323,7 +323,7 @@ namespace build2
                                                        e, s, r,
                                                        true /* diag */,
                                                        true /* enter */,
-                                                       true /* leave */));
+                                                       true /* leave */).first);
 
                   cout << "diag: " << diag << endl;
                 }
diff --git a/libbuild2/diagnostics.cxx b/libbuild2/diagnostics.cxx
index fda24f7..3246cae 100644
--- a/libbuild2/diagnostics.cxx
+++ b/libbuild2/diagnostics.cxx
@@ -3,7 +3,7 @@
 
 #include <libbuild2/diagnostics.hxx>
 
-#include <cstring>  // strchr(), memcpy()
+#include <cstring> // strchr(), memcpy()
 
 #include <libbutl/process-io.hxx>
 
@@ -47,7 +47,7 @@ namespace build2
   //
   const int stream_verb_index = ostream::xalloc ();
 
-  // print_{do,undo}()
+  // print_diag()
   //
   void
   print_diag_impl (const char* p, target_key* l, target_key&& r, const char* c)
@@ -82,6 +82,191 @@ namespace build2
     dr << r;
   }
 
+  template <typename L> // L can be target_key, path, or string.
+  static void
+  print_diag_impl (const char* p,
+                   const L* l, bool lempty,
+                   vector<target_key>&& rs,
+                   const char* c)
+  {
+    assert (rs.size () > 1);
+
+    // The overall plan is as follows:
+    //
+    // 1. Collect the printed names for all the group members.
+    //
+    //    Note if the printed representation is irregular (see
+    //    to_stream(target_key) for details). We will print such members each
+    //    on a separate line.
+    //
+    // 2. Move the names so that we end up with contiguous partitions of
+    //    targets with the same name.
+    //
+    // 3. Print the partitions, one per line.
+    //
+    vector<pair<optional<string>, const target_key*>> ns;
+    ns.reserve (rs.size ());
+
+    // Use the diag_record's ostringstream so that we get the appropriate
+    // stream verbosity, etc.
+    //
+    diag_record dr (text);
+    ostringstream& os (dr.os);
+    stream_verbosity sv (stream_verb (os));
+
+    for (const target_key& k: rs)
+    {
+      bool r;
+      if (auto p = k.type->print)
+        r = p (os, k, true /* name_only */);
+      else
+        r = to_stream (os, k, sv, true /* name_only */);
+
+      ns.push_back (make_pair (r ? optional<string> (os.str ()) : nullopt, &k));
+
+      os.clear ();
+      os.str (string ()); // Note: just seekp(0) is not enough.
+    }
+
+    // Partition.
+    //
+    auto cmp = [] (const pair<optional<string>, const target_key*>& x,
+                   const pair<optional<string>, const target_key*>& y)
+    {
+      return (x.second->dir->compare (*y.second->dir) == 0 &&
+              x.first->compare (*y.first) == 0);
+    };
+
+    // While at it also determine whether we have multiple partitions.
+    //
+    optional<string> ml;
+    for (auto b (ns.begin ()), e (ns.end ()); b != e; )
+    {
+      const pair<optional<string>, const target_key*>& x (*b++);
+
+      // Move all the elements that are equal to x to the front, preserving
+      // order.
+      //
+      b = stable_partition (
+        b, e,
+        [&cmp, &x] (const pair<optional<string>, const target_key*>& y)
+        {
+          return (x.first && y.first && cmp (x, y));
+        });
+
+      if (!ml && b != e)
+        ml = string ();
+    }
+
+    // Print.
+    //
+    os << p << ' ';
+
+    if (l != nullptr)
+      os << *l << (lempty ? "" : " ") << (c == nullptr ? "->" : c) << ' ';
+
+    if (ml)
+      ml = string (os.str ().size (), ' '); // Indentation.
+
+    for (auto b (ns.begin ()), i (b), e (ns.end ()); i != e; )
+    {
+      if (i != b)
+        os << '\n' << *ml;
+
+      const pair<optional<string>, const target_key*>& p (*i);
+
+      if (!p.first) // Irregular.
+      {
+        os << *p.second;
+        ++i;
+        continue;
+      }
+
+      // Calculate the number of members in this partition.
+      //
+      size_t n (1);
+      for (auto j (i + 1); j != e && j->first && cmp (*i, *j); ++j)
+        ++n;
+
+      // Similar code to to_stream(target_key).
+      //
+
+      // Print the directory.
+      //
+      {
+        const target_key& k (*p.second);
+
+        uint16_t dv (sv.path);
+
+        // Note: relative() returns empty for './'.
+        //
+        const dir_path& rd (dv < 1 ? relative (*k.dir) : *k.dir);
+
+        if (!rd.empty ())
+        {
+          if (dv < 1)
+            os << diag_relative (rd);
+          else
+            to_stream (os, rd, true /* representation */);
+        }
+      }
+
+      // Print target types.
+      //
+      {
+        if (n != 1)
+          os << '{';
+
+        for (auto j (i), e (i + n); j != e; ++j)
+          os << (j != i ? " " : "") << j->second->type->name;
+
+        if (n != 1)
+          os << '}';
+      }
+
+      // Print the target name (the same for all members of this partition).
+      //
+      os << '{' << *i->first << '}';
+
+      i += n;
+    }
+  }
+
+  void
+  print_diag_impl (const char* p,
+                   target_key* l, vector<target_key>&& rs,
+                   const char* c)
+  {
+    // Note: keep this implementation separate from the above for performance.
+    //
+    assert (!rs.empty ());
+
+    if (rs.size () == 1)
+    {
+      print_diag_impl (p, l, move (rs.front ()), c);
+      return;
+    }
+
+    // At the outset handle out-qualification as above. Here we assume that
+    // all the targets in the group have the same out.
+    //
+    if (l != nullptr)
+    {
+      if (!l->out->empty ())
+      {
+        if (rs.front ().out->empty ())
+          l->out = &empty_dir_path;
+      }
+      else if (!rs.front ().out->empty ())
+      {
+        for (target_key& r: rs)
+          r.out = &empty_dir_path;
+      }
+    }
+
+    print_diag_impl<target_key> (p, l, false /* empty */, move (rs), c);
+  }
+
   // Note: these can't be inline since need the target class definition.
   //
   void
@@ -105,6 +290,31 @@ namespace build2
   }
 
   void
+  print_diag (const char* p, const path& l, const target& r, const char* c)
+  {
+    return print_diag (p, l, r.key (), c);
+  }
+
+  void
+  print_diag (const char* p, const path& l, target_key&& r, const char* c)
+  {
+    text << p << ' ' << l << ' ' << (c == nullptr ? "->" : c) << ' ' << r;
+  }
+
+  void
+  print_diag (const char* p,
+              const path& l, vector<target_key>&& rs,
+              const char* c)
+  {
+    assert (!rs.empty ());
+
+    if (rs.size () == 1)
+      print_diag (p, l, move (rs.front ()), c);
+    else
+      print_diag_impl<path> (p, &l, false /* empty */, move (rs), c);
+  }
+
+  void
   print_diag (const char* p, const string& l, const target& r, const char* c)
   {
     return print_diag (p, l, r.key (), c);
@@ -120,6 +330,19 @@ namespace build2
   }
 
   void
+  print_diag (const char* p,
+              const string& l, vector<target_key>&& rs,
+              const char* c)
+  {
+    assert (!rs.empty ());
+
+    if (rs.size () == 1)
+      print_diag (p, l, move (rs.front ()), c);
+    else
+      print_diag_impl<string> (p, &l, l.empty (), move (rs), c);
+  }
+
+  void
   print_diag (const char* p, const target& r)
   {
     print_diag_impl (p, nullptr, r.key (), nullptr);
diff --git a/libbuild2/diagnostics.hxx b/libbuild2/diagnostics.hxx
index a4509e4..9b9f6d8 100644
--- a/libbuild2/diagnostics.hxx
+++ b/libbuild2/diagnostics.hxx
@@ -59,7 +59,7 @@ namespace build2
   // cli cli{foo} -> {hxx cxx}{foo}
   //
   // thrift thrift{foo} -> {hxx cxx}{foo-types}
-  //                    -> {hxx cxx}{foo-stubs}
+  //                       {hxx cxx}{foo-stubs}
   //
   // Potentially we could also support target groups for <l-target>:
   //
@@ -69,11 +69,17 @@ namespace build2
   //      {hxx cxx}{foo-stubs} -> {hxx cxx}{foo-insts}
   //                              {hxx cxx}{foo-impls}
   //
+  // See also the `diag` Buildscript pseudo-builtin which is reduced to one of
+  // the print_diag() calls (adhoc_buildscript_rule::print_custom_diag()). In
+  // particular, if you are adding a new overload, also consider if/how it
+  // should handled there.
+  //
   // Note: see GH issue #40 for additional background and rationale.
   //
   // If <comb> is not specified, then "->" is used by default.
 
   // prog target -> target
+  // prog target -> group
   //
   LIBBUILD2_SYMEXPORT void
   print_diag (const char* prog,
@@ -95,7 +101,33 @@ namespace build2
               target_key&& l, target_key&& r,
               const char* comb = nullptr);
 
+  // Note: using small_vector would require target_key definition.
+  //
+  void
+  print_diag (const char* prog,
+              target_key&& l, vector<target_key>&& r,
+              const char* comb = nullptr);
+
+  // prog path -> target
+  // prog path -> group
+  //
+  LIBBUILD2_SYMEXPORT void
+  print_diag (const char* prog,
+              const path& l, const target& r,
+              const char* comb = nullptr);
+
+  LIBBUILD2_SYMEXPORT void
+  print_diag (const char* prog,
+              const path& l, target_key&& r,
+              const char* comb = nullptr);
+
+  LIBBUILD2_SYMEXPORT void
+  print_diag (const char* prog,
+              const path& l, vector<target_key>&& r,
+              const char* comb = nullptr);
+
   // prog string -> target
+  // prog string -> group
   //
   // Use these versions if, for example, input information is passed as an
   // argument.
@@ -110,6 +142,11 @@ namespace build2
               const string& l, target_key&& r,
               const char* comb = nullptr);
 
+  LIBBUILD2_SYMEXPORT void
+  print_diag (const char* prog,
+              const string& l, vector<target_key>&& r,
+              const char* comb = nullptr);
+
   // prog target
   //
   LIBBUILD2_SYMEXPORT void
@@ -118,6 +155,11 @@ namespace build2
   void
   print_diag (const char* prog, target_key&&);
 
+  // prog group
+  //
+  void
+  print_diag (const char* prog, vector<target_key>&&);
+
   // prog path
   //
   // Special versions for cases like mkdir/rmdir, save, etc.
@@ -137,7 +179,7 @@ namespace build2
   //
   // Note: use path_name ("-") if the result is written to stdout.
 
-  // target -> path
+  // prog target -> path
   //
   void
   print_diag (const char* prog,
@@ -154,7 +196,7 @@ namespace build2
               const target& l, const path_name_view& r,
               const char* comb = nullptr);
 
-  // path -> path
+  // prog path -> path
   //
   void
   print_diag (const char* prog,
@@ -171,7 +213,7 @@ namespace build2
               const path& l, const path_name_view& r,
               const char* comb = nullptr);
 
-  // string -> path
+  // prog string -> path
   //
   // Use this version if, for example, input information is passed as an
   // argument.
diff --git a/libbuild2/diagnostics.ixx b/libbuild2/diagnostics.ixx
index 7c1a432..a082290 100644
--- a/libbuild2/diagnostics.ixx
+++ b/libbuild2/diagnostics.ixx
@@ -6,6 +6,11 @@ namespace build2
   LIBBUILD2_SYMEXPORT void
   print_diag_impl (const char*, target_key*, target_key&&, const char*);
 
+  LIBBUILD2_SYMEXPORT void
+  print_diag_impl (const char*,
+                   target_key*, vector<target_key>&& r,
+                   const char*);
+
   inline void
   print_diag (const char* p, target_key&& l, target_key&& r, const char* c)
   {
@@ -13,12 +18,26 @@ namespace build2
   }
 
   inline void
+  print_diag (const char* p,
+              target_key&& l, vector<target_key>&& r,
+              const char* c)
+  {
+    print_diag_impl (p, &l, move (r), c);
+  }
+
+  inline void
   print_diag (const char* p, target_key& r)
   {
     print_diag_impl (p, nullptr, move (r), nullptr);
   }
 
   inline void
+  print_diag (const char* p, vector<target_key>&& r)
+  {
+    print_diag_impl (p, nullptr, move (r), nullptr);
+  }
+
+  inline void
   print_diag (const char* p, const path& r)
   {
     print_diag (p, path_name (&r));
diff --git a/libbuild2/filesystem.cxx b/libbuild2/filesystem.cxx
index 32895c4..196d9bd 100644
--- a/libbuild2/filesystem.cxx
+++ b/libbuild2/filesystem.cxx
@@ -55,35 +55,30 @@ namespace build2
     // We don't want to print the command if the directory already exists.
     // This makes the below code a bit ugly.
     //
-    mkdir_status ms;
-
-    try
-    {
-      ms = try_mkdir (d);
-    }
-    catch (const system_error& e)
+    auto print = [v, &d] (bool ovr)
     {
-      if (verb >= v)
+      if (verb >= v || ovr)
       {
         if (verb >= 2)
           text << "mkdir " << d;
         else if (verb)
           print_diag ("mkdir", d);
       }
+    };
 
+    mkdir_status ms;
+    try
+    {
+      ms = try_mkdir (d);
+    }
+    catch (const system_error& e)
+    {
+      print (true);
       fail << "unable to create directory " << d << ": " << e << endf;
     }
 
     if (ms == mkdir_status::success)
-    {
-      if (verb >= v)
-      {
-        if (verb >= 2)
-          text << "mkdir " << d;
-        else if (verb)
-          print_diag ("mkdir", d);
-      }
-    }
+      print (false);
 
     return ms;
   }
@@ -94,35 +89,30 @@ namespace build2
     // We don't want to print the command if the directory already exists.
     // This makes the below code a bit ugly.
     //
-    mkdir_status ms;
-
-    try
-    {
-      ms = try_mkdir_p (d);
-    }
-    catch (const system_error& e)
+    auto print = [v, &d] (bool ovr)
     {
-      if (verb >= v)
+      if (verb >= v || ovr)
       {
         if (verb >= 2)
           text << "mkdir -p " << d;
         else if (verb)
           print_diag ("mkdir -p", d);
       }
+    };
 
+    mkdir_status ms;
+    try
+    {
+      ms = try_mkdir_p (d);
+    }
+    catch (const system_error& e)
+    {
+      print (true);
       fail << "unable to create directory " << d << ": " << e << endf;
     }
 
     if (ms == mkdir_status::success)
-    {
-      if (verb >= v)
-      {
-        if (verb >= 2)
-          text << "mkdir -p " << d;
-        else if (verb)
-          print_diag ("mkdir -p", d);
-      }
-    }
+      print (false);
 
     return ms;
   }
@@ -156,9 +146,9 @@ namespace build2
   fs_status<rmfile_status>
   rmsymlink (context& ctx, const path& p, bool d, uint16_t v)
   {
-    auto print = [&p, v] ()
+    auto print = [&p, v] (bool ovr)
     {
-      if (verb >= v)
+      if (verb >= v || ovr)
       {
         // Note: strip trailing directory separator (but keep as path for
         // relative).
@@ -182,12 +172,12 @@ namespace build2
     }
     catch (const system_error& e)
     {
-      print ();
+      print (true);
       fail << "unable to remove symlink " << p.string () << ": " << e << endf;
     }
 
     if (rs == rmfile_status::success)
-      print ();
+      print (false);
 
     return rs;
   }
diff --git a/libbuild2/filesystem.txx b/libbuild2/filesystem.txx
index 7e3a773..afdb48d 100644
--- a/libbuild2/filesystem.txx
+++ b/libbuild2/filesystem.txx
@@ -13,11 +13,12 @@ namespace build2
 
     // We don't want to print the command if we couldn't remove the file
     // because it does not exist (just like we don't print the update command
-    // if the file is up to date). This makes the below code a bit ugly.
+    // if the file is up to date). But we always want to print some command
+    // before we issue diagnostics. This makes the below code a bit ugly.
     //
-    auto print = [&f, &t, v] ()
+    auto print = [&f, &t, v] (bool ovr)
     {
-      if (verb >= v)
+      if (verb >= v || ovr)
       {
         if (verb >= 2)
           text << "rm " << f;
@@ -36,12 +37,12 @@ namespace build2
     }
     catch (const system_error& e)
     {
-      print ();
+      print (true);
       fail << "unable to remove file " << f << ": " << e << endf;
     }
 
     if (rs == rmfile_status::success)
-      print ();
+      print (false);
 
     return rs;
   }
@@ -54,11 +55,12 @@ namespace build2
 
     // We don't want to print the command if we couldn't remove the directory
     // because it does not exist (just like we don't print mkdir if it already
-    // exists) or if it is not empty. This makes the below code a bit ugly.
+    // exists) or if it is not empty. But we always want to print some command
+    // before we issue diagnostics. This makes the below code a bit ugly.
     //
-    auto print = [&d, &t, v] ()
+    auto print = [&d, &t, v] (bool ovr)
     {
-      if (verb >= v)
+      if (verb >= v || ovr)
       {
         if (verb >= 2)
           text << "rmdir " << d;
@@ -77,7 +79,7 @@ namespace build2
     }
     catch (const system_error& e)
     {
-      print ();
+      print (true);
       fail << "unable to remove directory " << d << ": " << e << endf;
     }
 
@@ -85,7 +87,7 @@ namespace build2
     {
     case rmdir_status::success:
       {
-        print ();
+        print (false);
         break;
       }
     case rmdir_status::not_empty:
diff --git a/libbuild2/target-key.hxx b/libbuild2/target-key.hxx
index c5690a9..9ac87dc 100644
--- a/libbuild2/target-key.hxx
+++ b/libbuild2/target-key.hxx
@@ -94,8 +94,21 @@ namespace build2
   LIBBUILD2_SYMEXPORT ostream&
   operator<< (ostream&, const target_key&);
 
-  LIBBUILD2_SYMEXPORT ostream&
-  to_stream (ostream&, const target_key&, optional<stream_verbosity> = nullopt);
+  // If name_only is true, then only print the target name (and extension, if
+  // necessary), without the directory or type.
+  //
+  // Return true if the result is regular, that is, in the
+  // <dir>/<type>{<name>}@<out>/ form with the individual components
+  // corresponding directly to the target_key members (that is, without moving
+  // parts around as would be the case for directories). This information is
+  // used when trying to print several targets in a combined form (for
+  // example, {hxx cxx}{foo}) in print_diag().
+  //
+  LIBBUILD2_SYMEXPORT bool
+  to_stream (ostream&,
+             const target_key&,
+             optional<stream_verbosity> = nullopt,
+             bool name_only = false);
 }
 
 namespace std
diff --git a/libbuild2/target-type.hxx b/libbuild2/target-type.hxx
index 09bc316..eae2caf 100644
--- a/libbuild2/target-type.hxx
+++ b/libbuild2/target-type.hxx
@@ -89,7 +89,9 @@ namespace build2
                      const location&,
                      bool reverse);
 
-    void (*print) (ostream&, const target_key&);
+    // See to_stream(ostream,target_key) for details.
+    //
+    bool (*print) (ostream&, const target_key&, bool name_only);
 
     const target* (*search) (const target&, const prerequisite_key&);
 
diff --git a/libbuild2/target.cxx b/libbuild2/target.cxx
index 6bd6cc1..34b131f 100644
--- a/libbuild2/target.cxx
+++ b/libbuild2/target.cxx
@@ -820,9 +820,14 @@ namespace build2
 
   static const optional<string> unknown_ext ("?");
 
-  ostream&
-  to_stream (ostream& os, const target_key& k, optional<stream_verbosity> osv)
+  bool
+  to_stream (ostream& os,
+             const target_key& k,
+             optional<stream_verbosity> osv,
+             bool name_only)
   {
+    // Note: similar code in print_diag_impl(vector<target_key>).
+
     stream_verbosity sv (osv ? *osv : stream_verb (os));
     uint16_t dv (sv.path);
     uint16_t ev (sv.extension);
@@ -832,22 +837,29 @@ namespace build2
     //
     bool n (!k.name->empty ());
 
-    // Note: relative() returns empty for './'.
-    //
-    const dir_path& rd (dv < 1 ? relative (*k.dir) : *k.dir); // Relative.
-    const dir_path& pd (n ? rd : rd.directory ());            // Parent.
+    const target_type& tt (*k.type);
 
-    if (!pd.empty ())
+    dir_path rds; // Storage.
+    if (!name_only)
     {
+      // Note: relative() returns empty for './'.
+      //
       if (dv < 1)
-        os << diag_relative (pd);
-      else
-        to_stream (os, pd, true /* representation */);
-    }
+        rds = relative (*k.dir);
 
-    const target_type& tt (*k.type);
+      const dir_path& rd (dv < 1 ? rds : *k.dir);    // Relative.
+      const dir_path& pd (n ? rd : rd.directory ()); // Parent.
 
-    os << tt.name << '{';
+      if (!pd.empty ())
+      {
+        if (dv < 1)
+          os << diag_relative (pd);
+        else
+          to_stream (os, pd, true /* representation */);
+      }
+
+      os << tt.name << '{';
+    }
 
     if (n)
     {
@@ -890,37 +902,47 @@ namespace build2
       }
     }
     else
+    {
+      if (name_only && dv < 1) // Already done if !name_only.
+        rds = relative (*k.dir);
+
+      const dir_path& rd (dv < 1 ? rds : *k.dir);
+
       to_stream (os,
                  rd.empty () ? dir_path (".") : rd.leaf (),
                  true /* representation */);
+    }
 
-    os << '}';
-
-    // If this target is from src, print its out.
-    //
-    if (!k.out->empty ())
+    if (!name_only)
     {
-      if (dv < 1)
+      os << '}';
+
+      // If this target is from src, print its out.
+      //
+      if (!k.out->empty ())
       {
-        // Don't print '@./'.
-        //
-        const string& o (diag_relative (*k.out, false));
+        if (dv < 1)
+        {
+          // Don't print '@./'.
+          //
+          const string& o (diag_relative (*k.out, false));
 
-        if (!o.empty ())
-          os << '@' << o;
+          if (!o.empty ())
+            os << '@' << o;
+        }
+        else
+          os << '@' << *k.out;
       }
-      else
-        os << '@' << *k.out;
     }
 
-    return os;
+    return n; // Regular if we had the name.
   }
 
   ostream&
   operator<< (ostream& os, const target_key& k)
   {
     if (auto p = k.type->print)
-      p (os, k);
+      p (os, k, false /* name_only */);
     else
       to_stream (os, k, stream_verb (os));
 
@@ -1109,20 +1131,20 @@ namespace build2
     return tk.ext->c_str ();
   }
 
-  void
-  target_print_0_ext_verb (ostream& os, const target_key& k)
+  bool
+  target_print_0_ext_verb (ostream& os, const target_key& k, bool no)
   {
     stream_verbosity sv (stream_verb (os));
     if (sv.extension == 1) sv.extension = 0; // Remap 1 to 0.
-    to_stream (os, k, sv);
+    return to_stream (os, k, sv, no);
   }
 
-  void
-  target_print_1_ext_verb (ostream& os, const target_key& k)
+  bool
+  target_print_1_ext_verb (ostream& os, const target_key& k, bool no)
   {
     stream_verbosity sv (stream_verb (os));
     if (sv.extension == 0) sv.extension = 1; // Remap 0 to 1.
-    to_stream (os, k, sv);
+    return to_stream (os, k, sv, no);
   }
 
   // type info
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index 8e1c3a1..684fce9 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -2422,14 +2422,14 @@ namespace build2
   // Target type uses the extension but it is fixed and there is no use
   // printing it (e.g., man1{}).
   //
-  LIBBUILD2_SYMEXPORT void
-  target_print_0_ext_verb (ostream&, const target_key&);
+  LIBBUILD2_SYMEXPORT bool
+  target_print_0_ext_verb (ostream&, const target_key&, bool);
 
   // Target type uses the extension and there is normally no default so it
   // should be printed (e.g., file{}).
   //
-  LIBBUILD2_SYMEXPORT void
-  target_print_1_ext_verb (ostream&, const target_key&);
+  LIBBUILD2_SYMEXPORT bool
+  target_print_1_ext_verb (ostream&, const target_key&, bool);
 
   // The default behavior, that is, look for an existing target in the
   // prerequisite's directory scope.
-- 
cgit v1.1