diff options
| -rw-r--r-- | bpkg/package-skeleton.cxx | 20 | ||||
| -rw-r--r-- | bpkg/package-skeleton.hxx | 14 | ||||
| -rw-r--r-- | bpkg/pkg-build.cxx | 255 |
3 files changed, 188 insertions, 101 deletions
diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx index 80df830..4813651 100644 --- a/bpkg/package-skeleton.cxx +++ b/bpkg/package-skeleton.cxx @@ -138,6 +138,26 @@ namespace bpkg // that it can be loaded if necessary. } + void package_skeleton:: + reset () + { + assert (db_ != nullptr); // Cannot be called after collect_config(). + + rs_ = nullptr; + ctx_ = nullptr; // Free. + + cmd_vars_.clear (); + cmd_vars_cache_ = false; + + dependent_vars_.clear (); + reflect_vars_.clear (); + reflect_frag_.clear (); + + dependency_reflect_.clear (); + dependency_reflect_index_ = 0; + dependency_reflect_pending_ = 0; + } + package_skeleton:: package_skeleton (const common_options& co, database& db, diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx index 0fc371d..d7cafe9 100644 --- a/bpkg/package-skeleton.hxx +++ b/bpkg/package-skeleton.hxx @@ -73,14 +73,13 @@ namespace bpkg // The following functions should be called in the following sequence // (* -- zero or more, ? -- zero or one): // - // * reload_defaults() - // * verify_sensible() + // * reload_defaults() | verify_sensible() // ? dependent_config() - // * evaluate_enable() | evaluate_reflect() + // * evaluate_*() // collect_config() // // Note that a copy of the skeleton is expected to continue with the - // sequence rather than starting from scratch. + // sequence rather than starting from scratch, unless reset() is called. // public: // Reload the default values and type information for configuration @@ -136,6 +135,13 @@ namespace bpkg evaluate_require (const dependency_configurations&, const string&, size_t depends_index); + // Reset the skeleton to the start of the call sequence. + // + // Note that this function cannot be called after collect_config(). + // + void + reset (); + // Return the accumulated configuration variables (first) and project // configuration variable sources (second). Note that the arrays are not // necessarily parallel (config_vars may contain non-project variables). diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx index 4194e3e..40228db 100644 --- a/bpkg/pkg-build.cxx +++ b/bpkg/pkg-build.cxx @@ -4478,7 +4478,7 @@ namespace bpkg // everything in this cluster is already negotiated) and we can // return true (no need to postpone any further steps). // - // But if we do have clusters not yet negotiated, or, worse, + // But if we merged clusters not yet negotiated, or, worse, // being in the middle of negotiation, then we need to get this // merged cluster into the fully negotiated state. The we do it // is by throwing merge_configuration (see below). @@ -4543,115 +4543,165 @@ namespace bpkg throw merge_configuration {cfg.depth}; } - if (r.second && !cfg.contains_shadow_dependent (pk, dp)) + // Up-negotiate the configuration and if it has changed, throw + // retry_configuration to the try/catch frame corresponding to + // the negotiation of the outermost merged cluster in order to + // retry the same steps (potentially refining the configuration + // as we go along) and likely (but not necessarily) ending up + // here again, at which point we up-negotiate again with the + // expectation that the configuration won't change (but if it + // does, then we throw again and do another refinement pass). + // + // In a sense, semantically, we should act like a one more + // iteration of the initial negotiation loop with the exception + // acting like a request to restart the refinement process from + // the beginning. + // + bool changed; { - assert (*r.second); // Would't be here otherwise. - - // The "first time" case. + // Similar to initial negotiation, resolve package skeletons + // for this dependent and its dependencies. // + package_skeleton* dept; + { + build_package* b (entered_build (pk)); + assert (b != nullptr && b->skeleton); + dept = &*b->skeleton; + } - // @@ TODO: if configuration has not changed, then we don't - // need to throw. Do it at the end (there is no harm in - // throwing even if not needed and it will change test - // trace). - // - // Also, we may have to throw multiple times if the - // configuration gets refined... In a sense, semantically, - // we should act like a one more iteration of the initial - // negotiation loop with the exception acting like a - // request to restart the refinement process from the - // beginning. - // - // It feels like the unchanged case semantics should be - // like falling through to the "second time" case. Though - // I wonder if there could be some missing steps, like - // collecting dependencies? Doesn't feel like it since - // we are re-using existing configuration which presumably - // means all this has already been done. - // - // Hm, won't we need to "reset" dependency package - // skeletons (which have by now potentially seen - // evaluate_*() calls) in order to negotiate. And if the - // configuration hasn't changed, then how will we restore - // them? Maybe we should make copies (that are "reset" to - // the initial state) and use that? + // If a dependency has already been recursively collected, + // then we can no longer call reload_defaults() or + // verify_sensible() on its skeleton. We could reset it, but + // then we wouldn't be able to continue using it if + // up_negotiate_configuration() below returns false. So it + // seems the most sensible approach is to make a temporary + // copy and reset that. // + small_vector<reference_wrapper<package_skeleton>, 1> depcs; + forward_list<package_skeleton> depcs_storage; // Ref stability. + { + depcs.reserve (cfg_deps.size ()); + for (const package_key& pk: cfg_deps) + { + build_package* b (entered_build (pk)); + assert (b != nullptr); + + package_skeleton* depc; + if (b->recursive_collection) + { + assert (b->skeleton); + depcs_storage.push_front (*b->skeleton); + depc = &depcs_storage.front (); + depc->reset (); + } + else + depc = &(b->skeleton + ? *b->skeleton + : b->init_skeleton (options)); + + depcs.push_back (*depc); + } + } + + changed = up_negotiate_configuration ( + cfg.dependency_configurations, *dept, dp, depcs); + } + + // If the configuration hasn't changed, then we carry on. + // Otherwise, retry the negotiation from the beginning to + // refine the resulting configuration (see the catch block + // for retry_configuration). + // + if (changed) + { l5 ([&]{trace << "cfg-postponing dependent " << pkg.available_name_version_db () << " involves (being) negotiated configurations " << "and results in " << cfg << ", throwing retry_configuration";}); - // up_negotiate (...); - // Don't print the "while satisfying..." chain. // dep_chain.clear (); throw retry_configuration {cfg.depth, move (pk), dp}; } - else + + l5 ([&]{trace << "configuration for cfg-postponed " + << "dependencies of dependent " + << pkg.available_name_version_db () << " " + << (r.second ? "" : "shadow-") << "negotiated";}); + + // Note that even in the fully negotiated case we may still add + // extra dependencies to this cluster which we still need to + // configure and recursively collect before indicating to the + // caller (returning true) that we are done with this depends + // value and the dependent is not postponed. + // + for (const package_key& p: cfg_deps) { - // The "second time" case. - // - assert (!r.second || // Shadow cluster was/is being negotiated. - *r.second); // Everything already negotiated. - - l5 ([&]{trace << "configuration for cfg-postponed " - << "dependencies of dependent " - << pkg.available_name_version_db () << " is " - << (r.second ? "" : "shadow-") << "negotiated";}); - - // Note that even in the fully negotiated case we may still - // add extra dependencies to this cluster which we still need - // to configure and recursively collect before indicating to - // the caller (returning true) that we are done with this - // depends value and the dependent is not postponed. - // - // @@ TODO call verify_sensible(), etc. DO NOW. - // - for (const package_key& p: cfg_deps) + build_package* b (entered_build (p)); + assert (b != nullptr); + + if (!b->recursive_collection) { - build_package* b (entered_build (p)); - assert (b != nullptr); + l5 ([&]{trace << "collecting cfg-postponed dependency " + << b->available_name_version_db () + << " of dependent " + << pkg.available_name_version_db ();}); - if (!b->recursive_collection) + // Similar to the inital negotiation case, verify and set + // the dependent configuration for this dependency. + // { - l5 ([&]{trace << "collecting cfg-postponed dependency " - << b->available_name_version_db () - << " of dependent " - << pkg.available_name_version_db ();}); - - collect_build_prerequisites (options, - *b, - fdb, - rpt_depts, - apc, - initial_collection, - replaced_vers, - dep_chain, - postponed_repo, - postponed_alts, - 0 /* max_alt_index */, - postponed_deps, - postponed_cfgs, - postponed_poss, - true /* force_configured */); + assert (b->skeleton); // Should have been init'ed above. + + const package_configuration& pc ( + cfg.dependency_configurations[p]); + + pair<bool, string> pr (b->skeleton->verify_sensible (pc)); + + if (!pr.first) + { + // @@ TODO: improve (see the other case). + // + fail << "unable to negotiate sensible configuration\n" + << " " << pr.second; + } + + b->skeleton->dependent_config (pc); } - else - l5 ([&]{trace << "dependency " - << b->available_name_version_db () - << " of dependent " - << pkg.available_name_version_db () - << " is already (being) recursively " - << "collected, skipping";}); - } - return true; + collect_build_prerequisites (options, + *b, + fdb, + rpt_depts, + apc, + initial_collection, + replaced_vers, + dep_chain, + postponed_repo, + postponed_alts, + 0 /* max_alt_index */, + postponed_deps, + postponed_cfgs, + postponed_poss, + true /* force_configured */); + } + else + l5 ([&]{trace << "dependency " + << b->available_name_version_db () + << " of dependent " + << pkg.available_name_version_db () + << " is already (being) recursively " + << "collected, skipping";}); } + + return true; #else - //@@ TMP: get rid of shadow_dependents if pans out. + //@@ TMP: get rid of shadow_dependents (including in + // retry_configuration) if pans out. // Case (3). // @@ -6124,14 +6174,15 @@ namespace bpkg { assert (b->skeleton); // Should have been init'ed above. - const package_configuration& cfg ( + const package_configuration& pc ( pcfg->dependency_configurations[p]); - pair<bool, string> pr (b->skeleton->verify_sensible (cfg)); + pair<bool, string> pr (b->skeleton->verify_sensible (pc)); if (!pr.first) { // @@ TODO: improve (print dependencies, dependents, config). + // (also in the up-negotiation case). // // Note that the diagnostics from the dependent will most // likely be in the "error ..." form (potentially with @@ -6142,7 +6193,7 @@ namespace bpkg << " " << pr.second; } - b->skeleton->dependent_config (cfg); + b->skeleton->dependent_config (pc); } build_package_refs dep_chain; @@ -6464,6 +6515,9 @@ namespace bpkg if (e.depth != pcd) throw; + package_configurations cfgs ( + move (pc->dependency_configurations)); + // Restore the state from snapshot. // // Note: postponed_cfgs is re-assigned. @@ -6480,22 +6534,29 @@ namespace bpkg << "to dependent " << e.dependent << ", adding " << "shadow dependent and re-negotiating";}); - // @@ TODO: need to copy dependency_configuration to the - // restored state. DO NOW. + // @@ TODO + // + // Note that there is also a possibility of having a "bogus" + // dependent/dependencies (i.e., we add them to the cluster but + // they never get re-visited). While seems harmless (or not), + // maybe we need to prune the part of the configuration that was + // added by them? Maybe we should have the confirmed flag which + // is set when the originating dependent confirms the value?! + + // Copy over the configuration for further refinement. + // + pc->dependency_configurations = move (cfgs); + +#if 0 + // @@ TMP drop if pans out. // Record the dependent/dependencies/position that triggered // this so that we don't repeat this up-negotiate/throw/catch // dance when we retry collect_build_postponed(). @@ Hm. could // this not be re-done via "unchanged configuration"? // - // Note that there is also a possibility of having a "bogus" - // dependent/dependencies (i.e., we add them to the cluster but - // they never get re-visited). @@ While seems harmless, maybe we - // need to prune the part of the configuration that was added - // by them? Maybe we should have the confirmed flag which is - // set when the originating dependent confirms the value?! - // pc->add_shadow (move (e.dependent), e.position); +#endif } catch (merge_configuration& e) { |