Prioritize an app's origin for its runtime

Currently when searching for a remote to provide the runtime for an app,
we search remotes in priority order. This commit makes it so we search
the remote providing the app before others with the same priority, and
otherwise still search in priority order.  This means for the common
case where every remote has the default priority of 1, the app's origin
will have the first chance to provide the runtime. This behavior seems
logical, but the impetus for this change was also to keep a unit test
passing in eos-updater[1] after a port to FlatpakTransaction.

Originally the eos-updater unit test was written to prioritize the
origin remote regardless of the priorities on any other remote, but
during code review it was decided to let higher priority remotes stay
above the app's origin.

In practice it's usually true that only one remote provides a runtime
and priorities aren't set at all, so this is an edge case that probably
doesn't come up much.

A unit test and documentation updates are included.

[1] https://github.com/endlessm/eos-updater/blob/eede0a8b9cb7c9cbb8539cfcaad9ce18913dcdd8/tests/test-update-install-flatpaks.c#L1919

7 files changed, 143 insertions, 14 deletions

diff --git a/common/flatpak-dir-private.h b/common/flatpak-dir-private.h
index d8834bf9..02c44f40 100644
--- a/common/flatpak-dir-private.h
+++ b/common/flatpak-dir-private.h
@@ -462,10 +462,12 @@ GFile *     flatpak_dir_get_unmaintained_extension_dir_if_exists (FlatpakDir   *
                                                                   GCancellable *cancellable);
 
 char **     flatpak_dir_search_for_local_dependency (FlatpakDir   *self,
+                                                     const char   *prioritized_remote,
                                                      const char   *runtime_ref,
                                                      GCancellable *cancellable,
                                                      GError      **error);
 char **     flatpak_dir_search_for_dependency (FlatpakDir   *self,
+                                               const char   *prioritized_remote,
                                                const char   *runtime_ref,
                                                GCancellable *cancellable,
                                                GError      **error);
diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c
index 0de14a49..a85a6902 100644
--- a/common/flatpak-dir.c
+++ b/common/flatpak-dir.c
@@ -12703,20 +12703,62 @@ flatpak_dir_list_enumerated_remotes (FlatpakDir   *self,
   return (char **) g_ptr_array_free (g_steal_pointer (&res), FALSE);
 }
 
+typedef struct {
+  FlatpakDir *dir;
+  const char *prioritized_remote;
+} RemoteSortData;
+
+static gint
+cmp_remote_with_prioritized (gconstpointer a,
+                             gconstpointer b,
+                             gpointer      user_data)
+{
+  RemoteSortData *rsd = user_data;
+  FlatpakDir *self = rsd->dir;
+  const char *a_name = *(const char **) a;
+  const char *b_name = *(const char **) b;
+  int prio_a, prio_b;
+
+  prio_a = flatpak_dir_get_remote_prio (self, a_name);
+  prio_b = flatpak_dir_get_remote_prio (self, b_name);
+
+  /* Here we are assuming the array is already sorted by cmp_remote() and only
+   * putting a particular remote at the top of its priority level */
+  if (prio_b != prio_a)
+    return prio_b - prio_a;
+  else
+    {
+      if (strcmp (a_name, rsd->prioritized_remote) == 0)
+        return -1;
+      if (strcmp (b_name, rsd->prioritized_remote) == 0)
+        return 1;
+    }
+
+  return 0;
+}
+
 char **
 flatpak_dir_search_for_dependency (FlatpakDir   *self,
+                                   const char   *prioritized_remote,
                                    const char   *runtime_ref,
                                    GCancellable *cancellable,
                                    GError      **error)
 {
   g_autoptr(GPtrArray) found = g_ptr_array_new_with_free_func (g_free);
   g_auto(GStrv) remotes = NULL;
+  RemoteSortData rsd = { NULL };
   int i;
 
   remotes = flatpak_dir_list_enumerated_remotes (self, cancellable, error);
   if (remotes == NULL)
     return NULL;
 
+  /* Put @prioritized_remote before the others at its priority level */
+  rsd.dir = self;
+  rsd.prioritized_remote = prioritized_remote;
+  g_qsort_with_data (remotes, g_strv_length (remotes), sizeof (char *),
+                     cmp_remote_with_prioritized, &rsd);
+
   for (i = 0; remotes != NULL && remotes[i] != NULL; i++)
     {
       const char *remote = remotes[i];
@@ -12735,18 +12777,26 @@ flatpak_dir_search_for_dependency (FlatpakDir   *self,
 
 char **
 flatpak_dir_search_for_local_dependency (FlatpakDir   *self,
+                                         const char   *prioritized_remote,
                                          const char   *runtime_ref,
                                          GCancellable *cancellable,
                                          GError      **error)
 {
   g_autoptr(GPtrArray) found = g_ptr_array_new_with_free_func (g_free);
   g_auto(GStrv) remotes = NULL;
+  RemoteSortData rsd = { NULL };
   int i;
 
   remotes = flatpak_dir_list_enumerated_remotes (self, cancellable, error);
   if (remotes == NULL)
     return NULL;
 
+  /* Put @prioritized_remote before the others at its priority level */
+  rsd.dir = self;
+  rsd.prioritized_remote = prioritized_remote;
+  g_qsort_with_data (remotes, g_strv_length (remotes), sizeof (char *),
+                     cmp_remote_with_prioritized, &rsd);
+
   for (i = 0; remotes != NULL && remotes[i] != NULL; i++)
     {
       const char *remote = remotes[i];
diff --git a/common/flatpak-transaction.c b/common/flatpak-transaction.c
index c14454a0..5880f08d 100644
--- a/common/flatpak-transaction.c
+++ b/common/flatpak-transaction.c
@@ -1939,6 +1939,7 @@ add_related (FlatpakTransaction          *self,
 static char *
 find_runtime_remote (FlatpakTransaction             *self,
                      const char                     *app_ref,
+                     const char                     *app_remote,
                      const char                     *runtime_ref,
                      FlatpakTransactionOperationType source_kind,
                      GError                        **error)
@@ -1952,10 +1953,11 @@ find_runtime_remote (FlatpakTransaction             *self,
   app_pref = strchr (app_ref, '/') + 1;
   runtime_pref = strchr (runtime_ref, '/') + 1;
 
+  /* Here we are passing along app_remote so it gets priority */
   if (transaction_is_local_only (self, source_kind))
-    remotes = flatpak_dir_search_for_local_dependency (priv->dir, runtime_ref, NULL, NULL);
+    remotes = flatpak_dir_search_for_local_dependency (priv->dir, app_remote, runtime_ref, NULL, NULL);
   else
-    remotes = flatpak_dir_search_for_dependency (priv->dir, runtime_ref, NULL, NULL);
+    remotes = flatpak_dir_search_for_dependency (priv->dir, app_remote, runtime_ref, NULL, NULL);
 
   if (remotes == NULL || *remotes == NULL)
     {
@@ -2037,7 +2039,7 @@ add_deps (FlatpakTransaction          *self,
               return FALSE;
             }
 
-          runtime_remote = find_runtime_remote (self, op->ref, full_runtime_ref, op->kind, error);
+          runtime_remote = find_runtime_remote (self, op->ref, op->remote, full_runtime_ref, op->kind, error);
           if (runtime_remote == NULL)
             return FALSE;
 
diff --git a/doc/flatpak-flatpakref.xml b/doc/flatpak-flatpakref.xml
index 1144a89e..55bd269c 100644
--- a/doc/flatpak-flatpakref.xml
+++ b/doc/flatpak-flatpakref.xml
@@ -132,7 +132,9 @@
                 </varlistentry>
                 <varlistentry>
                     <term><option>RuntimeRepo</option> (string)</term>
-                    <listitem><para>The url for a .flatpakrepo file for the remote where the runtime can be found.</para></listitem>
+                    <listitem><para>The url for a .flatpakrepo file for the remote where the runtime can be found.
+                    Note that if the runtime is available in the remote providing the app, that remote may be
+                    used instead but the one specified by this option will still be added.</para></listitem>
                 </varlistentry>
                 <varlistentry>
                     <term><option>SuggestRemoteName</option> (string)</term>
diff --git a/doc/flatpak-remote-add.xml b/doc/flatpak-remote-add.xml
index c785b541..b53cbbfb 100644
--- a/doc/flatpak-remote-add.xml
+++ b/doc/flatpak-remote-add.xml
@@ -120,7 +120,9 @@
 
                 <listitem><para>
                     Set the priority for the remote. Default is 1, higher is more prioritized. This is
-                    mainly used for graphical installation tools.
+                    mainly used for graphical installation tools. It is also used when searching for a
+                    remote to provide an app's runtime. The app's origin is checked before other
+                    remotes with the same priority.
                 </para></listitem>
             </varlistentry>
 
diff --git a/doc/flatpak-remote.xml b/doc/flatpak-remote.xml
index 1f1fb5be..4545e202 100644
--- a/doc/flatpak-remote.xml
+++ b/doc/flatpak-remote.xml
@@ -104,7 +104,9 @@
                 </varlistentry>
                 <varlistentry>
                     <term><option>xa.prio</option> (integer)</term>
-                    <listitem><para>The priority for the remote. This is used when listing remotes. Defaults to 1.</para></listitem>
+                    <listitem><para>The priority for the remote. This is used when listing remotes, and when
+                    searching them for the runtime needed by an app. The remote providing the app is
+                    searched for its runtime before others with equal priority. Defaults to 1.</para></listitem>
                 </varlistentry>
                 <varlistentry>
                     <term><option>xa.noenumerate</option> (boolean)</term>
diff --git a/tests/testlibrary.c b/tests/testlibrary.c
index 925c9cc2..8ab4daaa 100644
--- a/tests/testlibrary.c
+++ b/tests/testlibrary.c
@@ -2230,6 +2230,7 @@ launch_httpd (void)
 
 static void
 _add_remote (const char *remote_repo_name,
+             const char *remote_name_override,
              gboolean    system)
 {
   char *argv[] = { "flatpak", "remote-add", NULL, "--gpg-import=", "--collection-id=", "name", "url", NULL };
@@ -2240,7 +2241,10 @@ _add_remote (const char *remote_repo_name,
   gpgimport = g_strdup_printf ("--gpg-import=%s/pubring.gpg", gpg_homedir);
   repo_url = g_strdup_printf ("http://127.0.0.1:%s/%s", httpd_port, remote_repo_name);
   collection_id_arg = g_strdup_printf ("--collection-id=%s", repo_collection_id);
-  remote_name = g_strdup_printf ("%s-repo", remote_repo_name);
+  if (remote_name_override != NULL)
+    remote_name = g_strdup (remote_name_override);
+  else
+    remote_name = g_strdup_printf ("%s-repo", remote_repo_name);
 
   argv[2] = system ? "--system" : "--user";
   argv[3] = gpgimport;
@@ -2252,15 +2256,17 @@ _add_remote (const char *remote_repo_name,
 }
 
 static void
-add_remote_system (const char *remote_repo_name)
+add_remote_system (const char *remote_repo_name,
+                   const char *remote_name_override)
 {
-  _add_remote (remote_repo_name, TRUE);
+  _add_remote (remote_repo_name, remote_name_override, TRUE);
 }
 
 static void
-add_remote_user (const char *remote_repo_name)
+add_remote_user (const char *remote_repo_name,
+                 const char *remote_name_override)
 {
-  _add_remote (remote_repo_name, FALSE);
+  _add_remote (remote_repo_name, remote_name_override, FALSE);
 }
 
 static void
@@ -2366,7 +2372,7 @@ setup_repo (void)
   make_test_app ("test");
   update_repo ("test");
   launch_httpd ();
-  add_remote_user ("test");
+  add_remote_user ("test", NULL);
   add_flatpakrepo ("test");
   configure_languages ("de");
 
@@ -3262,8 +3268,8 @@ test_transaction_flatpakref_remote_creation (void)
 
   empty_installation (system_inst);
 
-  add_remote_system ("test-without-runtime");
-  add_remote_system ("test-runtime-only");
+  add_remote_system ("test-without-runtime", NULL);
+  add_remote_system ("test-runtime-only", NULL);
 
   user_inst = flatpak_installation_new_user (NULL, &error);
   g_assert_no_error (error);
@@ -3467,6 +3473,68 @@ test_transaction_install_local (void)
   g_assert_nonnull (remote);
 }
 
+/* Test that we pull the runtime from the same remote as the app even if
+ * another remote also provides the runtime and sorts earlier via strcmp() */
+static void
+test_transaction_app_runtime_same_remote (void)
+{
+  g_autoptr(FlatpakInstallation) inst = NULL;
+  g_autoptr(FlatpakTransaction) transaction = NULL;
+  g_autoptr(FlatpakInstalledRef) installed_runtime = NULL;
+  g_autoptr(GError) error = NULL;
+  g_autofree char *app = NULL;
+  const char *runtime_origin;
+  gboolean res;
+  int old_prio;
+
+  app = g_strdup_printf ("app/org.test.Hello/%s/master",
+                         flatpak_get_default_arch ());
+
+  inst = flatpak_installation_new_user (NULL, &error);
+  g_assert_no_error (error);
+  g_assert_nonnull (inst);
+
+  empty_installation (inst);
+
+  add_remote_user ("test-runtime-only", "aaatest-runtime-only-repo");
+
+  /* Drop caches so we find the new remote */
+  flatpak_installation_drop_caches (inst, NULL, &error);
+  g_assert_no_error (error);
+
+  /* Even with aaatest-runtime-only-repo sorting before test-repo, the runtime
+   * should come from test-repo */
+  g_assert_true (strcmp ("aaatest-runtime-only-repo", "test-repo") < 0);
+
+  transaction = flatpak_transaction_new_for_installation (inst, NULL, &error);
+  g_assert_no_error (error);
+  g_assert_nonnull (transaction);
+
+  res = flatpak_transaction_add_install (transaction, repo_name, app, NULL, &error);
+  g_assert_no_error (error);
+  g_assert_true (res);
+
+  res = flatpak_transaction_run (transaction, NULL, &error);
+  g_assert_no_error (error);
+  g_assert_true (res);
+
+  installed_runtime = flatpak_installation_get_installed_ref (inst,
+                                                              FLATPAK_REF_KIND_RUNTIME,
+                                                              "org.test.Platform",
+                                                              flatpak_get_default_arch (),
+                                                              "master", NULL, &error);
+  g_assert_nonnull (installed_runtime);
+  g_assert_no_error (error);
+
+  runtime_origin = flatpak_installed_ref_get_origin (installed_runtime);
+  g_assert_nonnull (runtime_origin);
+  g_assert_cmpstr (runtime_origin, ==, repo_name);
+
+  /* Reset things */
+  empty_installation (inst);
+  remove_remote_user ("aaatest-runtime-only");
+}
+
 typedef struct
 {
   GMainLoop *loop;
@@ -4259,6 +4327,7 @@ main (int argc, char *argv[])
   g_test_add_func ("/library/transaction-flatpakref-remote-creation", test_transaction_flatpakref_remote_creation);
   g_test_add_func ("/library/transaction-deps", test_transaction_deps);
   g_test_add_func ("/library/transaction-install-local", test_transaction_install_local);
+  g_test_add_func ("/library/transaction-app-runtime-same-remote", test_transaction_app_runtime_same_remote);
   g_test_add_func ("/library/instance", test_instance);
   g_test_add_func ("/library/update-subpaths", test_update_subpaths);
   g_test_add_func ("/library/overrides", test_overrides);