compose: Add --ex-lockfile-strict

src/app/rpmostree-compose-builtin-tree.c

@@ -79,6 +79,7 @@ static char *opt_write_composejson_to;
 static gboolean opt_no_parent;
 static char *opt_write_lockfile_to;
 static char **opt_lockfiles;
+static gboolean opt_lockfile_strict;
 static char *opt_parent;
 
 /* shared by both install & commit */
@@ -104,6 +105,7 @@ static GOptionEntry install_option_entries[] = {
   { "workdir-tmpfs", 0, G_OPTION_FLAG_HIDDEN, G_OPTION_ARG_NONE, &opt_workdir_tmpfs, "Use tmpfs for working state", NULL },
   { "ex-write-lockfile-to", 0, 0, G_OPTION_ARG_STRING, &opt_write_lockfile_to, "Write lockfile to FILE", "FILE" },
   { "ex-lockfile", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_lockfiles, "Read lockfile from FILE", "FILE" },
+  { "ex-lockfile-strict", 0, 0, G_OPTION_ARG_NONE, &opt_lockfile_strict, "With --ex-lockfile, require full match", NULL },
   { NULL }
 };
 
@@ -702,7 +704,7 @@ rpm_ostree_compose_context_new (const char    *treefile_pathstr,
       g_autoptr(GHashTable) map = ror_lockfile_read (opt_lockfiles, error);
       if (!map)
         return FALSE;
-      rpmostree_context_set_vlockmap (self->corectx, map);
+      rpmostree_context_set_vlockmap (self->corectx, map, opt_lockfile_strict);
       g_print ("Loaded lockfiles:\n  %s\n", g_strjoinv ("\n  ", opt_lockfiles));
     }
 

src/libpriv/rpmostree-core-private.h

@@ -73,6 +73,7 @@ struct _RpmOstreeContext {
   GHashTable *pkgs_to_replace; /* new gv_nevra --> old gv_nevra */
 
   GHashTable *vlockmap; /* nevra --> repochecksum */
+  gboolean vlockmap_strict;
 
   GLnxTmpDir tmpdir;
 

src/libpriv/rpmostree-core.c

@@ -1846,48 +1846,60 @@ setup_rojig_state (RpmOstreeContext *self,
   return TRUE;
 }
 
-static gboolean
-check_locked_pkgs (RpmOstreeContext *self,
-                   GError          **error)
+/* Return all the packages that match lockfile constraints. Multiple packages may be
+ * returned per NEVRA so that libsolv can respect e.g. repo costs. */
+static GPtrArray*
+find_locked_packages (RpmOstreeContext *self,
+                      GError          **error)
 {
-  if (!self->vlockmap)
-    return TRUE;
-
-  HyGoal goal = dnf_context_get_goal (self->dnfctx);
-
-  /* sanity check it's all pure installs */
-  { g_autoptr(GPtrArray) packages = dnf_goal_get_packages (goal,
-                                                           DNF_PACKAGE_INFO_REMOVE,
-                                                           DNF_PACKAGE_INFO_OBSOLETE,
-                                                           DNF_PACKAGE_INFO_REINSTALL,
-                                                           DNF_PACKAGE_INFO_UPDATE,
-                                                           DNF_PACKAGE_INFO_DOWNGRADE,
-                                                           -1);
-    if (packages->len > 0)
-      return throw_package_list (error, "Packages not marked for pure install", packages);
-  }
-
-  g_autoptr(GPtrArray) packages =
-    dnf_goal_get_packages (goal, DNF_PACKAGE_INFO_INSTALL, -1);
+  g_assert (self->vlockmap);
+  DnfContext *dnfctx = self->dnfctx;
+  DnfSack *sack = dnf_context_get_sack (dnfctx);
 
-  for (guint i = 0; i < packages->len; i++)
+  g_autoptr(GPtrArray) pkgs =
+    g_ptr_array_new_with_free_func ((GDestroyNotify)g_object_unref);
+  GLNX_HASH_TABLE_FOREACH_KV (self->vlockmap, const char*, nevra, const char*, chksum)
     {
-      DnfPackage *pkg = packages->pdata[i];
-      const char *nevra = dnf_package_get_nevra (pkg);
-      const char *chksum = g_hash_table_lookup (self->vlockmap, nevra);
-      if (!chksum || !*chksum)
-        continue;
+      g_assert (chksum);
+      hy_autoquery HyQuery query = hy_query_create (sack);
+      hy_query_filter (query, HY_PKG_NEVRA_STRICT, HY_EQ, nevra);
+      g_autoptr(GPtrArray) matches = hy_query_run (query);
 
-      g_autofree char *repodata_chksum = NULL;
-      if (!rpmostree_get_repodata_chksum_repr (pkg, &repodata_chksum, error))
-        return FALSE;
+      gboolean at_least_one = FALSE;
+      guint n_checksum_mismatches = 0;
+      for (guint i = 0; i < matches->len; i++)
+        {
+          DnfPackage *match = matches->pdata[i];
+          if (!*chksum)
+            {
+              /* we could optimize this path outside the loop in the future using the
+               * g_ptr_array_extend_and_steal API, though that's still too new for e.g. el8 */
+              g_ptr_array_add (pkgs, g_object_ref (match));
+              at_least_one = TRUE;
+            }
+          else
+            {
+              g_autofree char *repodata_chksum = NULL;
+              if (!rpmostree_get_repodata_chksum_repr (match, &repodata_chksum, error))
+                return NULL;
 
-      if (chksum && !g_str_equal (chksum, repodata_chksum))
-        return glnx_throw (error, "Locked package %s checksum mismatch: expected %s, got %s",
-                           nevra, chksum, repodata_chksum);
+              if (!g_str_equal (chksum, repodata_chksum))
+                n_checksum_mismatches++;
+              else
+                {
+                  g_ptr_array_add (pkgs, g_object_ref (match));
+                  at_least_one = TRUE;
+                }
+            }
+        }
+      if (!at_least_one)
+        return glnx_null_throw (error, "Couldn't find locked package '%s'%s%s "
+                                       "(pkgs matching NEVRA: %d; mismatched checksums: %d)",
+                                nevra, *chksum ? " with checksum " : "",
+                                *chksum ? chksum : "", matches->len, n_checksum_mismatches);
     }
 
-  return TRUE;
+  return g_steal_pointer (&pkgs);
 }
 
 /* Check for/download new rpm-md, then depsolve */
@@ -1930,6 +1942,7 @@ rpmostree_context_prepare (RpmOstreeContext *self,
       journal_rpmmd_info (self);
     }
   DnfSack *sack = dnf_context_get_sack (dnfctx);
+  HyGoal goal = dnf_context_get_goal (dnfctx);
 
   /* Don't try to keep multiple kernels per root; that's a traditional thing,
    * ostree binds kernel + userspace.
@@ -1945,6 +1958,15 @@ rpmostree_context_prepare (RpmOstreeContext *self,
       g_assert_cmpint (g_strv_length (removed_base_pkgnames), ==, 0);
     }
 
+  if (self->vlockmap)
+    {
+      /* we only support pure installs for now (compose case) */
+      g_assert (!self->rojig_pure);
+      g_assert_cmpint (g_strv_length (cached_pkgnames), ==, 0);
+      g_assert_cmpint (g_strv_length (cached_replace_pkgs), ==, 0);
+      g_assert_cmpint (g_strv_length (removed_base_pkgnames), ==, 0);
+    }
+
   /* track cached pkgs already added to the sack so far */
   g_autoptr(GHashTable) local_pkgs_to_install =
     g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref);
@@ -1999,40 +2021,60 @@ rpmostree_context_prepare (RpmOstreeContext *self,
    * uninstall. We don't want to mix those two steps, otherwise we might confuse libdnf,
    * see: https://github.com/rpm-software-management/libdnf/issues/700 */
 
-  /* Before adding any pkgs to the goal; filter out from the sack all pkgs which don't match
-   * our lockfile. (But of course, leave other pkgs untouched.) */
   if (self->vlockmap != NULL)
     {
-      g_assert_cmpuint (g_strv_length (cached_replace_pkgs), ==, 0);
-      g_assert_cmpuint (g_strv_length (removed_base_pkgnames), ==, 0);
+      /* first, find our locked pkgs in the rpmmd */
+      g_autoptr(GPtrArray) locked_pkgs = find_locked_packages (self, error);
+      if (!locked_pkgs)
+        return FALSE;
 
-      GLNX_HASH_TABLE_FOREACH_KV (self->vlockmap, const char*, nevra, const char*, chksum)
+      /* build a packageset from it */
+      DnfPackageSet *locked_pset = dnf_packageset_new (sack);
+      for (guint i = 0; i < locked_pkgs->len; i++)
+        dnf_packageset_add (locked_pset, locked_pkgs->pdata[i]);
+
+      if (self->vlockmap_strict)
         {
-          g_autofree char *name = NULL;
-          if (!rpmostree_decompose_nevra (nevra, &name, NULL, NULL, NULL, NULL, error))
-            return FALSE;
-          hy_autoquery HyQuery query = hy_query_create (sack);
-          hy_query_filter (query, HY_PKG_NAME, HY_EQ, name);
-          g_autoptr(GPtrArray) pkglist = hy_query_run (query);
+          /* In strict mode, we basically *only* want locked packages to be considered, so
+           * exclude everything else. Note we still don't directly do `hy_goal_install`
+           * here; we want the treefile to still be canonical, but we just make sure that
+           * the end result matches what we expect. */
           DnfPackageSet *pset = dnf_packageset_new (sack);
-          for (guint i = 0; i < pkglist->len; i++)
-            {
-              DnfPackage *pkg = pkglist->pdata[i];
-              const char *pkg_nevra = dnf_package_get_nevra (pkg);
-              if (!g_str_equal (pkg_nevra, nevra))
-                dnf_packageset_add (pset, pkg);
-              else if (chksum && *chksum)
-                {
-                  g_autofree char *pkg_chksum = NULL;
-                  if (!rpmostree_get_repodata_chksum_repr (pkg, &pkg_chksum, error))
-                    return FALSE;
-                  if (!g_str_equal (chksum, pkg_chksum))
-                    dnf_packageset_add (pset, pkg);
-                }
-            }
+          Map *map = dnf_packageset_get_map (pset);
+          map_setall (map);
+          map_subtract (map, dnf_packageset_get_map (locked_pset));
           dnf_sack_add_excludes (sack, pset);
           dnf_packageset_free (pset);
         }
+      else
+        {
+          /* In relaxed mode, we allow packages to be added or removed without having to
+           * edit lockfiles. However, we still want to make sure that if a package does get
+           * installed which is in the lockfile, it can only pick that NEVRA. To do this, we
+           * filter out from the sack all pkgs which don't match. */
+
+          /* map of all packages with names found in the lockfile */
+          DnfPackageSet *named_pkgs = dnf_packageset_new (sack);
+          Map *named_pkgs_map = dnf_packageset_get_map (named_pkgs);
+          GLNX_HASH_TABLE_FOREACH (self->vlockmap, const char*, nevra)
+            {
+              g_autofree char *name = NULL;
+              if (!rpmostree_decompose_nevra (nevra, &name, NULL, NULL, NULL, NULL, error))
+                return FALSE;
+              hy_autoquery HyQuery query = hy_query_create (sack);
+              hy_query_filter (query, HY_PKG_NAME, HY_EQ, name);
+              DnfPackageSet *pset = hy_query_run_set (query);
+              map_or (named_pkgs_map, dnf_packageset_get_map (pset));
+              dnf_packageset_free (pset);
+            }
+
+          /* remove our locked packages from the exclusion set */
+          map_subtract (named_pkgs_map, dnf_packageset_get_map (locked_pset));
+          dnf_sack_add_excludes (sack, named_pkgs);
+          dnf_packageset_free (named_pkgs);
+        }
+
+      dnf_packageset_free (locked_pset);
     }
 
   /* Process excludes */
@@ -2059,7 +2101,6 @@ rpmostree_context_prepare (RpmOstreeContext *self,
     }
 
   /* Then, handle local packages to install */
-  HyGoal goal = dnf_context_get_goal (dnfctx);
   GLNX_HASH_TABLE_FOREACH_V (local_pkgs_to_install, DnfPackage*, pkg)
     hy_goal_install (goal,  pkg);
 
@@ -2108,8 +2149,7 @@ rpmostree_context_prepare (RpmOstreeContext *self,
 
       /* XXX: consider a --allow-uninstall switch? */
       if (!dnf_goal_depsolve (goal, actions, error) ||
-          !check_goal_solution (self, removed_pkgnames, replaced_nevras, error) ||
-          !check_locked_pkgs (self, error))
+          !check_goal_solution (self, removed_pkgnames, replaced_nevras, error))
         return FALSE;
       g_clear_pointer (&self->pkgs, (GDestroyNotify)g_ptr_array_unref);
       self->pkgs = dnf_goal_get_packages (dnf_context_get_goal (dnfctx),
@@ -2177,11 +2217,15 @@ rpmostree_context_get_packages_to_import (RpmOstreeContext *self)
   return g_ptr_array_ref (self->pkgs_to_import);
 }
 
+/* Note this must be called *before* rpmostree_context_setup(). */
 void
-rpmostree_context_set_vlockmap (RpmOstreeContext *self, GHashTable *map)
+rpmostree_context_set_vlockmap (RpmOstreeContext *self,
+                                GHashTable       *map,
+                                gboolean          strict)
 {
   g_clear_pointer (&self->vlockmap, (GDestroyNotify)g_hash_table_unref);
   self->vlockmap = g_hash_table_ref (map);
+  self->vlockmap_strict = strict;
 }
 
 /* XXX: push this into libdnf */

src/libpriv/rpmostree-core.h

@@ -173,7 +173,10 @@ gboolean rpmostree_context_set_packages (RpmOstreeContext *self,
 
 GPtrArray *rpmostree_context_get_packages_to_import (RpmOstreeContext *self);
 
-void rpmostree_context_set_vlockmap (RpmOstreeContext *self, GHashTable *map);
+void
+rpmostree_context_set_vlockmap (RpmOstreeContext *self,
+                                GHashTable       *map,
+                                gboolean          strict);
 
 gboolean rpmostree_context_download (RpmOstreeContext *self,
                                      GCancellable     *cancellable,

tests/compose/libcomposetest.sh

@@ -56,6 +56,10 @@ else:
   tf['$1'] += $2"
 }
 
+treefile_remove() {
+    treefile_pyedit "tf['$1'].remove($2)"
+}
+
 # for tests that need direct control on rpm-ostree
 export compose_base_argv="\
     --unified-core \

tests/compose/test-lockfile.sh

@@ -58,15 +58,96 @@ EOF
 runcompose \
   --ex-lockfile="$PWD/versions.lock" \
   --ex-lockfile="$PWD/override.lock" \
-  --ex-write-lockfile-to="$PWD/versions.lock" \
+  --ex-write-lockfile-to="$PWD/versions.lock.new" \
   --dry-run "${treefile}" |& tee out.txt
-echo "ok compose with lockfile"
-
 assert_file_has_content out.txt 'test-pkg-1.0-1.x86_64'
 assert_file_has_content out.txt 'test-pkg-common-1.0-1.x86_64'
 assert_file_has_content out.txt 'another-test-pkg-2.0-1.x86_64'
-assert_jq versions.lock \
+assert_jq versions.lock.new \
   '.packages["test-pkg"].evra = "1.0-1.x86_64"' \
   '.packages["test-pkg-common"].evra = "1.0-1.x86_64"' \
   '.packages["another-test-pkg"].evra = "2.0-1.x86_64"'
 echo "ok override"
+
+# sanity-check that we can remove packages in relaxed mode
+treefile_remove "packages" '"another-test-pkg"'
+runcompose \
+  --ex-lockfile="$PWD/versions.lock" \
+  --ex-lockfile="$PWD/override.lock" \
+  --ex-write-lockfile-to="$PWD/versions.lock.new" \
+  --dry-run "${treefile}" |& tee out.txt
+assert_file_has_content out.txt 'test-pkg-1.0-1.x86_64'
+assert_file_has_content out.txt 'test-pkg-common-1.0-1.x86_64'
+assert_not_file_has_content out.txt 'another-test-pkg'
+echo "ok relaxed mode can remove pkg"
+
+# test strict mode
+
+# sanity-check that refeeding the output lockfile as input satisfies strict mode
+mv versions.lock{.new,}
+runcompose \
+  --ex-lockfile-strict \
+  --ex-lockfile="$PWD/versions.lock" \
+  --ex-write-lockfile-to="$PWD/versions.lock.new" \
+  --dry-run "${treefile}" |& tee out.txt
+assert_streq \
+  "$(jq .packages versions.lock | sha256sum)" \
+  "$(jq .packages versions.lock.new | sha256sum)"
+echo "ok strict mode sanity check"
+
+# check that trying to install a pkg that's not in the lockfiles fails
+build_rpm unlocked-pkg
+treefile_append "packages" '["unlocked-pkg"]'
+if runcompose \
+    --ex-lockfile-strict \
+    --ex-lockfile="$PWD/versions.lock" \
+    --dry-run "${treefile}" &>err.txt; then
+  fatal "compose unexpectedly succeeded"
+fi
+assert_file_has_content err.txt 'Packages not found: unlocked-pkg'
+echo "ok strict mode no unlocked pkgs"
+
+# check that a locked pkg with unlocked deps causes an error
+build_rpm unlocked-pkg version 2.0 requires unlocked-pkg-dep
+build_rpm unlocked-pkg-dep version 2.0
+# notice we add unlocked-pkg, but not unlocked-pkg-dep
+cat > override.lock <<EOF
+{
+  "packages": {
+    "unlocked-pkg": {
+      "evra": "2.0-1.x86_64"
+    }
+  }
+}
+EOF
+if runcompose \
+    --ex-lockfile-strict \
+    --ex-lockfile="$PWD/versions.lock" \
+    --ex-lockfile="$PWD/override.lock" \
+    --dry-run "${treefile}" &>err.txt; then
+  fatal "compose unexpectedly succeeded"
+fi
+assert_file_has_content err.txt 'Could not depsolve transaction'
+assert_file_has_content err.txt 'unlocked-pkg-dep-2.0-1.x86_64 is filtered out by exclude filtering'
+treefile_remove "packages" '"unlocked-pkg"'
+echo "ok strict mode no unlocked pkg deps"
+
+# check that a locked pkg which isn't actually in the repos causes an error
+cat > override.lock <<EOF
+{
+  "packages": {
+    "unmatched-pkg": {
+      "evra": "1.0-1.x86_64"
+    }
+  }
+}
+EOF
+if runcompose \
+    --ex-lockfile-strict \
+    --ex-lockfile="$PWD/versions.lock" \
+    --ex-lockfile="$PWD/override.lock" \
+    --dry-run "${treefile}" &>err.txt; then
+  fatal "compose unexpectedly succeeded"
+fi
+assert_file_has_content err.txt "Couldn't find locked package 'unmatched-pkg-1.0-1.x86_64'"
+echo "ok strict mode locked pkg missing from rpmmd"