Don't use origin subfolder for flatpak icons

[davidmhewitt]

While implementing AppStream support in elementary AppCenter, I've noticed the following behaviour:

When creating an AppStream pool and loading the metadata from a Flatpak appstream folder (e.g. flathub's metadata is stored in /var/lib/flatpak/appstream/flathub/x86_64/active/ on my computer), no icons are available on any of the components.

The components in flatpak appdata xmls all have icons all in the following format:
<icon type="cached" height="64" width="64">br.gov.cti.invesalius.png</icon>

i.e. always cached, and always with a width and a height

However, for cached icons, the as_component_refine_icons method checks to see if files exist in a subdirectory structure, where one of the subdirectories is the origin name. As far as I can tell, the origin name will always be flatpak due to these hardcoded lines in flatpak:
https://github.com/flatpak/flatpak/blob/2bb84cc52216559b8a8dad2a43ef1b4d309dae2d/common/flatpak-utils.c#L3724-L3725

However, all of the various flatpak repositories I have enabled on my machine don't include an origin subfolder and instead put the icon size directories directly in icons:

/var/lib/flatpak/appstream/flathub/x86_64/active/
├── appstream.xml
├── appstream.xml.gz
└── icons
    ├── 128x128 [472 entries exceeds filelimit, not opening dir]
    └── 64x64 [508 entries exceeds filelimit, not opening dir]

I appreciate this is an issue of whatever generates that directory structure not following the spec properly, but it seems that there are already a bunch of applications and scripts that depend on this directory structure (e.g. GNOME Software and scripts that extract appdata/icons from flatpak repos), plus getting every repo to change would be difficult.

So I propose handling this case in the appstream library. The proposed patch includes an exception to the origin folder when the origin name is flatpak, but I'd be open to suggestions of other ways to solve this by people with a better overview of the whole picture than I.

[ximion]

This is a bug in Flatpak, the specification is very explicit about how the directory layout should be: https://www.freedesktop.org/software/appstream/docs/sect-AppStream-IconCache.html (and this was never changed)
@hughsie why does this work with asglib? Does that library search even more folders?
@alexlarsson Has Flatpak always behaved like this, or is this new? Maybe there was no origin set before?

If I add a workaround to libas, I'd prefer one that doesn't add a lot of string comparisons to the refine method - those have a very high performance impact when they accumulate.
I also thing the origin field value is wrong here in general - it should be "flathub" not "flatpak" (flatpak is the bundle type, while flathub is where the software described in the metadata file originates from)

[davidmhewitt]

All good points and questions, thanks, Matthias.

I just wanted to raise a quick PR that fixes the issue as it helped me to articulate what was going on somewhat too. I appreciate it's not ideal, but I'll be happy to help resolve it in a better manner once we figure out the best way forward.

[davidmhewitt]

I've opened a couple of PRs in both asglib and flatpak to try and provoke some more discussion around this issue:
hughsie/appstream-glib#297
flatpak/flatpak#2779

[ximion]

Yes, I have seen those, thank you!
FTR, the reason that the origin string is included in the icon path is that the original intent was to have multiple repositories share one on-disk location for metadata. We don't want to load the wrong icon for apps from different locations, so the origin had to be in the icon cache path.

[alexlarsson]

The way flatpak manages appstream data is very specific to flatpak. Each installation (/var/lib/flatpak and ~/.local/share/flatpak) are completely isolated from each other and from the host system. The appstream data from each configured remote is pulled as an ostree commit and then checked out next to the repo (because for this to work well we need to guarantee that hardlinks from the repo to this dir works). This data is purely read-only, immutable data from the upstream ostree repo, so we don't do any local modification (like combining multiple remotes or installations into a combined appstream file where origins are needed).

I realize that this doesn't match what the appstream docs say, but that only means we're not using that part of the spec because it doesn't fit with how flatpak needs to use appstream. This has always been like this in flatpak, and the intent was not to affect the system use of appstream which is left as-is. No code following the appstream spec should be confused by anything flatpak does unless it is modified to look in flatpak specific locations.

Now, for the specific case of the origin field value, the current behaviour is not really fundamental.

When flatpak-builder creates the appdata for a file it calls appstream-compose to combine the various sources of info into a standard layout. This happens here:
https://github.com/flatpak/flatpak-builder/blob/master/src/builder-manifest.c#L2707

For this to even work we need to pass a value for origin, and by this point we don't really have any particular good value for it, because we don't know where the result will be distributed from. Also, as mentioned above we're not really using origin, because we never combine appstreams, so the value is not very important. Thus I just put 'flatpak' there to satisfy the need for something.

I can see it being useful to use the standard appstream libraries to parse the flatpak appstream directories though, so if we can make this work that would be a win.

We have lots of code in deployment (gnome-software, kde discover) looking at the current layout, so we can't easily change it. Nor will it be very easy to modify the origin (because we don't really have a good value for it when the app is built). However, we can easily extend the layout. For instance, we could put a symlink in $dir/icons/flatpak that points to ".", which would mean that $dir/icon/$origin/$size would resolve to the right thing.

Would that help? Such a change would be done on the repo server (i.e. flathub) and immediately propagate out to all clients, so no client updates would be necessary.

[davidmhewitt]

@alexlarsson Thank you for the in-depth, detailed response, that helps make things a lot clearer as a relative newcomer to flatpak technology.

I've just tested and a symlink does indeed work, libappstream is able to pick up the icons for the components. I guess the only thing to be careful of is that nothing is searching that directory for anything by recursively walking the tree, as it would get stuck in a circular symlink loop.

[alexlarsson]

@davidmhewitt yeah, maybe a "flatpak" subdirectory with $size symlinks is safer and more obvious

[alexlarsson]

flatpak 1.3.1 will have the symlinks.

[davidmhewitt]

@alexlarsson Thank you, it's much appreciated.

I'll close this PR as it's no longer relevant.

[ximion]

@alexlarsson Thank you very much for your detailed reply!

[...] we don't do any local modification (like combining multiple remotes or installations into a combined appstream file where origins are needed).

AppStream doesn't do that either, however data from multiple sources is combined in one bigger data pool, and knowing about the origin of a component makes sense there.

For this to even work we need to pass a value for origin, and by this point we don't really have any particular good value for it, because we don't know where the result will be distributed from. Also, as mentioned above we're not really using origin, because we never combine appstreams, so the value is not very important. Thus I just put 'flatpak' there to satisfy the need for something.

Would there be a way to change that? The origin value has nothing to do with combining XML files from multiple sources, but has everything to do with merging and displaying components from different sources. For example, people may have multiple Flatpak sources providing the same app. With the current model, a software center will choose one app at random (metadata-wise). However, if an origin is set, the two applications will be displayed alongside of each other and the user can choose from which source they want to install the app.
The origin is also displayed in software centers to the user, usually.
This is of course if Flatpak allows apps with the same name to be installed from different sources. If it doesn't allow for this case (which app is preferred then? the most recent one?), then setting an origin like "flatpak" makes sense, as in that case all apps are equal.
Something like this is done in AppStream internally for OS repositories (which do have an origin value like debian-sid-main, but where packages can't be installed individually from different origins).

I can see it being useful to use the standard appstream libraries to parse the flatpak appstream directories though, so if we can make this work that would be a win.

I think this is how this works already, at least all consumers of libappstream seem to do this.

We have lots of code in deployment (gnome-software, kde discover) looking at the current layout

This is something that I was wondering about: KDE Discover is using libappstream too, yet everything seems to work there. If David hadn't raised this issue, I wouldn't even have noticed that there was a problem. I need to look at what Discover does differently here, or whether there is also an issue there.

However, we can easily extend the layout. For instance, we could put a symlink in $dir/icons/flatpak that points to ".", which would mean that $dir/icon/$origin/$size would resolve to the right thing.
Would that help? Such a change would be done on the repo server (i.e. flathub) and immediately propagate out to all clients, so no client updates would be necessary.

This would be perfect! Thank you very much for the quick response and change!

I want the AppStream specification to be quite strict and I want libappstream to fail hard and visibly in case something doesn't comply with the specification (with the exception of backwards compatibility support). Every time an implementation is too generous in what it accepts, bad things will happen in the long run and reimplementations will be so much harder to achieve.
However, this doesn't mean that I wouldn't change the spec at all or allow for exceptions, as long as that's documented, I am fine with it. As long as I know about it, that is.
So, tl;dr, if you ever need any change in AppStream or think something the spec says is bogus or not optimal, please raise this as an issue instead of working around it :-)