New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
utils: Drop stripping .desktop suffixes from appstream component ids #5752
base: main
Are you sure you want to change the base?
Conversation
This will pass the exact appstream component ID to copy_icon This was introduced in flatpak@7dd92d8 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 flathub/flathub#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.
To repro:
Remove, rebuild and run build-update-repo repo from inside the sandbox again.
|
For historical reasons flatpak-builder passes ``$FLATPAK_ID.desktop` as a valid component ID to appstreamcli. This mismatch creates issues like the one described in flatpak/flatpak#5752 and flathub/flathub#4222 So require that both match and avoid new submissions leaking in with a mismatch.
For historical reasons flatpak-builder passes ``$FLATPAK_ID.desktop` as a valid component ID to appstreamcli. This mismatch creates issues like the one described in flatpak/flatpak#5752 and flathub/flathub#4222 So require that both match and avoid new submissions leaking in with a mismatch.
For historical reasons flatpak-builder passes ``$FLATPAK_ID.desktop` as a valid component ID to appstreamcli. This mismatch creates issues like the one described in flatpak/flatpak#5752 and flathub/flathub#4222 So require that both match and avoid new submissions leaking in with a mismatch.
For historical reasons flatpak-builder passes ``$FLATPAK_ID.desktop` as a valid component ID to appstreamcli. This mismatch creates issues like the one described in flatpak/flatpak#5752 and flathub/flathub#4222 So require that both match and avoid new submissions leaking in with a mismatch.
For historical reasons flatpak-builder passes ``$FLATPAK_ID.desktop` as a valid component ID to appstreamcli. This mismatch creates issues like the one described in flatpak/flatpak#5752 and flathub/flathub#4222 So require that both match and avoid new submissions leaking in with a mismatch.
For historical reasons flatpak-builder passes ``$FLATPAK_ID.desktop` as a valid component ID to appstreamcli. This mismatch creates issues like the one described in flatpak/flatpak#5752 and flathub/flathub#4222 So require that both match and avoid new submissions leaking in with a mismatch.
For historical reasons flatpak-builder passes ``$FLATPAK_ID.desktop` as a valid component ID to appstreamcli. This mismatch creates issues like the one described in flatpak/flatpak#5752 and flathub/flathub#4222 So require that both match and avoid new submissions leaking in with a mismatch.
Hey @TingPing any chance you can review this one? Flathub needs this on the runners which are based on Ubuntu and uses the PPA. |
Having read through all of it, this seems like reasonable change. |
After some thought I can also invoke copy_icon multiple times based on whether 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:
Let me know if I missed any other combination. Another option is to drop this mismatch handling and use the value of the |
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? |
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 |
Yes - but doesn't appstream-glib also put the right values into its 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: |
No it uses the
It is
Maybe I'm confusing something but seems ok to me? If both If they don't match ie. The
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 In case of appstream-glib it is 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 |
Alternative patch. Tested this to work with the combinations I mentioned above. Patchdiff --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; |
This will pass the exact appstream component ID to copy_icon
This was introduced in 7dd92d8 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 IDcom.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 ascopy_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 flathub/flathub#4222
An application with
$FLATPAK_ID
com.telegram.desktop
and appstream IDcom.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.