From 4f2a74494532065205caf508f18c7521b184ee32 Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Fri, 20 Oct 2023 12:14:33 +0300 Subject: Fix 'unordered build' assertion failure due to bug in build_packages::order() --- bpkg/pkg-build-collect.cxx | 138 +++++++++++---------- bpkg/pkg-build-collect.hxx | 14 +-- bpkg/pkg-build.cxx | 28 +---- .../dependency-alternatives/t8a/dax-1.0.0.tar.gz | Bin 0 -> 421 bytes .../dependency-alternatives/t8a/dix-1.0.0.tar.gz | Bin 0 -> 395 bytes tests/pkg-build.testscript | 62 ++++++++- 6 files changed, 139 insertions(+), 103 deletions(-) create mode 100644 tests/common/dependency-alternatives/t8a/dax-1.0.0.tar.gz create mode 100644 tests/common/dependency-alternatives/t8a/dix-1.0.0.tar.gz diff --git a/bpkg/pkg-build-collect.cxx b/bpkg/pkg-build-collect.cxx index 67357f8..16d0b0b 100644 --- a/bpkg/pkg-build-collect.cxx +++ b/bpkg/pkg-build-collect.cxx @@ -6780,12 +6780,11 @@ namespace bpkg build_packages::iterator build_packages:: order (database& db, const package_name& name, - optional buildtime, const function& fdb, bool reorder) { package_refs chain; - return order (db, name, buildtime, chain, fdb, reorder); + return order (db, name, chain, fdb, reorder); } void build_packages:: @@ -7581,23 +7580,11 @@ namespace bpkg build_packages::iterator build_packages:: order (database& db, const package_name& name, - optional buildtime, package_refs& chain, const function& fdb, bool reorder) { - package_map::iterator mi; - - if (buildtime) - { - database* ddb (fdb (db, name, *buildtime)); - - mi = ddb != nullptr - ? map_.find (*ddb, name) - : map_.find_dependency (db, name, *buildtime); - } - else - mi = map_.find (db, name); + package_map::iterator mi (map_.find (db, name)); // Every package that we order should have already been collected. // @@ -7607,18 +7594,16 @@ namespace bpkg assert (p.action); // Can't order just a pre-entered package. - database& pdb (p.db); - // Make sure there is no dependency cycle. // - package_ref cp {pdb, name}; + package_ref cp {db, name}; { auto i (find (chain.begin (), chain.end (), cp)); if (i != chain.end ()) { diag_record dr (fail); - dr << "dependency cycle detected involving package " << name << pdb; + dr << "dependency cycle detected involving package " << name << db; auto nv = [this] (const package_ref& cp) { @@ -7687,20 +7672,21 @@ namespace bpkg i = j; }; - // Similar to collect_build(), we can prune if the package is already - // configured, right? While in collect_build() we didn't need to add - // prerequisites of such a package, it doesn't mean that they actually - // never ended up in the map via another dependency path. For example, - // some can be a part of the initial selection. And in that case we must - // order things properly. + // Similar to collect_build_prerequisites(), we can prune if the package + // is already configured, right? While in collect_build_prerequisites() we + // didn't need to add prerequisites of such a package, it doesn't mean + // that they actually never ended up in the map via another dependency + // path. For example, some can be a part of the initial selection. And in + // that case we must order things properly. // // Also, if the package we are ordering is not a system one and needs to // be disfigured during the plan execution, then we must order its // (current) dependencies that also need to be disfigured. // // And yet, if the package we are ordering is a repointed dependent, then - // we must order not only its unamended and new prerequisites but also its - // replaced prerequisites, which can also be disfigured. + // we must order not only its unamended and new prerequisites + // (prerequisite replacements) but also its replaced prerequisites, which + // can also be disfigured. // bool src_conf (sp != nullptr && sp->state == package_state::configured && @@ -7720,34 +7706,38 @@ namespace bpkg if (build && !p.system) { // So here we are going to do things differently depending on whether - // the package is already configured or not. If it is and not as a - // system package, then that means we can use its prerequisites - // list. Otherwise, we use the manifest data. + // the package prerequisites builds are collected or not. If they are + // not, then the package is being reconfigured and we use its configured + // prerequisites list. Otherwise, we use its collected prerequisites + // builds. // - if (src_conf && - sp->version == p.available_version () && - (p.config_vars.empty () || - !has_buildfile_clause (ap->dependencies))) + if (!p.dependencies) { + assert (src_conf); // Shouldn't be here otherwise. + + // A repointed dependent have always its prerequisite replacements + // collected, so p.dependencies must always be present for them. + // + assert ((p.flags & build_package::build_repoint) == 0); + for (const auto& p: sp->prerequisites) { database& db (p.first.database ()); const package_name& name (p.first.object_id ()); - // The prerequisites may not necessarily be in the map. - // - // Note that for the repointed dependent we also order its new and - // replaced prerequisites here, since they all are in the selected - // package prerequisites set. + // The prerequisites may not necessarily be in the map or have an + // action be present, but they can never be dropped. // auto i (map_.find (db, name)); - if (i != map_.end () && i->second.package.action) - update (order (db, - name, - nullopt /* buildtime */, - chain, - fdb, - false /* reorder */)); + if (i != map_.end ()) + { + optional a (i->second.package.action); + + assert (!a || *a != build_package::drop); // See above. + + if (a) + update (order (db, name, chain, fdb, false /* reorder */)); + } } // We just ordered them among other prerequisites. @@ -7756,11 +7746,10 @@ namespace bpkg } else { - // The package prerequisites builds must already be collected and - // thus the resulting dependency list is complete. + // If the package prerequisites builds are collected, then the + // resulting dependency list must be complete. // - assert (p.dependencies && - p.dependencies->size () == ap->dependencies.size ()); + assert (p.dependencies->size () == ap->dependencies.size ()); // We are iterating in reverse so that when we iterate over the // dependency list (also in reverse), prerequisites will be built in @@ -7779,18 +7768,44 @@ namespace bpkg assert (das.size () == 1); + bool buildtime (das.buildtime); + for (const dependency& d: das.front ()) { + const package_name& n (d.name); + + // Use the custom search function to find the dependency's build + // configuration. Failed that, search for it recursively. + // + database* ddb (fdb (db, n, buildtime)); + + auto i (ddb != nullptr + ? map_.find (*ddb, n) + : map_.find_dependency (db, n, buildtime)); + // Note that for the repointed dependent we only order its new and - // unamended prerequisites here. Its replaced prerequisites will - // be ordered below. + // potentially unamended prerequisites here (see + // collect_build_prerequisites() for details). Thus its + // (unamended) prerequisites may not necessarily be in the map or + // have an action be present, but they can never be dropped. Its + // replaced prerequisites will be ordered below. // - update (order (pdb, - d.name, - das.buildtime, - chain, - fdb, - false /* reorder */)); + if (i != map_.end ()) + { + optional a ( + i->second.package.action); + + assert (!a || *a != build_package::drop); // See above. + + if (a) + { + update (order (i->first.db, + n, + chain, + fdb, + false /* reorder */)); + } + } } } } @@ -7815,12 +7830,7 @@ namespace bpkg // since we do not reorder. // if (i != map_.end () && disfigure (i->second.package)) - update (order (db, - name, - nullopt /* buildtime */, - chain, - fdb, - false /* reorder */)); + update (order (db, name, chain, fdb, false /* reorder */)); } } diff --git a/bpkg/pkg-build-collect.hxx b/bpkg/pkg-build-collect.hxx index 181a86e..5362c65 100644 --- a/bpkg/pkg-build-collect.hxx +++ b/bpkg/pkg-build-collect.hxx @@ -1491,23 +1491,16 @@ namespace bpkg const function&, postponed_configuration* = nullptr); - // Order the previously-collected package with the specified name - // returning its positions. + // Order the previously-collected package with the specified name and + // configuration returning its position. // - // If buildtime is nullopt, then search for the specified package build in - // only the specified configuration. Otherwise, treat the package as a - // dependency and use the custom search function to find its build - // configuration. Failed that, search for it recursively (see - // package_map::find_dependency() for details). - // - // Recursively order the package dependencies being ordered failing if a + // Recursively order the collected package dependencies, failing if a // dependency cycle is detected. If reorder is true, then reorder this // package to be considered as "early" as possible. // iterator order (database&, const package_name&, - optional buildtime, const function&, bool reorder = true); @@ -1674,7 +1667,6 @@ namespace bpkg iterator order (database&, const package_name&, - optional buildtime, package_refs& chain, const function&, bool reorder); diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx index 9cf466c..1aaed99 100644 --- a/bpkg/pkg-build.cxx +++ b/bpkg/pkg-build.cxx @@ -4551,24 +4551,16 @@ namespace bpkg // appear (e.g., on the plan) last. // for (const dep& d: deps) - pkgs.order (d.db, - d.name, - nullopt /* buildtime */, - find_prereq_database, - false /* reorder */); + pkgs.order (d.db, d.name, find_prereq_database, false /* reorder */); for (const build_package& p: reverse_iterate (hold_pkgs)) - pkgs.order (p.db, - p.name (), - nullopt /* buildtime */, - find_prereq_database); + pkgs.order (p.db, p.name (), find_prereq_database); for (const auto& rd: rpt_depts) pkgs.order (rd.first.db, rd.first.name, - nullopt /* buildtime */, find_prereq_database, - false /* reorder */); + false /* reorder */); for (const postponed_configuration& cfg: postponed_cfgs) { @@ -4577,11 +4569,7 @@ namespace bpkg if (d.second.existing) { const package_key& p (d.first); - - pkgs.order (p.db, - p.name, - nullopt /* buildtime */, - find_prereq_database); + pkgs.order (p.db, p.name, find_prereq_database); } } } @@ -4590,10 +4578,7 @@ namespace bpkg { assert (p->recursive_collection); - pkgs.order (p->db, - p->name (), - nullopt /* buildtime */, - find_prereq_database); + pkgs.order (p->db, p->name (), find_prereq_database); } // Collect and order all the dependents that we will need to @@ -4617,9 +4602,8 @@ namespace bpkg if (sp != nullptr && sp->hold_package) pkgs.order (db, p.name, - nullopt /* buildtime */, find_prereq_database, - false /* reorder */); + false /* reorder */); }; if (p.db != nullptr) diff --git a/tests/common/dependency-alternatives/t8a/dax-1.0.0.tar.gz b/tests/common/dependency-alternatives/t8a/dax-1.0.0.tar.gz new file mode 100644 index 0000000..34c3aae Binary files /dev/null and b/tests/common/dependency-alternatives/t8a/dax-1.0.0.tar.gz differ diff --git a/tests/common/dependency-alternatives/t8a/dix-1.0.0.tar.gz b/tests/common/dependency-alternatives/t8a/dix-1.0.0.tar.gz new file mode 100644 index 0000000..764e530 Binary files /dev/null and b/tests/common/dependency-alternatives/t8a/dix-1.0.0.tar.gz differ diff --git a/tests/pkg-build.testscript b/tests/pkg-build.testscript index 010f703..006dde3 100644 --- a/tests/pkg-build.testscript +++ b/tests/pkg-build.testscript @@ -158,6 +158,8 @@ # | | libbox >= 0.1.1 config.box.backend=libbox, # | | libbaz # | |-- bux-1.0.0.tar.gz -> bix +# | |-- dax-1.0.0.tar.gz -> libbar ? ($config.dax.extras) +# | |-- dix-1.0.0.tar.gz -> dax require {config.dax.extras=true} # | |-- fax-1.0.0.tar.gz -> libbar ^1.0.0 ? ($cxx.target.class == 'windows') config.fax.backend=libbar | # | | libbaz ^1.0.0 ? ($cxx.target.class != 'windows') config.fax.backend=libbaz, # | | libbiz ? ($config.fax.libbiz) config.fax.extras='[b\i$z]', @@ -4648,6 +4650,54 @@ test.arguments += --sys-no-query $pkg_drop fax } + : enable-indirect-dependency + : + { + $clone_cfg; + + test.arguments += --plan ""; + + $* dax 2>>~%EOE%; + new dax/1.0.0 + fetched dax/1.0.0 + unpacked dax/1.0.0 + configured dax/1.0.0 + %info: .+dax-1.0.0.+ is up to date% + updated dax/1.0.0 + EOE + + $pkg_status -r >>EOO; + !dax configured 1.0.0 + EOO + + $* dix 2>>~%EOE%; + new libbar/1.0.0 (required by dax) + reconfigure/update dax/1.0.0 (required by dix) + config.dax.extras=true (set by dix) + new dix/1.0.0 + disfigured dax/1.0.0 + fetched libbar/1.0.0 + unpacked libbar/1.0.0 + fetched dix/1.0.0 + unpacked dix/1.0.0 + configured libbar/1.0.0 + configured dax/1.0.0 + configured dix/1.0.0 + %info: .+dix-1.0.0.+ is up to date% + updated dix/1.0.0 + EOE + + $pkg_status -r >>EOO; + !dax configured 1.0.0 + libbar configured 1.0.0 + !dix configured 1.0.0 + !dax configured 1.0.0 + libbar configured 1.0.0 + EOO + + $pkg_drop dax dix + } + : reevaluate-alternatives : { @@ -17568,9 +17618,9 @@ test.arguments += --sys-no-query build plan: upgrade libbar/1.0.0 config.libbar.extras=true (set by bax) + upgrade libbiz/1.0.0 reconfigure/update bax/1.0.0 config.bax.libfoo_extras=true (set by bax) - upgrade libbiz/1.0.0 trace: execute_plan: simulate: no %.* EOE @@ -24494,14 +24544,14 @@ else % upgrade libbaz/1.1.0 \[h1.\] \(required by foo \[h1.\]\)% % drop libbuild2-bar/1.0.0 \[h1..bpkg.build2.\] \(unused\)% % upgrade foo/1.1.0 \[h1.\]% - % reconfigure libbox \(dependent of foo \[h1.\]\)% % reconfigure libbar \(dependent of foo \[h1.\]\)% + % reconfigure libbox \(dependent of foo \[h1.\]\)% % drop libfax/1.0.0 \(unused\)% reconfigure/update libfix/1.0.0 continue? [Y/n] update dependent packages? [Y/n] disfigured libfix/1.0.0 disfigured libfax/1.0.0 - disfigured libbar/1.0.0 disfigured libbox/1.0.0 + disfigured libbar/1.0.0 %disfigured foo/1.0.0 \[h1.\]% %disfigured libbuild2-bar/1.0.0 \[h1..bpkg.build2.\]% %disfigured libbaz/1.0.0 \[h1.\]% @@ -24516,19 +24566,19 @@ else %configured libfax/1.0.0 \[t2.\]% %configured libbaz/1.1.0 \[h1.\]% %configured foo/1.1.0 \[h1.\]% - configured libbox/1.0.0 configured libbar/1.0.0 + configured libbox/1.0.0 configured libfix/1.0.0 %info: t2.+libfax-1.0.0.+ is up to date% %info: h1.+foo-1.1.0.+ is up to date% %info: t1.+libfix-1.0.0.+ is up to date% - %info: t1.+libbox-1.0.0.+ is up to date% %info: t1.+libbar-1.0.0.+ is up to date% + %info: t1.+libbox-1.0.0.+ is up to date% %updated libfax/1.0.0 \[t2.\]% %updated foo/1.1.0 \[h1.\]% updated libfix/1.0.0 - updated libbox/1.0.0 updated libbar/1.0.0 + updated libbox/1.0.0 EOE $pkg_status -d t1 --link -r >>/EOO -- cgit v1.1