FlatpakTransaction end-of-lifed-with-rebase API improvement

[mwleeds]

Currently, when a FlatpakTransaction emits a end-of-lifed-with-rebase signal, the signal handler is expected to execute flatpak_transaction_add_uninstall() and flatpak_transaction_add_rebase(). However, as pointed out here, this isn't ideal because what you really want is to uninstall the old ref and rebase to the new one atomically, not potentially be in a situation where you do one but not the other (though rebasing without uninstalling is not as bad as uninstalling without rebasing).

So perhaps we need a way to remove something from the transaction, or a combined "uninstall and rebase" function.

[mwleeds]

It is also possibly an issue that there's no way to abort the transaction in case of errors here. Perhaps that could be desirable.

[pwithnall]

I’ve just been looking into this, and can reproduce it:

1. Add the following code in flatpak-transaction.c around line 5150, just before the _run_op_kind() call:

if ((op->kind == FLATPAK_TRANSACTION_OPERATION_INSTALL_OR_UPDATE ||
     op->kind == FLATPAK_TRANSACTION_OPERATION_INSTALL) &&
    g_str_equal (pref, "app.drey.Dialect/x86_64/stable"))
  {
    g_set_error (&local_error, FLATPAK_ERROR, FLATPAK_ERROR_OUT_OF_SPACE, "er ner, out of space");
    res = FALSE;
  }

2. Run flatpak uninstall --user -y app.drey.Dialect && flatpak install --user flathub app/com.github.gi_lom.dialect/x86_64/stable and when the second command asks you if you want to rebase, answer n
3. Run flatpak update --user and watch as it fails to install the rebased app, and happily uninstalls the old app

You’ll be left with com.github.gi_lom.dialect.Locale installed and nothing else from the new or old versions of Dialect.

It’s not possible to work around this in the operation_error() handler in FlatpakCliTransaction, because FlatpakTransaction doesn’t impose any order on the rebase and uninstall ops, meaning that it happens that the uninstall op seems to always happen first.

---

I agree with @mwleeds that the fix here is to add a new flatpak_transaction_add_rebase_and_uninstall() API which combines the two existing add_rebase() and add_uninstall() calls, adds an ordering dependency between their ops, and sets op->fail_if_op_fails between them too.

I’ll look into implementing that now.

[ramcq]

This always struck me as an odd API anyway; basically a signal that requires you to copy/paste code rather than like... return TRUE if you want to "do the thing".

[pwithnall]

This always struck me as an odd API anyway; basically a signal that requires you to copy/paste code rather than like... return TRUE if you want to "do the thing".

Yeah, the end-of-lifed and end-of-lifed-with-rebase signals do seem a bit odd. I decided to go with adding a new flatpak_transaction_add_rebase_and_uninstall() though because the existing flatpak_transaction_add_rebase() method is impossible to use correctly (because it’s not possible to link it to an uninstall op), so it would have to be replaced/deprecated at some point anyway.

The changes in #5332 don’t preclude some other changes in future to rearrange how FlatpakTransaction::end-of-lifed{,-with-rebase} work. It’s not something I’m digging into now though, since I haven’t entirely read up on the history and all the intended uses of those signals.

[pwithnall]

I’ll look into implementing that now.

Fix available as #5332. It adds a new flatpak_transaction_add_rebase_and_uninstall() method and doesn’t touch the FlatpakTransaction::end-of-lifed-with-rebase signal.