From 7cdfb696e37922f4740fb4b38d5a8773adcf3e38 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Wed, 15 Jun 2022 15:47:22 +0200 Subject: Don't print config.*.develop in plan if not used by package --- bpkg/package-skeleton.cxx | 222 ++++++++++++++++++++++++++++++++-------------- bpkg/package-skeleton.hxx | 14 +-- bpkg/pkg-build.cxx | 2 +- 3 files changed, 164 insertions(+), 74 deletions(-) diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx index 9459d9b..3bfd634 100644 --- a/bpkg/package-skeleton.cxx +++ b/bpkg/package-skeleton.cxx @@ -44,8 +44,9 @@ namespace bpkg namespace bpkg { - // Check whether the specified configuration variable override has a - // project variables (i.e., its name starts with config.). + // Check whether the specified configuration variable override has a project + // variable (i.e., its name starts with config.). If the last + // argument is not NULL, then set it to the length of the variable portion. // // Note that some user-specified variables may have qualifications // (global, scope, etc) but there is no reason to expect any project @@ -53,14 +54,33 @@ namespace bpkg // only apply to one project). So we ignore all qualified variables. // static inline bool - project_override (const string& v, const string& p) + project_override (const string& v, const string& p, size_t* l = nullptr) { size_t n (p.size ()); - return v.compare (0, n, p) == 0 && strchr (".=+ \t", v[n]) != nullptr; + + if (v.compare (0, n, p) == 0) + { + if (v[n] == '.') + { + if (l != nullptr) + *l = v.find_first_of ("=+ \t", n + 1); + + return true; + } + else if (strchr ("=+ \t", v[n]) != nullptr) + { + if (l != nullptr) + *l = n; + + return true; + } + } + + return false; } // Check whether the specified configuration variable name is a project - // variables (i.e., its name starts with config.). + // variable (i.e., its name starts with config.). // static inline bool project_variable (const string& v, const string& p) @@ -150,6 +170,8 @@ namespace bpkg out_root_ (move (v.out_root_)), created_ (v.created_), verified_ (v.verified_), + loaded_old_config_ (v.loaded_old_config_), + develop_ (v.develop_), ctx_ (move (v.ctx_)), rs_ (v.rs_), cmd_vars_ (move (v.cmd_vars_)), @@ -185,6 +207,8 @@ namespace bpkg out_root_ = move (v.out_root_); created_ = v.created_; verified_ = v.verified_; + loaded_old_config_ = v.loaded_old_config_; + develop_ = v.develop_; ctx_ = move (v.ctx_); rs_ = v.rs_; cmd_vars_ = move (v.cmd_vars_); @@ -220,6 +244,8 @@ namespace bpkg out_root_ (v.out_root_), created_ (v.created_), verified_ (v.verified_), + loaded_old_config_ (v.loaded_old_config_), + develop_ (v.develop_), cmd_vars_ (v.cmd_vars_), cmd_vars_cache_ (v.cmd_vars_cache_), dependent_vars_ (v.dependent_vars_), @@ -304,6 +330,17 @@ namespace bpkg config_srcs_ = nullptr; } + // We don't need to load old user configuration if there isn't any and + // there is no new project configuration specified by the user. + // + loaded_old_config_ = + (config_srcs_ == nullptr) && + find_if (config_vars_.begin (), config_vars_.end (), + [this] (const string& v) + { + return project_override (v, var_prefix_); + }) == config_vars_.end (); + if (src_root) { src_root_ = move (*src_root); @@ -429,7 +466,7 @@ namespace bpkg dependency_reflect_.empty () && ctx_ == nullptr); - if (config_srcs_ != nullptr) + if (!loaded_old_config_) load_old_config (); try @@ -483,12 +520,6 @@ namespace bpkg // variable), then the saved variables map seem like the natural place // to keep this information. // - // @@ One potentially-bogus config variable could be config.*.develop. - // Would have been nice not to drag it around if not used by the - // package. And, could be helpful to warn that configuration variable - // does not exist. But we cannot do it consistently since we don't - // always load the skeleton. - // for (const variable& var: rs.ctx.var_pool) { if (!project_variable (var.name, var_prefix_)) @@ -566,7 +597,7 @@ namespace bpkg dependency_reflect_.empty () && ctx_ == nullptr); - if (config_srcs_ != nullptr) + if (!loaded_old_config_) load_old_config (); try @@ -1668,9 +1699,9 @@ namespace bpkg } bool package_skeleton:: - empty () + empty_print () { - if (config_srcs_ != nullptr) + if (!loaded_old_config_) load_old_config (); return (dependent_vars_.empty () && @@ -1678,14 +1709,28 @@ namespace bpkg find_if (config_vars_.begin (), config_vars_.end (), [this] (const string& v) { - return project_override (v, var_prefix_); + // See print_config() for details. + // + size_t vn; + if (project_override (v, var_prefix_, &vn)) + { + if (!develop_) + { + size_t pn (var_prefix_.size ()); + if (v.compare (pn, vn - pn, ".develop") == 0) + return false; + } + + return true; + } + return false; }) == config_vars_.end ()); } void package_skeleton:: print_config (ostream& os, const char* indent) { - if (config_srcs_ != nullptr) + if (!loaded_old_config_) load_old_config (); auto print = [&os, @@ -1701,12 +1746,27 @@ namespace bpkg return os; }; + // NOTE: see also empty_print() if changing anything here. + // First comes the user configuration. // for (const string& v: config_vars_) { - if (project_override (v, var_prefix_)) + size_t vn; + if (project_override (v, var_prefix_, &vn)) + { + // To reduce the noise (e.g., during bdep-init), skip + // config..develop if the package doesn't use it. + // + if (!develop_) + { + size_t pn (var_prefix_.size ()); + if (v.compare (pn, vn - pn, ".develop") == 0) + continue; + } + print (v) << " (user configuration)"; + } } // Next dependent configuration. @@ -1732,7 +1792,7 @@ namespace bpkg { assert (db_ != nullptr); // Must be called only once. - if (config_srcs_ != nullptr) + if (!loaded_old_config_) load_old_config (); // Merge all the variables into a single list in the correct order @@ -1899,7 +1959,7 @@ namespace bpkg void package_skeleton:: load_old_config () { - assert (config_srcs_ != nullptr && ctx_ == nullptr); + assert (!loaded_old_config_ && ctx_ == nullptr); try { @@ -1909,7 +1969,7 @@ namespace bpkg // would be nice to optimize for the common case where the only load is // to get the old configuration (e.g., config.*.develop) as part of // collect_config(). So instead of calling merge_cmd_vars() we will do - // own (but consistent) thing. + // our own (but consistent) thing. // const strings* cmd_vars; { @@ -1918,14 +1978,22 @@ namespace bpkg const strings& vs1 (build2_cmd_vars); const strings& vs2 (config_vars_); - cmd_vars = (vs2.empty () ? &vs1 : vs1.empty () ? &vs2 : nullptr); + if (!disfigure_) + cmd_vars = (vs2.empty () ? &vs1 : vs1.empty () ? &vs2 : nullptr); if (cmd_vars == nullptr) { // Note: the order is important (see merge_cmd_vars()). // - cmd_vars_.reserve (vs1.size () + vs2.size ()); - cmd_vars_.assign (vs1.begin (), vs1.end ()); + cmd_vars_.reserve ((disfigure_ ? 1 : 0) + vs1.size () + vs2.size ()); + + // If the package is being disfigured, then don't load config.build + // at all. + // + if (disfigure_) + cmd_vars_.push_back ("config.config.unload=true"); + + cmd_vars_.insert (cmd_vars_.end (), vs1.begin (), vs1.end ()); cmd_vars_.insert (cmd_vars_.end (), vs2.begin (), vs2.end ()); cmd_vars = &cmd_vars_; @@ -1938,61 +2006,81 @@ namespace bpkg // load_root (rs); + if (const variable* var = rs.var_pool ().find (var_prefix_ + ".develop")) + { + // Use the fact that the variable is typed as a proxy for it being + // defined with config directive (the more accurate way would be via + // the config module's saved variables map). + // + develop_ = (var->type != nullptr); + } + + // @@ TODO: should we also verify user-specified project configuration + // variables are not bogus? But they could be untyped... + // + // Also, build2 warns about unused variables being dropped. + + // Extract and merge old user configuration variables from config.build // (or equivalent) into config_vars. // - auto i (config_vars_.begin ()); // Insert position, see below. - - names storage; - for (const config_variable& v: *config_srcs_) + if (config_srcs_ != nullptr) { - if (v.source != config_source::user) - continue; - - using config::variable_origin; + assert (!disfigure_); - pair ol (config::origin (rs, v.name)); + auto i (config_vars_.begin ()); // Insert position, see below. - switch (ol.first) + names storage; + for (const config_variable& v: *config_srcs_) { - case variable_origin::override_: - { - // Already in config_vars. - // - // @@ TODO: theoretically, this could be an append/prepend - // override(s) and to make this work correctly we would need - // to replace them with an assign override with the final - // value. Maybe one day. - // - break; - } - case variable_origin::buildfile: - { - // Doesn't really matter where we add them though conceptually - // feels like old should go before new (and in the original - // order). - // - i = config_vars_.insert ( - i, - serialize_cmdline (v.name, *ol.second, storage)) + 1; + if (v.source != config_source::user) + continue; - break; - } - case variable_origin::undefined: - case variable_origin::default_: + using config::variable_origin; + + pair ol (config::origin (rs, v.name)); + + switch (ol.first) { - // Old user configuration no longer in config.build. We could - // complain but that feels overly drastic. Seeing that we will - // recalculate the new set of config variable sources, let's - // just ignore this (we could issue a warning, but who knows how - // many times it will be issued with all this backtracking). - // - break; + case variable_origin::override_: + { + // Already in config_vars. + // + // @@ TODO: theoretically, this could be an append/prepend + // override(s) and to make this work correctly we would need + // to replace them with an assign override with the final + // value. Maybe one day. + // + break; + } + case variable_origin::buildfile: + { + // Doesn't really matter where we add them though conceptually + // feels like old should go before new (and in the original + // order). + // + i = config_vars_.insert ( + i, + serialize_cmdline (v.name, *ol.second, storage)) + 1; + + break; + } + case variable_origin::undefined: + case variable_origin::default_: + { + // Old user configuration no longer in config.build. We could + // complain but that feels overly drastic. Seeing that we will + // recalculate the new set of config variable sources, let's + // just ignore this (we could issue a warning, but who knows how + // many times it will be issued with all this backtracking). + // + break; + } } } } - config_srcs_ = nullptr; + loaded_old_config_ = true; verified_ = true; // Managed to load without errors. ctx_ = nullptr; } @@ -2015,7 +2103,7 @@ namespace bpkg ctx_ = nullptr; } - if (config_srcs_ != nullptr) + if (!loaded_old_config_) load_old_config (); try diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx index ad0fc42..2250f57 100644 --- a/bpkg/package-skeleton.hxx +++ b/bpkg/package-skeleton.hxx @@ -143,12 +143,10 @@ namespace bpkg reset (); // Return true if there are no accumulated *project* configuration - // variables meaning that print_config() will not print anything while - // collect_config() will return an empty list of project configuration - // variable sources. + // variables that will be printed by print_config(). // bool - empty (); + empty_print (); // Print the accumulated *project* configuration variables as command line // overrides one per line with the specified indentation. @@ -184,7 +182,8 @@ namespace bpkg private: // Load old user configuration variables from config.build (or equivalent) - // and merge them into config_vars_. + // and merge them into config_vars_. Also verify new user configuration + // already in config_vars_ makes sense. // // This should be done before any attempt to load the configuration with // config.config.disfigure and, if this did not happen, inside @@ -242,6 +241,9 @@ namespace bpkg bool created_ = false; bool verified_ = false; + bool loaded_old_config_; + bool develop_ = false; // Package has config.*.develop. + unique_ptr ctx_; build2::scope* rs_ = nullptr; @@ -296,7 +298,7 @@ namespace bpkg // reflect clause (see prefer_accept_ below for details). // strings dependency_var_prefixes_; - size_t dependency_var_prefixes_pending_; + size_t dependency_var_prefixes_pending_ = 0; // Position of the last successfully evaluated prefer/accept clauses. // diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx index 9c10817..318ce91 100644 --- a/bpkg/pkg-build.cxx +++ b/bpkg/pkg-build.cxx @@ -12005,7 +12005,7 @@ namespace bpkg if (!rb.empty ()) act += " (" + cause + rb + ')'; - if (cfg != nullptr && !cfg->empty ()) + if (cfg != nullptr && !cfg->empty_print ()) { ostringstream os; cfg->print_config (os, o.print_only () ? " " : " "); -- cgit v1.1