From 442cf4e38a4661f77b413fc2a90646515d2bee2f Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Thu, 12 Dec 2024 10:59:37 +0200 Subject: Fix incorrect logic based on dry_run_option value --- libbuild2/adhoc-rule-buildscript.cxx | 26 ++++++++++++++++++++++---- libbuild2/build/script/parser.cxx | 4 ++++ libbuild2/cc/compile-rule.cxx | 27 ++++++++++++++++++++++++--- libbuild2/context.hxx | 17 ++++++++++++++++- 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 { + // 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 { + // 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 { 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; // 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; -- cgit v1.1