From 3419b77efca19b206f21d6fe23006b4227933641 Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Thu, 16 Nov 2023 16:32:31 +0300 Subject: Fix pkg-build by denying dropping package if it has dependents --- bpkg/pkg-build-collect.cxx | 177 +++++++++++++++++++-------- bpkg/pkg-build-collect.hxx | 17 +-- bpkg/pkg-build.cxx | 4 + tests/common/satisfy/libfox-2.1.0.tar.gz | Bin 0 -> 374 bytes tests/common/satisfy/t4j/libfix-1.0.0.tar.gz | 1 + tests/common/satisfy/t4j/libfox-0.0.1.tar.gz | 1 + tests/common/satisfy/t4j/libfox-2.1.0.tar.gz | 1 + tests/pkg-build.testscript | 95 ++++++++++++-- 8 files changed, 227 insertions(+), 69 deletions(-) create mode 100644 tests/common/satisfy/libfox-2.1.0.tar.gz create mode 120000 tests/common/satisfy/t4j/libfix-1.0.0.tar.gz create mode 120000 tests/common/satisfy/t4j/libfox-0.0.1.tar.gz create mode 120000 tests/common/satisfy/t4j/libfox-2.1.0.tar.gz diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx index 1eae206..ff6547f 100644 --- a/bpkg/pkg-build-collect.cxx +++ b/bpkg/pkg-build-collect.cxx @@ -257,6 +257,10 @@ namespace bpkg // assert ((flags & build_repoint) == 0 || (p.flags & build_repoint) == 0); + // If true, then add the user-selection tag. + // + bool add_user_selection (false); + // Copy the user-specified options/variables. // if (p.user_selection ()) @@ -304,13 +308,31 @@ namespace bpkg // Propagate the user-selection tag. // - required_by.emplace (db.get ().main_database (), "command line"); + add_user_selection = true; } - // Copy the required-by package names only if semantics matches. + // Merge in the required-by package names only if semantics matches. + // Otherwise, prefer the "required by dependents" semantics since we, in + // particular, should never replace such package builds in the map with + // package drops (see collect_drop() for details). // if (p.required_by_dependents == required_by_dependents) + { required_by.insert (p.required_by.begin (), p.required_by.end ()); + } + else if (p.required_by_dependents) + { + // Restore the user-selection tag. + // + if (user_selection ()) + add_user_selection = true; + + required_by_dependents = true; + required_by = move (p.required_by); + } + + if (add_user_selection) + required_by.emplace (db.get ().main_database (), "command line"); // Copy constraints, suppressing duplicates. // @@ -1455,7 +1477,9 @@ namespace bpkg // Apply the version replacement, if requested, and indicate that it was // applied. Ignore the replacement if its version doesn't satisfy the - // dependency constraints specified by the caller. + // dependency constraints specified by the caller. Also ignore if this is + // a drop and the required-by package names of the specified build package + // object have the "required by dependents" semantics // auto vi (replaced_vers.find (pk)); @@ -1499,17 +1523,35 @@ namespace bpkg } else { - v.replaced = true; + if (!pkg.required_by_dependents) + { + v.replaced = true; - l5 ([&]{trace << "replacement: drop";}); + l5 ([&]{trace << "replacement: drop";}); - // 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); + // We shouldn't be replacing a package build with the drop if someone + // depends on this package. + // + assert (pkg.selected != nullptr); + + collect_drop (options, pkg.db, pkg.selected, replaced_vers); + return nullptr; + } + else + { + assert (!pkg.required_by.empty ()); - collect_drop (options, pkg.db, pkg.selected, replaced_vers); - return nullptr; + l5 ([&] + { + diag_record dr (trace); + dr << "replacement to drop is denied since " << pk + << " is required by "; + for (auto b (pkg.required_by.begin ()), i (b); + i != pkg.required_by.end (); + ++i) + dr << (i != b ? ", " : "") << *i; + }); + } } } @@ -3590,7 +3632,14 @@ namespace bpkg if (p == nullptr) { p = entered_build (dpk); - assert (p != nullptr); + + // We don't expect the collected build to be replaced with the + // drop since its required-by package names have the "required + // by dependents" semantics. + // + assert (p != nullptr && + p->action && + *p->action == build_package::build); } bool collect_prereqs (!p->recursive_collection); @@ -5361,58 +5410,78 @@ namespace bpkg { build_package& bp (i->second.package); - if (bp.available != nullptr) + // Don't overwrite the build object if its required-by package names + // have the "required by dependents" semantics. + // + if (!bp.required_by_dependents) { - // Similar to the version replacement in collect_build(), see if - // in-place drop is possible (no dependencies, etc) and set scratch to - // false if that's the case. - // - bool scratch (true); + if (bp.available != nullptr) + { + // Similar to the version replacement in collect_build(), see if + // in-place drop is possible (no dependencies, etc) and set scratch + // to false if that's the case. + // + bool scratch (true); - // While checking if the package has any dependencies skip the - // toolchain build-time dependencies since they should be quite - // common. - // - // An update: it turned out that just absence of dependencies is not - // the only condition that causes a package to be dropped in place. - // The following conditions must also be met: - // - // - The package must also not participate in any configuration - // negotiation on the dependency side (otherwise it could have been - // added to a cluster as a dependency). - // - // - The package must not be added to unsatisfied_depts on the - // dependency side. - // - // This feels quite hairy at the moment, so we won't be dropping in - // place for now. - // + // While checking if the package has any dependencies skip the + // toolchain build-time dependencies since they should be quite + // common. + // + // An update: it turned out that just absence of dependencies is not + // the only condition that causes a package to be dropped in place. + // The following conditions must also be met: + // + // - The package must also not participate in any configuration + // negotiation on the dependency side (otherwise it could have + // been added to a cluster as a dependency). + // + // - The package must not be added to unsatisfied_depts on the + // dependency side. + // + // This feels quite hairy at the moment, so we won't be dropping in + // place for now. + // #if 0 - if (!has_dependencies (options, bp.available->dependencies)) - scratch = false; + if (!has_dependencies (options, bp.available->dependencies)) + scratch = false; #endif - l5 ([&]{trace << bp.available_name_version_db () - << " package version needs to be replaced " - << (!scratch ? "in-place " : "") << "with drop";}); + l5 ([&]{trace << bp.available_name_version_db () + << " package version needs to be replaced " + << (!scratch ? "in-place " : "") << "with drop";}); - if (scratch) - { - if (vi != replaced_vers.end ()) - vi->second = replaced_version (); - else - replaced_vers.emplace (move (pk), replaced_version ()); + if (scratch) + { + if (vi != replaced_vers.end ()) + vi->second = replaced_version (); + else + replaced_vers.emplace (move (pk), replaced_version ()); - throw replace_version (); + throw replace_version (); + } } - } - // Overwrite the existing (possibly pre-entered, adjustment, or repoint) - // entry. - // - l4 ([&]{trace << "overwrite " << pk;}); + // Overwrite the existing (possibly pre-entered, adjustment, or + // repoint) entry. + // + l4 ([&]{trace << "overwrite " << pk;}); - bp = move (p); + bp = move (p); + } + else + { + assert (!bp.required_by.empty ()); + + l5 ([&] + { + diag_record dr (trace); + dr << pk << " cannot be dropped since it is required by "; + for (auto b (bp.required_by.begin ()), i (b); + i != bp.required_by.end (); + ++i) + dr << (i != b ? ", " : "") << *i; + }); + } } else { diff --git a/bpkg/pkg-build-collect.hxx b/bpkg/pkg-build-collect.hxx index ece3d90..122ed22 100644 --- a/bpkg/pkg-build-collect.hxx +++ b/bpkg/pkg-build-collect.hxx @@ -1237,12 +1237,13 @@ namespace bpkg // // Consult replaced_vers for an existing version replacement entry and // 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. + // Ignore the entry if its version doesn't satisfy the specified + // dependency constraints or the entry is a package drop and the specified + // required-by package names have the "required by dependents" semantics. + // 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 @@ -1544,7 +1545,9 @@ namespace bpkg const function&, const function&); - // Collect the package being dropped. + // Collect the package being dropped. Noop if the specified package is + // already being built and its required-by package names have the + // "required by dependents" semantics. // // Add entry to replaced_vers and throw replace_version if the existing // version needs to be dropped but this can't be done in-place (see diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx index c1dd312..51e1fb1 100644 --- a/bpkg/pkg-build.cxx +++ b/bpkg/pkg-build.cxx @@ -4858,6 +4858,10 @@ namespace bpkg ++i; } + if (scratch_exe) + l5 ([&]{trace << "one of dependency evaluation decisions has " + << "changed, re-collecting from scratch";}); + // If the execute_plan() call was noop, there are no user expectations // regarding any dependency, and no upgrade is requested, then the // only possible refinement outcome can be recommendations to drop diff --git a/tests/common/satisfy/libfox-2.1.0.tar.gz b/tests/common/satisfy/libfox-2.1.0.tar.gz new file mode 100644 index 0000000..60a4cce Binary files /dev/null and b/tests/common/satisfy/libfox-2.1.0.tar.gz differ diff --git a/tests/common/satisfy/t4j/libfix-1.0.0.tar.gz b/tests/common/satisfy/t4j/libfix-1.0.0.tar.gz new file mode 120000 index 0000000..aad4c49 --- /dev/null +++ b/tests/common/satisfy/t4j/libfix-1.0.0.tar.gz @@ -0,0 +1 @@ +../libfix-1.0.0.tar.gz \ No newline at end of file diff --git a/tests/common/satisfy/t4j/libfox-0.0.1.tar.gz b/tests/common/satisfy/t4j/libfox-0.0.1.tar.gz new file mode 120000 index 0000000..674ac04 --- /dev/null +++ b/tests/common/satisfy/t4j/libfox-0.0.1.tar.gz @@ -0,0 +1 @@ +../libfox-0.0.1.tar.gz \ No newline at end of file diff --git a/tests/common/satisfy/t4j/libfox-2.1.0.tar.gz b/tests/common/satisfy/t4j/libfox-2.1.0.tar.gz new file mode 120000 index 0000000..157a046 --- /dev/null +++ b/tests/common/satisfy/t4j/libfox-2.1.0.tar.gz @@ -0,0 +1 @@ +../libfox-2.1.0.tar.gz \ No newline at end of file diff --git a/tests/pkg-build.testscript b/tests/pkg-build.testscript index f4c79dc..bc435f2 100644 --- a/tests/pkg-build.testscript +++ b/tests/pkg-build.testscript @@ -131,6 +131,8 @@ # | |-- libbar-0.1.0.tar.gz # | |-- libbar-1.2.0.tar.gz # | |-- libfoo-3.0.0.tar.gz -> libbar +# | |-- libfox-0.0.1.tar.gz +# | |-- libfox-2.1.0.tar.gz -> libbar, libbaz == 1.2.0 # | |-- 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 @@ -4026,13 +4028,13 @@ test.arguments += --sys-no-query } } - : version-replacement + : denied-version-replacements : { +$clone_root_cfg +$rep_add $rep/t4j && $rep_fetch - : denied + : unsatisfactory-version : { $clone_cfg; @@ -4113,6 +4115,82 @@ test.arguments += --sys-no-query %.* EOE } + + : collect-drop + : + { + $clone_cfg; + + $* libfix ?libfox/0.0.1 libbaz 2>!; + + $pkg_status -ar >>EOO; + libfox configured !0.0.1 available 3.0.0 2.1.0 + !libfix configured 1.0.0 + libfox configured !0.0.1 available 3.0.0 2.1.0 + !libbaz configured 2.1.0 + EOO + + $* ?libbaz ?libfox/2.1.0 --verbose 5 2>>~%EOE%; + %.* + trace: pkg_build: refine package collection/plan execution from scratch + trace: execute_plan: simulate: yes + trace: evaluate_dependency: libfox/0.0.1: update to libfox/2.1.0 + trace: evaluate_dependency: libbaz/2.1.0: unused + trace: pkg_build: refine package collection/plan execution + trace: collect_build_prerequisites: pre-reeval libfix/1.0.0 + trace: collect_build_prerequisites: pre-reevaluated libfix/1.0.0: end reached + trace: collect_build_prerequisites: begin libfox/2.1.0 + trace: collect_build: add libbar/1.2.0 + trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libfox/2.1.0 + trace: collect_build_prerequisites: begin libbar/1.2.0 + trace: collect_build_prerequisites: end libbar/1.2.0 + warning: package libfox dependency on (libbaz == 1.2.0) is forcing downgrade of libbaz/2.1.0 to 1.2.0 + trace: collect_build_prerequisites: no cfg-clause for dependency libbaz/1.2.0 of dependent libfox/2.1.0 + trace: collect_build_prerequisites: begin libbaz/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: end libbaz/1.2.0 + trace: collect_build_prerequisites: end libfox/2.1.0 + trace: collect_drop: libbaz cannot be dropped since it is required by command line, libfox/2.1.0 + trace: execute_plan: simulate: yes + %.* + trace: pkg_build: one of dependency evaluation decisions has changed, re-collecting from scratch + trace: pkg_build: refine package collection/plan execution from scratch + trace: collect_build_prerequisites: pre-reeval libfix/1.0.0 + trace: collect_build_prerequisites: pre-reevaluated libfix/1.0.0: end reached + trace: collect_build_prerequisites: begin libfox/2.1.0 + trace: collect_build: add libbar/1.2.0 + trace: collect_build_prerequisites: no cfg-clause for dependency libbar/1.2.0 of dependent libfox/2.1.0 + trace: collect_build_prerequisites: begin libbar/1.2.0 + trace: collect_build_prerequisites: end libbar/1.2.0 + warning: package libfox dependency on (libbaz == 1.2.0) is forcing downgrade of libbaz/2.1.0 to 1.2.0 + trace: collect_build_prerequisites: no cfg-clause for dependency libbaz/1.2.0 of dependent libfox/2.1.0 + trace: collect_build_prerequisites: begin libbaz/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: end libbaz/1.2.0 + trace: collect_build_prerequisites: end libfox/2.1.0 + trace: execute_plan: simulate: yes + %.* + trace: execute_plan: simulate: no + %.* + EOE + + $pkg_status -ar >>EOO; + libfox configured !2.1.0 available 3.0.0 + libbar configured 1.2.0 + libbaz configured 1.2.0 available 2.1.0 + libbar configured 1.2.0 + !libfix configured 1.0.0 + libfox configured !2.1.0 available 3.0.0 + libbar configured 1.2.0 + libbaz configured 1.2.0 available 2.1.0 + libbar configured 1.2.0 + libbaz configured 1.2.0 available 2.1.0 + libbar configured 1.2.0 + libbar configured 1.2.0 + EOO + + $pkg_drop libfix + } } : constraint-resolution @@ -5632,7 +5710,7 @@ test.arguments += --sys-no-query % new libbaz/1.1.0 \[cfg2.\]% drop libbaz/1.1.0 (unused) new libbar/1.0.0 (required by dax) - % reconfigure/update dax/1.0.0 \(dependent of libbaz \[cfg2.\]\)% + reconfigure/update dax/1.0.0 (required by dix) config.dax.extras=true (set by dix) upgrade dix/1.0.0 reconfigure dux (dependent of dix) @@ -7507,6 +7585,7 @@ test.arguments += --sys-no-query trace: execute_plan: simulate: yes %.* trace: evaluate_dependency: fux/1.0.0: unused + trace: pkg_build: one of dependency evaluation decisions has changed, re-collecting from scratch trace: pkg_build: refine package collection/plan execution from scratch trace: collect_build: add libfoo/0.1.0 trace: collect_build_prerequisites: skip configured libfoo/0.1.0 @@ -14831,7 +14910,7 @@ test.arguments += --sys-no-query build plan: upgrade libfoo/1.0.0 config.libfoo.extras=true (set by tex) - reconfigure/update tex/1.0.0 (dependent of libbar, libfoo) + reconfigure/update tex/1.0.0 (required by tix) config.tex.extras=true (set by tix) config.tex.libfoo_extras=true (set by tex) reconfigure/update tix/1.0.0 @@ -15005,7 +15084,7 @@ test.arguments += --sys-no-query build plan: upgrade libfoo/1.0.0 config.libfoo.extras=true (set by tex) - reconfigure/update tex/1.0.0 (dependent of libbar, libfoo) + reconfigure/update tex/1.0.0 (required by tiz) config.tex.extras=true (set by tiz) config.tex.libfoo_extras=true (set by tex) reconfigure/update tiz/1.0.0 @@ -15833,7 +15912,7 @@ test.arguments += --sys-no-query build plan: upgrade libbar/1.0.0 config.libbar.extras=true (set by diz) - reconfigure/update bar/1.0.0 (dependent of libbar) + reconfigure/update bar/1.0.0 (required by dex) config.bar.extras=true (set by dex) reconfigure/update dex/1.0.0 (required by dox) config.dex.extras=true (set by dox) @@ -16198,7 +16277,7 @@ test.arguments += --sys-no-query build plan: upgrade libbar/1.0.0 config.libbar.extras=true (set by diz) - reconfigure/update bar/1.0.0 (dependent of libbar) + reconfigure/update bar/1.0.0 (required by dex) config.bar.extras=true (set by dex) reconfigure/update dex/1.0.0 config.dex.extras=true (set by dox) @@ -16359,7 +16438,7 @@ test.arguments += --sys-no-query build plan: upgrade libbar/1.0.0 config.libbar.extras=true (set by tex) - reconfigure/update tex/1.0.0 (dependent of libbar) + reconfigure/update tex/1.0.0 (required by tiz) config.tex.extras=true (set by tiz) config.tex.libfoo_extras=true (set by tex) reconfigure/update tiz/1.0.0 -- cgit v1.1