diff options
| -rw-r--r-- | bpkg/package-skeleton.cxx | 226 | ||||
| -rw-r--r-- | bpkg/package-skeleton.hxx | 37 | ||||
| -rw-r--r-- | tests/pkg-build.testscript | 2 |
3 files changed, 202 insertions, 63 deletions
diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx index 6539077..72192ff 100644 --- a/bpkg/package-skeleton.cxx +++ b/bpkg/package-skeleton.cxx @@ -40,6 +40,90 @@ namespace bpkg void build2_init (const common_options&); +} + +namespace bpkg +{ + // Check whether the specified configuration variable override has a + // project variables (i.e., its name starts with config.<project>). + // + // Note that some user-specified variables may have qualifications + // (global, scope, etc) but there is no reason to expect any project + // configuration variables to use such qualifications (since they can + // only apply to one project). So we ignore all qualified variables. + // + static inline bool + project_override (const string& v, const string& p) + { + size_t n (p.size ()); + return v.compare (0, n, p) == 0 && strchr (".=+ \t", v[n]) != nullptr; + } + + // Check whether the specified configuration variable name is a project + // variables (i.e., its name starts with config.<project>). + // + static inline bool + project_variable (const string& v, const string& p) + { + size_t n (p.size ()); + return v.compare (0, n, p) == 0 && (v[n] == '.' || v[n] == '\0'); + } + + // Customized buildfile parser that is used to detect and diagnose + // references to undefined dependency configuration variables. + // + class buildfile_parser: public build2::parser + { + public: + buildfile_parser (build2::context& ctx, + const strings& dvps, + optional<size_t> dvps_pending = {}) + : parser (ctx), + dependency_var_prefixes_ (dvps), + dependency_var_prefixes_pending_ (dvps_pending) {} + + protected: + virtual build2::lookup + lookup_variable (build2::name&& qual, + string&& name, + const build2::location& loc) override + { + using namespace build2; + using build2::fail; + using build2::info; + + // To avoid making copies of the name, pre-check if it is from one + // of the dependencies. + // + optional<string> dep; + if (!pre_parse_ && qual.empty ()) + { + auto b (dependency_var_prefixes_.begin ()); + auto e (dependency_var_prefixes_pending_ + ? b + *dependency_var_prefixes_pending_ + : dependency_var_prefixes_.end ()); + + if (find_if (b, e, + [&name] (const string& p) + { + return project_variable (name, p); + }) != e) + dep = name; + } + + lookup l (parser::lookup_variable (move (qual), move (name), loc)); + + if (dep && !l.defined ()) + fail (loc) << "undefined dependency configuration variable " << *dep << + info << "was " << *dep << " set in earlier prefer or require clause?"; + + return l; + } + + private: + const strings& dependency_var_prefixes_; + optional<size_t> dependency_var_prefixes_pending_; + }; // Note: cannot be package_skeleton member function due to iterator return // (build2 stuff is only forward-declared in the header). @@ -77,6 +161,8 @@ namespace bpkg dependency_reflect_ (move (v.dependency_reflect_)), dependency_reflect_index_ (v.dependency_reflect_index_), dependency_reflect_pending_ (v.dependency_reflect_pending_), + dependency_var_prefixes_ (move (v.dependency_var_prefixes_)), + dependency_var_prefixes_pending_ (v.dependency_var_prefixes_pending_), prefer_accept_ (v.prefer_accept_) { v.db_ = nullptr; @@ -110,6 +196,8 @@ namespace bpkg dependency_reflect_ = move (v.dependency_reflect_); dependency_reflect_index_ = v.dependency_reflect_index_; dependency_reflect_pending_ = v.dependency_reflect_pending_; + dependency_var_prefixes_ = move (v.dependency_var_prefixes_); + dependency_var_prefixes_pending_ = v.dependency_var_prefixes_pending_; prefer_accept_ = v.prefer_accept_; v.db_ = nullptr; @@ -141,6 +229,8 @@ namespace bpkg dependency_reflect_ (v.dependency_reflect_), dependency_reflect_index_ (v.dependency_reflect_index_), dependency_reflect_pending_ (v.dependency_reflect_pending_), + dependency_var_prefixes_ (v.dependency_var_prefixes_), + dependency_var_prefixes_pending_ (v.dependency_var_prefixes_pending_), prefer_accept_ (v.prefer_accept_) { // The idea here is to create an "unloaded" copy but with enough state @@ -174,6 +264,9 @@ namespace bpkg dependency_reflect_index_ = 0; dependency_reflect_pending_ = 0; + dependency_var_prefixes_.clear (); + dependency_var_prefixes_pending_ = 0; + prefer_accept_ = nullopt; } @@ -395,13 +488,9 @@ namespace bpkg // package. And, could be helpful to warn that configuration variable // does not exist. // - const string& p (var_prefix_); - size_t n (p.size ()); - for (const variable& var: rs.ctx.var_pool) { - if (var.name.compare (0, n, p) != 0 || (var.name[n] != '.' && - var.name[n] != '\0')) + if (!project_variable (var.name, var_prefix_)) continue; using config::variable_origin; @@ -571,7 +660,7 @@ namespace bpkg dr << build2::info (build2::location (mf, nv.value_line, nv.value_column)) - << "in depends value defined here"; + << "in depends manifest value defined here"; break; } } @@ -616,21 +705,23 @@ namespace bpkg uint64_t il (1); auto df = build2::make_diag_frame ( - [&cond, &rs, depends_index] (const build2::diag_record& dr) + [this, &cond, &rs, depends_index] (const build2::diag_record& dr) { dr << info << "enable condition: (" << cond << ")"; // For external packages we have the manifest so print the location // of the depends value in questions. // - if (!rs.out_eq_src ()) + if (rs.out_eq_src ()) + dr << info << "in depends manifest value of package " << key.name; + else depends_location (dr, rs.src_path () / manifest_file, depends_index); }); lexer l (is, in, il /* start line */); - parser p (rs.ctx); + buildfile_parser p (rs.ctx, dependency_var_prefixes_); value v (p.parse_eval (l, rs, rs, parser::pattern_mode::expand)); try @@ -715,6 +806,7 @@ namespace bpkg // Drop the state from the previous evaluation of prefer/accept if it's // from the wrong position. // + optional<size_t> dependency_var_prefixes_pending; if (prefer_accept_) { if (*prefer_accept_ != indexes) @@ -723,12 +815,16 @@ namespace bpkg prefer_accept_ = nullopt; } else + { // This could theoretically happen if we make a copy of the skeleton // after evaluate_prefer_accept() and then attempt to continue with // the call on the copy to evaluate_reflect() passing the same // position. But it doesn't appear our usage should trigger this. // assert (ctx_ != nullptr); + + dependency_var_prefixes_pending = dependency_var_prefixes_pending_; + } } scope& rs (load ()); @@ -740,7 +836,7 @@ namespace bpkg uint64_t il (1); auto df = build2::make_diag_frame ( - [&refl, &rs, depends_index] (const build2::diag_record& dr) + [this, &refl, &rs, depends_index] (const build2::diag_record& dr) { // Probably safe to assume a one-line fragment contains a variable // assignment. @@ -754,7 +850,9 @@ namespace bpkg // For external packages we have the manifest so print the location // of the depends value in questions. // - if (!rs.out_eq_src ()) + if (rs.out_eq_src ()) + dr << info << "in depends manifest value of package " << key.name; + else depends_location (dr, rs.src_path () / manifest_file, depends_index); @@ -822,7 +920,9 @@ namespace bpkg process (true); lexer l (is, in, il /* start line */); - parser p (rs.ctx); + buildfile_parser p (rs.ctx, + dependency_var_prefixes_, + dependency_var_prefixes_pending); p.parse_buildfile (l, &rs, rs); process (false); @@ -1040,9 +1140,12 @@ namespace bpkg // // Additionally, for all origins we need to typify the variables. // - // All of this is done by load(). + // All of this is done by load(), including removing and returning the + // dependency variable prefixes (config.<project>) which we later add + // to dependency_var_prefixes_. // - scope& rs (load (cfgs)); + strings dvps; + scope& rs (load (cfgs, &dvps, true /* defaults */)); // Evaluate the prefer clause. // @@ -1054,7 +1157,7 @@ namespace bpkg uint64_t il (1); auto df = build2::make_diag_frame ( - [&prefer, &rs, depends_index] (const build2::diag_record& dr) + [this, &prefer, &rs, depends_index] (const build2::diag_record& dr) { dr << info << "prefer clause:\n" << trim_right (string (prefer)); @@ -1062,22 +1165,25 @@ namespace bpkg // For external packages we have the manifest so print the // location of the depends value in questions. // - if (!rs.out_eq_src ()) + if (rs.out_eq_src ()) + dr << info << "in depends manifest value of package " << key.name; + else depends_location (dr, rs.src_path () / manifest_file, depends_index); }); lexer l (is, in, il /* start line */); - parser p (rs.ctx); + buildfile_parser p (rs.ctx, dependency_var_prefixes_); p.parse_buildfile (l, &rs, rs); // Check if the dependent set any stray configuration variables. // - for (package_configuration& cfg: cfgs) + for (size_t i (0); i != cfgs.size (); ++i) { - string ns ("config." + cfg.package.name.variable ()); + package_configuration& cfg (cfgs[i]); + const string& ns (dvps[i]); // Parallel. for (auto p (rs.vars.lookup_namespace (ns)); p.first != p.second; ++p.first) @@ -1112,21 +1218,23 @@ namespace bpkg uint64_t il (1); auto df = build2::make_diag_frame ( - [&accept, &rs, depends_index] (const build2::diag_record& dr) + [this, &accept, &rs, depends_index] (const build2::diag_record& dr) { dr << info << "accept condition: (" << accept << ")"; - // For external packages we have the manifest so print the location - // of the depends value in questions. + // For external packages we have the manifest so print the + // location of the depends value in questions. // - if (!rs.out_eq_src ()) + if (rs.out_eq_src ()) + dr << info << "in depends manifest value of package " << key.name; + else depends_location (dr, rs.src_path () / manifest_file, depends_index); }); lexer l (is, in, il /* start line */); - parser p (rs.ctx); + buildfile_parser p (rs.ctx, dependency_var_prefixes_); value v (p.parse_eval (l, rs, rs, parser::pattern_mode::expand)); try @@ -1151,10 +1259,11 @@ namespace bpkg dependency_reflect_index_ = depends_index; dependency_reflect_pending_ = dependency_reflect_.size (); - for (package_configuration& cfg: cfgs) + for (size_t i (0); i != cfgs.size (); ++i) { - string ns ("config." + cfg.package.name.variable ()); + package_configuration& cfg (cfgs[i]); + const string& ns (dvps[i]); for (auto p (rs.vars.lookup_namespace (ns)); p.first != p.second; ++p.first) @@ -1262,6 +1371,15 @@ namespace bpkg } } + // Note that because we add it here, the following reflect clause will + // not be able to expand undefined values. We handle this by keeping a + // pending position. + // + dependency_var_prefixes_pending_ = dependency_var_prefixes_.size (); + dependency_var_prefixes_.insert (dependency_var_prefixes_.end (), + make_move_iterator (dvps.begin ()), + make_move_iterator (dvps.end ())); + // Note: do not drop the build system state yet since it should be // reused by the following reflect clause, if any. // @@ -1315,7 +1433,8 @@ namespace bpkg // information as well as overrides (see up_negotiate_configuration() // for details). // - scope& rs (load (cfgs, false /* defaults */)); + strings dvps; + scope& rs (load (cfgs, &dvps, false /* defaults */)); // Evaluate the require clause. // @@ -1327,7 +1446,7 @@ namespace bpkg uint64_t il (1); auto df = build2::make_diag_frame ( - [&require, &rs, depends_index] (const build2::diag_record& dr) + [this, &require, &rs, depends_index] (const build2::diag_record& dr) { dr << info << "require clause:\n" << trim_right (string (require)); @@ -1335,30 +1454,34 @@ namespace bpkg // For external packages we have the manifest so print the // location of the depends value in questions. // - if (!rs.out_eq_src ()) + if (rs.out_eq_src ()) + dr << info << "in depends manifest value of package " << key.name; + else depends_location (dr, rs.src_path () / manifest_file, depends_index); }); lexer l (is, in, il /* start line */); - parser p (rs.ctx); + buildfile_parser p (rs.ctx, dependency_var_prefixes_); p.parse_buildfile (l, &rs, rs); // Check for stray variables and enforce all the require restrictions // (bool, set to true, etc). // - for (package_configuration& cfg: cfgs) + for (size_t i (0); i != cfgs.size (); ++i) { - string ns ("config." + cfg.package.name.variable ()); + package_configuration& cfg (cfgs[i]); - // Note that because we didn't set any default (or other dependent) - // values, all the values we see are set by this dependent. - // + const string& ns (dvps[i]); // Parallel. for (auto p (rs.vars.lookup_namespace (ns)); p.first != p.second; ++p.first) { + // Note that because we didn't set any default (or other + // dependent) values, all the values we see are set by this + // dependent. + // const variable& var (p.first->first); // This can be one of the overrides (__override, __prefix, etc), @@ -1401,10 +1524,11 @@ namespace bpkg // First determine if acceptable. // bool r (true); - for (package_configuration& cfg: cfgs) + for (size_t i (0); i != cfgs.size (); ++i) { - string ns ("config." + cfg.package.name.variable ()); + package_configuration& cfg (cfgs[i]); + const string& ns (dvps[i]); for (auto p (rs.vars.lookup_namespace (ns)); p.first != p.second; ++p.first) @@ -1465,10 +1589,11 @@ namespace bpkg dependency_reflect_index_ = depends_index; dependency_reflect_pending_ = dependency_reflect_.size (); - for (package_configuration& cfg: cfgs) + for (size_t i (0); i != cfgs.size (); ++i) { - string ns ("config." + cfg.package.name.variable ()); + package_configuration& cfg (cfgs[i]); + const string& ns (dvps[i]); for (auto p (rs.vars.lookup_namespace (ns)); p.first != p.second; ++p.first) @@ -1517,6 +1642,10 @@ namespace bpkg } } } + + dependency_var_prefixes_.insert (dependency_var_prefixes_.end (), + make_move_iterator (dvps.begin ()), + make_move_iterator (dvps.end ())); } // Drop the build system state since it needs reloading (while it may @@ -1545,7 +1674,7 @@ namespace bpkg find_if (config_vars_.begin (), config_vars_.end (), [this] (const string& v) { - return project_override (v); + return project_override (v, var_prefix_); }) == config_vars_.end ()); } @@ -1572,7 +1701,7 @@ namespace bpkg // for (const string& v: config_vars_) { - if (project_override (v)) + if (project_override (v, var_prefix_)) print (v) << " (user configuration)"; } @@ -1639,7 +1768,7 @@ namespace bpkg // for (const string& v: config_vars_) { - if (project_override (v)) + if (project_override (v, var_prefix_)) { string n (var_name (v)); @@ -1870,7 +1999,7 @@ namespace bpkg } build2::scope& package_skeleton:: - load (const dependency_configurations& cfgs, bool defaults) + load (const dependency_configurations& cfgs, strings* dvps, bool defaults) { if (ctx_ != nullptr) { @@ -1893,6 +2022,7 @@ namespace bpkg // If we have any dependency configurations, then here we need to add // dependency configuration variables with the override origin to the // command line overrides (see evaluate_prefer_accept() for details). + // While at it, handle dependency variable prefixes. // strings dependency_vars; for (const package_configuration& cfg: cfgs) @@ -1902,6 +2032,16 @@ namespace bpkg if (v.origin == variable_origin::override_) dependency_vars.push_back (v.serialize_cmdline ()); } + + string p ("config." + cfg.package.name.variable ()); + + auto i (find (dependency_var_prefixes_.begin (), + dependency_var_prefixes_.end (), + p)); + if (i != dependency_var_prefixes_.end ()) + dependency_var_prefixes_.erase (i); + + dvps->push_back (move (p)); } // If there aren't any, then we can reuse already merged cmd_vars (they diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx index aa9b2a9..ad0fc42 100644 --- a/bpkg/package-skeleton.hxx +++ b/bpkg/package-skeleton.hxx @@ -201,10 +201,15 @@ namespace bpkg // If dependency configurations are specified, then typify the variables // and set their values. If defaults is false, then only typify the // variables and set overrides without setting the default/buildfile - // values. Note that buildfile values have value::extra set to 2. + // values. Note that buildfile values have value::extra set to 2. While + // at it, also remove from dependency_var_prefixes_ and add to + // dependency_var_prefixes variable prefixes (config.<project>) for + // the passed dependencies. // build2::scope& - load (const dependency_configurations& = {}, bool defaults = true); + load (const dependency_configurations& = {}, + strings* dependency_var_prefixes = nullptr, + bool defaults = true); // Merge command line variable overrides into one list (normally to be // passed to bootstrap()). @@ -217,23 +222,6 @@ namespace bpkg const strings& dependency_vars = {}, bool cache = false); - // Check whether the specified configuration variable override has a - // project variables (i.e., its name start with config.<project>). - // - // Note that some user-specified variables may have qualifications - // (global, scope, etc) but there is no reason to expect any project - // configuration variables to use such qualifications (since they can - // only apply to one project). So we ignore all qualified variables. - // - bool - project_override (const string& v) const - { - const string& p (var_prefix_); - size_t n (p.size ()); - - return v.compare (0, n, p) == 0 && strchr (".=+ \t", v[n]) != nullptr; - } - // Implementation details (public for bootstrap()). // public: @@ -299,6 +287,17 @@ namespace bpkg size_t dependency_reflect_index_ = 0; size_t dependency_reflect_pending_ = 0; + // List of variable prefixes (config.<project>) of all known dependencies. + // + // This information is used to detect and diagnose references to undefined + // dependency configuration variables (for example, those that were not + // set and therefore not reflected). The pending index is used to ignore + // the entries added by the last evaluate_prefer_accept() in the following + // reflect clause (see prefer_accept_ below for details). + // + strings dependency_var_prefixes_; + size_t dependency_var_prefixes_pending_; + // Position of the last successfully evaluated prefer/accept clauses. // // This information is used to make all (as opposed to only those set by diff --git a/tests/pkg-build.testscript b/tests/pkg-build.testscript index 8d3945f..d5f767b 100644 --- a/tests/pkg-build.testscript +++ b/tests/pkg-build.testscript @@ -4320,7 +4320,7 @@ test.options += --no-progress $* fax/ 2>>~%EOE% != 0; <depends-enable-clause>:1: error: invalid bool value: multiple names info: enable condition: (config.fax.libbiz = true) - % fax.manifest:10:10: info: in depends value defined here% + % fax.manifest:10:10: info: in depends manifest value defined here% info: while satisfying fax/1.0.0#3 EOE |