From 8d916d308d010cb26aa8cdbb8a7522b5776dc0d0 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Tue, 9 Jun 2020 12:23:07 -0400 Subject: [PATCH] WIP: Use SOLVER_LOCK for locking base packages While investigating https://github.com/coreos/fedora-coreos-tracker/issues/525, I realized that libsolv does have a flag which allows us to express that our base packages shouldn't be upgraded: `SOLVER_LOCK`. This then lets us not have to sanity-check the solution post-solve for "forbidden replacements", and lets libsolv/libdnf print exactly what the conflict is. --- src/libpriv/rpmostree-core.c | 183 +++++++++-------------------------- 1 file changed, 45 insertions(+), 138 deletions(-) diff --git a/src/libpriv/rpmostree-core.c b/src/libpriv/rpmostree-core.c index 4aedcc45f7..032e768958 100644 --- a/src/libpriv/rpmostree-core.c +++ b/src/libpriv/rpmostree-core.c @@ -1564,146 +1564,37 @@ gv_nevra_hash (gconstpointer v) return g_str_hash (g_variant_get_string (nevra, NULL)); } -/* We need to make sure that only the pkgs in the base allowed to be - * removed are removed. The issue is that marking a package for DNF_INSTALL - * could uninstall a base pkg if it's an update or obsoletes it. There doesn't - * seem to be a way to tell libsolv to not touch some pkgs in the base layer, - * so we just inspect its solution in retrospect. libdnf has the concept of - * protected packages, but it still allows updating protected packages. - */ static gboolean -check_goal_solution (RpmOstreeContext *self, - GPtrArray *removed_pkgnames, - GPtrArray *replaced_nevras, - GError **error) +collect_replaced_packages (RpmOstreeContext *self, + GPtrArray *replaced_pkgnames, + GError **error) { HyGoal goal = dnf_context_get_goal (self->dnfctx); - /* check that we're not removing anything we didn't expect */ - { g_autoptr(GPtrArray) forbidden = g_ptr_array_new_with_free_func (g_free); - - /* also collect info about those we're removing */ - g_assert (!self->pkgs_to_remove); - self->pkgs_to_remove = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, - (GDestroyNotify)g_variant_unref); - g_autoptr(GPtrArray) packages = dnf_goal_get_packages (goal, - DNF_PACKAGE_INFO_REMOVE, - DNF_PACKAGE_INFO_OBSOLETE, -1); - for (guint i = 0; i < packages->len; i++) - { - DnfPackage *pkg = packages->pdata[i]; - const char *name = dnf_package_get_name (pkg); - const char *nevra = dnf_package_get_nevra (pkg); - - /* did we expect this package to be removed? */ - if (rpmostree_str_ptrarray_contains (removed_pkgnames, name)) - g_hash_table_insert (self->pkgs_to_remove, g_strdup (name), - gv_nevra_from_pkg (pkg)); - else - g_ptr_array_add (forbidden, g_strdup (nevra)); - } - - if (forbidden->len > 0) - return throw_package_list (error, "Base packages would be removed", forbidden); - } - - /* check that all the pkgs we expect to remove are marked for removal */ - { g_autoptr(GPtrArray) forbidden = g_ptr_array_new (); - - for (guint i = 0; i < removed_pkgnames->len; i++) - { - const char *pkgname = removed_pkgnames->pdata[i]; - if (!g_hash_table_contains (self->pkgs_to_remove, pkgname)) - g_ptr_array_add (forbidden, (gpointer)pkgname); - } - - if (forbidden->len > 0) - return throw_package_list (error, "Base packages not marked to be removed", forbidden); - } - - /* REINSTALLs should never happen since it doesn't make sense in the rpm-ostree flow, and - * we check very early whether a package is already in the rootfs or not, but let's check - * for it anyway so that we get a bug report in case it somehow happens. */ - { g_autoptr(GPtrArray) packages = - dnf_goal_get_packages (goal, DNF_PACKAGE_INFO_REINSTALL, -1); - g_assert_cmpint (packages->len, ==, 0); - } - - /* Look at UPDATE and DOWNGRADE, and see whether they're doing what we expect */ - { g_autoptr(GHashTable) forbidden_replacements = - g_hash_table_new_full (NULL, NULL, (GDestroyNotify)g_object_unref, - (GDestroyNotify)g_object_unref); - - /* also collect info about those we're replacing */ - g_assert (!self->pkgs_to_replace); - self->pkgs_to_replace = g_hash_table_new_full (gv_nevra_hash, g_variant_equal, - (GDestroyNotify)g_variant_unref, - (GDestroyNotify)g_variant_unref); - g_autoptr(GPtrArray) packages = dnf_goal_get_packages (goal, - DNF_PACKAGE_INFO_UPDATE, - DNF_PACKAGE_INFO_DOWNGRADE, -1); - for (guint i = 0; i < packages->len; i++) - { - DnfPackage *pkg = packages->pdata[i]; - const char *nevra = dnf_package_get_nevra (pkg); - - /* just pick the first pkg */ - g_autoptr(GPtrArray) old = hy_goal_list_obsoleted_by_package (goal, pkg); - g_assert_cmpint (old->len, >, 0); - DnfPackage *old_pkg = old->pdata[0]; - - /* did we expect this nevra to replace a base pkg? */ - if (rpmostree_str_ptrarray_contains (replaced_nevras, nevra)) - g_hash_table_insert (self->pkgs_to_replace, gv_nevra_from_pkg (pkg), - gv_nevra_from_pkg (old_pkg)); - else - g_hash_table_insert (forbidden_replacements, g_object_ref (old_pkg), - g_object_ref (pkg)); - } - - if (g_hash_table_size (forbidden_replacements) > 0) - { - rpmostree_output_message ("Forbidden base package replacements:"); - GLNX_HASH_TABLE_FOREACH_KV (forbidden_replacements, DnfPackage*, old_pkg, - DnfPackage*, new_pkg) - { - const char *old_name = dnf_package_get_name (old_pkg); - const char *new_name = dnf_package_get_name (new_pkg); - const char *new_repo = dnf_package_get_reponame (new_pkg); - if (g_str_equal (old_name, new_name)) - rpmostree_output_message (" %s %s -> %s (%s)", old_name, - dnf_package_get_evr (old_pkg), - dnf_package_get_evr (new_pkg), new_repo); - else - rpmostree_output_message (" %s -> %s (%s)", - dnf_package_get_nevra (old_pkg), - dnf_package_get_nevra (new_pkg), new_repo); - } - rpmostree_output_message ("This likely means that some of your layered packages " - "have requirements on newer or older versions of some " - "base packages. Doing `rpm-ostree cleanup -m` and " - "`rpm-ostree upgrade` first may help. For " - "more details, see: " - "https://github.com/projectatomic/rpm-ostree/issues/415"); - - return glnx_throw (error, "Some base packages would be replaced"); - } - } + g_assert (!self->pkgs_to_replace); + self->pkgs_to_replace = g_hash_table_new_full (gv_nevra_hash, g_variant_equal, + (GDestroyNotify)g_variant_unref, + (GDestroyNotify)g_variant_unref); + g_autoptr(GPtrArray) packages = dnf_goal_get_packages (goal, + DNF_PACKAGE_INFO_UPDATE, + DNF_PACKAGE_INFO_DOWNGRADE, -1); + for (guint i = 0; i < packages->len; i++) + { + DnfPackage *pkg = packages->pdata[i]; + const char *pkgname = dnf_package_get_name (pkg); - /* check that all the pkgs we expect to replace are marked for replacement */ - { g_autoptr(GPtrArray) forbidden = g_ptr_array_new_with_free_func (g_free); + /* just pick the first pkg */ + g_autoptr(GPtrArray) old = hy_goal_list_obsoleted_by_package (goal, pkg); + g_assert_cmpint (old->len, >, 0); + DnfPackage *old_pkg = old->pdata[0]; - GLNX_HASH_TABLE_FOREACH_KV (self->pkgs_to_replace, GVariant*, new, GVariant*, old) - { - g_autoptr(GVariant) nevra_v = g_variant_get_child_value (new, 0); - const char *nevra = g_variant_get_string (nevra_v, NULL); - if (!rpmostree_str_ptrarray_contains (replaced_nevras, nevra)) - g_ptr_array_add (forbidden, g_strdup (nevra)); - } + /* just a final sanity-check here that it's a package we expected to be replaced */ + if (!rpmostree_str_ptrarray_contains (replaced_pkgnames, pkgname)) + return glnx_throw (error, "Found unexpected upgrade/downgrade for '%s'", pkgname); - if (forbidden->len > 0) - return throw_package_list (error, "Base packages not marked to be installed", forbidden); - } + g_hash_table_insert (self->pkgs_to_replace, gv_nevra_from_pkg (pkg), + gv_nevra_from_pkg (old_pkg)); + } return TRUE; } @@ -2002,7 +1893,7 @@ rpmostree_context_prepare (RpmOstreeContext *self, g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); /* Handle packages to replace; only add them to the sack for now */ - g_autoptr(GPtrArray) replaced_nevras = g_ptr_array_new (); + g_autoptr(GPtrArray) replaced_pkgnames = g_ptr_array_new_with_free_func (g_free); for (char **it = cached_replace_pkgs; it && *it; it++) { const char *nevra = *it; @@ -2016,7 +1907,7 @@ rpmostree_context_prepare (RpmOstreeContext *self, if (!add_pkg_from_cache (self, nevra, sha256, &pkg, cancellable, error)) return FALSE; - g_ptr_array_add (replaced_nevras, (gpointer)nevra); + g_ptr_array_add (replaced_pkgnames, g_strdup (dnf_package_get_name (pkg))); g_hash_table_insert (local_pkgs_to_install, (gpointer)nevra, g_steal_pointer (&pkg)); } @@ -2171,6 +2062,21 @@ rpmostree_context_prepare (RpmOstreeContext *self, } } + /* And lock all the base packages we don't expect to be replaced. */ + { hy_autoquery HyQuery query = hy_query_create (sack); + hy_query_filter (query, HY_PKG_REPONAME, HY_EQ, HY_SYSTEM_REPO_NAME); + g_autoptr(GPtrArray) pkgs = hy_query_run (query); + for (guint i = 0; i < pkgs->len; i++) + { + DnfPackage *pkg = pkgs->pdata[i]; + const char *pkgname = dnf_package_get_name (pkg); + if (rpmostree_str_ptrarray_contains (removed_pkgnames, pkgname) || + rpmostree_str_ptrarray_contains (replaced_pkgnames, pkgname)) + continue; + hy_goal_lock (goal, pkg); + } + } + if (missing_pkgs && missing_pkgs->len > 0) return throw_package_list (error, "Packages not found", missing_pkgs); @@ -2193,11 +2099,12 @@ rpmostree_context_prepare (RpmOstreeContext *self, rpmostree_output_task_begin (&task, "Resolving dependencies"); /* XXX: consider a --allow-uninstall switch? */ - if (!dnf_goal_depsolve (goal, actions, error) || - !check_goal_solution (self, removed_pkgnames, replaced_nevras, error)) + if (!dnf_goal_depsolve (goal, actions, error)) + return FALSE; + if (!collect_replaced_packages (self, replaced_pkgnames, error)) return FALSE; g_clear_pointer (&self->pkgs, (GDestroyNotify)g_ptr_array_unref); - self->pkgs = dnf_goal_get_packages (dnf_context_get_goal (dnfctx), + self->pkgs = dnf_goal_get_packages (goal, DNF_PACKAGE_INFO_INSTALL, DNF_PACKAGE_INFO_UPDATE, DNF_PACKAGE_INFO_DOWNGRADE, -1);