From 3bbbe09e8629ab5311a1bcbb9f56aa6a33e36f55 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Wed, 30 Nov 2022 09:08:53 +0200 Subject: Deal with order dependence in dist rule --- libbuild2/dist/init.cxx | 14 +++-- libbuild2/dist/module.hxx | 14 ++++- libbuild2/dist/operation.cxx | 129 +++++++++++++++++++++++++++---------------- libbuild2/dist/rule.cxx | 43 +++++++++++++-- libbuild2/dist/rule.hxx | 11 +++- libbuild2/dist/types.hxx | 40 ++++++++++++++ 6 files changed, 191 insertions(+), 60 deletions(-) create mode 100644 libbuild2/dist/types.hxx diff --git a/libbuild2/dist/init.cxx b/libbuild2/dist/init.cxx index 26ff86d..9de84ce 100644 --- a/libbuild2/dist/init.cxx +++ b/libbuild2/dist/init.cxx @@ -21,8 +21,6 @@ namespace build2 { namespace dist { - static const rule rule_; - void boot (scope& rs, const location&, module_boot_extra& extra) { @@ -196,7 +194,7 @@ namespace build2 const location& l, bool first, bool, - module_init_extra&) + module_init_extra& extra) { tracer trace ("dist::init"); @@ -208,13 +206,19 @@ namespace build2 l5 ([&]{trace << "for " << rs;}); + module& mod (extra.module_as ()); + auto& vp (rs.var_pool (true /* public */)); // All qualified. // Register our wildcard rule. Do it explicitly for the alias to prevent // something like insert(dist_id, test_id) taking precedence. // - rs.insert_rule (dist_id, 0, "dist", rule_); - rs.insert_rule (dist_id, 0, "dist.alias", rule_); + { + const rule& r (mod); + + rs.insert_rule (dist_id, 0, "dist", r); + rs.insert_rule (dist_id, 0, "dist.alias", r); + } // We need this rule for out-of-any-project dependencies (for example, // executables imported from /usr/bin, etc). We are registering it on diff --git a/libbuild2/dist/module.hxx b/libbuild2/dist/module.hxx index 9c682d0..dbe2a3e 100644 --- a/libbuild2/dist/module.hxx +++ b/libbuild2/dist/module.hxx @@ -10,14 +10,19 @@ #include #include +#include +#include + #include namespace build2 { namespace dist { - struct LIBBUILD2_SYMEXPORT module: build2::module + class LIBBUILD2_SYMEXPORT module: public build2::module, + public rule { + public: static const string name; const variable& var_dist_package; @@ -38,6 +43,10 @@ namespace build2 adhoc.push_back (move (f)); } + // List of postponed prerequisites. + // + postponed_prerequisites postponed; + // Distribution post-processing callbacks. // // Only the last component in the pattern may contain wildcards. If the @@ -69,8 +78,9 @@ namespace build2 // Implementation details. // + public: module (const variable& v_d_p) - : var_dist_package (v_d_p) {} + : rule (postponed), var_dist_package (v_d_p) {} public: bool distributed = false; // True if this project is being distributed. diff --git a/libbuild2/dist/operation.cxx b/libbuild2/dist/operation.cxx index c7ce7f2..e5c9307 100644 --- a/libbuild2/dist/operation.cxx +++ b/libbuild2/dist/operation.cxx @@ -15,6 +15,7 @@ #include #include +#include #include using namespace std; @@ -236,6 +237,11 @@ namespace build2 fail << "unknown distribution package name" << info << "did you forget to set dist.package?"; + // Need non-const in order to clear postponed. This is safe since only + // doing it when running serially. + // + module& mod (const_cast (*rs.find_module (module::name))); + const string& dist_package (cast (l)); const process_path& dist_cmd (cast (rs.vars["dist.cmd"])); @@ -272,60 +278,91 @@ namespace build2 values params; path_name pn (""); const location loc (pn); // Dummy location. + action_targets ts {tgt}; + + auto process_postponed = [&ctx, &mod] () { - action_targets ts {tgt}; + if (!mod.postponed.list.empty ()) + { + // Re-grab the phase lock similar to perform_match(). + // + phase_lock l (ctx, run_phase::match); + + // Note that we don't need to bother with the mutex since we do + // all of this serially. But we can end up with new elements at + // the end. + // + // Strictly speaking, to handle this correctly we would need to do + // multiple passes over this list and only give up when we cannot + // make any progress since earlier entries that we cannot resolve + // could be "fixed" by later entries. But this feels far-fetched + // and so let's wait for a real example before complicating this. + // + for (auto i (mod.postponed.list.begin ()); + i != mod.postponed.list.end (); + ++i) + { + rule::match_postponed (i->action, i->target, i->prereq); + } + } + }; - auto mog = make_guard ([&ctx] () {ctx.match_only = false;}); - ctx.match_only = true; + auto mog = make_guard ([&ctx] () {ctx.match_only = false;}); + ctx.match_only = true; - const operations& ops (rs.root_extra->operations); - for (operations::size_type id (default_id + 1); // Skip default_id. - id < ops.size (); - ++id) + const operations& ops (rs.root_extra->operations); + for (operations::size_type id (default_id + 1); // Skip default_id. + id < ops.size (); + ++id) + { + if (const operation_info* oif = ops[id]) { - if (const operation_info* oif = ops[id]) - { - // Skip aliases (e.g., update-for-install). In fact, one can - // argue the default update should be sufficient since it is - // assumed to update all prerequisites and we no longer support - // ad hoc stuff like test.input. Though here we are using the - // dist meta-operation, not perform. - // - if (oif->id != id) - continue; + // Skip aliases (e.g., update-for-install). In fact, one can argue + // the default update should be sufficient since it is assumed to + // update all prerequisites and we no longer support ad hoc stuff + // like test.input. Though here we are using the dist + // meta-operation, not perform. + // + if (oif->id != id) + continue; - // Use standard (perform) match. - // - if (auto pp = oif->pre_operation) + // Use standard (perform) match. + // + if (auto pp = oif->pre_operation) + { + if (operation_id pid = pp (ctx, params, dist_id, loc)) { - if (operation_id pid = pp (ctx, params, dist_id, loc)) - { - const operation_info* poif (ops[pid]); - ctx.current_operation (*poif, oif, false /* diag_noise */); - action a (dist_id, poif->id, oif->id); - perform_match (params, a, ts, - 1 /* diag (failures only) */, - false /* progress */); - } + const operation_info* poif (ops[pid]); + ctx.current_operation (*poif, oif, false /* diag_noise */); + action a (dist_id, poif->id, oif->id); + mod.postponed.list.clear (); + perform_match (params, a, ts, + 1 /* diag (failures only) */, + false /* progress */); + process_postponed (); } + } - ctx.current_operation (*oif, nullptr, false /* diag_noise */); - action a (dist_id, oif->id); - perform_match (params, a, ts, - 1 /* diag (failures only) */, - false /* progress */); + ctx.current_operation (*oif, nullptr, false /* diag_noise */); + action a (dist_id, oif->id); + mod.postponed.list.clear (); + perform_match (params, a, ts, + 1 /* diag (failures only) */, + false /* progress */); + process_postponed (); - if (auto po = oif->post_operation) + if (auto po = oif->post_operation) + { + if (operation_id pid = po (ctx, params, dist_id)) { - if (operation_id pid = po (ctx, params, dist_id)) - { - const operation_info* poif (ops[pid]); - ctx.current_operation (*poif, oif, false /* diag_noise */); - action a (dist_id, poif->id, oif->id); - perform_match (params, a, ts, - 1 /* diag (failures only) */, - false /* progress */); - } + const operation_info* poif (ops[pid]); + ctx.current_operation (*poif, oif, false /* diag_noise */); + action a (dist_id, poif->id, oif->id); + mod.postponed.list.clear (); + perform_match (params, a, ts, + 1 /* diag (failures only) */, + false /* progress */); + process_postponed (); } } } @@ -381,7 +418,7 @@ namespace build2 dir_path out_nroot (out_root / pd); const scope& nrs (ctx.scopes.find_out (out_nroot)); - if (nrs.out_path () != out_nroot) // This subproject not loaded. + if (nrs.out_path () != out_nroot) // This subproject is not loaded. continue; if (!nrs.src_path ().sub (src_root)) // Not a strong amalgamation. @@ -508,8 +545,6 @@ namespace build2 // Copy over all the files. Apply post-processing callbacks. // - const module& mod (*rs.find_module (module::name)); - prog = prog && show_progress (1 /* max_verb */); size_t prog_percent (0); diff --git a/libbuild2/dist/rule.cxx b/libbuild2/dist/rule.cxx index 0c72ff5..7233eba 100644 --- a/libbuild2/dist/rule.cxx +++ b/libbuild2/dist/rule.cxx @@ -72,6 +72,8 @@ namespace build2 // Search for an existing target or existing file in src. // + // Note: see also similar code in match_postponed() below. + // const prerequisite_key& k (p.key ()); pt = k.tk.type->search (t, k); @@ -85,12 +87,13 @@ namespace build2 !p.dir.sub (out_root)) continue; - // @@ TODO: this can actually be order-dependent: for example - // libs{} prerequisite may be unknown because we haven't - // matched the lib{} group yet. + // This can be order-dependent: for example libs{} prerequisite + // may be unknown because we haven't matched the lib{} group + // yet. So we postpone this for later (see match_postponed()). // - fail << "prerequisite " << k << " is not existing source file " - << "nor known output target" << endf; + mlock l (postponed_.mutex); + postponed_.list.push_back (postponed_prerequisite {a, t, p,}); + continue; } search_custom (p, *pt); // Cache. @@ -107,5 +110,35 @@ namespace build2 return noop_recipe; // We will never be executed. } + + void rule:: + match_postponed (action a, const target& t, const prerequisite& p) + { + const prerequisite_key& k (p.key ()); + const target* pt (k.tk.type->search (t, k)); + + if (pt == nullptr) + { + // Note that we do loose the diag frame that we normally get when + // failing during match. So let's mention the target manually. + // + fail << "prerequisite " << k << " is not existing source file nor " + << "known output target" << + info << "while applying rule dist to " << diag_do (a, t); + } + + search_custom (p, *pt); // Cache. + + // It's theoretically possible that the target gets entered but nobody + // else depends on it but us. So we need to make sure it's matched + // (since it, in turns, can pull in other targets). Note that this could + // potentially add new postponed prerequisites to the list. + // + if (!pt->matched (a)) + { + if (pt->dir.sub (t.root_scope ().out_path ())) + match_direct_sync (a, *pt); + } + } } } diff --git a/libbuild2/dist/rule.hxx b/libbuild2/dist/rule.hxx index a864015..ec6c41a 100644 --- a/libbuild2/dist/rule.hxx +++ b/libbuild2/dist/rule.hxx @@ -11,6 +11,8 @@ #include #include +#include + namespace build2 { namespace dist @@ -26,13 +28,20 @@ namespace build2 class rule: public simple_rule { public: - rule () {} + explicit + rule (postponed_prerequisites& p): postponed_ (p) {} virtual bool match (action, target&) const override; virtual recipe apply (action, target&) const override; + + static void + match_postponed (action, const target&, const prerequisite&); + + private: + postponed_prerequisites& postponed_; }; } } diff --git a/libbuild2/dist/types.hxx b/libbuild2/dist/types.hxx new file mode 100644 index 0000000..f3b1009 --- /dev/null +++ b/libbuild2/dist/types.hxx @@ -0,0 +1,40 @@ +// file : libbuild2/dist/types.hxx -*- C++ -*- +// license : MIT; see accompanying LICENSE file + +#ifndef LIBBUILD2_DIST_TYPES_HXX +#define LIBBUILD2_DIST_TYPES_HXX + +#include +#include + +#include + +namespace build2 +{ + namespace dist + { + // List of prerequisites that could not be searched to a target and were + // postponed for later re-search. This can happen, for example, because a + // prerequisite would resolve to a member of a group that hasn't been + // matched yet (for example, libs{} of lib{}). See rule::apply() for + // details. + // + // Note that we are using list instead of vector because new elements can + // be added at the end while we are iterating over the list. + // + struct postponed_prerequisite + { + build2::action action; + reference_wrapper target; + reference_wrapper prereq; + }; + + struct postponed_prerequisites + { + build2::mutex mutex; + build2::list list; + }; + } +} + +#endif // LIBBUILD2_DIST_TYPES_HXX -- cgit v1.1