From 0cc51166f379f14169601b8f845db730a9c910de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Correa=20G=C3=B3mez?= Date: Thu, 14 Sep 2023 17:38:24 +0200 Subject: [PATCH 1/5] gs-plugin-apk: allow to upgrade only needing one list to set "after-state" This is to simplify the future change to the async API. Only having to take care of one list simplifies the work there. It should have no behavior change in the current code, apart from moving complexity from the after-math logic to the preparation one --- src/gs-plugin-apk/gs-plugin-apk.c | 60 +++++++++++++++---------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/src/gs-plugin-apk/gs-plugin-apk.c b/src/gs-plugin-apk/gs-plugin-apk.c index 3f6c1f6..de7244b 100644 --- a/src/gs-plugin-apk/gs-plugin-apk.c +++ b/src/gs-plugin-apk/gs-plugin-apk.c @@ -436,12 +436,15 @@ gs_plugin_app_remove (GsPlugin *plugin, * over the apps from @list, takes care that it is possible to update them, * and when they are ready to be updated, adds them to @ready. * + * Returns: Number of non-proxy apps added to the list **/ -static void +static unsigned int gs_plugin_apk_prepare_update (GsPlugin *plugin, GsAppList *list, GsAppList *ready) { + unsigned int added = 0; + for (guint i = 0; i < gs_app_list_length (list); i++) { GsApp *app; @@ -451,8 +454,14 @@ gs_plugin_apk_prepare_update (GsPlugin *plugin, * a proxy (and thus might contain some apps owned by us) */ if (gs_app_has_quirk (app, GS_APP_QUIRK_IS_PROXY)) { - gs_plugin_apk_prepare_update (plugin, gs_app_get_related (app), ready); - gs_app_set_state (app, GS_APP_STATE_INSTALLING); + unsigned int proxy_added; + proxy_added = gs_plugin_apk_prepare_update (plugin, gs_app_get_related (app), ready); + if (proxy_added) + { + gs_app_set_state (app, GS_APP_STATE_INSTALLING); + gs_app_list_add (ready, app); + added += proxy_added; + } continue; } @@ -464,9 +473,10 @@ gs_plugin_apk_prepare_update (GsPlugin *plugin, } gs_app_set_state (app, GS_APP_STATE_INSTALLING); - gs_app_list_add (ready, app); + added++; } + return added; } gboolean @@ -477,19 +487,25 @@ gs_plugin_update (GsPlugin *plugin, { GsPluginApk *self = GS_PLUGIN_APK (plugin); g_autoptr (GError) local_error = NULL; - g_autoptr (GsAppList) update_list = gs_app_list_new (); + g_autoptr (GsAppList) list_installing = gs_app_list_new (); + unsigned int num_sources; g_autofree const gchar **source_array = NULL; /* update UI as this might take some time */ gs_plugin_status_update (plugin, NULL, GS_PLUGIN_STATUS_WAITING); - gs_plugin_apk_prepare_update (plugin, list, update_list); + num_sources = gs_plugin_apk_prepare_update (plugin, list, list_installing); - source_array = g_new0 (const gchar *, gs_app_list_length (update_list) + 1); - for (int i = 0; i < gs_app_list_length (update_list); i++) + source_array = g_new0 (const gchar *, num_sources + 1); + for (int i = 0; i < num_sources;) { - GsApp *app = gs_app_list_index (update_list, i); - source_array[i] = gs_app_get_source_default (app); + GsApp *app = gs_app_list_index (list_installing, i); + const gchar *source = gs_app_get_source_default (app); + if (source) + { + source_array[i] = source; + i++; + } } if (!apk_polkit2_call_upgrade_packages_sync (self->proxy, source_array, @@ -507,37 +523,21 @@ gs_plugin_update (GsPlugin *plugin, * takes care of fixing things in the aftermath. */ g_dbus_error_strip_remote_error (local_error); g_propagate_error (error, g_steal_pointer (&local_error)); - for (int i = 0; i < gs_app_list_length (update_list); i++) + for (int i = 0; i < gs_app_list_length (list_installing); i++) { - GsApp *app = gs_app_list_index (update_list, i); + GsApp *app = gs_app_list_index (list_installing, i); gs_app_set_state_recover (app); } - /* Roll-back apps from the original list with a quirk */ - for (int i = 0; i < gs_app_list_length (list); i++) - { - GsApp *app = gs_app_list_index (list, i); - if (gs_app_has_quirk (app, GS_APP_QUIRK_IS_PROXY)) - gs_app_set_state_recover (app); - } - return FALSE; } - for (int i = 0; i < gs_app_list_length (update_list); i++) + for (int i = 0; i < gs_app_list_length (list_installing); i++) { - GsApp *app = gs_app_list_index (update_list, i); + GsApp *app = gs_app_list_index (list_installing, i); gs_app_set_state (app, GS_APP_STATE_INSTALLED); } - /* Roll-back apps from the original list with a quirk */ - for (int i = 0; i < gs_app_list_length (list); i++) - { - GsApp *app = gs_app_list_index (list, i); - if (gs_app_has_quirk (app, GS_APP_QUIRK_IS_PROXY)) - gs_app_set_state (app, GS_APP_STATE_INSTALLED); - } - gs_plugin_updates_changed (plugin); return TRUE; } From 46cbb218e1e3be7f385f10043f0dff03c9ab4960 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Correa=20G=C3=B3mez?= Date: Thu, 14 Sep 2023 19:07:43 +0200 Subject: [PATCH 2/5] gs-self-test: do not depend on return value from gs_plugin_loader_job_process We don't really want to test the GS library API. What we want is to make sure that the apps we wanted updated are updated, and the ones that we didn't want updated stay as they were. Since we have the apps in the context, use them! --- tests/gs-self-test.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tests/gs-self-test.c b/tests/gs-self-test.c index f188f9f..1691e52 100644 --- a/tests/gs-self-test.c +++ b/tests/gs-self-test.c @@ -111,7 +111,7 @@ gs_plugins_apk_updates (GsPluginLoader *plugin_loader) GsApp *system_app = NULL; g_autoptr (GsApp) foreign_app = NULL; g_autoptr (GsAppList) update_list = NULL; - g_autoptr (GsAppList) updated_list = NULL; + gboolean ret; GsAppList *related = NULL; // List updates @@ -137,29 +137,31 @@ gs_plugins_apk_updates (GsPluginLoader *plugin_loader) system_app = gs_app_list_index (related, 0); g_assert_cmpint (gs_app_get_state (system_app), ==, GS_APP_STATE_UPDATABLE_LIVE); - // Execute update! + // Add app that shouldn't be updated foreign_app = gs_app_new ("foreign"); + gs_app_set_state (foreign_app, GS_APP_STATE_UPDATABLE_LIVE); gs_app_list_add (update_list, foreign_app); // No management plugin, should get ignored! + // Execute update! g_object_unref (plugin_job); plugin_job = gs_plugin_job_newv (GS_PLUGIN_ACTION_UPDATE, "list", update_list, NULL); - updated_list = gs_plugin_loader_job_process (plugin_loader, plugin_job, NULL, &error); + ret = gs_plugin_loader_job_action (plugin_loader, plugin_job, NULL, &error); gs_test_flush_main_context (); g_assert_no_error (error); - g_assert_nonnull (updated_list); + g_assert_true (ret); // Check desktop app: TODO: Check logs! - desktop_app = gs_app_list_index (updated_list, 0); - g_assert_nonnull (desktop_app); g_assert_cmpint (gs_app_get_state (desktop_app), ==, GS_APP_STATE_INSTALLED); // Check generic proxy app: TODO: Check logs! - generic_app = gs_app_list_index (updated_list, 1); g_assert_true (gs_app_has_quirk (generic_app, GS_APP_QUIRK_IS_PROXY)); + g_assert_cmpint (gs_app_get_state (generic_app), ==, GS_APP_STATE_INSTALLED); related = gs_app_get_related (generic_app); g_assert_cmpint (gs_app_list_length (related), ==, 1); system_app = gs_app_list_index (related, 0); g_assert_cmpint (gs_app_get_state (system_app), ==, GS_APP_STATE_INSTALLED); + // Check foreign app: As it was! + g_assert_cmpint (gs_app_get_state (foreign_app), ==, GS_APP_STATE_UPDATABLE_LIVE); } static void From 94e285f2615beb6f536abcb7601232510b9c9f0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Correa=20G=C3=B3mez?= Date: Thu, 14 Sep 2023 19:47:41 +0200 Subject: [PATCH 3/5] Port gs_plugin_update to new async variant This was quite trivial, and the only change in cycles 44 and 45, so a great success overall --- meson.build | 2 +- src/gs-plugin-apk/gs-plugin-apk.c | 84 ++++++++++++++++++++++++++----- tests/gs-self-test.c | 5 +- 3 files changed, 74 insertions(+), 17 deletions(-) diff --git a/meson.build b/meson.build index 55a0439..14e0472 100644 --- a/meson.build +++ b/meson.build @@ -5,7 +5,7 @@ project( meson_version: '>=0.58' ) -gnome_software_dep = dependency('gnome-software', version: '>=43.0') +gnome_software_dep = dependency('gnome-software', version: '>=44.0') plugin_install_dir = gnome_software_dep.get_variable('plugindir') cargs = ['-DG_LOG_DOMAIN="GsPluginApk"', '-DI_KNOW_THE_GNOME_SOFTWARE_API_IS_SUBJECT_TO_CHANGE'] diff --git a/src/gs-plugin-apk/gs-plugin-apk.c b/src/gs-plugin-apk/gs-plugin-apk.c index de7244b..aa0eb7b 100644 --- a/src/gs-plugin-apk/gs-plugin-apk.c +++ b/src/gs-plugin-apk/gs-plugin-apk.c @@ -479,23 +479,55 @@ gs_plugin_apk_prepare_update (GsPlugin *plugin, return added; } -gboolean -gs_plugin_update (GsPlugin *plugin, - GsAppList *list, - GCancellable *cancellable, - GError **error) +static void +upgrade_apk_packages_cb (GObject *object_source, + GAsyncResult *res, + gpointer user_data); + +static void +gs_plugin_apk_update_apps_async (GsPlugin *plugin, + GsAppList *list, + GsPluginUpdateAppsFlags flags, + GsPluginProgressCallback progress_callback, + gpointer progress_user_data, + GsPluginAppNeedsUserActionCallback app_needs_user_action_callback, + gpointer app_needs_user_action_data, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) { GsPluginApk *self = GS_PLUGIN_APK (plugin); - g_autoptr (GError) local_error = NULL; - g_autoptr (GsAppList) list_installing = gs_app_list_new (); + g_autoptr (GTask) task = NULL; + GsAppList *list_installing = gs_app_list_new (); unsigned int num_sources; g_autofree const gchar **source_array = NULL; + g_debug ("Updating apps"); + + task = g_task_new (plugin, cancellable, callback, user_data); + g_task_set_source_tag (task, gs_plugin_apk_update_apps_async); + + if (!(flags & GS_PLUGIN_UPDATE_APPS_FLAGS_NO_DOWNLOAD)) + { + // This needs polkit changes. Ideally we'd download first, and + // apply from cache then. We should probably test this out in + // pmOS release upgrader script first + g_warning ("We don't implement 'NO_DOWNLOAD'"); + } + + if (flags & GS_PLUGIN_UPDATE_APPS_FLAGS_NO_APPLY) + { + g_task_return_boolean (task, TRUE); + return; + } + /* update UI as this might take some time */ gs_plugin_status_update (plugin, NULL, GS_PLUGIN_STATUS_WAITING); num_sources = gs_plugin_apk_prepare_update (plugin, list, list_installing); + g_debug ("Found %u apps to update", num_sources); + source_array = g_new0 (const gchar *, num_sources + 1); for (int i = 0; i < num_sources;) { @@ -508,8 +540,22 @@ gs_plugin_update (GsPlugin *plugin, } } - if (!apk_polkit2_call_upgrade_packages_sync (self->proxy, source_array, - cancellable, &local_error)) + g_task_set_task_data (task, g_steal_pointer (&list_installing), g_object_unref); + apk_polkit2_call_upgrade_packages (self->proxy, source_array, cancellable, + upgrade_apk_packages_cb, g_steal_pointer (&task)); +} + +static void +upgrade_apk_packages_cb (GObject *object_source, + GAsyncResult *res, + gpointer user_data) +{ + g_autoptr (GTask) task = G_TASK (g_steal_pointer (&user_data)); + GsPluginApk *self = g_task_get_source_object (task); + GsAppList *list_installing = g_task_get_task_data (task); + g_autoptr (GError) local_error = NULL; + + if (!apk_polkit2_call_upgrade_packages_finish (self->proxy, res, &local_error)) { /* When and upgrade transaction failed, it could be out of two reasons: * - The world constraints couldn't match. In that case, nothing was @@ -522,14 +568,14 @@ gs_plugin_update (GsPlugin *plugin, * and which didn't, so also recover everything and hope the refine * takes care of fixing things in the aftermath. */ g_dbus_error_strip_remote_error (local_error); - g_propagate_error (error, g_steal_pointer (&local_error)); for (int i = 0; i < gs_app_list_length (list_installing); i++) { GsApp *app = gs_app_list_index (list_installing, i); gs_app_set_state_recover (app); } - return FALSE; + g_task_return_error (task, g_steal_pointer (&local_error)); + return; } for (int i = 0; i < gs_app_list_length (list_installing); i++) @@ -538,8 +584,18 @@ gs_plugin_update (GsPlugin *plugin, gs_app_set_state (app, GS_APP_STATE_INSTALLED); } - gs_plugin_updates_changed (plugin); - return TRUE; + g_debug ("All apps updated correctly"); + + gs_plugin_updates_changed (GS_PLUGIN (self)); + g_task_return_boolean (task, TRUE); +} + +static gboolean +gs_plugin_apk_update_apps_finish (GsPlugin *plugin, + GAsyncResult *result, + GError **error) +{ + return g_task_propagate_boolean (G_TASK (result), error); } void @@ -1327,6 +1383,8 @@ gs_plugin_apk_class_init (GsPluginApkClass *klass) plugin_class->install_repository_finish = gs_plugin_apk_install_repository_finish; plugin_class->remove_repository_async = gs_plugin_apk_remove_repository_async; plugin_class->remove_repository_finish = gs_plugin_apk_remove_repository_finish; + plugin_class->update_apps_async = gs_plugin_apk_update_apps_async; + plugin_class->update_apps_finish = gs_plugin_apk_update_apps_finish; } GType diff --git a/tests/gs-self-test.c b/tests/gs-self-test.c index 1691e52..16cacb0 100644 --- a/tests/gs-self-test.c +++ b/tests/gs-self-test.c @@ -143,9 +143,8 @@ gs_plugins_apk_updates (GsPluginLoader *plugin_loader) gs_app_list_add (update_list, foreign_app); // No management plugin, should get ignored! // Execute update! g_object_unref (plugin_job); - plugin_job = gs_plugin_job_newv (GS_PLUGIN_ACTION_UPDATE, - "list", update_list, - NULL); + plugin_job = gs_plugin_job_update_apps_new (update_list, + GS_PLUGIN_UPDATE_APPS_FLAGS_NO_DOWNLOAD); ret = gs_plugin_loader_job_action (plugin_loader, plugin_job, NULL, &error); gs_test_flush_main_context (); g_assert_no_error (error); From b00c27413cbc621450c0f645bdabd95da2c8dde2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Correa=20G=C3=B3mez?= Date: Thu, 14 Sep 2023 19:50:35 +0200 Subject: [PATCH 4/5] Dockerfile: update image to branch with necessary patches Which is basically current main, with https://gitlab.gnome.org/GNOME/gnome-software/-/merge_requests/1798 applied. --- .github/Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/Dockerfile b/.github/Dockerfile index a95625f..bd4f075 100644 --- a/.github/Dockerfile +++ b/.github/Dockerfile @@ -1,6 +1,6 @@ FROM ghcr.io/distroless/alpine-base:latest -ENV BRANCH=gnome-43 +ENV BRANCH=profiler-private # distroless/alpine-base only has main by default RUN echo "" >> /etc/apk/repositories \ && echo https://dl-cdn.alpinelinux.org/alpine/edge/community >> /etc/apk/repositories @@ -8,7 +8,7 @@ RUN apk add --no-cache meson apk-polkit-rs-dev build-base \ curl gdk-pixbuf-dev libxmlb-dev glib-dev gtk4.0-dev libadwaita-dev \ json-glib-dev libsoup3-dev gspell-dev polkit-dev libgudev-dev appstream-dev \ desktop-file-utils gsettings-desktop-schemas-dev dbus -RUN curl -L -O https://gitlab.gnome.org/GNOME/gnome-software/-/archive/${BRANCH}/gnome-software-${BRANCH}.tar.gz \ +RUN curl -L -O https://gitlab.gnome.org/pabloyoyoista/gnome-software/-/archive/${BRANCH}/gnome-software-${BRANCH}.tar.gz \ && tar xf gnome-software-${BRANCH}.tar.gz \ && cd gnome-software-${BRANCH} \ && meson \ From 7ea573df22edf9cdd4596e744abb33505c6ae380 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Correa=20G=C3=B3mez?= Date: Tue, 3 Oct 2023 00:53:33 +0200 Subject: [PATCH 5/5] Run tests just on the session bus Avoid the system bus, since otherwise needs roots permissions, and is more cumbersome to setup --- .github/workflows/docker-image.yaml | 2 +- .github/workflows/unittests.yaml | 2 +- src/gs-plugin-apk/gs-plugin-apk.c | 5 ++++- tests/apkpolkit2.py | 2 +- tests/test.py | 3 +++ 5 files changed, 10 insertions(+), 4 deletions(-) diff --git a/.github/workflows/docker-image.yaml b/.github/workflows/docker-image.yaml index df7f3fd..98fb38e 100644 --- a/.github/workflows/docker-image.yaml +++ b/.github/workflows/docker-image.yaml @@ -46,7 +46,7 @@ jobs: labels: ${{ steps.meta.outputs.labels }} - name: Build and test plugin - run: docker run --mount type=bind,src=${{ github.workspace }},dst=/repo --rm ${{ env.REGISTRY }}/${{ env.IMAGE_NAME}}:${{ env.VERSION}} /bin/ash -c "cd repo && meson build && ninja -C build && dbus-daemon --system --fork --nopidfile && dbus-run-session -- meson test -v -C build" + run: docker run --mount type=bind,src=${{ github.workspace }},dst=/repo --rm ${{ env.REGISTRY }}/${{ env.IMAGE_NAME}}:${{ env.VERSION}} /bin/ash -c "cd repo && meson build && ninja -C build && dbus-run-session -- meson test -v -C build" - name: Push Docker image if: github.event_name == 'push' diff --git a/.github/workflows/unittests.yaml b/.github/workflows/unittests.yaml index 84371da..2f948de 100644 --- a/.github/workflows/unittests.yaml +++ b/.github/workflows/unittests.yaml @@ -22,4 +22,4 @@ jobs: run: meson build && ninja -C build - name: Test - run: dbus-daemon --system --fork --nopidfile && dbus-run-session -- meson test -v -C build + run: dbus-run-session -- meson test -v -C build diff --git a/src/gs-plugin-apk/gs-plugin-apk.c b/src/gs-plugin-apk/gs-plugin-apk.c index aa0eb7b..a92397e 100644 --- a/src/gs-plugin-apk/gs-plugin-apk.c +++ b/src/gs-plugin-apk/gs-plugin-apk.c @@ -214,13 +214,16 @@ gs_plugin_apk_setup_async (GsPlugin *plugin, gpointer user_data) { g_autoptr (GTask) task = NULL; + GBusType bus_type = G_BUS_TYPE_SYSTEM; task = g_task_new (plugin, cancellable, callback, user_data); g_task_set_source_tag (task, gs_plugin_apk_setup_async); g_debug ("Initializing plugin"); - apk_polkit2_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, + if (g_getenv ("G_TEST_SRCDIR")) + bus_type = G_BUS_TYPE_SESSION; + apk_polkit2_proxy_new_for_bus (bus_type, G_DBUS_PROXY_FLAGS_NONE, "dev.Cogitri.apkPolkit2", "/dev/Cogitri/apkPolkit2", diff --git a/tests/apkpolkit2.py b/tests/apkpolkit2.py index 503e256..ae6b3a9 100644 --- a/tests/apkpolkit2.py +++ b/tests/apkpolkit2.py @@ -18,7 +18,7 @@ BUS_NAME = 'dev.Cogitri.apkPolkit2' MAIN_OBJ = '/dev/Cogitri/apkPolkit2' MAIN_IFACE = 'dev.Cogitri.apkPolkit2' -SYSTEM_BUS = True +SYSTEM_BUS = False def load(mock, parameters): repos = [ diff --git a/tests/test.py b/tests/test.py index 463be30..3b030a3 100755 --- a/tests/test.py +++ b/tests/test.py @@ -27,6 +27,9 @@ def setUp(self): def tearDown(self): if self.log is not None: self.log.close() + if self.p_mock: + self.p_mock.terminate() + self.p_mock.wait() def test_apk(self): builddir = os.getenv('G_TEST_BUILDDIR')