From 6cb9d0e810c3336106b6c1f3c8a80cdec6fbdcf0 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Wed, 22 Nov 2017 11:31:35 +0200 Subject: Fix dist bug where missing source file would be silently ignored --- build2/algorithm.hxx | 5 +++++ build2/algorithm.ixx | 35 +++++++++++++++++------------------ build2/dist/operation.cxx | 16 +++++++--------- build2/dist/rule.cxx | 39 ++++++++++++++++++++++++++++++++++++--- build2/search.cxx | 33 ++++++++++++++++++++++++--------- build2/search.hxx | 7 +++++-- build2/target.cxx | 2 +- build2/target.hxx | 12 ++++++------ 8 files changed, 101 insertions(+), 48 deletions(-) diff --git a/build2/algorithm.hxx b/build2/algorithm.hxx index dbc7d9b..ff70b21 100644 --- a/build2/algorithm.hxx +++ b/build2/algorithm.hxx @@ -29,6 +29,11 @@ namespace build2 const target* search_existing (const prerequisite&); + // As above but cache a target searched in a custom way. + // + const target& + search_custom (const prerequisite&, const target&); + // As above but specify the prerequisite to search as a key. // const target& diff --git a/build2/algorithm.ixx b/build2/algorithm.ixx index 928386d..565414a 100644 --- a/build2/algorithm.ixx +++ b/build2/algorithm.ixx @@ -23,16 +23,7 @@ namespace build2 const target* r (p.target.load (memory_order_consume)); if (r == nullptr) - { - r = &search (t, p.key ()); - - const target* e (nullptr); - if (!p.target.compare_exchange_strong ( - e, r, - memory_order_release, - memory_order_consume)) - assert (e == r); - } + r = &search_custom (p, search (t, p.key ())); return *r; } @@ -49,20 +40,28 @@ namespace build2 r = search_existing (p.key ()); if (r != nullptr) - { - const target* e (nullptr); - if (!p.target.compare_exchange_strong ( - e, r, - memory_order_release, - memory_order_consume)) - assert (e == r); - } + search_custom (p, *r); } return r; } inline const target& + search_custom (const prerequisite& p, const target& t) + { + assert (phase == run_phase::match || phase == run_phase::execute); + + const target* e (nullptr); + if (!p.target.compare_exchange_strong ( + e, &t, + memory_order_release, + memory_order_consume)) + assert (e == &t); + + return t; + } + + inline const target& search (const target& t, const target_type& tt, const prerequisite_key& k) { return search ( diff --git a/build2/dist/operation.cxx b/build2/dist/operation.cxx index f42217e..6692156 100644 --- a/build2/dist/operation.cxx +++ b/build2/dist/operation.cxx @@ -79,8 +79,8 @@ namespace build2 fail << "in-tree distribution of target " << t << info << "distribution requires out-of-tree build"; - // Make sure we have the necessary configuration before - // we get down to business. + // Make sure we have the necessary configuration before we get down to + // business. // auto l (rs->vars["dist.root"]); @@ -155,10 +155,9 @@ namespace build2 } } - // Add buildfiles that are not normally loaded as part of the - // project, for example, the export stub. They will still be - // ignored on the next step if the user explicitly marked them - // nodist. + // Add buildfiles that are not normally loaded as part of the project, + // for example, the export stub. They will still be ignored on the next + // step if the user explicitly marked them dist=false. // auto add_adhoc = [&trace] (const scope& rs, const path& f) { @@ -204,9 +203,8 @@ namespace build2 } } - // Collect the files. We want to take the snapshot of targets - // since updating some of them may result in more targets being - // entered. + // Collect the files. We want to take the snapshot of targets since + // updating some of them may result in more targets being entered. // action_targets files; const variable& dist_var (var_pool["dist"]); diff --git a/build2/dist/rule.cxx b/build2/dist/rule.cxx index ab00d41..b288a66 100644 --- a/build2/dist/rule.cxx +++ b/build2/dist/rule.cxx @@ -36,12 +36,45 @@ namespace build2 if (p.proj ()) continue; - const target& pt (p.search (t)); + // We used to always search and match but that resulted in the + // undesirable behavior in case one of the "source" files is + // missing. In this case we would enter a target as "output", this + // rule would match it, and then dist_execute() would ignore it by + // default. + // + // So now if this is a file target (we still want to always "see + // through" other targets like aliases), we will only match it if (1) + // it exists in src or (2) it exists as a target. It feels like we + // don't need the stronger "... and not implied" condition since if it + // is mentioned as a target, then it is in out (we don't do the same + // target in both src/out). + // + const target* pt (nullptr); + if (p.is_a ()) + { + pt = p.load (); + + if (pt == nullptr) + { + // Search for an existing target or existing file in src. + // + const prerequisite_key& k (p.key ()); + pt = k.tk.type->search (t, k); + + if (pt == nullptr) + fail << "prerequisite " << k << " is not existing source file " + << "nor known output target" << endf; + + search_custom (p.prerequisite, *pt); // Cache. + } + } + else + pt = &p.search (t); // Don't match targets that are outside of our project. // - if (pt.dir.sub (out_root)) - build2::match (a, pt); + if (pt->dir.sub (out_root)) + build2::match (a, *pt); } return noop_recipe; // We will never be executed. diff --git a/build2/search.cxx b/build2/search.cxx index 892141a..a7d40c6 100644 --- a/build2/search.cxx +++ b/build2/search.cxx @@ -86,7 +86,15 @@ namespace build2 tracer trace ("search_existing_file"); const target_key& ctk (cpk.tk); - assert (ctk.dir->relative ()); + const scope* s (cpk.scope); + + if (ctk.dir->absolute ()) + { + // Bail out if not inside project's src_root. + // + if (s == nullptr || !ctk.dir->sub (s->root_scope ()->src_path ())) + return nullptr; + } // Figure out the extension. Pretty similar logic to file::derive_path(). // @@ -95,7 +103,7 @@ namespace build2 if (!ext) { if (auto f = ctk.type->extension) - ext = f (ctk, *cpk.scope, true); + ext = f (ctk, *s, true); if (!ext) { @@ -118,14 +126,21 @@ namespace build2 // Check if there is a file. // - const dir_path& s (pk.scope->src_path ()); + path f; - path f (s); - if (!tk.dir->empty ()) + if (tk.dir->absolute ()) + f = *tk.dir; // Already normalized. + else { - f /= *tk.dir; - f.normalize (); + f = s->src_path (); + + if (!tk.dir->empty ()) + { + f /= *tk.dir; + f.normalize (); + } } + f /= *tk.name; if (!ext->empty ()) @@ -163,8 +178,8 @@ namespace build2 if (tk.out->empty ()) { - if (pk.scope->out_path () != s) - out = out_src (d, *pk.scope->root_scope ()); + if (s->out_path () != s->src_path ()) + out = out_src (d, *s->root_scope ()); } else out = *tk.out; diff --git a/build2/search.hxx b/build2/search.hxx index 043a216..79c7853 100644 --- a/build2/search.hxx +++ b/build2/search.hxx @@ -18,8 +18,11 @@ namespace build2 const target* search_existing_target (const prerequisite_key&); - // Search for an existing file in the scope's src directory. Prerequisite - // directory should be relative. + // Search for an existing file. If the prerequisite directory is relative, + // then look in the scope's src directory. Otherwise, if the absolute + // directory is inside the project's root scope, look there. In case of + // the absolute directory, if the scope is NULL, assume the file is not + // in src. // // Originally the plan was to have a target-type specific variable that // contains the search paths. But there wasn't any need for this yet. diff --git a/build2/target.cxx b/build2/target.cxx index d81ccd5..64ab412 100644 --- a/build2/target.cxx +++ b/build2/target.cxx @@ -640,7 +640,7 @@ namespace build2 // Then look for an existing file in the src tree. // - return pk.tk.dir->relative () ? search_existing_file (pk) : nullptr; + return search_existing_file (pk); } optional diff --git a/build2/target.hxx b/build2/target.hxx index 0ed1aac..e541667 100644 --- a/build2/target.hxx +++ b/build2/target.hxx @@ -935,12 +935,6 @@ namespace build2 } const target_type* - load (memory_order mo = memory_order_consume) - { - return target != nullptr ? target : prerequisite.target.load (mo); - } - - const target_type* search_existing () const { return target != nullptr @@ -948,6 +942,12 @@ namespace build2 : build2::search_existing (prerequisite); } + const target_type* + load (memory_order mo = memory_order_consume) + { + return target != nullptr ? target : prerequisite.target.load (mo); + } + // Return as a new prerequisite instance. // prerequisite_type -- cgit v1.1