flatpak-builder icon indicating copy to clipboard operation
flatpak-builder copied to clipboard

WIP: Port to libappstream

Open pwithnall opened this issue 4 years ago • 66 comments

appstream-glib is mostly unmaintained, and appstream is more actively developed (and up to date with the AppStream specification).

This ports flatpak-builder to use the command line tools from appstream (appstreamcli and appstreamcli-compose) rather than the ones from appstream-glib (appstream-util, appstream-builder and appstream-compose).

This should result in no changes for most things, but should fix propagation of <requires>/<recommends> elements, screenshot <caption>s, and other elements which have been added to the AppStream specification recently.

For example, see https://github.com/flathub/flathub/issues/2439

Signed-off-by: Philip Withnall [email protected]

pwithnall avatar Sep 21 '21 14:09 pwithnall

See https://github.com/flatpak/flatpak/pull/4426 for the related flatpak changes.

CC @ximion

pwithnall avatar Sep 21 '21 14:09 pwithnall

My main worry is if there are subtle differences between the behavior of the CLI tools that affect Flathub. But in general it makes sense to move to this.

TingPing avatar Sep 21 '21 15:09 TingPing

Yeah, this is going to need some good testing.

pwithnall avatar Sep 21 '21 15:09 pwithnall

Note I don't think this is going to work. flatpak-builder is supposed to support newer and older runtimes so the command in sandbox probably needs to remain parameter by parameter (and command name) identical. There could probably be a shim that passes to libappstream.

nanonyme avatar Sep 21 '21 16:09 nanonyme

Screenshot captions? Wow, those have been part of AppStream since the February 2014 release! I had no idea this was missing! appstreamcli-compose is rather new and currently not installed in $PATH to avoid issues - if it is installed, it can be run via appstreamcli compose, but I'm not sure yet whether I like this behavior.

Compose will cache screenshots in newer versions soon, I am not sure of you want this (feature can be disabled). There probably are differences, appstreamcli-compose and appstream-compose are quite different in what they can do, but the resulting data should be compatible with appstream-glib and older libappstream versions (older versions will just ignore any new elements). So I don't expect much breakage in other tools which just consume the metadata.

ximion avatar Sep 21 '21 18:09 ximion

As said, the very fact the command name is different is a starting point for pain here. And it looks like parameters are not. If this is merged and released, it means this flatpak-builder version can no longer be used to build any apps using old runtimes. This does not seem acceptable. We need some approach that will be backwards and forwards compatible.

nanonyme avatar Sep 21 '21 19:09 nanonyme

I see a few options we have:

  • Make sure all old runtimes include AppStream, so it'd have to be retroactively added for them to work again
  • Make people not use a newer flatpak-builder with older runtimes (which basically means force everyone to upgrade)
  • Add some compatibility code so flatpak-builder can use appstreamcli if that's available, but fall back to appstream-util if it's not there for older runtimes

I think the last option would probably be the most compatible and the one associated with the least amount of pain.

ximion avatar Sep 21 '21 19:09 ximion

I'm pretty sure Flathub infra would not work with the second option (@barthalion can probably say something about that) so that's probably a no-go. The first option is nigh impossible. Even if we could sort out freedesktop-sdk , there are gnome runtime which need to be dealt with separately. And then there's external child runtimes.

nanonyme avatar Sep 21 '21 20:09 nanonyme

The third option is the only realistic one. Supporting both.

TingPing avatar Sep 22 '21 01:09 TingPing

I think the third option is the way to go. Ironically, I think using appstream-util from the runtime was not the best idea, so from Flathub's perspective, it would be vastly preferred to use appstream from the host, and fall back to appstream-util inside the sandbox.

barthalion avatar Sep 22 '21 06:09 barthalion

Boo host dependency. If flatpak-builder is going to invoke a binary from outside the build environment, shouldn't it be a Flatpak? Then it's independent of whatever runtime it needs, the Flathub buildbot/flat-manager can use the same flatpak, and we can expect/assume the same behaviour from flatpak-builder of this version or later, regardless of the host or the SDK being used. (We could potentially even remove the fallback behaviour and just say, flatpak-builder after X.Y will always need org.whatever.appstream-badger to be installed.)

ramcq avatar Sep 22 '21 09:09 ramcq

@ramcq you mean like org.flatpak.Builder? FWIW flatpak-builder is primarily invoking tools from outside sandbox (stripping, fetching sources). Appstream handling is an exception here.

nanonyme avatar Sep 22 '21 10:09 nanonyme

No I mean like we provide an appstreamcli equiv of https://github.com/flathub/org.freedesktop.appstream-glib and flatpak-builder uses that going forward. Maybe with a host fallback? But appstream is an evolving spec and we've already suffered on our infra from being pinned to a host version of appstream-glib, which is why that flatpak exists. Let's not re-invent that problem here?

ramcq avatar Sep 22 '21 10:09 ramcq

Being a host dependency is exactly what lets us to easily decouple it from runtimes, like we already do with org.freedesktop.appstream-glib, which is a symlink to appstream-util on the build infra. I don't think tying flatpak-builder directly to a flatpak hosted on Flathub is something everyone will be happy about.

barthalion avatar Sep 22 '21 10:09 barthalion

Host dependency means org.flatpak.Builder could be theoretically self-contained as well.

barthalion avatar Sep 22 '21 11:09 barthalion

Being a host dependency is exactly what lets us to easily decouple it from runtimes, like we already do with org.freedesktop.appstream-glib, which is a symlink to appstream-util on the build infra. I don't think tying flatpak-builder directly to a flatpak hosted on Flathub is something everyone will be happy about.

Alright, makes sense. Host vs flatpak can be a deployment decision.

ramcq avatar Sep 22 '21 11:09 ramcq

Since AppStream may add new things in future, having its revision pinned to the runtime is probably unexpected - when it's a host dependency, Flatpak could say "require at least version X or higher" and be done with it, while when it's part of the runtime users of older runtimes can't get new stuff until AppStream is updated in the runtime as well. If it would be part of the host, you'd also only need to test AppStream-related changes once with one runtime, instead of verifying them with all older versions as well. I see arguments for both, but having it as host dependency does make some sense to me.

We also haven't had any breaking changes in AppStream for many years now, and the 1.0 release will just be removal of deprecated stuff (which hopefully will not break much stuff), so I think the only issue you might run into is a stricter validator (which I don't think is a huge issue, as only errors and warnings fail validation, and those are all quite severe issues that need to be fixed).

ximion avatar Sep 22 '21 11:09 ximion

What's the current state of this? Flatpak itself has already switched to libappstream in the latest pre-release and I really want to use the new features like in my Programs.

JakobDev avatar Mar 16 '22 09:03 JakobDev

Rebased it on top of the main branch and added a commit to run appstreamcli commands on the host. CI will likely break but it's a start.

barthalion avatar Apr 08 '22 07:04 barthalion

I've kicked off a test build at https://github.com/flathub/org.flatpak.Builder/pull/81. When I manage to build it, I will do some testing on Flathub apps.

barthalion avatar Apr 08 '22 07:04 barthalion

OK, so this is something that produces a supposedly working appstreamcli invocation:

Running appstreamcli-compose
FB: Running: appstreamcli compose --prefix= --origin=flatpak --result-root=/var/home/bpiotrowski/dev/flathub/org.gabmus.whatip/.flatpak-builder/rofiles/rofiles-q1EnZD/files --components=org.gabmus.whatip /var/home/bpiotrowski/dev/flathub/org.gabmus.whatip/.flatpak-builder/rofiles/rofiles-q1EnZD/files
Only accepting component: org.gabmus.whatip
Processing directory: /var/home/bpiotrowski/dev/flathub/org.gabmus.whatip/.flatpak-builder/rofiles/rofiles-q1EnZD/files
Composing metadata...
Success!

But the resulting builddir/files/share/swcatalog/xml/flatpak.xml.gz is empty, and I don't see app-info anywhere. Have I missed something about how the new compose command works?

barthalion avatar Apr 08 '22 10:04 barthalion

build-update-repo confirms that it's not really correct:

> flatpak build-update-repo repo
Updating appstream branch
No appstream data for app/org.gabmus.whatip/x86_64/localtest: No such file or directory: /files/share/app-info

So I guess that build-update-repo needs to start reading files from the swcatalog directory and the compose command needs to actually produce a proper XML file.

barthalion avatar Apr 08 '22 10:04 barthalion

@barthalion The appstreamcli compose command will generate mostly legacy-free output - but nothing is stopping you from just adding a mv /files/share/swcatalog /files/share/app-info after the compose to keep things running without many changes :-) As for the empty output file: Can you run compose with --verbose --print-report=short to see what's going on here? I can't reproduce this here... (the next version of AppStream will emit an error hint in case filters are set but no output was created though, to make an issue like this easily noticeable without having to check the generated output).

ximion avatar Apr 10 '22 02:04 ximion

@ximion probably makes sense to add WIP commit those flags into this branch?

nanonyme avatar Apr 15 '22 19:04 nanonyme

@ximion Added --verbose --print-report=short, an output from failed attempt to build GoLand can be seen here: https://github.com/flathub/org.flatpak.Builder/runs/6452841114?check_suite_focus=true#step%3A10%3A2366= I'm waiting for bot, build to pass to poke around locally.

barthalion avatar May 16 '22 13:05 barthalion

How to reproduce:

flatpak remote-add --user --if-not-exists flathub https://dl.flathub.org/repo/flathub.flatpakrepo
flatpak install --user https://dl.flathub.org/build-repo/89476/org.flatpak.Builder.flatpakref
git clone https://github.com/flathub/com.jetbrains.GoLand
cd com.jetbrains.GoLand
flatpak run org.flatpak.Builder --verbose --user --sandbox --force-clean --repo=repo --install-deps-from=flathub builddir com.jetbrains.GoLand.yaml
Running appstreamcli-compose
FB: Running: appstreamcli compose --prefix= --origin=flatpak --verbose --print-report=short --result-root=/home/bpiotrowski/.cache/flatpak-builder/rofiles/rofiles-guPqDs/files --components=com.jetbrains.GoLand /home/bpiotrowski/.cache/flatpak-builder/rofiles/rofiles-guPqDs/files
** (appstreamcli:272): DEBUG: 15:57:11.287: run appstreamcli: compose
Only accepting component: com.jetbrains.GoLand
Processing directory: /home/bpiotrowski/.cache/flatpak-builder/rofiles/rofiles-guPqDs/files
Composing metadata...
** (appstreamcli-compose:272): DEBUG: 15:57:11.380: No media export directory set, but icon 128x128@1 is set for remote delivery. Disabling remote for this icon type.
** (appstreamcli-compose:272): DEBUG: 15:57:11.380: No media export directory set, but icon 128x128@2 is set for remote delivery. Disabling remote for this icon type.
** (appstreamcli-compose:272): DEBUG: 15:57:11.380: No media export directory set, but screenshots are supposed to be stored. Disabling screenshot storage.
** (appstreamcli-compose:272): DEBUG: 15:57:11.395: Will use temporary directory '/tmp/as-compose_5slX1E' and delete it after this run.
** (appstreamcli-compose:272): DEBUG: 15:57:11.396: Creating contents index for directory: /home/bpiotrowski/.cache/flatpak-builder/rofiles/rofiles-guPqDs/files
** (appstreamcli-compose:272): DEBUG: 15:57:11.398: Index done for directory: /home/bpiotrowski/.cache/flatpak-builder/rofiles/rofiles-guPqDs/files
(appstreamcli-compose:272): GLib-GIO-DEBUG: 15:57:11.408: _g_io_module_get_default: Found default implementation local (GLocalVfs) for ‘gio-vfs’
** (appstreamcli-compose:272): DEBUG: 15:57:11.409: Removing temporary directory '/tmp/as-compose_5slX1E'
Run failed, some data was ignored.
Errors were raised during this compose run:
general
  E: filters-but-no-output
Refer to the generated issue report data for details on the individual problems.
Error: ERROR: appstreamcli-compose failed: Child process exited with code 1

barthalion avatar May 16 '22 14:05 barthalion

The metainfo file is there, and flatpak-builder no longer attempts to rename it to metainfo, so I assume I'm doing something wrong with appstreamcli compose.

barthalion avatar May 16 '22 14:05 barthalion

@barthalion This was very helpful, thanks! The problem was that AppStream didn't expect that someone would set --prefix= to an empty string (which would mean "look in the current directory" which makes no sense to the code as it can't jump beyond the given root directory). Instead, it expects / there. I adjusted AppStream's code to sanitize that value and work with an empty prefix as probably most people would expect, and also squashed another issue which made it count results incorrectly that was introduced in the last version. So, taking that patch should solve your issues (and I expect a new 0.15.4 AppStream release soonish as well, likely next week).

ximion avatar May 16 '22 15:05 ximion

@pwithnall @mwleeds @smcv Can you please review everything I pushed here since April? I will squash commits a bit once I resolve your comments, if you have any.

barthalion avatar May 17 '22 07:05 barthalion

FYI @smcv, we're going to need the yet-nonexistent appstream 0.15.4 in flatpak PPA.

barthalion avatar May 17 '22 11:05 barthalion