From ef059257001f7b2fbd6a52c56d5dab2df399d0f7 Mon Sep 17 00:00:00 2001 From: Phaedrus Leeds Date: Thu, 14 Oct 2021 15:47:06 -0700 Subject: [PATCH 1/4] dir: Don't mask the main ref of a noenumerate remote When we create origin remotes for apps installed via .flatpakref files, we set xa.noenumerate=true and xa.main-ref=app/com.example.App/arch/branch so that the remote is only used for the app it was intended for. This is implemented in flatpak_dir_list_remote_refs() by only listing refs in the remote which are already installed. This works fine after the ref is installed but in the short timespan between when the origin remote is created and the app is installed from it, it means that the remote appears as empty. The use case where I ran into this is in attempting to use flatpak_installation_fetch_remote_ref_sync() in the gnome-software flatpak plugin, in the handling of flatpakref files, which is intended to just display information to the user and let them decide whether to install the app. But I was not able to create a FlatpakRemoteRef due to the behavior described above; see #4453 for more details. So, change the behavior so that the main ref for an origin remote is visible even before it is installed. This technically could be a breaking change for some consumer of libflatpak but that seems very unlikely, and the new behavior makes more sense. Also add a unit test for this behavior. --- common/flatpak-dir.c | 11 +++++- tests/testlibrary.c | 93 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-) diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c index 5cba0f5d..30436e45 100644 --- a/common/flatpak-dir.c +++ b/common/flatpak-dir.c @@ -14351,9 +14351,10 @@ flatpak_dir_list_remote_refs (FlatpakDir *self, GHashTableIter hash_iter; gpointer key; g_autofree char *refspec_prefix = g_strconcat (state->remote_name, ":.", NULL); + g_autofree char *remote_main_ref = NULL; /* For noenumerate remotes, only return data for already locally - * available refs */ + * available refs or the ref set as xa.main-ref on the remote */ if (!ostree_repo_list_refs (self->repo, refspec_prefix, &local_refs, cancellable, error)) @@ -14368,6 +14369,14 @@ flatpak_dir_list_remote_refs (FlatpakDir *self, g_hash_table_insert (decomposed_local_refs, g_steal_pointer (&d), NULL); } + remote_main_ref = flatpak_dir_get_remote_main_ref (self, state->remote_name); + if (remote_main_ref != NULL && *remote_main_ref != '\0') + { + g_autoptr(FlatpakDecomposed) d = flatpak_decomposed_new_from_col_ref (remote_main_ref, state->collection_id, NULL); + if (d) + g_hash_table_insert (decomposed_local_refs, g_steal_pointer (&d), NULL); + } + /* Then we remove all remote refs not in the local refs set */ g_hash_table_foreach_remove (*refs, remove_unless_decomposed_in_hash, diff --git a/tests/testlibrary.c b/tests/testlibrary.c index 28d68050..3f7912fe 100644 --- a/tests/testlibrary.c +++ b/tests/testlibrary.c @@ -2845,6 +2845,9 @@ empty_installation (FlatpakInstallation *inst) flatpak_installation_prune_local_repo (inst, NULL, &error); g_assert_no_error (error); + + flatpak_installation_drop_caches (inst, NULL, &error); + g_assert_no_error (error); } static int ready_count; @@ -3541,6 +3544,95 @@ test_transaction_flatpakref_remote_creation (void) remove_remote_system ("test-runtime-only-repo"); } +static gboolean +ready_check_origin_remote (FlatpakTransaction *transaction) +{ + g_autoptr(FlatpakInstallation) installation = NULL; + g_autoptr(FlatpakRemoteRef) remote_ref = NULL; + g_autoptr(FlatpakRemote) remote = NULL; + g_autoptr(GError) error = NULL; + g_autofree char *app = NULL; + + installation = flatpak_transaction_get_installation (transaction); + app = g_strdup_printf ("app/org.test.Hello/%s/master", + flatpak_get_default_arch ()); + + /* The remote should return the ref set as xa.main-ref on it despite having xa.noenumerate set */ + remote = flatpak_installation_get_remote_by_name (installation, "hello-origin", NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (remote); + g_assert_true (flatpak_remote_get_noenumerate (remote)); + g_assert_cmpstr (flatpak_remote_get_main_ref (remote), ==, app); + + remote_ref = flatpak_installation_fetch_remote_ref_sync (installation, + "hello-origin", + FLATPAK_REF_KIND_APP, + "org.test.Hello", + flatpak_get_default_arch (), + "master", + NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (remote_ref); + + return TRUE; +} + +/* Test that installing a flatpakref causes an origin remote to be created when + * the file has no SuggestRemoteName= option, and check the options set on the + * remote */ +static void +test_transaction_flatpakref_origin_remote_creation (void) +{ + g_autoptr(FlatpakInstallation) user_inst = NULL; + g_autoptr(FlatpakTransaction) transaction = NULL; + g_autoptr(GError) error = NULL; + g_autofree char *s = NULL; + g_autoptr(GBytes) data = NULL; + gboolean res; + + user_inst = flatpak_installation_new_user (NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (user_inst); + + empty_installation (user_inst); + + assert_remote_not_in_installation (user_inst, "hello-origin"); + assert_remote_not_in_installation (user_inst, "test-runtime-only-repo"); + + transaction = flatpak_transaction_new_for_installation (user_inst, NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (transaction); + + s = g_strconcat ("[Flatpak Ref]\n" + "Title=Test App\n" + "Name=org.test.Hello\n" + "Branch=master\n" + "Url=http://127.0.0.1:", httpd_port, "/test-without-runtime\n" + "IsRuntime=False\n" + "RuntimeRepo=http://127.0.0.1:", httpd_port, "/test-runtime-only/test-runtime-only-repo.flatpakrepo\n", + NULL); + + data = g_bytes_new (s, strlen (s)); + res = flatpak_transaction_add_install_flatpakref (transaction, data, &error); + g_assert_no_error (error); + g_assert_true (res); + + g_signal_connect (transaction, "add-new-remote", G_CALLBACK (add_new_remote3), NULL); + g_signal_connect (transaction, "ready", G_CALLBACK (ready_check_origin_remote), NULL); + + res = flatpak_transaction_run (transaction, NULL, &error); + g_assert_no_error (error); + g_assert_true (res); + + assert_remote_in_installation (user_inst, "hello-origin"); + assert_remote_in_installation (user_inst, "test-runtime-only-repo"); + + empty_installation (user_inst); + /* note: the origin remote will be removed automatically in the above prune */ + assert_remote_not_in_installation (user_inst, "hello-origin"); + remove_remote_user ("test-runtime-only-repo"); +} + static gboolean check_ready1_abort (FlatpakTransaction *transaction) { @@ -4675,6 +4767,7 @@ main (int argc, char *argv[]) g_test_add_func ("/library/transaction-install-uninstall", test_transaction_install_uninstall); g_test_add_func ("/library/transaction-install-flatpakref", test_transaction_install_flatpakref); g_test_add_func ("/library/transaction-flatpakref-remote-creation", test_transaction_flatpakref_remote_creation); + g_test_add_func ("/library/transaction-flatpakref-origin-remote-creation", test_transaction_flatpakref_origin_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); From 7f3556d92ca7af1eaabeaf893eefa2d970433368 Mon Sep 17 00:00:00 2001 From: Phaedrus Leeds Date: Fri, 15 Oct 2021 09:38:10 -0700 Subject: [PATCH 2/4] Fix implementation of xa.noenumerate remote option Currently the xa.noenumerate option on a remote is documented as causing the remote not to be used when presenting available apps/runtimes or when searching for dependencies. The idea is that the remote is only used for providing updates for things installed from it, and this functionality is used when creating an origin remote for something installed via a flatpakref file. However, the implementation of this in flatpak_dir_list_remote_refs() is buggy. It returns an empty set of refs even if something is both locally installed and available from the remote. This is because it is using hash table comparisons of FlatpkDecomposed objects (via flatpak_decomposed_hash()) which take into account both the ref (or refspec) and the collection ID, and the local refs' FlatpakDecomposed objects are created from a refspec whereas the remote refs' FlatpakDecomposed objects are created from a ref alone. We could fix this by having them both use refspecs, but it is better to use a collection-ref tuple for the following reasons: (1) Changing flatpak_dir_list_all_remote_refs() to use a refspec to create the FlatpakDecomposed objects would be a breaking change for the other users of that function. (2) Both the local and remote refs are from the same remote so we don't need to use the remote name to disambiguate them, even if no collection ID is configured. (3) The whole point of collection IDs is to make refs uniquely identifiable, so we're using them for the intended purpose. In addition to fixing this bug, this commit adds a unit test in testlibrary.c so it stays fixed. --- common/flatpak-dir.c | 10 ++++++-- tests/testlibrary.c | 58 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c index 30436e45..a10e5951 100644 --- a/common/flatpak-dir.c +++ b/common/flatpak-dir.c @@ -12283,7 +12283,7 @@ flatpak_dir_list_all_remote_refs (FlatpakDir *self, { summary = var_summary_from_gvariant (subsummary); ref_map = var_summary_get_ref_map (summary); - populate_hash_table_from_refs_map (ret_all_refs, NULL, ref_map, NULL, state); + populate_hash_table_from_refs_map (ret_all_refs, NULL, ref_map, state->collection_id, state); } } else if (state->summary != NULL) @@ -14364,7 +14364,13 @@ flatpak_dir_list_remote_refs (FlatpakDir *self, while (g_hash_table_iter_next (&hash_iter, &key, NULL)) { const char *refspec = key; - g_autoptr(FlatpakDecomposed) d = flatpak_decomposed_new_from_refspec (refspec, NULL); + g_autofree char *ref = NULL; + g_autoptr(FlatpakDecomposed) d = NULL; + + if (!ostree_parse_refspec (refspec, NULL, &ref, error)) + return FALSE; + + d = flatpak_decomposed_new_from_col_ref (ref, state->collection_id, NULL); if (d) g_hash_table_insert (decomposed_local_refs, g_steal_pointer (&d), NULL); } diff --git a/tests/testlibrary.c b/tests/testlibrary.c index 3f7912fe..e331a7fa 100644 --- a/tests/testlibrary.c +++ b/tests/testlibrary.c @@ -1129,6 +1129,63 @@ test_list_remote_refs (void) } } +/* Test the xa.noenumerate option on a remote, which should mask non-installed refs */ +static void +test_list_remote_refs_noenumerate (void) +{ + g_autoptr(FlatpakInstallation) inst = NULL; + g_autoptr(GError) error = NULL; + g_autoptr(GPtrArray) refs = NULL; + g_autoptr(FlatpakRemote) remote = NULL; + g_autoptr(FlatpakInstalledRef) runtime_ref = NULL; + gboolean res; + + inst = flatpak_installation_new_user (NULL, &error); + g_assert_no_error (error); + + empty_installation (inst); + + refs = flatpak_installation_list_remote_refs_sync (inst, repo_name, NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (refs); + g_assert_cmpint (refs->len, ==, 4); + + /* Install a runtime */ + G_GNUC_BEGIN_IGNORE_DEPRECATIONS + runtime_ref = flatpak_installation_install (inst, + repo_name, + FLATPAK_REF_KIND_RUNTIME, + "org.test.Platform", + NULL, "master", NULL, NULL, NULL, + &error); + G_GNUC_END_IGNORE_DEPRECATIONS + g_assert_no_error (error); + g_assert_true (FLATPAK_IS_INSTALLED_REF (runtime_ref)); + + /* Set xa.noenumerate=true */ + remote = flatpak_installation_get_remote_by_name (inst, repo_name, NULL, &error); + g_assert_no_error (error); + flatpak_remote_set_noenumerate (remote, TRUE); + res = flatpak_installation_modify_remote (inst, remote, NULL, &error); + g_assert_no_error (error); + g_assert_true (res); + + /* Only the platform should be visible */ + g_clear_pointer (&refs, g_ptr_array_unref); + refs = flatpak_installation_list_remote_refs_sync (inst, repo_name, NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (refs); + g_assert_cmpint (refs->len, ==, 1); + + empty_installation (inst); + + /* Set xa.noenumerate=false */ + flatpak_remote_set_noenumerate (remote, FALSE); + res = flatpak_installation_modify_remote (inst, remote, NULL, &error); + g_assert_no_error (error); + g_assert_true (res); +} + static void test_update_installed_ref_if_missing_runtime (void) { @@ -4754,6 +4811,7 @@ main (int argc, char *argv[]) g_test_add_func ("/library/remote-new", test_remote_new); g_test_add_func ("/library/remote-new-from-file", test_remote_new_from_file); g_test_add_func ("/library/list-remote-refs", test_list_remote_refs); + g_test_add_func ("/library/list-remote-refs-noenumerate", test_list_remote_refs_noenumerate); g_test_add_func ("/library/list-remote-related-refs", test_list_remote_related_refs); g_test_add_func ("/library/list-remote-related-refs-for-installed", test_list_remote_related_refs_for_installed); g_test_add_func ("/library/list-refs", test_list_refs); From 6de22f0e62de3ae1cb6cc1ba986951cf31476508 Mon Sep 17 00:00:00 2001 From: Phaedrus Leeds Date: Fri, 15 Oct 2021 12:24:15 -0700 Subject: [PATCH 3/4] Include extensions in listing origin remote refs In a recent commit I changed flatpak_dir_list_remote_refs() to make main refs (xa.main-ref) visible for noenumerate remotes (xa.noenumerate), meaning that an origin remote created for a flatpakref file can be used to get information about the ref from the file even before it is installed. This commit extends that work to include extensions of the main ref, and updates the unit test to check for this. The reasoning is that if GNOME Software wants to show the user information about, say, VLC from a flatpakref file before installing it, Software would want to also show the available plugins so the user can decide if they want to install those as well. I haven't checked if Software actually does that; I'm only focusing on making libflatpak do the right thing. --- common/flatpak-dir.c | 48 +++++++++++++++++++++++++------------------- tests/testlibrary.c | 12 +++++++++++ 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c index a10e5951..c5b03d3e 100644 --- a/common/flatpak-dir.c +++ b/common/flatpak-dir.c @@ -14316,17 +14316,6 @@ flatpak_dir_modify_remote (FlatpakDir *self, return TRUE; } -static gboolean -remove_unless_decomposed_in_hash (gpointer key, - gpointer value, - gpointer user_data) -{ - GHashTable *table = user_data; - const FlatpakDecomposed *d = key; - - return !g_hash_table_contains (table, d); -} - gboolean flatpak_dir_list_remote_refs (FlatpakDir *self, FlatpakRemoteState *state, @@ -14348,13 +14337,15 @@ flatpak_dir_list_remote_refs (FlatpakDir *self, g_autoptr(GHashTable) decomposed_local_refs = g_hash_table_new_full ((GHashFunc)flatpak_decomposed_hash, (GEqualFunc)flatpak_decomposed_equal, (GDestroyNotify)flatpak_decomposed_unref, NULL); g_autoptr(GHashTable) local_refs = NULL; + g_autoptr(FlatpakDecomposed) decomposed_main_ref = NULL; GHashTableIter hash_iter; gpointer key; g_autofree char *refspec_prefix = g_strconcat (state->remote_name, ":.", NULL); g_autofree char *remote_main_ref = NULL; /* For noenumerate remotes, only return data for already locally - * available refs or the ref set as xa.main-ref on the remote */ + * available refs, or the ref set as xa.main-ref on the remote, or + * extensions of that main ref */ if (!ostree_repo_list_refs (self->repo, refspec_prefix, &local_refs, cancellable, error)) @@ -14377,16 +14368,31 @@ flatpak_dir_list_remote_refs (FlatpakDir *self, remote_main_ref = flatpak_dir_get_remote_main_ref (self, state->remote_name); if (remote_main_ref != NULL && *remote_main_ref != '\0') - { - g_autoptr(FlatpakDecomposed) d = flatpak_decomposed_new_from_col_ref (remote_main_ref, state->collection_id, NULL); - if (d) - g_hash_table_insert (decomposed_local_refs, g_steal_pointer (&d), NULL); - } + decomposed_main_ref = flatpak_decomposed_new_from_col_ref (remote_main_ref, state->collection_id, NULL); - /* Then we remove all remote refs not in the local refs set */ - g_hash_table_foreach_remove (*refs, - remove_unless_decomposed_in_hash, - decomposed_local_refs); + /* Then we remove all remote refs not in the local refs set, not the main + * ref, and not an extension of the main ref */ + GLNX_HASH_TABLE_FOREACH_IT (*refs, it, FlatpakDecomposed *, d, void *, v) + { + if (g_hash_table_contains (decomposed_local_refs, d)) + continue; + + if (decomposed_main_ref != NULL) + { + g_autofree char *main_ref_id = NULL; + g_autofree char *main_ref_prefix = NULL; + + if (flatpak_decomposed_equal (decomposed_main_ref, d)) + continue; + + main_ref_id = flatpak_decomposed_dup_id (decomposed_main_ref); + main_ref_prefix = g_strconcat (main_ref_id, ".", NULL); + if (flatpak_decomposed_id_has_prefix (d, main_ref_prefix)) + continue; + } + + g_hash_table_iter_remove (&it); + } } return TRUE; diff --git a/tests/testlibrary.c b/tests/testlibrary.c index e331a7fa..ca7a1030 100644 --- a/tests/testlibrary.c +++ b/tests/testlibrary.c @@ -3631,6 +3631,18 @@ ready_check_origin_remote (FlatpakTransaction *transaction) g_assert_no_error (error); g_assert_nonnull (remote_ref); + /* An extension with the main ref as a prefix should also be visible */ + g_clear_object (&remote_ref); + remote_ref = flatpak_installation_fetch_remote_ref_sync (installation, + "hello-origin", + FLATPAK_REF_KIND_RUNTIME, + "org.test.Hello.Plugin.fun", + flatpak_get_default_arch (), + "v1", + NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (remote_ref); + return TRUE; } From df3d85e7539281de6810ea93ba81b161e3e71774 Mon Sep 17 00:00:00 2001 From: Phaedrus Leeds Date: Fri, 15 Oct 2021 13:13:06 -0700 Subject: [PATCH 4/4] testlibrary: Add test for remote nodeps option Always good to expand the test coverage --- tests/testlibrary.c | 110 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/tests/testlibrary.c b/tests/testlibrary.c index ca7a1030..cac8a893 100644 --- a/tests/testlibrary.c +++ b/tests/testlibrary.c @@ -3921,6 +3921,115 @@ test_transaction_app_runtime_same_remote (void) remove_remote_user ("aaatest-runtime-only-repo"); } +static gboolean +ready_set_nodeps_on_remote (FlatpakTransaction *transaction) +{ + g_autoptr(FlatpakInstallation) installation = NULL; + g_autoptr(FlatpakRemote) remote = NULL; + g_autoptr(GError) error = NULL; + gboolean res; + + installation = flatpak_transaction_get_installation (transaction); + + /* Set the nodeps option on the runtime remote */ + remote = flatpak_installation_get_remote_by_name (installation, "test-runtime-only-repo", NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (remote); + g_assert_false (flatpak_remote_get_nodeps (remote)); + + flatpak_remote_set_nodeps (remote, TRUE); + res = flatpak_installation_modify_remote (installation, remote, NULL, &error); + g_assert_no_error (error); + g_assert_true (res); + + /* Abort since this transaction is already resolved; we need a new one */ + return FALSE; +} + +/* Test that setting xa.nodeps=true on the remote providing the runtime causes + * app installation to fail */ +static void +test_remote_nodeps_option (void) +{ + g_autoptr(FlatpakInstallation) user_inst = NULL; + g_autoptr(FlatpakRemote) remote = NULL; + g_autoptr(FlatpakTransaction) transaction = NULL; + g_autoptr(GError) error = NULL; + g_autofree char *s = NULL; + g_autoptr(GBytes) data = NULL; + gboolean res; + + user_inst = flatpak_installation_new_user (NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (user_inst); + + empty_installation (user_inst); + + assert_remote_not_in_installation (user_inst, "hello-origin"); + assert_remote_not_in_installation (user_inst, "test-runtime-only-repo"); + + /* Set the nodeps option on the test-repo remote */ + remote = flatpak_installation_get_remote_by_name (user_inst, "test-repo", NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (remote); + g_assert_false (flatpak_remote_get_nodeps (remote)); + flatpak_remote_set_nodeps (remote, TRUE); + res = flatpak_installation_modify_remote (user_inst, remote, NULL, &error); + g_assert_no_error (error); + g_assert_true (res); + + transaction = flatpak_transaction_new_for_installation (user_inst, NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (transaction); + + s = g_strconcat ("[Flatpak Ref]\n" + "Title=Test App\n" + "Name=org.test.Hello\n" + "Branch=master\n" + "Url=http://127.0.0.1:", httpd_port, "/test-without-runtime\n" + "IsRuntime=False\n" + "RuntimeRepo=http://127.0.0.1:", httpd_port, "/test-runtime-only/test-runtime-only-repo.flatpakrepo\n", + NULL); + + data = g_bytes_new (s, strlen (s)); + res = flatpak_transaction_add_install_flatpakref (transaction, data, &error); + g_assert_no_error (error); + g_assert_true (res); + + g_signal_connect (transaction, "add-new-remote", G_CALLBACK (add_new_remote3), NULL); + g_signal_connect (transaction, "ready", G_CALLBACK (ready_set_nodeps_on_remote), NULL); + + res = flatpak_transaction_run (transaction, NULL, &error); + g_assert_error (error, FLATPAK_ERROR, FLATPAK_ERROR_ABORTED); + g_assert_false (res); + g_clear_error (&error); + + /* Make a new transaction so the runtime resolution happens again */ + g_clear_object (&transaction); + transaction = flatpak_transaction_new_for_installation (user_inst, NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (transaction); + res = flatpak_transaction_add_install_flatpakref (transaction, data, &error); + g_assert_no_error (error); + g_assert_true (res); + g_signal_connect (transaction, "add-new-remote", G_CALLBACK (add_new_remote3), NULL); + + res = flatpak_transaction_run (transaction, NULL, &error); + g_assert_error (error, FLATPAK_ERROR, FLATPAK_ERROR_RUNTIME_NOT_FOUND); + g_assert_false (res); + g_clear_error (&error); + + /* Set nodeps back to false */ + flatpak_remote_set_nodeps (remote, FALSE); + res = flatpak_installation_modify_remote (user_inst, remote, NULL, &error); + g_assert_no_error (error); + g_assert_true (res); + + empty_installation (user_inst); + remove_remote_user ("hello-origin"); + remove_remote_user ("test-runtime-only-repo"); +} + typedef struct { GMainLoop *loop; @@ -4841,6 +4950,7 @@ main (int argc, char *argv[]) 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/remote-nodeps-option", test_remote_nodeps_option); 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);