From 2ee693000ff6ea44cfd4fc51c7b058258056610a Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Mon, 20 Jun 2022 09:55:17 +0200 Subject: Drop old implementation of package_skeleton::evaluate_reflect() --- bpkg/package-skeleton.cxx | 351 ---------------------------------------------- 1 file changed, 351 deletions(-) diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx index 8b4b09f..d58c846 100644 --- a/bpkg/package-skeleton.cxx +++ b/bpkg/package-skeleton.cxx @@ -865,8 +865,6 @@ namespace bpkg } } -#if 1 - void package_skeleton:: evaluate_reflect (const string& refl, pair indexes) { @@ -1179,355 +1177,6 @@ namespace bpkg } } -#else - - // @@ TMP: keep this in dep-config-neg but drop in master. - - void package_skeleton:: - evaluate_reflect (const string& refl, pair 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 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. - // - // 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 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 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 ()); - - istringstream is (refl); - is.exceptions (istringstream::failbit | istringstream::badbit); - - path_name in (""); - uint64_t il (1); - - 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); - }); - - // Note: a lot of this code is inspired by the config module. - // - - // Collect all the set config..* variables on the first pass and - // filter out unchanged on the second. - // - auto& vp (rs.var_pool ()); - const string& ns (var_prefix_); - - struct value_data - { - const value* val; - size_t ver; - }; - - // @@ Maybe redo as small_vector? - // - map vars; - - auto process = [&rs, &ns, &vars] (bool collect) - { - 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; - - // What happens to version if overriden? A: appears to be still - // incremented! - // - const variable_map::value_data& val (p.first->second); - - if (collect) - { - vars.emplace (&var, value_data {nullptr, val.version}); - } - else - { - auto i (vars.find (&var)); - - if (i != vars.end ()) - { - if (i->second.ver == val.version) - vars.erase (i); // Unchanged. - else - i->second.val = &val; - } - else - vars.emplace (&var, value_data {&val, 0}); - } - } - }; - - process (true); - - lexer l (is, in, il /* start line */); - buildfile_parser p (rs.ctx, - dependency_var_prefixes_, - dependency_var_prefixes_pending); - p.parse_buildfile (l, &rs, rs); - - process (false); - - // Add to the map the reflect variables collected previously. Note that - // we can re-purpose the override since we re-populate it. - // - for (string& n: reflect_vars_) - { - // Transform `name=value` to just `name`. - // - { - size_t p (n.find ('=')); - assert (p != string::npos); // We've serialized it so must be there. - n.resize (p); - } - - auto p (vars.emplace (&vp.insert (move (n)), value_data {nullptr, 0})); - - if (p.second) - { - // The value got to be there since it's set by the accumulated - // fragment we've evaluated before root.build. - // - p.first->second.val = rs.vars[p.first->first].value; - } - } - - // Re-populate everything from the map. - // - reflect_vars_.clear (); - reflect_frag_.clear (); - - // Collect the config..* variables that were changed by this - // and previous reflect clauses. - // - // Specifically, we update both the accumulated fragment to be evaluated - // before root.build on the next load (if any) as well as the merged - // configuration variable overrides to be passed during the package - // configuration. Doing both of these now (even though one of them won't - // be needed) allows us to immediately drop the build context and - // release its memory. It also makes the implementation a bit simpler - // (see, for example, the copy constructor). - // - names storage; - for (const auto& p: vars) - { - const variable& var (*p.first); - const value& val (*p.second.val); - - // For the accumulated fragment we always save the original and let - // the standard overriding take its course. - // - serialize_buildfile (reflect_frag_, var.name, val, storage); - - // Note: this is currently disabled and is likely very outdated. - // - // For the accumulated overrides we have to merge user config_vars_ - // with reflect values. Essentially, we have three possibilities: - // - // 1. There is no corresponding reflect value for a user value. In - // this case we just copy over the user value. - // - // 2. There is no corresponding user value for a reflect value. In - // this case we just copy over the reflect value. - // - // 3. There are both reflect and user values. In this case we replace - // the user value with the final (overriden) value using plain - // assignment (`=`). We do it this way to cover append overrides, - // for example: - // - // config.hello.backend = foo # reflect - // config.hello.backend += bar # user - // - - // @@ Can't we redo it via origin() call like in other places? - // - pair org {lookup {val, var, rs.vars}, 1 /* depth */}; - pair ovr; - - if (var.overrides == nullptr) - ovr = org; // Case #2. - else - { - // NOTE: see also below if enabling this. - // -#if 0 - // Case #3. - // - // The override can come from two places: config_vars_ or one of the - // "global" sources (environment variable, default options file; see - // load() for details). The latter can only be a global override and - // can be found (together with global overrides from config_vars_) - // in context::global_var_overrides. - // - // It feels like mixing global overrides and reflect is misguided: - // we probably don't want to rewrite it with a global override (per - // case #3 above) since it will apply globally. So let's diagnose it - // for now. - // - { - const strings& ovs (ctx_->global_var_overrides); - auto i (find_if (ovs.begin (), ovs.end (), - [&var] (const string& o) - { - // TODO: extracting name is not easy. - })); - - if (i != ovs.end ()) - { - fail << "global override for reflect clause variable " << var << - info << "global override: " << *i; - } - } - - // Ok, this override must be present in config_vars_. - // - // @@ Extracting the name from config_vars_ and similar is not easy: - // they are buildfile fragments and context actually parses them. - // - // @@ What if we have multiple overrides? - // - // @@ What if it's some scoped override or some such (e.g., all - // these .../x=y, etc). - // - // @@ Does anything change if we have an override but it does not - // apply (i.e., ovr == org && var.overrides != nullptr)? - // - // @@ Perhaps a sensible approach is to start relaxing/allowing - // this for specific, sensible cases (e.g., single unqualified - // override)? - // - // 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. - // - 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; -#endif - } - - reflect_vars_.push_back ( - serialize_cmdline (var.name, *ovr.first, storage)); - } - -#if 0 - // TODO: copy over config_vars_ that are not in the map (case #1). -#endif - - // 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. - } - } - -#endif - bool package_skeleton:: evaluate_prefer_accept (const dependency_configurations& cfgs, const string& prefer, -- cgit v1.1