utils: Drop stripping .desktop suffixes from appstream component ids

[bbhtt]

This will pass the exact appstream component ID to copy_icon

This was introduced in https://github.com/flatpak/flatpak/commit/7dd92d8a9be2e14313e2cda7dea44298fbb005a4 to handle appstream component IDs that ended in two .desktop suffixes. Recent analysis of appstream data shows that on Flathub no such appstream cid exists anymore and Telegram has component ID com.telegram.desktop now.

With the switch to libappstream, appstreamcli-compose produces icons in share/app-info/flatpak named by the appstream component ID instead of the $FLATPAK_ID used by appstream-glib. This causes applications whose $FLATPAK_ID does not end with .desktop but their appstream-component ID ends in .desktop ie. $FLATPAK_ID != appstream-cid to loose icons from the appstream ostree ref as copy_icon was being fed the id without .desktop but icons were created by appstreamcli with .desktop in them.

This will avoid adding anymore ID heuristics/workarounds on either side, per the discussion in https://github.com/flathub/flathub/issues/4222

An application with $FLATPAK_ID com.telegram.desktop and appstream ID com.telegram.desktop.desktop will be broken with this change but such dual .desktop IDs are non existent and should be fixed individually or be blocked on an app store level.

[bbhtt]

To repro:

1. Find an app with $FLATPAK_ID not ending in .desktop but appstream ID ending in .desktop ie a "mismatch". Eg. https://github.com/flathub/com.discordapp.Discord.git
2. git clone https://github.com/flathub/com.discordapp.Discord.git && cd com.discordapp.Discord
3. flatpak run org.flatpak.Builder --branch=stable --force-clean --sandbox --ccache --mirror-screenshots-url=https://dl.flathub.org/media/ --repo=repo --install-deps-from=flathub builddir com.discordapp.Discord.json
4. flatpak run --branch=stable --command=sh --filesystem=$(pwd) org.flatpak.Builder
5. flatpak --verbose build-update-repo repo
6. See that it failed to copy icons:

F: No icon at size 64x64 for com.discordapp.Discord
F: No icon at size 128x128 for com.discordapp.Discord

7. Now use the build from https://github.com/flathub/org.flatpak.Builder/pull/232#issuecomment-2023754294

flatpak run org.flatpak.Builder --branch=test --force-clean --sandbox --ccache --mirror-screenshots-url=https://dl.flathub.org/media/ --repo=repo --install-deps-from=flathub builddir com.discordapp.Discord.json flatpak run --branch=test --command=sh --filesystem=$(pwd) org.flatpak.Builder

Remove, rebuild and run build-update-repo repo from inside the sandbox again.

8. Extract the appstream refs to check (for good measure):

ostree --repo=repo checkout --union-add --user-mode --subpath=/ appstream/x86_64 ../appstream
ostree --repo=repo checkout --union-add --user-mode --subpath=/ appstreams/x86_64 ../appstream2

9. See that the icon is now present

[bbhtt]

Hey @TingPing any chance you can review this one? Flathub needs this on the runners which are based on Ubuntu and uses the PPA.

[swick]

Having read through all of it, this seems like reasonable change.

[bbhtt]

After some thought I can also invoke copy_icon multiple times based on whether FLATPAK_ID and component_id matches or not.

legacy_id = g_strconcat (id, ".desktop", NULL);

if (g_strcmp0 (component_id_text, id) == 0) {
   copy_icon(component_id_text, ...)
}
else if (g_strcmp0 (component_id_text, legacy_id) == 0) {
   copy_icon(legacy_id, ...)
}
else
   g_print (_("No icons found for components %s, %s\n"), component_id_text, legacy_id);

This will lead to:

• When FLATPAK_ID == component_id -> goes through the first condition
• When FLATPAK_ID == com.foo.bar but component_id == com.foo.bar.desktop -> goes through the second condition
  • The reverse of this ie. FLATPAK_ID == com.foo.bar.desktop and component_id == com.foo.bar is not possible because flatpak-builder passes $FLATPAK_ID and $FLATPAK_ID.desktop.
• When FLATPAK_ID == com.foo.desktop and component_id == com.foo.desktop -> same as (1)
• When FLATPAK_ID == com.foo.desktop and component_id == com.foo.desktop.desktop -> goes through the second condition
  • Reverse of this is again not possible

Let me know if I missed any other combination.

Another option is to drop this mismatch handling and use the value of the <icon type=cached> tag since we know that is how the actual icon filename will be named. But the issue there is id is more reliable and icon tags may not always exist.

[ximion]

But the issue there is id is more reliable and icon tags may not always exist.

If there is an icon, and icon tag will also exist, so you can rely on that.

Since the icon name will always follow the component ID, is that logic really needed? Why not use the component ID unconditionally to copy the icon, and ignore the Flatpak ID in this instance?

[bbhtt]

I was thinking about preserving compatibility with appstream-glib because you can be using flatpak with this patch, but flatpak-builder with composing done by appstream-glib.

Then in the case when FLATPAK_ID is com.foo.bar and appstream-cid is com.foo.bar.desktop, appstream-glib creates the icons as com.foo.bar.png, so if I always rely on cid, then it gets never copied.

[ximion]

I was thinking about preserving compatibility with appstream-glib because you can be using flatpak with this patch, but flatpak-builder with composing done by appstream-glib.

Yes - but doesn't appstream-glib also put the right values into its icon tags? And wouldn't that result in the things working as expected? Or does Flatpak simply not export the icons properly in that case?

In the latter case, your proposed change makes sense to me, and I don't think you missed a case. It was just a bit confusing: legacy_id is the FLATPAK_ID? Is it maybe meant to be the component-ID without desktop, in which case your first line would append a ".desktop" but strip it instead? Either that, or your copy_icon(legacy_id, ...) should actually be a copy_icon(id, ...) instead.

[bbhtt]

Yes - but doesn't appstream-glib also put the right values into its icon tags?

No it uses the $FLATPAK_ID, not the appstream-cid. So for flatpak_id == com.foo.bar, appstream-cid == com.foo.bar.desktop icon tag has com.foo.bar.png. You can check https://hub.flathub.org/repo/appstream/x86_64/appstream.xml.gz which still has older entries by picking an app from the list I posted that didn't get rebuilt with appstreamcli.

legacy_id is the FLATPAK_ID?

It is $FLATPAK_ID + ".desktop"

id here is $FLATPAK_ID, component_id_text is appstream-cid.

Either that, or your copy_icon(legacy_id, ...) should actually be a copy_icon(id, ...) instead.

Maybe I'm confusing something but seems ok to me?

If both FLATPAK_ID and cid match, I copy using id.

If they don't match ie. component_id_text != FLATPAK_ID but instead component_id_text == FLATPAK_ID + ".desktop" (legacy id) I copy using legacy_id instead.

The .desktop addition always occurs in appstream-cid.

Either that, or your copy_icon(legacy_id, ...) should actually be a copy_icon(id, ...) instead.

This would make it copy the same thing as the first condition?

I'm trying to "predict" how the filename produced by appstream would look instead of relying on appstream cid directly.

It can only be either $FLATPAK_ID.png, $FLATPAK_ID.desktop.png - as those two are the only components ids (minus png suffix) passed to appstream via flatpak-builder.

In case of appstream-glib it is always $FLATPAK_ID.png - so it'll match the first condition always.

In case of appstreamcli - if cid matches flatpak id - we're good and it goes through the first condition again.

If it doesn't match, it must be of the form of id + .desktop - so I'm trying that instead.

[bbhtt]

Alternative patch. Tested this to work with the combinations I mentioned above.

Patch

diff --git a/common/flatpak-utils.c b/common/flatpak-utils.c
index 7c2c60f2..319d721c 100644
--- a/common/flatpak-utils.c
+++ b/common/flatpak-utils.c
@@ -5234,6 +5234,7 @@ extract_appstream (OstreeRepo        *repo,
   g_autoptr(GInputStream) in = NULL;
   g_autoptr(FlatpakXml) xml_root = NULL;
   g_autoptr(GKeyFile) keyfile = NULL;
+  g_autofree char *legacy_id = NULL;
 
   if (!ostree_repo_read_commit (repo, flatpak_decomposed_get_ref (ref), &root, NULL, NULL, error))
     return FALSE;
@@ -5257,6 +5258,7 @@ extract_appstream (OstreeRepo        *repo,
   xmls_dir = g_file_resolve_relative_path (app_info_dir, "xmls");
   icons_dir = g_file_resolve_relative_path (app_info_dir, "icons/flatpak");
 
+  legacy_id = g_strconcat (id, ".desktop", NULL);
   appstream_basename = g_strconcat (id, ".xml.gz", NULL);
   appstream_file = g_file_get_child (xmls_dir, appstream_basename);
 
@@ -5305,21 +5307,36 @@ extract_appstream (OstreeRepo        *repo,
               continue;
             }
 
-          if (g_str_has_suffix (component_id_suffix, ".desktop"))
-            component_id_suffix[strlen (component_id_suffix) - strlen (".desktop")] = 0;
-
-          if (!copy_icon (component_id_text, icons_dir, repo, size1_mtree, "64x64", &my_error))
+          if (g_strcmp0 (component_id_text, id) == 0)
             {
-              g_print (_("Error copying 64x64 icon for component %s: %s\n"), component_id_text, my_error->message);
-              g_clear_error (&my_error);
-            }
+              if (!copy_icon (id, icons_dir, repo, size1_mtree, "64x64", &my_error))
+                {
+                  g_print (_("Error copying 64x64 icon for component %s: %s\n"), id, my_error->message);
+                  g_clear_error (&my_error);
+                }
 
-          if (!copy_icon (component_id_text, icons_dir, repo, size2_mtree, "128x128", &my_error))
-             {
-               g_print (_("Error copying 128x128 icon for component %s: %s\n"), component_id_text, my_error->message);
-               g_clear_error (&my_error);
-             }
+              if (!copy_icon (id, icons_dir, repo, size2_mtree, "128x128", &my_error))
+                {
+                   g_print (_("Error copying 128x128 icon for component %s: %s\n"), id, my_error->message);
+                   g_clear_error (&my_error);
+                }
+            }
+          else if (g_strcmp0 (component_id_text, legacy_id) == 0)
+            {
+              if (!copy_icon (legacy_id, icons_dir, repo, size1_mtree, "64x64", &my_error))
+                {
+                  g_print (_("Error copying 64x64 icon for component %s: %s\n"), component_id_text, my_error->message);
+                  g_clear_error (&my_error);
+                }
 
+              if (!copy_icon (legacy_id, icons_dir, repo, size2_mtree, "128x128", &my_error))
+                {
+                   g_print (_("Error copying 128x128 icon for component %s: %s\n"), component_id_text, my_error->message);
+                   g_clear_error (&my_error);
+                }
+            }
+          else
+            g_print (_("No icons found for components %s, %s\n"), id, legacy_id);
 
           /* We might match other prefixes, so keep on going */
           component = component->next_sibling;