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 ++- tests/common/satisfy/libbaz-1.2.0.tar.gz | Bin 0 -> 400 bytes tests/common/satisfy/libbaz-2.1.0.tar.gz | Bin 0 -> 386 bytes tests/common/satisfy/libfoo-3.0.0.tar.gz | Bin 0 -> 360 bytes tests/common/satisfy/libfox-3.0.0.tar.gz | Bin 0 -> 379 bytes tests/common/satisfy/t4j/libbar-0.1.0.tar.gz | 1 + tests/common/satisfy/t4j/libbar-1.2.0.tar.gz | 1 + tests/common/satisfy/t4j/libbaz-1.2.0.tar.gz | 1 + tests/common/satisfy/t4j/libbaz-2.1.0.tar.gz | 1 + tests/common/satisfy/t4j/libfoo-3.0.0.tar.gz | 1 + tests/common/satisfy/t4j/libfox-3.0.0.tar.gz | 1 + tests/common/satisfy/t4j/repositories.manifest | 1 + tests/pkg-build.testscript | 99 +++++++++++++++++++++++ tests/pkg-build/t4j | 1 + 15 files changed, 167 insertions(+), 59 deletions(-) create mode 100644 tests/common/satisfy/libbaz-1.2.0.tar.gz create mode 100644 tests/common/satisfy/libbaz-2.1.0.tar.gz create mode 100644 tests/common/satisfy/libfoo-3.0.0.tar.gz create mode 100644 tests/common/satisfy/libfox-3.0.0.tar.gz create mode 120000 tests/common/satisfy/t4j/libbar-0.1.0.tar.gz create mode 120000 tests/common/satisfy/t4j/libbar-1.2.0.tar.gz create mode 120000 tests/common/satisfy/t4j/libbaz-1.2.0.tar.gz create mode 120000 tests/common/satisfy/t4j/libbaz-2.1.0.tar.gz create mode 120000 tests/common/satisfy/t4j/libfoo-3.0.0.tar.gz create mode 120000 tests/common/satisfy/t4j/libfox-3.0.0.tar.gz create mode 100644 tests/common/satisfy/t4j/repositories.manifest create mode 120000 tests/pkg-build/t4j 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). // diff --git a/tests/common/satisfy/libbaz-1.2.0.tar.gz b/tests/common/satisfy/libbaz-1.2.0.tar.gz new file mode 100644 index 0000000..86b6e25 Binary files /dev/null and b/tests/common/satisfy/libbaz-1.2.0.tar.gz differ diff --git a/tests/common/satisfy/libbaz-2.1.0.tar.gz b/tests/common/satisfy/libbaz-2.1.0.tar.gz new file mode 100644 index 0000000..7567b65 Binary files /dev/null and b/tests/common/satisfy/libbaz-2.1.0.tar.gz differ diff --git a/tests/common/satisfy/libfoo-3.0.0.tar.gz b/tests/common/satisfy/libfoo-3.0.0.tar.gz new file mode 100644 index 0000000..55dc602 Binary files /dev/null and b/tests/common/satisfy/libfoo-3.0.0.tar.gz differ diff --git a/tests/common/satisfy/libfox-3.0.0.tar.gz b/tests/common/satisfy/libfox-3.0.0.tar.gz new file mode 100644 index 0000000..0bc246e Binary files /dev/null and b/tests/common/satisfy/libfox-3.0.0.tar.gz differ diff --git a/tests/common/satisfy/t4j/libbar-0.1.0.tar.gz b/tests/common/satisfy/t4j/libbar-0.1.0.tar.gz new file mode 120000 index 0000000..f622e36 --- /dev/null +++ b/tests/common/satisfy/t4j/libbar-0.1.0.tar.gz @@ -0,0 +1 @@ +../libbar-0.1.0.tar.gz \ No newline at end of file diff --git a/tests/common/satisfy/t4j/libbar-1.2.0.tar.gz b/tests/common/satisfy/t4j/libbar-1.2.0.tar.gz new file mode 120000 index 0000000..b4a7773 --- /dev/null +++ b/tests/common/satisfy/t4j/libbar-1.2.0.tar.gz @@ -0,0 +1 @@ +../libbar-1.2.0.tar.gz \ No newline at end of file diff --git a/tests/common/satisfy/t4j/libbaz-1.2.0.tar.gz b/tests/common/satisfy/t4j/libbaz-1.2.0.tar.gz new file mode 120000 index 0000000..d43cdcd --- /dev/null +++ b/tests/common/satisfy/t4j/libbaz-1.2.0.tar.gz @@ -0,0 +1 @@ +../libbaz-1.2.0.tar.gz \ No newline at end of file diff --git a/tests/common/satisfy/t4j/libbaz-2.1.0.tar.gz b/tests/common/satisfy/t4j/libbaz-2.1.0.tar.gz new file mode 120000 index 0000000..11cd8c8 --- /dev/null +++ b/tests/common/satisfy/t4j/libbaz-2.1.0.tar.gz @@ -0,0 +1 @@ +../libbaz-2.1.0.tar.gz \ No newline at end of file diff --git a/tests/common/satisfy/t4j/libfoo-3.0.0.tar.gz b/tests/common/satisfy/t4j/libfoo-3.0.0.tar.gz new file mode 120000 index 0000000..7678898 --- /dev/null +++ b/tests/common/satisfy/t4j/libfoo-3.0.0.tar.gz @@ -0,0 +1 @@ +../libfoo-3.0.0.tar.gz \ No newline at end of file diff --git a/tests/common/satisfy/t4j/libfox-3.0.0.tar.gz b/tests/common/satisfy/t4j/libfox-3.0.0.tar.gz new file mode 120000 index 0000000..2aef930 --- /dev/null +++ b/tests/common/satisfy/t4j/libfox-3.0.0.tar.gz @@ -0,0 +1 @@ +../libfox-3.0.0.tar.gz \ No newline at end of file diff --git a/tests/common/satisfy/t4j/repositories.manifest b/tests/common/satisfy/t4j/repositories.manifest new file mode 100644 index 0000000..5b70556 --- /dev/null +++ b/tests/common/satisfy/t4j/repositories.manifest @@ -0,0 +1 @@ +: 1 diff --git a/tests/pkg-build.testscript b/tests/pkg-build.testscript index fe01161..f4c79dc 100644 --- a/tests/pkg-build.testscript +++ b/tests/pkg-build.testscript @@ -127,6 +127,15 @@ # | |-- libbar-0.1.0.tar.gz # | `-- repositories.manifest # | +# |-- t4j +# | |-- libbar-0.1.0.tar.gz +# | |-- libbar-1.2.0.tar.gz +# | |-- libfoo-3.0.0.tar.gz -> libbar +# | |-- libfox-3.0.0.tar.gz -> libbar == 0.1.0, libbaz == 1.2.0 +# | |-- libbaz-1.2.0.tar.gz -> libbar == 1.2.0 +# | |-- libbaz-2.1.0.tar.gz +# | `-- repositories.manifest +# | # |-- t5 # | |-- libbar-1.2.0.tar.gz # | |-- libbox-1.2.0.tar.gz @@ -509,6 +518,7 @@ posix = ($cxx.target.class != 'windows') cp -r $src/t4e $out/t4e && $rep_create $out/t4e &$out/t4e/packages.manifest cp -r $src/t4f $out/t4f && $rep_create $out/t4f &$out/t4f/packages.manifest cp -r $src/t4i $out/t4i && $rep_create $out/t4i &$out/t4i/packages.manifest + cp -r $src/t4j $out/t4j && $rep_create $out/t4j &$out/t4j/packages.manifest cp -r $src/t5 $out/t5 && $rep_create $out/t5 &$out/t5/packages.manifest cp -r $src/t6 $out/t6 && $rep_create $out/t6 &$out/t6/packages.manifest cp -r $src/t7a $out/t7a && $rep_create $out/t7a &$out/t7a/packages.manifest @@ -4016,6 +4026,95 @@ test.arguments += --sys-no-query } } + : version-replacement + : + { + +$clone_root_cfg + +$rep_add $rep/t4j && $rep_fetch + + : denied + : + { + $clone_cfg; + + $* libbaz libfoo libfox --verbose 5 2>>~%EOE% != 0 + %.* + trace: pkg_build: refine package collection/plan execution from scratch + trace: collect_build: add libbaz/2.1.0 + trace: collect_build: add libfoo/3.0.0 + trace: collect_build: add libfox/3.0.0 + trace: collect_build_prerequisites: begin libbaz/2.1.0 + trace: collect_build_prerequisites: end libbaz/2.1.0 + trace: collect_build_prerequisites: begin libfoo/3.0.0 + trace: collect_build: add libbar/1.2.0 + trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libfoo/3.0.0 + trace: collect_build_prerequisites: begin libbar/1.2.0 + trace: collect_build_prerequisites: end libbar/1.2.0 + trace: collect_build_prerequisites: end libfoo/3.0.0 + trace: collect_build_prerequisites: begin libfox/3.0.0 + trace: collect_build: pick libbar/0.1.0 over libbar/1.2.0 + trace: collect_build: libbar/1.2.0 package version needs to be replaced with libbar/0.1.0 + trace: pkg_build: collection failed due to package version replacement, retry from scratch + trace: pkg_build: refine package collection/plan execution from scratch + trace: collect_build: add libbaz/2.1.0 + trace: collect_build: add libfoo/3.0.0 + trace: collect_build: add libfox/3.0.0 + trace: collect_build_prerequisites: begin libbaz/2.1.0 + trace: collect_build_prerequisites: end libbaz/2.1.0 + trace: collect_build_prerequisites: begin libfoo/3.0.0 + trace: collect_build: apply version replacement for libbar/1.2.0 + trace: collect_build: replacement: libbar/0.1.0 + trace: collect_build: add libbar/0.1.0 + trace: collect_build_prerequisites: no cfg-clause for dependency libbar/0.1.0 of dependent libfoo/3.0.0 + trace: collect_build_prerequisites: begin libbar/0.1.0 + trace: collect_build_prerequisites: end libbar/0.1.0 + trace: collect_build_prerequisites: end libfoo/3.0.0 + trace: collect_build_prerequisites: begin libfox/3.0.0 + trace: collect_build_prerequisites: no cfg-clause for dependency libbar/0.1.0 of dependent libfox/3.0.0 + trace: collect_build: pick libbaz/1.2.0 over libbaz/2.1.0 + trace: collect_build: libbaz/2.1.0 package version needs to be replaced with libbaz/1.2.0 + trace: pkg_build: collection failed due to package version replacement, retry from scratch + trace: pkg_build: refine package collection/plan execution from scratch + trace: collect_build: apply version replacement for libbaz/2.1.0 + trace: collect_build: replacement: libbaz/1.2.0 + trace: collect_build: add libbaz/1.2.0 + trace: collect_build: add libfoo/3.0.0 + trace: collect_build: add libfox/3.0.0 + trace: collect_build_prerequisites: begin libbaz/1.2.0 + trace: collect_build: apply version replacement for libbar/1.2.0 + trace: collect_build: replacement to 0.1.0 is denied since libbaz/1.2.0 depends on (libbar == 1.2.0) + trace: collect_build: add libbar/1.2.0 + trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libbaz/1.2.0 + trace: collect_build_prerequisites: begin libbar/1.2.0 + trace: collect_build_prerequisites: end libbar/1.2.0 + trace: collect_build_prerequisites: end libbaz/1.2.0 + trace: collect_build_prerequisites: begin libfoo/3.0.0 + trace: collect_build: apply version replacement for libbar/1.2.0 + trace: collect_build: replacement: libbar/0.1.0 + trace: collect_build: pick libbar/1.2.0 over libbar/0.1.0 + trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libfoo/3.0.0 + trace: collect_build_prerequisites: end libfoo/3.0.0 + trace: collect_build_prerequisites: begin libfox/3.0.0 + trace: collect_build: postpone failure for dependent libfox unsatisfied with dependency libbar/1.2.0 (== 0.1.0) + trace: collect_build: pick libbar/1.2.0 over libbar/0.1.0 + trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libfox/3.0.0 + trace: collect_build_prerequisites: no cfg-clause for dependency libbaz/1.2.0 of dependent libfox/3.0.0 + trace: collect_build_prerequisites: end libfox/3.0.0 + trace: execute_plan: simulate: yes + %.* + error: unable to satisfy constraints on package libbar + info: libbaz/1.2.0 depends on (libbar == 1.2.0) + libfox/3.0.0 requires (libbaz == 1.2.0) + info: libfox/3.0.0 depends on (libbar == 0.1.0) + info: available libbar/1.2.0 + info: available libbar/0.1.0 + info: while satisfying libfox/3.0.0 + info: explicitly specify libbar version to manually satisfy both constraints + %.* + EOE + } + } + : constraint-resolution : { diff --git a/tests/pkg-build/t4j b/tests/pkg-build/t4j new file mode 120000 index 0000000..3e18229 --- /dev/null +++ b/tests/pkg-build/t4j @@ -0,0 +1 @@ +../common/satisfy/t4j \ No newline at end of file -- cgit v1.1