From 64015e757529ae7ee8f3dd2444fb7444b436fefb Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Tue, 7 Jun 2022 06:59:14 +0200 Subject: Implementation of dependency reflect --- bpkg/package-configuration.hxx | 12 +- bpkg/package-skeleton.cxx | 339 +++++++++++++++++++++++++++-------------- bpkg/package-skeleton.hxx | 34 ++++- 3 files changed, 256 insertions(+), 129 deletions(-) diff --git a/bpkg/package-configuration.hxx b/bpkg/package-configuration.hxx index ba7240e..84f5256 100644 --- a/bpkg/package-configuration.hxx +++ b/bpkg/package-configuration.hxx @@ -33,23 +33,19 @@ namespace bpkg // build2::config::variable_origin origin; + // Variable type name with absent signifying untyped. + // + optional type; + // If origin is not undefined, then this is the reversed variable value // with absent signifying NULL. // optional value; - // Variable type name with absent signifying untyped. - // - optional type; - // If origin is buildfile, then this is the "originating dependent" which // first set this variable to this value. // optional dependent; - - // Value version (used internally by package_skeleton). - // - size_t version; }; class package_configuration: public vector diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx index 24f155c..8e71a53 100644 --- a/bpkg/package-skeleton.cxx +++ b/bpkg/package-skeleton.cxx @@ -71,7 +71,10 @@ namespace bpkg cmd_vars_cache_ (v.cmd_vars_cache_), dependent_vars_ (move (v.dependent_vars_)), reflect_vars_ (move (v.reflect_vars_)), - reflect_frag_ (move (v.reflect_frag_)) + reflect_frag_ (move (v.reflect_frag_)), + dependency_reflect_ (move (v.dependency_reflect_)), + dependency_reflect_index_ (v.dependency_reflect_index_), + dependency_reflect_pending_ (v.dependency_reflect_pending_) { v.db_ = nullptr; } @@ -99,6 +102,9 @@ namespace bpkg dependent_vars_ = move (v.dependent_vars_); reflect_vars_ = move (v.reflect_vars_); reflect_frag_ = move (v.reflect_frag_); + dependency_reflect_ = move (v.dependency_reflect_); + dependency_reflect_index_ = v.dependency_reflect_index_; + dependency_reflect_pending_ = v.dependency_reflect_pending_; v.db_ = nullptr; } @@ -123,7 +129,10 @@ namespace bpkg cmd_vars_cache_ (v.cmd_vars_cache_), dependent_vars_ (v.dependent_vars_), reflect_vars_ (v.reflect_vars_), - reflect_frag_ (v.reflect_frag_) + reflect_frag_ (v.reflect_frag_), + dependency_reflect_ (v.dependency_reflect_), + dependency_reflect_index_ (v.dependency_reflect_index_), + dependency_reflect_pending_ (v.dependency_reflect_pending_) { // The idea here is to create an "unloaded" copy but with enough state // that it can be loaded if necessary. @@ -301,6 +310,7 @@ namespace bpkg // assert (dependent_vars_.empty () && reflect_vars_.empty () && + dependency_reflect_.empty () && ctx_ == nullptr); if (config_srcs_ != nullptr) @@ -380,7 +390,7 @@ namespace bpkg case variable_origin::override_: case variable_origin::undefined: { - config_variable_value v {var.name, ol.first, {}, {}, {}, 0}; + config_variable_value v {var.name, ol.first, {}, {}, {}}; // Override could mean user override from config_vars_ or the // dependent override that we have merged above. @@ -435,6 +445,7 @@ namespace bpkg // assert (dependent_vars_.empty () && reflect_vars_.empty () && + dependency_reflect_.empty () && ctx_ == nullptr); if (config_srcs_ != nullptr) @@ -932,6 +943,8 @@ namespace bpkg const string& accept, size_t depends_index) { + assert (dependency_reflect_index_ <= depends_index); + try { using namespace build2; @@ -940,6 +953,12 @@ namespace bpkg using build2::info; using build2::endf; + // Drop any dependency reflect values from the previous evaluation of + // this clause, if any. + // + if (dependency_reflect_index_ == depends_index) + dependency_reflect_.resize (dependency_reflect_pending_); + // This is what needs to happen to the variables of different origins in // the passed dependency configurations: // @@ -1053,12 +1072,14 @@ namespace bpkg // If acceptable, update the configuration with the new values, if any. // - // @@ TODO: we also need to save the subset of values that were "set" - // (for some definition of "set") by this dependent to be reflected - // to further clauses. + // We also save the subset of values that were set by this dependent to + // be reflected to further clauses. // if (r) { + dependency_reflect_index_ = depends_index; + dependency_reflect_pending_ = dependency_reflect_.size (); + for (package_configuration& cfg: cfgs) { string ns ("config." + cfg.package.name.variable ()); @@ -1072,7 +1093,7 @@ namespace bpkg if (var.override ()) continue; - const variable_map::value_data& val (p.first->second); + const value& val (p.first->second); pair ol ( config::origin (rs, @@ -1083,22 +1104,38 @@ namespace bpkg config_variable_value& v (*cfg.find (var.name)); // An override cannot become a non-override. And a non-override - // cannot become an override. + // cannot become an override. Except that the dependency override + // could be specified (only) for the dependent. // - assert ((v.origin == variable_origin::override_) == - (ol.first == variable_origin::override_)); + if (v.origin == variable_origin::override_) + { + assert (ol.first == variable_origin::override_); + } + else if (ol.first == variable_origin::override_ && + v.origin != variable_origin::override_) + { + fail << "dependency override " << var.name << " specified for " + << "dependent " << key.string () << " but not dependency" << + info << "did you mean to specify ?" << cfg.package.name + << " +{ " << var.name << "=... }"; + } switch (ol.first) { case variable_origin::buildfile: { + optional ns (reverse_value (val)); + + // This value was set so save it as a dependency reflect. + // + dependency_reflect_.push_back ( + reflect_variable_value {v.name, ol.first, v.type, ns}); + // Possible transitions: // // default/undefine -> buildfile -- override dependency default // buildfile -> buildfile -- override other dependent // - optional ns (reverse_value (val)); - if (v.origin == variable_origin::buildfile) { // If unchanged, then we keep the old originating dependent @@ -1116,23 +1153,34 @@ namespace bpkg break; } case variable_origin::default_: - case variable_origin::undefined: { - // An undefined can only come from undefined. Likewise, - // default can only come from default. + // A default can only come from a default. // assert (ol.first == v.origin); break; } case variable_origin::override_: { - // @@ We may still need to see if there is original set - // by this dependent and if so, reflect the overridden - // value. + // If the value was set by this dependent then we need to + // reflect it even if it was overridden (but as the overridden + // value). Note that the mere presence of the value in rs.vars + // is not enough to say that it was set -- it could also be + // the default value. But we can detect that by examining + // value::extra. // - // Maybe we don't need the version: the presence of the - // value in rs is enough. + if (val.extra == 0) + { + dependency_reflect_.push_back ( + reflect_variable_value { + v.name, ol.first, v.type, reverse_value (*ol.second)}); + } + break; + } + case variable_origin::undefined: + { + // Not possible since we have the defined original. // + assert (false); break; } } @@ -1158,6 +1206,8 @@ namespace bpkg evaluate_require (const dependency_configurations& cfgs, const string& require, size_t depends_index) { + assert (dependency_reflect_index_ <= depends_index); + try { using namespace build2; @@ -1166,6 +1216,12 @@ namespace bpkg using build2::info; using build2::endf; + // Drop any dependency reflect values from the previous evaluation of + // this clause, if any. + // + if (dependency_reflect_index_ == depends_index) + dependency_reflect_.resize (dependency_reflect_pending_); + // A require clause can only set bool configuration variables and only // to true and may not have any conditions on other configuration // variables (including their origin). As a result, we don't need to set @@ -1177,85 +1233,104 @@ namespace bpkg // Evaluate the require clause. // - istringstream is (require); - is.exceptions (istringstream::failbit | istringstream::badbit); + { + istringstream is (require); + is.exceptions (istringstream::failbit | istringstream::badbit); - path_name in (""); - uint64_t il (1); + path_name in (""); + uint64_t il (1); - // Note: keep the diag frame beyond parse_buildfile(). - // - auto df = build2::make_diag_frame ( - [&require, &rs, depends_index] (const build2::diag_record& dr) + auto df = build2::make_diag_frame ( + [&require, &rs, depends_index] (const build2::diag_record& dr) + { + dr << info << "require clause:\n" + << trim_right (string (require)); + + // For external packages we have the manifest so print the + // location of the depends value in questions. + // + if (!rs.out_eq_src ()) + depends_location (dr, + rs.src_path () / manifest_file, + depends_index); + }); + + lexer l (is, in, il /* start line */); + parser p (rs.ctx); + 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) { - dr << info << "require clause:\n" - << trim_right (string (require)); + string ns ("config." + cfg.package.name.variable ()); - // For external packages we have the manifest so print the - // location of the depends value in questions. + // Note that because we didn't set any default (or other dependent) + // values, all the values we see are set by this dependent. // - if (!rs.out_eq_src ()) - depends_location (dr, - rs.src_path () / manifest_file, - depends_index); - }); + for (auto p (rs.vars.lookup_namespace (ns)); + p.first != p.second; + ++p.first) + { + const variable& var (p.first->first); - lexer l (is, in, il /* start line */); - parser p (rs.ctx); - p.parse_buildfile (l, &rs, rs); + // This can be one of the overrides (__override, __prefix, etc), + // which we skip. + // + if (var.override ()) + continue; + + const config_variable_value* v (cfg.find (var.name)); + + if (v == nullptr) + { + fail << "package " << cfg.package.name << " has no configuration " + << "variable " << var.name << + info << var.name << " set in require clause of dependent " + << key.string (); + } + + if (!v->type || *v->type != "bool") + { + fail << "configuration variable " << var.name << " is not of " + << "bool type" << + info << var.name << " set in require clause of dependent " + << key.string (); + } + + const value& val (p.first->second); + + if (!cast_false (val)) + { + fail << "configuration variable " << var.name << " is not set " + << "to true" << + info << var.name << " set in require clause of dependent " + << key.string (); + } + } + } + } // First determine if acceptable. // - // While at it, also check for stray variables and enforce all the - // require restrictions (bool, set to true, etc). - // bool r (true); for (package_configuration& cfg: cfgs) { string ns ("config." + cfg.package.name.variable ()); - // Note that because we didn't set any default (or other dependent) - // values, all the values we see are set by this dependent. - // for (auto p (rs.vars.lookup_namespace (ns)); 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; - const config_variable_value* v (cfg.find (var.name)); - - if (v == nullptr) - { - fail << "package " << cfg.package.name << " has no configuration " - << "variable " << var.name << - info << var.name << " set in require clause of dependent " - << key.string (); - } - - if (!v->type || *v->type != "bool") - { - fail << "configuration variable " << var.name << " is not of " - << "bool type" << - info << var.name << " set in require clause of dependent " - << key.string (); - } - const value& val (p.first->second); - if (!cast_false (val)) - { - fail << "configuration variable " << var.name << " is not set " - << "to true" << - info << var.name << " set in require clause of dependent " - << key.string (); - } + const config_variable_value& v (*cfg.find (var.name)); // The only situation where the result would not be acceptable is if // one of the values were overridden to false. @@ -1267,10 +1342,21 @@ namespace bpkg lookup {val, var, rs.vars}, 1 /* depth */})); // An override cannot become a non-override. And a non-override - // cannot become an override. + // cannot become an override. Except that the dependency override + // could be specified (only) for the dependent. // - assert ((v->origin == variable_origin::override_) == - (ol.first == variable_origin::override_)); + if (v.origin == variable_origin::override_) + { + assert (ol.first == variable_origin::override_); + } + else if (ol.first == variable_origin::override_ && + v.origin != variable_origin::override_) + { + fail << "dependency override " << var.name << " specified for " + << "dependent " << key.string () << " but not dependency" << + info << "did you mean to specify ?" << cfg.package.name + << " +{ " << var.name << "=... }"; + } if (ol.first == variable_origin::override_) { @@ -1285,12 +1371,14 @@ namespace bpkg // Note that we cannot easily combine this loop with the above because // we should not modify configurations if the result is not acceptable. // - // @@ TODO: we also need to save the subset of values that were "set" - // (for some definition of "set") by this dependent to be reflected - // to further clauses. + // We also save the subset of values that were set by this dependent to + // be reflected to further clauses. // if (r) { + dependency_reflect_index_ = depends_index; + dependency_reflect_pending_ = dependency_reflect_.size (); + for (package_configuration& cfg: cfgs) { string ns ("config." + cfg.package.name.variable ()); @@ -1306,6 +1394,17 @@ namespace bpkg config_variable_value& v (*cfg.find (var.name)); + // This value was set so save it as a dependency reflect. + // + // Note that unlike the equivalent evaluate_prefer_accept() logic, + // here the value cannot be the default (since we don't set + // defaults). + // + optional ns (names {build2::name ("true")}); + + dependency_reflect_.push_back ( + reflect_variable_value {v.name, v.origin, v.type, ns}); + if (v.origin != variable_origin::override_) { // Possible transitions: @@ -1313,9 +1412,6 @@ namespace bpkg // default/undefine -> buildfile -- override dependency default // buildfile -> buildfile -- override other dependent // - optional ns (names {build2::name ("true")}); - - // @@ TODO: reflect (before continue) if (v.origin == variable_origin::buildfile) { @@ -1332,11 +1428,6 @@ namespace bpkg v.value = move (ns); v.dependent = key; // We are the originating dependent. } - else - { - // @@ TODO: reflect (true). Could probably handle with the - // same code as !=override case. - } } } } @@ -1406,9 +1497,7 @@ namespace bpkg return string (v, 0, p); }; - // Note that for now we assume the three sets of variables do not clash. - // - // @@ TODO: make sure it's the case for dependent. + // Note that we assume the three sets of variables do not clash. // // First comes the user configuration. @@ -1446,7 +1535,7 @@ namespace bpkg if (!dependent_vars_.empty ()) { // These are all project variables. There should also be no duplicates - // by construction (see @@). + // by construction. // for (const string& v: dependent_vars_) srcs.push_back ( @@ -1677,15 +1766,12 @@ namespace bpkg // command line overrides (see evaluate_prefer_accept() for details). // strings dependency_vars; - for (package_configuration& cfg: cfgs) + for (const package_configuration& cfg: cfgs) { - for (config_variable_value& v: cfg) + for (const config_variable_value& v: cfg) { if (v.origin == variable_origin::override_) - { dependency_vars.push_back (serialize_cmdline (v.name, v.value)); - v.version = 0; // No value, only override. - } } } @@ -1725,12 +1811,28 @@ namespace bpkg bool defaults; } d {rs, cfgs, defaults}; - if (!reflect_frag_.empty () || !cfgs.empty ()) + if (!reflect_frag_.empty () || + !dependency_reflect_.empty () || + !cfgs.empty ()) { pre = [this, &d] (parser& p) { scope& rs (d.rs); + auto insert_var = [&rs] (const string& name, + const optional& type) + -> const variable& + { + const value_type* vt (nullptr); + if (type) + { + vt = parser::find_value_type (&rs, *type); + assert (vt != nullptr); + } + + return rs.var_pool ().insert (name, vt); + }; + if (!reflect_frag_.empty ()) { istringstream is (reflect_frag_); @@ -1743,18 +1845,28 @@ namespace bpkg p.parse_buildfile (is, in, &rs, rs); } - for (package_configuration& cfg: d.cfgs) + // Note that for now we don't bother setting overridden reflect + // values as overrides. It seems the only reason to go through the + // trouble would be to get the accurate $origin() result. But basing + // any decisions on whether the reflect value was overridden or not + // seems far fetched. + // + for (const reflect_variable_value& v: dependency_reflect_) { - for (config_variable_value& v: cfg) - { - const value_type* vt (nullptr); - if (v.type) - { - vt = parser::find_value_type (&rs, *v.type); - assert (vt != nullptr); - } + const variable& var (insert_var (v.name, v.type)); + value& val (rs.assign (var)); - const variable& var (rs.var_pool ().insert (v.name, vt)); + if (v.value) + val.assign (names (*v.value), &var); + else + val = nullptr; + } + + for (const package_configuration& cfg: d.cfgs) + { + for (const config_variable_value& v: cfg) + { + const variable& var (insert_var (v.name, v.type)); switch (v.origin) { @@ -1774,20 +1886,11 @@ namespace bpkg if (v.origin == variable_origin::default_) val.extra = 1; - - v.version = val.version; } - else - v.version = 0; - break; } case variable_origin::undefined: - { - v.version = 0; - break; - } - case variable_origin::override_: break; // Already handled. + case variable_origin::override_: break; } } } diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx index e949784..0fc371d 100644 --- a/bpkg/package-skeleton.hxx +++ b/bpkg/package-skeleton.hxx @@ -179,9 +179,8 @@ namespace bpkg // Call this function before evaluating every clause. // // If dependency configurations are specified, then typify the variables - // and set their values saving the resulting value versions in - // config_variable_value::version. If defaults is false, then only typify - // the variables and set overrides without setting the default/buildfile + // and set their values. If defaults is false, then only typify the + // variables and set overrides without setting the default/buildfile // values. // build2::scope& @@ -226,9 +225,38 @@ namespace bpkg strings cmd_vars_; bool cmd_vars_cache_ = false; + // Reflect variable value storage. Used for both real reflect and + // dependency reflect. + // + struct reflect_variable_value + { + string name; + build2::config::variable_origin origin; + optional type; + optional value; + }; + + using reflect_variable_values = vector; + strings dependent_vars_; // Dependent configuration variable overrides. strings reflect_vars_; // Reflect configuration variable overrides. string reflect_frag_; // Reflect configuration variables fragment. + + // Dependency configuration variables set by the prefer/require clauses + // and that should be reflected in subsequent clauses. + // + // The same prefer/require clause could be re-evaluated multiple times in + // which case the previous dependency reflect values from this clause (but + // not from any previous clauses) should be dropped. This is achieved by + // keeping track of the depends_index for the most recently evaluated + // prefer/require clause along with the position of the first element that + // was added by this clause. Note also that this logic does the right + // thing if we move to a different dependency alternative withing the same + // depends value. + // + reflect_variable_values dependency_reflect_; + size_t dependency_reflect_index_ = 0; + size_t dependency_reflect_pending_ = 0; }; } -- cgit v1.1