From bee8c4afc182e20de9b7e7bd907bee72cfa55889 Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Wed, 15 Nov 2023 22:11:03 +0300 Subject: Fix pkg-build by ignoring version replacement if it doesn't satisfy dependency constraints --- bpkg/pkg-build-collect.cxx | 108 +++++++++++++++++++++------------------------ bpkg/pkg-build-collect.hxx | 11 ++++- 2 files changed, 60 insertions(+), 59 deletions(-) (limited to 'bpkg') diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx index c0a592f..1eae206 100644 --- a/bpkg/pkg-build-collect.cxx +++ b/bpkg/pkg-build-collect.cxx @@ -1426,6 +1426,8 @@ namespace bpkg { using std::swap; // ...and not list::swap(). + using constraint_type = build_package::constraint_type; + tracer trace ("collect_build"); assert (pkg.repository_fragment == nullptr || @@ -1452,7 +1454,8 @@ namespace bpkg package_key pk (pkg.db, pkg.available->id.name); // Apply the version replacement, if requested, and indicate that it was - // applied. + // applied. Ignore the replacement if its version doesn't satisfy the + // dependency constraints specified by the caller. // auto vi (replaced_vers.find (pk)); @@ -1463,22 +1466,47 @@ namespace bpkg replaced_version& v (vi->second); - v.replaced = true; - if (v.available != nullptr) { - pkg.available = v.available; - pkg.repository_fragment = v.repository_fragment; - pkg.system = v.system; + const version& rv (v.system + ? *v.available->system_version (pk.db) + : v.available->version); - l5 ([&]{trace << "replacement: " - << pkg.available_name_version_db ();}); + bool replace (true); + for (const constraint_type& c: pkg.constraints) + { + if (!satisfies (rv, c.value)) + { + replace = false; + + l5 ([&]{trace << "replacement to " << rv << " is denied since " + << c.dependent << " depends on (" << pk.name << ' ' + << c.value << ')';}); + } + } + + if (replace) + { + v.replaced = true; + + pkg.available = v.available; + pkg.repository_fragment = v.repository_fragment; + pkg.system = v.system; + + l5 ([&]{trace << "replacement: " + << pkg.available_name_version_db ();}); + } } else { + v.replaced = true; + l5 ([&]{trace << "replacement: drop";}); - assert (pkg.selected != nullptr); + // We shouldn't be replacing a package build with the drop if someone + // depends on this package. + // + assert (pkg.selected != nullptr && !pkg.required_by_dependents); collect_drop (options, pkg.db, pkg.selected, replaced_vers); return nullptr; @@ -1486,51 +1514,18 @@ namespace bpkg } // Add the version replacement entry, call the verification function if - // specified, and throw replace_version, unless the package is in the - // unsatisfied dependents list on the dependency side and the version - // replacement doesn't satisfy the ignored constraint. In the later case, - // report the first encountered ignored (unsatisfied) dependency - // constraint and fail. + // specified, and throw replace_version. // - // The thinking for the above failure is as follows: if we add the - // replacement entry and throw, then on the next iteration there won't be - // any information preserved that this version is actually unsatisfactory - // for some dependent and we need to ignore the respective constraint - // during simulating the configuration of this dependent and to fail if - // the problem doesn't resolve naturally (see unsatisfied_depts for - // details). This approach may potentially end up with some failures which - // we could potentially avoid if postpone them for some longer. Let's, - // however, keep it simple for now and reconsider if it turn out to be an - // issue. + // Note that this package can potentially be present in the unsatisfied + // dependents list on the dependency side with the replacement version + // being unsatisfactory for the ignored constraint. In this case, during + // the from-scratch re-collection this replacement will be ignored if/when + // this package is collected with this constraint specified. But it can + // still be applied for some later collect_build() call or potentially + // turn out bogus. // - auto replace_ver = [&pk, - &vpb, - &vi, - &replaced_vers, - &unsatisfied_depts, - &trace, - this] (const build_package& p) + auto replace_ver = [&pk, &vpb, &vi, &replaced_vers] (const build_package& p) { - - const version& v (p.available_version ()); - - for (const unsatisfied_dependent& ud: unsatisfied_depts) - { - for (const auto& c: ud.ignored_constraints) - { - if (c.dependency == pk && !satisfies (v, c.constraint)) - { - l5 ([&]{trace << "dependency " << p.available_name_version_db () - << " is unsatisfactory for dependent " - << ud.dependent << " (" << p.name () << ' ' - << c.constraint << "), so the failure can't be " - << "postponed anymore";}); - - unsatisfied_depts.diag (*this); - } - } - } - replaced_version rv (p.available, p.repository_fragment, p.system); if (vi != replaced_vers.end ()) @@ -1601,13 +1596,13 @@ namespace bpkg // // If neither of the versions is satisfactory, then ignore those // unsatisfied constraints which prevent us from picking the package - // version which is currently in the map (this way we try to avoid the - // postponed failure; see replace_ver() lambda for details). + // version which is currently in the map. It feels that the version in + // the map is no worse than the other one and we choose it + // specifically for the sake of optimization, trying to avoid throwing + // the replace_version exception. // if (p1->available_version () != p2->available_version ()) { - using constraint_type = build_package::constraint_type; - // See if pv's version satisfies pc's constraints, skipping those // which are meant to be ignored (ics). Return the pointer to the // unsatisfied constraint or NULL if all are satisfied. @@ -1653,8 +1648,7 @@ namespace bpkg // command line and so p may not be NULL. If that (suddenly) // is not the case, then we will have to ignore the constraint // imposed by the dependent which is not in the map, replace - // the version, and as a result fail immediately (see - // replace_ver() lambda for details). + // the version, and call replace_ver(). // if (p == nullptr) { diff --git a/bpkg/pkg-build-collect.hxx b/bpkg/pkg-build-collect.hxx index 86878f0..ece3d90 100644 --- a/bpkg/pkg-build-collect.hxx +++ b/bpkg/pkg-build-collect.hxx @@ -1236,8 +1236,15 @@ namespace bpkg // replaced with the drop. So can be used as bool. // // Consult replaced_vers for an existing version replacement entry and - // follow it, if present, potentially collecting the package drop - // instead. Add entry to replaced_vers and throw replace_version if the + // follow it, if present, potentially collecting the package drop instead. + // Ignore the entry if its version doesn't satisfy the dependency + // constraints specified by the caller. In this case it's likely that this + // replacement will be applied for some later collect_build() call but can + // potentially turn out bogus. Note that a version replacement for a + // specific package may only be applied once during the collection + // iteration. + // + // Add entry to replaced_vers and throw replace_version if the // existing version needs to be replaced but the new version cannot be // re-collected recursively in-place (see replaced_versions for details). // -- cgit v1.1