lib/package: fix package diffs for multilib

Our complicated heuristics for handling multiple packages of the same
name comes back to bite us. In the multilib case, we can have packages
of the same NEVR, but different arch, sitting in the same tree.

Previously, even if the arch was different, we would still mark it as an
upgrade or downgrade. But that complicates things in the case of
multiple packages of the same name in the same tree.

We greatly simplify things here by making the diff algorithm dumber. We
now only consider a package as "modified" (i.e. upgraded/downgraded) if
it has the same NA (but different EVR). This makes handling multilib
cases natural and seems worth it overall vs trying to handle the odd
e.g. noarch <--> archful pkg transitions that could happen.

Closes: #1230
Approved by: cgwalters

src/lib/rpmostree-package.c

@@ -299,22 +299,9 @@ _rpm_ostree_package_list_for_commit (OstreeRepo   *repo,
 
 /* Kinda like `comm(1)`, but for RpmOstreePackage lists. Assuming the pkglists are sorted,
  * this is more efficient than launching hundreds of queries and works with both dnf-based
- * and rpmdb.pkglist-based RpmOstreePackage arrays.
- *
- * Note that we handle multiple pkgs with the same name as follow:
- *   - if there are N pkgs of the same name in @a, and 0 pkgs of the same name in @b, then
- *     there will be N entries in @unique_a (and vice-versa for @b/@unique_b)
- *   - if there are N pkgs of the same name in @a, and M pkgs of the same name in @b, then
- *     there will be M entries in @modified_b, where all M entries will be paired with the
- *     same arbitrary pkg coming from one of the N entries.
- *
- * This was designed to be fully compatible with the semantics implemented by the
- * rpm_ostree_db_diff API, though note that its description did not previously reflect the
- * implementation. Since there isn't one way that's clearly better, I decided to stick with
- * it to be backwards compatible.
- *
- * Anyway, this is a super corner case. AFAIK, only "kernel" falls in this category, and in
- * rpm-ostree systems by nature we (should) only have a single kernel pkg. */
+ * and rpmdb.pkglist-based RpmOstreePackage arrays. Packages with different arches (e.g.
+ * multilib) are counted as different packages.
+ * */
 gboolean
 _rpm_ostree_diff_package_lists (GPtrArray  *a,
                                 GPtrArray  *b,
@@ -340,42 +327,15 @@ _rpm_ostree_diff_package_lists (GPtrArray  *a,
   /* allocate a sack just for comparisons */
   g_autoptr(DnfSack) sack = dnf_sack_new ();
 
-  const char *prev_a_name = NULL;
-  const char *prev_b_name = NULL;
-
   guint cur_a = 0;
   guint cur_b = 0;
   while (cur_a < an && cur_b < bn)
     {
+      int cmp;
       RpmOstreePackage *pkg_a = g_ptr_array_index (a, cur_a);
       RpmOstreePackage *pkg_b = g_ptr_array_index (b, cur_b);
 
-      /* see function description; we need to gracefully handle duplicate pkgnames in @b */
-      if (prev_a_name && g_str_equal (pkg_b->name, prev_a_name) &&
-          prev_b_name && g_str_equal (pkg_b->name, prev_b_name))
-        {
-          /* multiple copies exist in @b for a corresponding pkg in @a; point them all back
-           * to that same entry in @a */
-          g_assert_cmpuint (cur_a, >, 0);
-          RpmOstreePackage *prev_pkg_a = g_ptr_array_index (a, cur_a-1);
-          g_ptr_array_add (modified_a, g_object_ref (prev_pkg_a));
-          g_ptr_array_add (modified_b, g_object_ref (pkg_b));
-          cur_b++;
-          continue;
-        }
-      else if (prev_a_name && g_str_equal (pkg_a->name, prev_a_name) &&
-               prev_b_name && g_str_equal (pkg_a->name, prev_b_name))
-        {
-          /* Multiple copies exist in @a for a pkg that's in @b. Multiple copies might also
-           * exist in @b, but we paired them off with the first match in @a already above;
-           * we just skip over the dupes in @a now. */
-          cur_a++;
-          continue;
-        }
-
-      /* the rest below is just the obvious algorithm you'd expect for this */
-
-      int cmp = strcmp (pkg_a->name, pkg_b->name);
+      cmp = strcmp (pkg_a->name, pkg_b->name);
       if (cmp < 0)
         {
           g_ptr_array_add (unique_a, g_object_ref (pkg_a));
@@ -388,22 +348,33 @@ _rpm_ostree_diff_package_lists (GPtrArray  *a,
         }
       else
         {
-          cmp = dnf_sack_evr_cmp (sack, pkg_a->evr, pkg_b->evr);
-          if (cmp == 0)
+          cmp = strcmp (pkg_a->arch, pkg_b->arch);
+          if (cmp < 0)
             {
-              g_ptr_array_add (common, g_object_ref (pkg_a));
+              g_ptr_array_add (unique_a, g_object_ref (pkg_a));
+              cur_a++;
+            }
+          else if (cmp > 0)
+            {
+              g_ptr_array_add (unique_b, g_object_ref (pkg_b));
+              cur_b++;
             }
           else
             {
-              g_ptr_array_add (modified_a, g_object_ref (pkg_a));
-              g_ptr_array_add (modified_b, g_object_ref (pkg_b));
+              cmp = dnf_sack_evr_cmp (sack, pkg_a->evr, pkg_b->evr);
+              if (cmp == 0)
+                {
+                  g_ptr_array_add (common, g_object_ref (pkg_a));
+                }
+              else
+                {
+                  g_ptr_array_add (modified_a, g_object_ref (pkg_a));
+                  g_ptr_array_add (modified_b, g_object_ref (pkg_b));
+                }
             }
           cur_a++;
           cur_b++;
         }
-
-      prev_a_name = pkg_a->name;
-      prev_b_name = pkg_b->name;
     }
 
   /* flush out remaining a */

tests/common/libtest.sh

@@ -485,8 +485,16 @@ rm -rf %{buildroot}
 /usr/bin/$name
 $files
 EOF
+
+    # because it'd be overkill to set up mock for this, let's just fool
+    # rpmbuild using setarch
+    local buildarch=$arch
+    if [ "$arch" -eq "noarch" ]; then
+        buildarch=$(uname -m)
+    fi
+
     (cd $test_tmpdir/yumrepo/specs &&
-     rpmbuild -ba $name.spec \
+     setarch $buildarch rpmbuild --target $arch -ba $name.spec \
         --define "_topdir $PWD" \
         --define "_sourcedir $PWD" \
         --define "_specdir $PWD" \

tests/vmcheck/test-db.sh

@@ -36,6 +36,13 @@ check_diff() {
   assert_file_has_content diff.txt "$@"
 }
 
+check_not_diff() {
+  from=$1; shift
+  to=$1; shift
+  vm_rpmostree db diff --format=diff $from $to > diff.txt
+  assert_not_file_has_content diff.txt "$@"
+}
+
 vm_build_rpm pkg-to-remove
 vm_build_rpm pkg-to-replace
 vm_rpmostree install pkg-to-remove pkg-to-replace
@@ -59,14 +66,29 @@ echo "ok setup"
 vm_rpmostree override remove pkg-to-remove
 vm_build_rpm pkg-to-replace version 2.0
 vm_rpmostree override replace $YUMREPO/pkg-to-replace-2.0-1.x86_64.rpm
-vm_build_rpm pkg-to-overlay
-vm_rpmostree install pkg-to-overlay
+vm_build_rpm pkg-to-overlay build 'echo same > pkg-to-overlay'
+# some multilib handling tests (override default /bin script to skip conflicts)
+vm_build_rpm pkg-to-overlay build 'echo same > pkg-to-overlay' arch i686
+vm_build_rpm glibc arch i686
+vm_rpmostree install pkg-to-overlay.{x86_64,i686} glibc.i686
 pending_layered_csum=$(vm_get_pending_csum)
 check_diff $booted_csum $pending_layered_csum \
-  +pkg-to-overlay \
+  +pkg-to-overlay-1.0-1.x86_64 \
+  +pkg-to-overlay-1.0-1.i686 \
+  +glibc-1.0-1.i686 \
   +pkg-to-replace-2.0
+# check that regular glibc is *not* in the list of modified/dropped packages
+check_not_diff $booted_csum $pending_layered_csum \
+  =glibc \
+  !glibc \
+  -glibc \
+  =pkg-to-overlay \
+  !pkg-to-overlay \
+  -pkg-to-overlay
 check_diff $pending_csum $pending_layered_csum \
-  +pkg-to-overlay \
+  +pkg-to-overlay-1.0-1.x86_64 \
+  +pkg-to-overlay-1.0-1.i686 \
+  +glibc-1.0-1.i686 \
   -pkg-to-remove \
   !pkg-to-replace-1.0 \
   =pkg-to-replace-2.0
@@ -84,7 +106,9 @@ if vm_cmd ostree checkout --subpath /usr/share/rpm $pending_layered_csum; then
   assert_not_reached "Was able to checkout /usr/share/rpm?"
 fi
 check_diff $pending_csum $pending_layered_csum \
-  +pkg-to-overlay \
+  +pkg-to-overlay-1.0-1.x86_64 \
+  +pkg-to-overlay-1.0-1.i686 \
+  +glibc-1.0-1.i686 \
   -pkg-to-remove \
   !pkg-to-replace-1.0 \
   =pkg-to-replace-2.0