transaction: Add new flatpak_transaction_add_rebase_and_uninstall() API

[pwithnall]

This mostly replaces flatpak_transaction_add_rebase(). It’s necessary
because the uninstall op for an eol-rebased app needs to be linked to
the install/update op for the rebased app, otherwise one op can proceed
after the other has failed (or they can be run in the wrong order) and
result in the old app being uninstalled but the new one not installed.

The following commit will port the internal flatpak FlatpakTransaction
subclasses to use it. Other consumers of FlatpakTransaction (such as
gnome-software) will have to be ported as well.

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

Fixes: #3991

[pwithnall]

I’ve tested this using the reproducer instructions in #3991 and it fixes the issue.

With the hack applied to cause the installation of the rebased app to fail with FLATPAK_ERROR_OUT_OF_SPACE (for testing), the flatpak CLI correctly does the following when this PR is applied:

$ flatpak update --user com.github.gi_lom.dialect
Looking for updates…

Info: app com.github.gi_lom.dialect branch stable is end-of-life, in favor of app.drey.Dialect branch stable
Replace? [Y/n]: y

app.drey.Dialect permissions:
    ipc     network     fallback-x11     pulseaudio     wayland     x11     dri



        ID                                       Branch         Op         Remote          Download
 1. [✓] app.drey.Dialect.Locale                  stable         i          flathub           4.7 kB / 227.6 kB
 2. [✗] app.drey.Dialect                         stable         i          flathub         < 1.5 MB
 3. [⍻] com.github.gi_lom.dialect                stable         r
 4. [⍻] com.github.gi_lom.dialect.Locale         stable         r

Error: Not enough disk space to complete this operation
Info: com.github.gi_lom.dialect was skipped
Info: com.github.gi_lom.dialect.Locale was skipped
Changes complete.
error: There were one or more errors
$ flatpak list --user
Name                                   Application ID                   Version      Branch      Origin
Dialect translations                   app.drey.Dialect.Locale                       stable      flathub
Dialect                                com.github.gi_lom.dialect        1.4.1        stable      flathub

Because the rebase op fails, the uninstall op for com.github.gi_lom.dialect{,.Locale} is correctly skipped.

[pwithnall]

If this is accepted and lands, gnome-software’s flatpak plugin will need to be updated to use it.

[dbnicholson]

This looks pretty good to me, but I think it should be possible to add a test for the reproducer. I didn't run this, but I think it should work:

diff --git a/tests/test-repo.sh b/tests/test-repo.sh
index 55403259..c9f0996f 100644
--- a/tests/test-repo.sh
+++ b/tests/test-repo.sh
@@ -439,6 +439,26 @@ ${FLATPAK} build-commit-from --no-update-summary --end-of-life-rebase=org.test.H
 GPGARGS="${FL_GPGARGS}" $(dirname $0)/make-test-app.sh repos/test-rebase org.test.NewHello master "${REBASE_COLLECTION_ID}" "NEW" > /dev/null
 update_repo test-rebase
 
+# Temporarily set the minimum free space to 1 to make the install of the
+# rebased app fail.
+orig_min_free_space=$(ostree --repo=$FL_DIR/repo config get core.min-free-space-size || :)
+ostree --repo=$FL_DIR/repo config set core.min-free-space-size 1
+
+if ${FLATPAK} ${U} update -y org.test.Hello >&2; then
+    assert_not_reached "flatpak update should fail with insufficient free space"
+fi
+
+# Check that the old app is still installed and the new app is not installed.
+assert_has_dir $FL_DIR/app/org.test.Hello/$ARCH/master/active/files
+assert_not_has_dir $FL_DIR/app/org.test.NewHello/$ARCH/master/active/files
+
+# Restore the original minimum free space.
+if [ -n "$orig_min_free_space" ]; then
+    ostree --repo=$FL_DIR/repo config set core.min-free-space-size "$orig_min_free_space"
+else
+    ostree --repo=$FL_DIR/repo config unset core.min-free-space-size
+fi
+
 ${FLATPAK} ${U} update -y org.test.Hello >&2
 
 # Make sure we got the new version installed

[pwithnall]

I think it should be possible to add a test for the reproducer. I didn't run this, but I think it should work:

You were right. I had to change 1 to 999TB to get the sense right. I also spent waaaaaaaay too long with the update succeeding despite the min-free-size being set, before I realised that the previous step in the test was a --no-deploy update which would have just pulled all the files, so no more pulls were happening to hit the min-free-size checks, and hence the rebase update was not failing.

But now the test works and it fails if I revert the atomicity fixes, so that’s all good.

Since the test is 99% your code, I’ve put your name on the commit to add it. No changes made to the other commits.

[dbnicholson]

You were right. I had to change 1 to 999TB to get the sense right. I also spent waaaaaaaay too long with the update succeeding despite the min-free-size being set, before I realised that the previous step in the test was a --no-deploy update which would have just pulled all the files, so no more pulls were happening to hit the min-free-size checks, and hence the rebase update was not failing.

🙄 double whoops. I even wrote the previous --no-deploy part.

👍 from me. I think the code makes sense and the test proves it works correctly.

[smcv]

I haven't reviewed the implementation in detail: I'm more familiar with the sandboxing part of Flatpak than the "package management" layer. This could benefit from review from @mwleeds or @alexlarsson when finished.

[pwithnall]

This could benefit from review from @mwleeds or @alexlarsson when finished.

Just in case it’s not clear: as far as I’m concerned, this is finished and ready to be merged whenever the right people have reviewed it.

[pwithnall]

This could benefit from review from @mwleeds or @alexlarsson when finished.

Actually, are either of them actively reviewing things at the moment? If not, this could block indefinitely, which would be a bit of a shame.

Is the combination of your review and Dan’s review not sufficient?

[alexlarsson]

This looks fine to me. And yes, I'm currently very lame at following flatpak stuff. If I'm not seeing something important, please seek me out and I can help.