From b61e0de250d522ec9a8e16146ef979a65c181db1 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Thu, 20 Jul 2023 07:44:36 +0200 Subject: Change inner rule/prerequisites match order in install::file_rule The old order messed up the for-install signaling logic. See the long comment in install::file_rule::apply_impl() for background and details. --- libbuild2/algorithm.cxx | 14 ++++++++++++++ libbuild2/algorithm.hxx | 7 +++++++ libbuild2/bash/rule.cxx | 10 +++++++++- libbuild2/in/rule.cxx | 11 +++++++++-- libbuild2/in/rule.hxx | 5 +++++ libbuild2/install/rule.cxx | 33 ++++++++++++++++++++++++++++++++- libbuild2/version/rule.cxx | 6 ++++++ 7 files changed, 82 insertions(+), 4 deletions(-) diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx index 4db3d72..ad4c406 100644 --- a/libbuild2/algorithm.cxx +++ b/libbuild2/algorithm.cxx @@ -1265,6 +1265,20 @@ namespace build2 return ct.try_matched_state (a, false); } + void + match_only_sync (action a, const target& t) + { + assert (t.ctx.phase == run_phase::match); + + target_lock l (lock_impl (a, t, scheduler::work_none)); + + if (l.target != nullptr && l.offset < target::offset_matched) + { + if (match_impl (l, true /* step */).second == target_state::failed) + throw failed (); + } + } + // Note: lock is a reference to avoid the stacking overhead. // static group_view diff --git a/libbuild2/algorithm.hxx b/libbuild2/algorithm.hxx index 8bdf737..7cc42c2 100644 --- a/libbuild2/algorithm.hxx +++ b/libbuild2/algorithm.hxx @@ -376,6 +376,13 @@ namespace build2 pair match_sync (action, const target&, unmatch); + // As above but only match the target (unless already matched) without + // applying the match (which is normally done with match_sync()). You will + // most likely regret using this function. + // + LIBBUILD2_SYMEXPORT void + match_only_sync (action, const target&); + // As above but without incrementing the target's dependents count. Should // be executed with execute_direct_*(). // diff --git a/libbuild2/bash/rule.cxx b/libbuild2/bash/rule.cxx index 29c6a2a..502a206 100644 --- a/libbuild2/bash/rule.cxx +++ b/libbuild2/bash/rule.cxx @@ -63,10 +63,12 @@ namespace build2 // in_rule // bool in_rule:: - match (action a, target& t, const string& hint, match_extra&) const + match (action a, target& xt, const string& hint, match_extra&) const { tracer trace ("bash::in_rule::match"); + file& t (xt.as ()); // Only registered for exe{} and bash{}. + // Note that for bash{} and for exe{} with hint we match even if the // target does not depend on any modules (while it could have been // handled by the in module, that would require loading it). @@ -89,6 +91,12 @@ namespace build2 l4 ([&]{trace << "no bash module prerequisite or hint for target " << t;}); + // If we match, derive the file name early as recommended by the in + // rule. + // + if (fi && fm) + t.derive_path (); + return fi && fm; } diff --git a/libbuild2/in/rule.cxx b/libbuild2/in/rule.cxx index 74bc2a7..31a9d94 100644 --- a/libbuild2/in/rule.cxx +++ b/libbuild2/in/rule.cxx @@ -47,6 +47,13 @@ namespace build2 if (!fi) l5 ([&]{trace << "no in file prerequisite for target " << t;}); + // If we match, derive the file name here instead of in apply() to make + // it available early for the in{} prerequisite search (see + // install::file_rule::apply_impl() for background). + // + if (fi) + t.derive_path (); + return fi; } @@ -55,9 +62,9 @@ namespace build2 { file& t (xt.as ()); - // Derive the file name. + // Make sure derived rules assign the path in match(). // - t.derive_path (); + assert (!t.path ().empty ()); // Inject dependency on the output directory. // diff --git a/libbuild2/in/rule.hxx b/libbuild2/in/rule.hxx index 369fd93..67c2509 100644 --- a/libbuild2/in/rule.hxx +++ b/libbuild2/in/rule.hxx @@ -22,6 +22,11 @@ namespace build2 // cache data (e.g., in match() or apply()) to be used in substitute() and // lookup() calls. // + // A derived rule is also required to derive the target file name in + // match() instead of apply() to make it available early for the in{} + // prerequisite search (see install::file_rule::apply_impl() for + // background). + // // Note also that currently this rule ignores the dry-run mode (see // perform_update() for the rationale). // diff --git a/libbuild2/install/rule.cxx b/libbuild2/install/rule.cxx index 20a4bc3..4dd11e8 100644 --- a/libbuild2/install/rule.cxx +++ b/libbuild2/install/rule.cxx @@ -378,11 +378,36 @@ namespace build2 // (actual update). We used to do this after matching the prerequisites // but the inner rule may provide some rule-specific information (like // the target extension for exe{}) that may be required during the - // prerequisite search (like the base name for in{}). + // prerequisite search (like the base name for in{}; this no longer + // reproduces likely due to the changes to exe{} extension derivation + // but a contrived arrangement can still be made to trigger this). // + // But then we discovered that doing this before the prerequisites messes + // up with the for-install signaling. Specifically, matching the + // prerequisites may signal that they are being updated for install, + // for example, for a library via a metadata library used in a moc + // recipe. While matching the inner rule may trigger updating during + // match of such prerequisites, for example, a source file generated by + // that moc recipe that depends on this metadata library. If we match + // prerequisites before, then the library that is pulled by the metadata + // library will be updated before we had a chance to signal that it + // should be updated for install. + // + // To try to accommodate both cases (as best as we can) we now split the + // inner rule match into two steps: we do the match before and apply + // after. This allows rules that deal with tricky prerequisites like + // in{} to assign the target path in match() instead of apply() (see + // in::rule, for example). + // +#if 0 optional unchanged; if (a.operation () == update_id) unchanged = match_inner (a, t, unmatch::unchanged).first; +#else + action ia (a.inner_action ()); + if (a.operation () == update_id) + match_only_sync (ia, t); +#endif optional is; // Installation scope (resolve lazily). @@ -454,6 +479,12 @@ namespace build2 pts.push_back (prerequisite_target (pt, pi)); } +#if 1 + optional unchanged; + if (a.operation () == update_id) + unchanged = match_sync (ia, t, unmatch::unchanged).first; +#endif + if (a.operation () == update_id) { return *unchanged diff --git a/libbuild2/version/rule.cxx b/libbuild2/version/rule.cxx index 98dc2da..65c1117 100644 --- a/libbuild2/version/rule.cxx +++ b/libbuild2/version/rule.cxx @@ -93,6 +93,12 @@ namespace build2 if (!fi) l5 ([&]{trace << "no in file prerequisite for target " << t;}); + // If we match, derive the file name early as recommended by the in + // rule. + // + if (fm && fi) + t.derive_path (); + return fm && fi; } -- cgit v1.1