From fb5c711d0a2ccac1bc014a9a8580e85753e4067b Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Wed, 21 Sep 2022 14:18:03 +0200 Subject: Fix regression in dynamic dependency extraction, byproduct mode --- libbuild2/adhoc-rule-buildscript.cxx | 10 +++++++++ libbuild2/dyndep.cxx | 40 ++++++++++++++++++++++++------------ libbuild2/dyndep.hxx | 5 +++-- libbuild2/target.hxx | 3 +++ 4 files changed, 43 insertions(+), 15 deletions(-) diff --git a/libbuild2/adhoc-rule-buildscript.cxx b/libbuild2/adhoc-rule-buildscript.cxx index c5eaa60..505be1b 100644 --- a/libbuild2/adhoc-rule-buildscript.cxx +++ b/libbuild2/adhoc-rule-buildscript.cxx @@ -949,10 +949,20 @@ namespace build2 { normalize_external (fp, what); + // Note that unless we take into account dynamic targets, the skip + // logic below falls apart since we neither see targets entered via + // prerequsites (skip static prerequisites) nor by the cache=true code + // above (skip depdb entries). + // + // If this turns out to be racy (which is the reason we would skip + // dynamic targets; see the fine_file() implementation for details), + // then the only answer for now is to not use the byproduct mode. + // if (const build2::file* ft = dyndep::find_file ( trace, what, a, bs, t, fp, false /* cache */, true /* normalized */, + true /* dynamic */, map_ext, *byp.default_type).first) { // Skip if this is one of the static prerequisites provided it was diff --git a/libbuild2/dyndep.cxx b/libbuild2/dyndep.cxx index 76eac9c..94d2028 100644 --- a/libbuild2/dyndep.cxx +++ b/libbuild2/dyndep.cxx @@ -427,6 +427,7 @@ namespace build2 action a, const scope& bs, const target& t, path& fp, bool cache, bool norm, bool insert, + bool dynamic, const function& map_extension, const target_type& fallback, const function& get_pfx_map, @@ -435,13 +436,21 @@ namespace build2 // NOTE: see enter_header() caching logic if changing anyting here with // regards to the target and base scope usage. - // Find or maybe insert the target. The directory is only moved from if - // insert is true. Note that it must be normalized. + // Find or maybe insert the target. + // + // If insert is false, then don't consider dynamically-created targets + // (i.e., those that are not real or implied) unless dynamic is true, in + // which case return the target that would have been inserted. + // + // The directory is only moved from if insert is true. Note that it must + // be normalized. // auto find = [&trace, what, &t, - &map_extension, &fallback] (dir_path&& d, - path&& f, - bool insert) -> const file* + &map_extension, + &fallback] (dir_path&& d, + path&& f, + bool insert, + bool dynamic = false) -> const file* { // Split the file into its name part and extension. Here we can assume // the name part is a valid filesystem name. @@ -496,9 +505,9 @@ namespace build2 if (tts.empty ()) { // If the project doesn't "know" this extension then we can't possibly - // find an explicit target of this type. + // find a real or implied target of this type. // - if (!insert) + if (!insert && !dynamic) { l6 ([&]{trace << "unknown " << what << ' ' << n << " extension '" << e << "'";}); @@ -552,7 +561,7 @@ namespace build2 // Cache the dynamic target corresponding to tts[0] since that's // what we will be inserting (see below). // - if (insert && i == 0) + if ((insert || dynamic) && i == 0) f = x; l6 ([&]{trace << "dynamic target with target type " << tt.name;}); @@ -594,7 +603,7 @@ namespace build2 // @@ OPT: move d, out, n // if (r == nullptr && insert) - r = &search (t, *tts[0], d, out, n, &e, nullptr); + r = &search (t, *tts[0], d, out, n, &e); return static_cast (r); }; @@ -695,7 +704,11 @@ namespace build2 // Maybe for diagnostics (i.e., we will actually try to build // something there instead of just saying no mapping). // - pt = find (pd / d, fp.leaf (), insert && !i->first.empty ()); + if (i->first.empty ()) + pt = find (pd / d, fp.leaf (), false); + else + pt = find (pd / d, fp.leaf (), insert, dynamic); + if (pt != nullptr) { fp = pd / fp; @@ -755,7 +768,7 @@ namespace build2 if (pt == nullptr) { l6 ([&]{trace << (insert ? "entering " : "finding ") << fp;}); - pt = find (fp.directory (), fp.leaf (), insert); + pt = find (fp.directory (), fp.leaf (), insert, dynamic); } } @@ -774,7 +787,7 @@ namespace build2 return enter_file_impl (trace, what, a, bs, t, fp, cache, norm, - true /* insert */, + true /* insert */, false, map_ext, fallback, pfx_map, so_map); } @@ -782,6 +795,7 @@ namespace build2 find_file (tracer& trace, const char* what, action a, const scope& bs, const target& t, path& fp, bool cache, bool norm, + bool dynamic, const function& map_ext, const target_type& fallback, const function& pfx_map, @@ -790,7 +804,7 @@ namespace build2 return enter_file_impl (trace, what, a, bs, t, fp, cache, norm, - false /* insert */, + false /* insert */, dynamic, map_ext, fallback, pfx_map, so_map); } diff --git a/libbuild2/dyndep.hxx b/libbuild2/dyndep.hxx index 0789e78..a6f800e 100644 --- a/libbuild2/dyndep.hxx +++ b/libbuild2/dyndep.hxx @@ -210,13 +210,14 @@ namespace build2 const srcout_map& = {}); // As above but do not insert the target if it doesn't already exist. This - // function also returns NULL if the target exists but is implied (that - // is, not declared in a buildfile). + // function also returns NULL if the target exists but is dynamic (that + // is, not real or implied), unless the dynamic argument is true. // static pair find_file (tracer&, const char* what, action, const scope& base, const target&, path& prerequisite, bool cache, bool normalized, + bool dynamic, const function&, const target_type& fallback, const function& = nullptr, diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx index 44e8538..f745f59 100644 --- a/libbuild2/target.hxx +++ b/libbuild2/target.hxx @@ -258,6 +258,9 @@ namespace build2 // Note that the order of the enumerators is arranged so that their // integral values indicate whether one "overrides" the other. // + // We refer to the targets other than real and implied as + // dynamically-created or just dynamic. + // // @@ We have cases (like pkg-config extraction) where it should probably be // prereq_file rather than implied (also audit targets.insert<> calls). // -- cgit v1.1