diff options
| -rw-r--r-- | bpkg/package-configuration.cxx | 15 | ||||
| -rw-r--r-- | bpkg/package-configuration.hxx | 17 | ||||
| -rw-r--r-- | bpkg/package-skeleton.cxx | 475 | ||||
| -rw-r--r-- | bpkg/package-skeleton.hxx | 24 |
4 files changed, 415 insertions, 116 deletions
diff --git a/bpkg/package-configuration.cxx b/bpkg/package-configuration.cxx index 42c17ab..e8ea13a 100644 --- a/bpkg/package-configuration.cxx +++ b/bpkg/package-configuration.cxx @@ -11,7 +11,7 @@ namespace bpkg { using build2::config::variable_origin; - static string + string serialize_cmdline (const string& name, const optional<build2::names>& value) { using namespace build2; @@ -35,19 +35,6 @@ namespace bpkg return r; } - - string config_variable_value:: - serialize_cmdline () const - { - return bpkg::serialize_cmdline (name, value); - } - - string dependent_config_variable_value:: - serialize_cmdline () const - { - return bpkg::serialize_cmdline (name, value); - } - void package_configuration:: print (diag_record& dr, const char* indent, diff --git a/bpkg/package-configuration.hxx b/bpkg/package-configuration.hxx index 0229f31..73f05ff 100644 --- a/bpkg/package-configuration.hxx +++ b/bpkg/package-configuration.hxx @@ -18,6 +18,11 @@ namespace bpkg { class package_skeleton; + // Serialize the variable value as a command line override. + // + string + serialize_cmdline (const string& name, const optional<build2::names>& value); + struct config_variable_value { string name; @@ -61,10 +66,11 @@ namespace bpkg confirmed = false; } - // Serialize the variable value as a command line override. - // string - serialize_cmdline () const; + serialize_cmdline () const + { + return bpkg::serialize_cmdline (name, value); + } }; void @@ -81,7 +87,10 @@ namespace bpkg public: string - serialize_cmdline () const; + serialize_cmdline () const + { + return bpkg::serialize_cmdline (name, value); + } }; inline bool diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx index 3034758..f202a5f 100644 --- a/bpkg/package-skeleton.cxx +++ b/bpkg/package-skeleton.cxx @@ -182,8 +182,7 @@ namespace bpkg cmd_vars_cache_ (v.cmd_vars_cache_), dependent_vars_ (move (v.dependent_vars_)), dependent_orgs_ (move (v.dependent_orgs_)), - reflect_vars_ (move (v.reflect_vars_)), - reflect_frag_ (move (v.reflect_frag_)), + reflect_ (move (v.reflect_)), dependency_reflect_ (move (v.dependency_reflect_)), dependency_reflect_index_ (v.dependency_reflect_index_), dependency_reflect_pending_ (v.dependency_reflect_pending_), @@ -220,8 +219,7 @@ namespace bpkg cmd_vars_cache_ = v.cmd_vars_cache_; dependent_vars_ = move (v.dependent_vars_); dependent_orgs_ = move (v.dependent_orgs_); - reflect_vars_ = move (v.reflect_vars_); - reflect_frag_ = move (v.reflect_frag_); + reflect_ = move (v.reflect_); dependency_reflect_ = move (v.dependency_reflect_); dependency_reflect_index_ = v.dependency_reflect_index_; dependency_reflect_pending_ = v.dependency_reflect_pending_; @@ -256,8 +254,7 @@ namespace bpkg cmd_vars_cache_ (v.cmd_vars_cache_), dependent_vars_ (v.dependent_vars_), dependent_orgs_ (v.dependent_orgs_), - reflect_vars_ (v.reflect_vars_), - reflect_frag_ (v.reflect_frag_), + reflect_ (v.reflect_), dependency_reflect_ (v.dependency_reflect_), dependency_reflect_index_ (v.dependency_reflect_index_), dependency_reflect_pending_ (v.dependency_reflect_pending_), @@ -289,8 +286,7 @@ namespace bpkg dependent_vars_.clear (); dependent_orgs_.clear (); - reflect_vars_.clear (); - reflect_frag_.clear (); + reflect_.clear (); dependency_reflect_.clear (); dependency_reflect_index_ = 0; @@ -380,36 +376,6 @@ namespace bpkg assert (!out_root); } - // Serialize a variable assignment for a buildfile fragment. - // - static void - serialize_buildfile (string& r, - const string& var, const build2::value& val, - build2::names& storage) - { - using namespace build2; - - r += var; - r += " = "; - - if (val.null) - r += "[null]"; - else - { - storage.clear (); - names_view nv (reverse (val, storage)); - - if (!nv.empty ()) - { - ostringstream os; - to_stream (os, nv, quote_mode::normal, '@'); - r += os.str (); - } - } - - r += '\n'; - } - // Serialize a variable assignment for a command line override. // static string @@ -490,7 +456,7 @@ namespace bpkg // Should only be called before dependent_config()/evaluate_*(). // assert (dependent_vars_.empty () && - reflect_vars_.empty () && + reflect_.empty () && dependency_reflect_.empty () && available != nullptr && ctx_ == nullptr); @@ -623,7 +589,7 @@ namespace bpkg // on system package without skeleton info. // assert (dependent_vars_.empty () && - reflect_vars_.empty () && + reflect_.empty () && dependency_reflect_.empty () && available == nullptr && system); @@ -712,7 +678,7 @@ namespace bpkg // Should only be called before dependent_config()/evaluate_*(). // assert (dependent_vars_.empty () && - reflect_vars_.empty () && + reflect_.empty () && dependency_reflect_.empty () && available != nullptr && ctx_ == nullptr); @@ -847,6 +813,8 @@ namespace bpkg scope& rs (load ()); + // Evaluate the enable condition. + // istringstream is ('(' + cond + ')'); is.exceptions (istringstream::failbit | istringstream::badbit); @@ -897,6 +865,8 @@ namespace bpkg } } +#if 1 + void package_skeleton:: evaluate_reflect (const string& refl, pair<size_t, size_t> indexes) { @@ -909,10 +879,10 @@ namespace bpkg // also argue we should diagnose this case and fail not to cause more // confusion. // - // @@ They could also clash with dependent configuration. Probably should - // be handled in the same way (it's just another type of "user"). Yes, - // since dependent_vars_ are entered as cmd line overrides, this is - // how they are treated (but may need to adjust the diagnostics). + // They could also clash with dependent configuration. Probably should be + // handled in the same way (it's just another type of "user"). Yes, since + // dependent_vars_ are entered as cmd line overrides, this is how they are + // treated. // // It seems like the most straightforward way to achieve the desired // semantics with the mechanisms that we have (in other words, without @@ -921,21 +891,329 @@ namespace bpkg // variables set by root.build in conditions, (2) override default values // of configuration variables (and those loaded from config.build), and // (3) be overriden by configuration variables specified by the user. - // Naturally, this approach is not without a few corner cases: + // Naturally, this approach is probably not without a few corner cases. // - // 1. Append in the reflect clause may not produce the desired result - // (i.e., it will append to the default value in root.build) rather - // than overriding it, as would have happen if it were a real variable - // override. + // We may also have configuration values from the previous reflect clause + // which we want to "factor in" before evaluating the next clause (enable, + // reflect etc.; so that they can use the previously reflected values or + // values that are derived from them in root.build). It seems like we have + // two options here: either enter them as true overrides similar to + // config_vars_ or just evaluate them similar to loading config.build + // (which, BTW, we might have, in case of an external package). The big + // problem with the former approach is that it will then prevent any + // further reflect clauses from modifying the same values. // - // config.hello.x ?= 1 # root.build - // config.hello.x += 2 # reflect clause + // So overall it feels like we have iterative/compartmentalized + // configuration process. A feedback loop, in a sense. And it's the + // responsibility of the package author (who is in control of both + // root.build and manifest) to arrange for suitable compartmentalization. + // + try + { + // Note: similar in many respects to evaluate_enable(). + // + using namespace build2; + using config::variable_origin; + using build2::diag_record; + using build2::fail; + using build2::info; + using build2::endf; + + // 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) + { + ctx_ = nullptr; + 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 ()); + + // Collect all the set config.<name>.* variables on the first pass and + // filter out unchanged on the second. + // + // Note: a lot of this code is inspired by the config module. + // + struct rvar + { + const variable* var; + const value* val; + size_t ver; + }; + + class rvars: public vector<rvar> + { + public: + pair<iterator, bool> + insert (const rvar& v) + { + auto i (find_if (begin (), end (), + [&v] (const rvar& o) {return v.var == o.var;})); + if (i != end ()) + return make_pair (i, false); + + push_back (v); + return make_pair (--end (), true); + } + }; + + rvars vars; + + auto process = [this, &rs, &vars] (bool collect) + { + // @@ TODO: would be nice to verify no bogus variables are set (can + // probably only be done via the saved variables map). + + for (auto p (rs.vars.lookup_namespace (var_prefix_)); + p.first != p.second; + ++p.first) + { + const variable& var (p.first->first); + + // This can be one of the overrides (__override, __prefix, etc), + // which we skip. + // + if (var.override ()) + continue; + + // What happens to version if overriden? A: appears to be still + // incremented! + // + const variable_map::value_data& val (p.first->second); + + if (collect) + vars.insert (rvar {&var, nullptr, val.version}); + else + { + auto p (vars.insert (rvar {&var, &val, 0})); + + if (!p.second) + { + auto i (p.first); + + if (i->ver == val.version) + vars.erase (i); // Unchanged. + else + i->val = &val; + } + } + } + }; + + // Evaluate the reflect clause. + // + istringstream is (refl); + is.exceptions (istringstream::failbit | istringstream::badbit); + + path_name in ("<depends-reflect-clause>"); + uint64_t il (1); + + // Note: keep it active until the end (see the override detection). + // + auto df = build2::make_diag_frame ( + [this, &refl, &rs, depends_index] (const build2::diag_record& dr) + { + // Probably safe to assume a one-line fragment contains a variable + // assignment. + // + if (refl.find ('\n') == string::npos) + dr << info << "reflect variable: " << trim (string (refl)); + else + dr << info << "reflect clause:\n" + << trim_right (string (refl)); + + // For external packages we have the manifest so print the + // location of the depends value in questions. + // + if (rs.out_eq_src ()) + dr << info << "in depends manifest value of package " + << package.name; + else + depends_location (dr, + rs.src_path () / manifest_file, + depends_index); + }); + + lexer l (is, in, il /* start line */); + buildfile_parser p (rs.ctx, + dependency_var_prefixes_, + dependency_var_prefixes_pending); + + process (true); + p.parse_buildfile (l, &rs, rs); + process (false); + + // Add to the vars set the reflect variables collected previously. + // + auto& vp (rs.var_pool ()); + for (const reflect_variable_value& v: reflect_) + { + const variable* var (vp.find (v.name)); + assert (var != nullptr); // Must be there (set by load()). + + auto p (vars.insert (rvar {var, nullptr, 0})); + + if (p.second) + p.first->val = rs.vars[*var].value; // Must be there (set by load()). + } + + // Re-populate everything from the var set but try to re-use buffers as + // much a possible (normally we would just be appending more variables + // at the end). + // + reflect_.resize (vars.size ()); + + // Collect the config.<name>.* variables that were changed by this + // and previous reflect clauses. + // + for (size_t i (0); i != vars.size (); ++i) + { + const variable& var (*vars[i].var); + const value& val (*vars[i].val); + + pair<variable_origin, lookup> ol ( + config::origin (rs, + var, + pair<lookup, size_t> { + lookup {val, var, rs.vars}, 1 /* depth */})); + + reflect_variable_value& v (reflect_[i]); + v.name = var.name; + v.origin = ol.first; + + if (ol.first == variable_origin::override_) + { + // Detect an overridden reflect value, but allowing it if the values + // match (think both user/dependent and reflect trying to enable the + // same feature). + // + // What would be the plausible scenarios for an override? + // + // 1. Append override that adds some backend or some such to the + // reflect value. + // + // 2. A reflect may enable a feature based on the dependency + // alternative selected (e.g., I see we are using Qt6 so we might + // as well enable feature X). The user may want do disable it + // with an override. + // + // Note also that a sufficiently smart reflect clause can detect if + // a variable is overridden (with $config.origin()) and avoid the + // clash. Perhaps that should be the recommendation for reflect + // variables that can also plausibly be set by the user (it feels + // like configuration variables have the intf/impl split similar + // to libraries)? + // + if (val != *ol.second) + { + // See if this is a dependent or user override. + // + const package_key* d (nullptr); + { + for (size_t i (0); i != dependent_vars_.size (); ++i) + { + const string& v (dependent_vars_[i]); + size_t n (var.name.size ()); + if (v.compare (0, n, var.name) == 0 && v[n] == '=') + { + d = &dependent_orgs_[i]; + break; + } + } + } + + diag_record dr (fail); + + dr << "reflect variable " << var << " overriden by "; + + if (d != nullptr) + dr << "dependent " << *d; + else + dr << "user configuration"; + + names storage; + dr << info << "reflect value: " + << serialize_cmdline (var.name, val, storage); + + dr << info << (d != nullptr ? "dependent" : "user") << " value: " + << serialize_cmdline (var.name, *ol.second, storage); + } + + // Skipped in load(), collect_config(), but used in print_config(). + } + else + { + assert (ol.first == variable_origin::buildfile); + + // Note that we keep it always untyped letting the config directives + // in root.build to take care of typing. + // + v.value = reverse_value (val); + } + } + + // Drop the build system state since it needs reloading (some computed + // values in root.build may depend on the new configuration values). + // + ctx_ = nullptr; + } + catch (const build2::failed&) + { + throw failed (); // Assume the diagnostics has already been issued. + } + } + +#else + + // @@ TMP: keep this in dep-config-neg but drop in master. + + void package_skeleton:: + evaluate_reflect (const string& refl, pair<size_t, size_t> indexes) + { + size_t depends_index (indexes.first); + + // The reflect configuration variables are essentially overrides that will + // be passed on the command line when we configure the package. They could + // clash with configuration variables specified by the user (config_vars_) + // and it feels like user values should take precedence. Though one could + // also argue we should diagnose this case and fail not to cause more + // confusion. + // + // They could also clash with dependent configuration. Probably should be + // handled in the same way (it's just another type of "user"). Yes, since + // dependent_vars_ are entered as cmd line overrides, this is how they are + // treated. + // + // It seems like the most straightforward way to achieve the desired + // semantics with the mechanisms that we have (in other words, without + // inventing another "level" of overrides) is to evaluate the reflect + // fragment after loading root.build. This way it will (1) be able to use + // variables set by root.build in conditions, (2) override default values + // of configuration variables (and those loaded from config.build), and + // (3) be overriden by configuration variables specified by the user. + // Naturally, this approach is probably not without a few corner cases. // // We may also have configuration values from the previous reflect clause - // which we want to "factor in" before evaluating the next enable or - // reflect clauses (so that they can use the previously reflected values - // or values that are derived from them in root.build). It seems like we - // have two options here: either enter them as true overrides similar to + // which we want to "factor in" before evaluating the next clause (enable, + // reflect etc.; so that they can use the previously reflected values or + // values that are derived from them in root.build). It seems like we have + // two options here: either enter them as true overrides similar to // config_vars_ or just evaluate them similar to loading config.build // (which, BTW, we might have, in case of an external package). The big // problem with the former approach is that it will then prevent any @@ -946,10 +1224,6 @@ namespace bpkg // responsibility of the package author (who is in control of both // root.build and manifest) to arrange for suitable compartmentalization. // - // BTW, a plan B would be to restrict reflect to just config vars in which - // case we could merge them with true overrides. Though how exactly would - // we do this merging is unclear. Hm, but they are config vars... - // try { // Note: similar in many respects to evaluate_enable(). @@ -1225,6 +1499,8 @@ namespace bpkg ovr = rs.lookup_override (var, org); #else // @@ TODO: probably also depends, not just user. + // @@ TODO: maybe we should allow if the same value (say a bool + // feature set to true but both)?! fail << "command line override of reflect clause variable " << var << endf; @@ -1250,6 +1526,8 @@ namespace bpkg } } +#endif + bool package_skeleton:: evaluate_prefer_accept (const dependency_configurations& cfgs, const string& prefer, @@ -1365,7 +1643,7 @@ namespace bpkg } } - // Evaluate the accept clause. + // Evaluate the accept condition. // bool r; { @@ -1884,7 +2162,7 @@ namespace bpkg load_old_config (); return (dependent_vars_.empty () && - reflect_vars_.empty () && + reflect_.empty () && find_if (config_vars_.begin (), config_vars_.end (), [this] (const string& v) { @@ -1909,6 +2187,8 @@ namespace bpkg void package_skeleton:: print_config (ostream& os, const char* indent) { + using build2::config::variable_origin; + if (!loaded_old_config_) load_old_config (); @@ -1927,6 +2207,12 @@ namespace bpkg // NOTE: see also empty_print() if changing anything here. + // @@ TODO: we could have added the package itself to "set by ..." + // for overriden (to the same value) reflect. But then why not + // do the same for dependent that is overriden by user (we could + // have kept them as reflect_variable_values rather than strings)? + // Maybe one day. + // First comes the user configuration. // for (const string& v: config_vars_) @@ -1960,11 +2246,15 @@ namespace bpkg << d << ')'; } - // Finally reflect. + // Finally reflect (but skip overriden). // - for (const string& v: reflect_vars_) + for (const reflect_variable_value& v: reflect_) { - print (v) << " (" << (system ? "expected" : "set") << " by " + if (v.origin == variable_origin::override_) + continue; + + string s (serialize_cmdline (v.name, v.value)); + print (s) << " (" << (system ? "expected" : "set") << " by " << package.name << ')'; } } @@ -1974,6 +2264,8 @@ namespace bpkg { assert (db_ != nullptr); // Must be called only once. + using build2::config::variable_origin; + if (!loaded_old_config_) load_old_config (); @@ -1985,7 +2277,7 @@ namespace bpkg if (size_t n = (config_vars_.size () + dependent_vars_.size () + - reflect_vars_.size ())) + reflect_.size ())) { // For vars we will steal the first non-empty *_vars_. But for sources // reserve the space. @@ -2058,23 +2350,20 @@ namespace bpkg // Finally reflect. // - if (!reflect_vars_.empty ()) + if (!reflect_.empty ()) { + vars.reserve (n); + // These are all project variables. There should also be no duplicates // by construction (see evaluate_reflect()). // - for (const string& v: reflect_vars_) - srcs.push_back ( - config_variable {var_name (v), config_source::reflect}); - - if (vars.empty ()) - vars = move (reflect_vars_); - else + for (const reflect_variable_value& v: reflect_) { - vars.reserve (n); - vars.insert (vars.end (), - make_move_iterator (reflect_vars_.begin ()), - make_move_iterator (reflect_vars_.end ())); + if (v.origin == variable_origin::override_) + continue; + + vars.push_back (serialize_cmdline (v.name, v.value)); + srcs.push_back (config_variable {v.name, config_source::reflect}); } } } @@ -2333,9 +2622,9 @@ namespace bpkg scope& rs (*rsi->second.front ()); // Load project's root.build as well as potentially accumulated reflect - // fragment. + // variables. // - // If we have the accumulated reflect fragment, wedge it just before + // If we have the accumulated reflect variables, wedge them just before // loading root.build (but after initializing config which may load // config.build and which we wish to override). // @@ -2356,11 +2645,11 @@ namespace bpkg bool defaults; } d {rs, cfgs, defaults}; - if (!reflect_frag_.empty () || - !dependency_reflect_.empty () || - !cfgs.empty ()) + if (!cfgs.empty () || + !reflect_.empty () || + !dependency_reflect_.empty ()) { - pre = [this, &d] (parser& p) + pre = [this, &d] (parser&) { scope& rs (d.rs); @@ -2378,16 +2667,18 @@ namespace bpkg return rs.var_pool ().insert (name, vt); }; - if (!reflect_frag_.empty ()) + for (const reflect_variable_value& v: reflect_) { - istringstream is (reflect_frag_); - is.exceptions (istringstream::failbit | istringstream::badbit); + if (v.origin == variable_origin::override_) + continue; - // Note that the fragment is just a bunch of variable assignments - // and thus unlikely to cause any errors. - // - path_name in ("<accumulated-reflect-fragment>"); - p.parse_buildfile (is, in, &rs, rs); + const variable& var (insert_var (v.name, v.type)); // Note: untyped. + value& val (rs.assign (var)); + + if (v.value) + val.assign (names (*v.value), &var); + else + val = nullptr; } // Note that for now we don't bother setting overridden reflect diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx index 1e274c0..7224d1d 100644 --- a/bpkg/package-skeleton.hxx +++ b/bpkg/package-skeleton.hxx @@ -266,6 +266,9 @@ namespace bpkg strings cmd_vars_; bool cmd_vars_cache_ = false; + strings dependent_vars_; // Dependent variable overrides. + vector<package_key> dependent_orgs_; // Dependent originators (parallel). + // Reflect variable value storage. Used for both real reflect and // dependency reflect. // @@ -277,13 +280,22 @@ namespace bpkg optional<build2::names> value; }; - using reflect_variable_values = vector<reflect_variable_value>; - - strings dependent_vars_; // Dependent variable overrides. - vector<package_key> dependent_orgs_; // Dependent originators (parallel). + class reflect_variable_values: public vector<reflect_variable_value> + { + public: + const reflect_variable_value* + find (const string& name) + { + auto i (find_if (begin (), end (), + [&name] (const reflect_variable_value& v) + { + return v.name == name; + })); + return i != end () ? &*i : nullptr; + } + }; - strings reflect_vars_; // Reflect variable overrides. - string reflect_frag_; // Reflect variables fragment. + reflect_variable_values reflect_; // Reflect variables. // Dependency configuration variables set by the prefer/require clauses // and that should be reflected in subsequent clauses. |