From 651a9e01f4fbaa4df818c4fc4d0c25ff23f49b9b Mon Sep 17 00:00:00 2001
From: Jonathan Lebon <jonathan@jlebon.com>
Date: Sat, 8 Sep 2018 17:19:15 -0400
Subject: [PATCH] core: Accumulate all missing pkgs into a single error

Accumulate the list of requested packages that were not found and error
out with the full list rather than failing early. This fixes a small UX
papercut in certain situations.

Closes: #1540

Closes: #1541
Approved by: cgwalters
---
 src/libpriv/rpmostree-core.c           | 32 +++++++++++++++++++-------
 tests/vmcheck/test-layering-basic-1.sh | 13 ++++++++++-
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/src/libpriv/rpmostree-core.c b/src/libpriv/rpmostree-core.c
index f3ca333b..8112a54d 100644
--- a/src/libpriv/rpmostree-core.c
+++ b/src/libpriv/rpmostree-core.c
@@ -1397,13 +1397,12 @@ sort_packages (RpmOstreeContext *self,
 
 /* Set @error with a string containing an error relating to @pkgs */
 static gboolean
-throw_package_list (GError **error, const char *suffix, GPtrArray *pkgs)
+throw_package_list (GError **error, const char *prefix, GPtrArray *pkgs)
 {
   if (!error)
     return FALSE; /* Note early simultaneously happy and sad return */
 
-  g_autoptr(GString) msg = g_string_new ("The following base packages ");
-  g_string_append (msg, suffix);
+  g_autoptr(GString) msg = g_string_new (prefix);
   g_string_append (msg, ": ");
 
   gboolean first = TRUE;
@@ -1480,7 +1479,7 @@ check_goal_solution (RpmOstreeContext *self,
       }
 
     if (forbidden->len > 0)
-      return throw_package_list (error, "would be removed", forbidden);
+      return throw_package_list (error, "Base packages would be removed", forbidden);
   }
 
   /* check that all the pkgs we expect to remove are marked for removal */
@@ -1494,7 +1493,7 @@ check_goal_solution (RpmOstreeContext *self,
       }
 
     if (forbidden->len > 0)
-      return throw_package_list (error, "are not marked to be removed", forbidden);
+      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
@@ -1572,7 +1571,7 @@ check_goal_solution (RpmOstreeContext *self,
       }
 
     if (forbidden->len > 0)
-      return throw_package_list (error, "are not marked to be installed", forbidden);
+      return throw_package_list (error, "Base packages not marked to be installed", forbidden);
   }
 
   return TRUE;
@@ -1828,14 +1827,31 @@ rpmostree_context_prepare (RpmOstreeContext *self,
     }
 
   /* Loop over each named package, and tell libdnf to add it to the goal */
+  g_autoptr(GPtrArray) missing_pkgs = NULL;
   for (char **it = pkgnames; it && *it; it++)
     {
       const char *pkgname = *it;
+      g_autoptr(GError) local_error = NULL;
+
       g_assert (!self->rojig_pure);
-      if (!dnf_context_install (dnfctx, pkgname, error))
-        return FALSE;
+      if (!dnf_context_install (dnfctx, pkgname, &local_error))
+        {
+          /* Only keep going if it's ENOENT, so we coalesce into one msg at the end */
+          if (!g_error_matches (local_error, DNF_ERROR, DNF_ERROR_PACKAGE_NOT_FOUND))
+            {
+              g_propagate_error (error, g_steal_pointer (&local_error));
+              return FALSE;
+            }
+          /* lazy init since it's unlikely in the common case (e.g. upgrades) */
+          if (!missing_pkgs)
+            missing_pkgs = g_ptr_array_new ();
+          g_ptr_array_add (missing_pkgs, (gpointer)pkgname);
+        }
     }
 
+  if (missing_pkgs && missing_pkgs->len > 0)
+    return throw_package_list (error, "Packages not found", missing_pkgs);
+
   if (self->rojig_spec)
     {
       if (!setup_rojig_state (self, error))
diff --git a/tests/vmcheck/test-layering-basic-1.sh b/tests/vmcheck/test-layering-basic-1.sh
index 6e90a260..5220d18d 100755
--- a/tests/vmcheck/test-layering-basic-1.sh
+++ b/tests/vmcheck/test-layering-basic-1.sh
@@ -59,6 +59,17 @@ assert_file_has_content err.txt "See https://github.com/projectatomic/rpm-ostree
 
 echo "ok failed to install in /opt and /usr/local"
 
+# Check that trying to install multiple nonexistent pkgs at once provides an
+# error including all of them at once
+fakes="foobar barbaz bazboo"
+if vm_rpmostree install $fakes &> err.txt; then
+  assert_not_reached "successfully layered non-existent pkgs $fakes?"
+fi
+assert_file_has_content_literal err.txt "Packages not found:"
+# ordering can be different, so check one at a time
+for pkg in $fakes; do assert_file_has_content_literal err.txt $pkg; done
+echo "ok one error for multiple missing pkgs"
+
 # Explicit epoch of 0 as it's a corner case:
 # https://github.com/projectatomic/rpm-ostree/issues/349
 vm_build_rpm foo epoch 0
@@ -163,7 +174,7 @@ vm_build_rpm bar conflicts foo
 if vm_rpmostree install bar &> err.txt; then
   assert_not_reached "successfully layered conflicting pkg bar?"
 fi
-assert_file_has_content err.txt "The following base packages would be removed"
+assert_file_has_content err.txt "Base packages would be removed"
 assert_file_has_content err.txt "foo-1.0-1.x86_64"
 vm_rpmostree cleanup -p
 vm_cmd ostree reset vmcheck $(vm_cmd ostree rev-parse "vmcheck^")