Fix incorrect logic based on dry_run_option value

4 files changed, 66 insertions, 8 deletions

diff --git a/libbuild2/adhoc-rule-buildscript.cxx b/libbuild2/adhoc-rule-buildscript.cxx
index b125ac5..e3ed0a4 100644
--- a/libbuild2/adhoc-rule-buildscript.cxx
+++ b/libbuild2/adhoc-rule-buildscript.cxx
@@ -1207,9 +1207,11 @@ namespace build2
       }
 
       // Note that in case of dry run we will have an incomplete (but valid)
-      // database which will be updated on the next non-dry run.
+      // database which will be updated on the next non-dry run. Except that
+      // we may still end up performing a non-dry-run update due to update
+      // during match or load.
       //
-      if (!update || ctx.dry_run_option)
+      if (!update /*|| ctx.dry_run_option*/)
         dd.close (false /* mtime_check */);
       else
         mdb->dd = dd.close_to_reopen ();
@@ -1246,8 +1248,24 @@ namespace build2
                                          md->deferred_failure);
       }
 
-      if (update && dd.reading () && !ctx.dry_run_option)
-        dd.touch = timestamp_unknown;
+      // Update depdb timestamp if nothing changed. Failed that, we will keep
+      // re-validating the information store in depdb (see similar logic in
+      // cc::compile_rule).
+      //
+      if (update && dd.reading ())
+      {
+        // What will happen if dry_run_option is true but we still end up
+        // performing a non-dry-run update due to update during match or
+        // load? In this case the target will become up-to-date and we will
+        // keep re-validating the cache until the depdb will get touched due
+        // to other reasons, which would be bad. So it feels like the least
+        // bad option is to keep re-touching the database on dry-run.
+        //
+#if 0
+        if (!ctx.dry_run_option)
+#endif
+          dd.touch = timestamp_unknown;
+      }
 
       dd.close (false /* mtime_check */);
 
diff --git a/libbuild2/build/script/parser.cxx b/libbuild2/build/script/parser.cxx
index 3ecf23d..c9193ff 100644
--- a/libbuild2/build/script/parser.cxx
+++ b/libbuild2/build/script/parser.cxx
@@ -2501,6 +2501,10 @@ namespace build2
           //
           auto fail = [this, what, &ctx] (const auto& f) -> optional<bool>
           {
+            // Note that this test will give a false negative if this target
+            // ends up being updated during load or match. At least it's
+            // conservative.
+            //
             bool df (!ctx.match_only && !ctx.dry_run_option);
 
             diag_record dr;
diff --git a/libbuild2/cc/compile-rule.cxx b/libbuild2/cc/compile-rule.cxx
index 29a26b5..c8955bc 100644
--- a/libbuild2/cc/compile-rule.cxx
+++ b/libbuild2/cc/compile-rule.cxx
@@ -1544,8 +1544,20 @@ namespace build2
         // to keep re-validating the file on every subsequent dry-run as well
         // on the real run).
         //
-        if (u && dd.reading () && !ctx.dry_run_option)
-          dd.touch = timestamp_unknown;
+        if (u && dd.reading ())
+        {
+          // What will happen if dry_run_option is true but we still end up
+          // performing a non-dry-run update due to update during match or
+          // load? In this case the target will become up-to-date and we will
+          // keep re-validating the cache until the depdb will get touched due
+          // to other reasons, which would be bad. So it feels like the least
+          // bad option is to keep re-touching the database on dry-run.
+          //
+#if 0
+          if (!ctx.dry_run_option)
+#endif
+            dd.touch = timestamp_unknown;
+        }
 
         dd.close (false /* mtime_check */);
         md.dd = move (dd.path);
@@ -4042,6 +4054,10 @@ namespace build2
         //
         auto fail = [&ctx] (const auto& h) -> optional<bool>
         {
+          // Note that this test will give a false negative if this target
+          // ends up being updated during load or match. At least it's
+          // conservative.
+          //
           bool df (!ctx.match_only && !ctx.dry_run_option);
 
           diag_record dr;
@@ -4104,7 +4120,6 @@ namespace build2
                        this] (path hp, path bp, timestamp mt) -> optional<bool>
       {
         context& ctx (t.ctx);
-        bool df (!ctx.match_only && !ctx.dry_run_option);
 
         const file* ht (
           enter_header (a, bs, t, li,
@@ -4113,6 +4128,12 @@ namespace build2
 
         if (ht == nullptr) // hp is still valid.
         {
+          // Note that this test will give a false negative if this target
+          // ends up being updated during load or match. At least it's
+          // conservative.
+          //
+          bool df (!ctx.match_only && !ctx.dry_run_option);
+
           diag_record dr;
           dr << error << "header " << hp << " not found and no rule to "
              << "generate it";
diff --git a/libbuild2/context.hxx b/libbuild2/context.hxx
index 81ac970..46dbfaa 100644
--- a/libbuild2/context.hxx
+++ b/libbuild2/context.hxx
@@ -230,6 +230,10 @@ namespace build2
 
     // Match only flag/level (see --{load,match}-only but also dist).
     //
+    // See also dry_run, which is in some sense a weaker version of match-
+    // only: the target is executed but nothing is actually being done (unless
+    // executed during match or load, that is).
+    //
     optional<match_only_level> match_only;
 
     // Skip booting external modules flag (see --no-external-modules).
@@ -263,11 +267,22 @@ namespace build2
     //
     // Note also that sometimes it makes sense to do a bit more than
     // absolutely necessary or to discard information in order to keep the
-    // rule logic sane.  And some rules may choose to ignore this flag
+    // rule logic sane. And some rules may choose to ignore this flag
     // altogether. In this case, however, the rule should be careful not to
     // rely on functions (notably from filesystem) that respect this flag in
     // order not to end up with a job half done.
     //
+    // Finally, sometimes you may need to know during match whether there will
+    // be a non-dry-run execute and use the dry_run_option for that. This can
+    // be problematic because even when dry_run_option is true, the target may
+    // end up being executed in the non-dry-run mode during load or match. As
+    // a result, any logic that is based on dry_run_option should be capable
+    // of functioning correctly in the non-dry-run execute.
+    //
+    // See also match_only, which is in some sense a stronger version of
+    // dry-run: the target is not executed at all, again, unless during match
+    // or load.
+    //
     bool dry_run = false;
     bool dry_run_option;