Merge pull request #481 from VoiceLessQ/fix/flatpak-bundle-reinstall

fix(update): handle a bundle Flatpak before 1.16.2 won't reinstall

crates/update/src/install.rs

@@ -119,7 +119,8 @@ pub fn install_at(
         }
         #[cfg(target_os = "linux")]
         PackageKind::Deb | PackageKind::Rpm | PackageKind::ArchPkg | PackageKind::Flatpak => {
-            linux::install_package(kind, &file, job.stop, &mut *job.phase).map(|_| None)
+            let id = app.bundle_id();
+            linux::install_package(kind, &file, Some(&id), job.stop, &mut *job.phase).map(|_| None)
         }
         #[cfg(target_os = "linux")]
         PackageKind::AppImage => {
@@ -770,9 +771,11 @@ fn cancelled(stop: &Stop) -> impl Fn() -> bool + '_ {
 pub(crate) mod linux {
     use std::ffi::{OsStr, OsString};
     use std::os::unix::fs::PermissionsExt;
+    use std::time::Duration;
 
     use crate::distro;
     use crate::protocol::CANCELLED;
+    use crate::version::{self, Version};
 
     use super::{
         App, INSTALLER, InstallSource, Installed, JobPhase, PackageKind, Path, PathBuf, Paths, Record, SHARED_CALL, SYSTEM_BIN_DIRS, Stop, cancelled,
@@ -792,9 +795,10 @@ pub(crate) mod linux {
     pub struct Root<'a> {
         pub queue: &'a crate::queue::Queue,
         pub run: &'a (dyn Fn(&[&OsStr]) -> Result<(), String> + Sync),
+        pub flatpak: &'a (dyn Fn() -> std::process::Command + Sync),
     }
 
-    const SYSTEM: Root<'static> = Root { queue: &INSTALLER, run: &run_as_root };
+    const SYSTEM: Root<'static> = Root { queue: &INSTALLER, run: &run_as_root, flatpak: &process::flatpak_command_c };
 
     /// Runs `command target` as root. Jobs waiting for the same command go into one call
     /// (`apt-get install a.deb b.deb`), so the password is asked for once.
@@ -962,19 +966,48 @@ pub(crate) mod linux {
         }
     }
 
-    /// Reinstalls a downloaded `.flatpak` bundle. `system` replaces the system-wide copy through
-    /// `pkexec`. Per-user stays a normal `flatpak install --user`, which needs no password.
-    pub(crate) fn install_flatpak_bundle(file: &Path, system: bool, stop: &Stop, phase: &mut dyn FnMut(JobPhase)) -> Result<(), String> {
-        install_flatpak_bundle_via(&SYSTEM, file, system, stop, phase)
+    /// Puts a self-update's `.flatpak` bundle in place. `system` replaces the system-wide copy
+    /// through `pkexec`. Per-user stays a normal `flatpak install --user`, which needs no password.
+    pub(crate) fn update_flatpak_bundle(file: &Path, id: &str, system: bool, stop: &Stop, phase: &mut dyn FnMut(JobPhase)) -> Result<(), String> {
+        update_flatpak_bundle_via(&SYSTEM, file, id, system, stop, phase)
     }
 
-    fn install_flatpak_bundle_via(root: &Root<'_>, file: &Path, system: bool, stop: &Stop, phase: &mut dyn FnMut(JobPhase)) -> Result<(), String> {
+    /// An app never uninstalls itself here: its download goes after this, so a failed install would
+    /// leave nothing. "Already installed" means an earlier update landed and never restarted.
+    pub(crate) fn update_flatpak_bundle_via(
+        root: &Root<'_>,
+        file: &Path,
+        id: &str,
+        system: bool,
+        stop: &Stop,
+        phase: &mut dyn FnMut(JobPhase),
+    ) -> Result<(), String> {
+        match install_flatpak_bundle_via(root, file, None, system, stop, phase) {
+            Err(e) if already_installed(&e, id) => {
+                log::info!("{id} is already installed: only the restart is left");
+                Ok(())
+            }
+            result => result,
+        }
+    }
+
+    /// Installs a downloaded `.flatpak` bundle. `bundle` is the app id it holds, for a per-user install
+    /// that may uninstall that app first.
+    fn install_flatpak_bundle_via(
+        root: &Root<'_>,
+        file: &Path,
+        bundle: Option<&str>,
+        system: bool,
+        stop: &Stop,
+        phase: &mut dyn FnMut(JobPhase),
+    ) -> Result<(), String> {
         let install_one = || {
             log::info!("executing flatpak install --reinstall --bundle for {}", file.display());
             let mut cmd = if system {
                 system_bundle_command(file)?
             } else {
-                let mut cmd = process::flatpak_command();
+                // The C locale keeps the "already installed" error below readable.
+                let mut cmd = (root.flatpak)();
                 cmd.args(flatpak_bundle_flags(false));
                 cmd.arg(file);
                 cmd
@@ -985,27 +1018,72 @@ pub(crate) mod linux {
             }
             flatpak_bundle_error(&output)
         };
-        root.queue.alone(&cancelled(stop), &mut turn(phase, JobPhase::Installing), install_one).unwrap_or(Err(CANCELLED.into()))
+        // Inside the queue: an uninstall racing another bundle install can drop its remote.
+        let install = || {
+            let result = install_one();
+            match bundle {
+                Some(id) if result.as_ref().is_err_and(|e| already_installed(e, id)) && reinstall_by_hand(root) => {
+                    log::info!("{id} is already installed and this Flatpak can't reinstall a bundle: uninstalling it first");
+                    // No `--delete-data`: the app's files stay for the next install.
+                    if !(root.flatpak)().args(["uninstall", "--user", "-y", "--noninteractive", id]).status().is_ok_and(|s| s.success()) {
+                        log::warn!("couldn't uninstall {id} to install its bundle again");
+                        return result;
+                    }
+                    install_one()
+                        .map_err(|e| format!("{e} (It was uninstalled first. Its data is kept, and Install tries again with the same download.)"))
+                }
+                _ => result,
+            }
+        };
+        root.queue.alone(&cancelled(stop), &mut turn(phase, JobPhase::Installing), install).unwrap_or(Err(CANCELLED.into()))
     }
 
-    /// Installs (or upgrades) a downloaded package with the system's own tools.
-    pub fn install_package(kind: PackageKind, file: &Path, stop: &Stop, phase: &mut dyn FnMut(JobPhase)) -> Result<(), String> {
+    /// Whether a bundle install of `id` failed because the installation already has the bundle's
+    /// commit: Flatpak's "already installed", in the C locale. Any other failure leaves the app alone.
+    pub(crate) fn already_installed(err: &str, id: &str) -> bool {
+        err.contains(&format!("{id} already installed"))
+    }
+
+    /// Whether the user's copy has to be uninstalled before its bundle installs again: Flatpak
+    /// before 1.16.2 ignores `--reinstall` for a bundle.
+    fn reinstall_by_hand(root: &Root<'_>) -> bool {
+        let mut cmd = (root.flatpak)();
+        cmd.arg("--version");
+        process::run_command(cmd, "flatpak", Duration::from_secs(10)).is_ok_and(|v| ignores_bundle_reinstall(&v))
+    }
+
+    /// Whether `flatpak --version` names a Flatpak that ignores `--reinstall` for a bundle (flatpak
+    /// commit 4c5693b8 added it in 1.16.2).
+    pub(crate) fn ignores_bundle_reinstall(flatpak_version: &str) -> bool {
+        flatpak_version.split_whitespace().last().and_then(version::parse).is_some_and(|v| v < Version::new(1, 16, 2))
+    }
+
+    /// Installs (or upgrades) a downloaded package with the system's own tools. `bundle` is the app
+    /// id a Flatpak bundle holds.
+    pub fn install_package(kind: PackageKind, file: &Path, bundle: Option<&str>, stop: &Stop, phase: &mut dyn FnMut(JobPhase)) -> Result<(), String> {
         if kind == PackageKind::Deb && system_tool("apt-get").is_none() {
             return Err("this system has no apt-get to install a Debian package with".into());
         }
         if kind == PackageKind::ArchPkg && system_tool("pacman").is_none() {
             return Err("this system has no pacman to install an Arch package with".into());
         }
-        install_package_via(&SYSTEM, kind, file, stop, phase)
+        install_package_via(&SYSTEM, kind, file, bundle, stop, phase)
     }
 
-    pub fn install_package_via(root: &Root<'_>, kind: PackageKind, file: &Path, stop: &Stop, phase: &mut dyn FnMut(JobPhase)) -> Result<(), String> {
+    pub fn install_package_via(
+        root: &Root<'_>,
+        kind: PackageKind,
+        file: &Path,
+        bundle: Option<&str>,
+        stop: &Stop,
+        phase: &mut dyn FnMut(JobPhase),
+    ) -> Result<(), String> {
         let f = file.as_os_str();
         if let Some(command) = install_command(kind) {
             return as_root(root, command, f, stop, phase, JobPhase::Installing);
         }
         match kind {
-            PackageKind::Flatpak => install_flatpak_bundle_via(root, file, false, stop, phase),
+            PackageKind::Flatpak => install_flatpak_bundle_via(root, file, bundle, false, stop, phase),
             other => Err(format!("{} isn't a system package", other.label())),
         }
     }
@@ -2482,7 +2560,7 @@ mod tests {
             ran.lock().unwrap().push(args.iter().map(|a| a.to_string_lossy().into_owned()).collect::<Vec<_>>());
             root_outcome(Some(code))
         };
-        let root = linux::Root { queue: &queue, run: &run };
+        let root = linux::Root { queue: &queue, run: &run, flatpak: &process::flatpak_command_c };
         let (started, has_started) = mpsc::channel();
         let (release, released) = mpsc::channel::<()>();
         let (waiting, waits) = mpsc::channel();
@@ -2503,7 +2581,7 @@ mod tests {
                             waiting.send(()).unwrap();
                         }
                     };
-                    linux::install_package_via(root, PackageKind::Deb, Path::new(file), &Stop::default(), &mut phase)
+                    linux::install_package_via(root, PackageKind::Deb, Path::new(file), None, &Stop::default(), &mut phase)
                 })
             });
             waits.recv().unwrap();
@@ -2545,13 +2623,151 @@ mod tests {
             return;
         }
         let queue = crate::queue::Queue::new();
-        let root = linux::Root { queue: &queue, run: &|_| Ok(()) };
-        let result = linux::install_package_via(&root, PackageKind::Flatpak, Path::new("/nonexistent/file.flatpak"), &Stop::default(), &mut |_| ());
+        let root = linux::Root { queue: &queue, run: &|_| Ok(()), flatpak: &process::flatpak_command_c };
+        let result =
+            linux::install_package_via(&root, PackageKind::Flatpak, Path::new("/nonexistent/file.flatpak"), None, &Stop::default(), &mut |_| ());
         assert!(result.is_err());
         let err = result.unwrap_err();
         assert!(err.starts_with("Flatpak couldn't install the bundle: "), "unexpected error: {err}");
     }
 
+    #[cfg(target_os = "linux")]
+    const FLATPAK_ID: &str = "ai.storyteller.soundcraft";
+    #[cfg(target_os = "linux")]
+    const ALREADY_THERE: &str = "ai.storyteller.soundcraft already installed";
+    #[cfg(target_os = "linux")]
+    const INSTALL_CALL: &str = "install --user -y --noninteractive --reinstall --bundle /x.flatpak";
+
+    /// Runs `install` against a stand-in `flatpak` that logs its calls and fails its first `failures`
+    /// installs with `why`. Returns the result and the calls.
+    #[cfg(target_os = "linux")]
+    fn with_fake_flatpak(
+        flatpak_version: &str,
+        failures: u32,
+        why: &str,
+        uninstall_exit: u32,
+        install: impl FnOnce(&linux::Root<'_>) -> Result<(), String>,
+    ) -> (Result<(), String>, Vec<String>) {
+        let dir = tempfile::tempdir().unwrap();
+        let (script, calls) = (dir.path().join("flatpak"), dir.path().join("calls"));
+        let log = calls.display();
+        std::fs::write(
+            &script,
+            format!(
+                r#"echo "$*" >> "{log}"
+case "$1" in
+--version) echo "Flatpak {flatpak_version}" ;;
+uninstall) exit {uninstall_exit} ;;
+install)
+    [ "$(grep -c '^install' "{log}")" -gt {failures} ] && exit 0
+    echo "error: Failed to install bundle: {why}" >&2
+    exit 1 ;;
+esac
+"#
+            ),
+        )
+        .unwrap();
+        let flatpak = || {
+            let mut cmd = std::process::Command::new("/bin/sh");
+            cmd.arg(&script);
+            cmd
+        };
+        let queue = crate::queue::Queue::new();
+        let root = linux::Root { queue: &queue, run: &|_| Ok(()), flatpak: &flatpak };
+        let result = install(&root);
+        (result, std::fs::read_to_string(calls).unwrap().lines().map(str::to_string).collect())
+    }
+
+    #[cfg(target_os = "linux")]
+    fn install_bundle(root: &linux::Root<'_>, bundle: Option<&str>) -> Result<(), String> {
+        linux::install_package_via(root, PackageKind::Flatpak, Path::new("/x.flatpak"), bundle, &Stop::default(), &mut |_| {})
+    }
+
+    #[cfg(target_os = "linux")]
+    fn update_bundle(root: &linux::Root<'_>, system: bool) -> Result<(), String> {
+        linux::update_flatpak_bundle_via(root, Path::new("/x.flatpak"), FLATPAK_ID, system, &Stop::default(), &mut |_| {})
+    }
+
+    #[cfg(target_os = "linux")]
+    #[test]
+    fn a_bundle_goes_in_again_by_hand_only_when_flatpak_needs_it() {
+        let uninstall = format!("uninstall --user -y --noninteractive {FLATPAK_ID}");
+        let refused = format!("Flatpak couldn't install the bundle: Failed to install bundle: {ALREADY_THERE}");
+        let id = Some(FLATPAK_ID);
+
+        // An old Flatpak: the data stays, because nothing else goes on the uninstall's command line.
+        let (result, calls) = with_fake_flatpak("1.14.6", 1, ALREADY_THERE, 0, |root| install_bundle(root, id));
+        assert_eq!(result, Ok(()));
+        assert_eq!(calls, [INSTALL_CALL, "--version", &uninstall, INSTALL_CALL]);
+
+        // A first install that works, or a call with no bundle id, asks nothing more.
+        assert_eq!(with_fake_flatpak("1.14.6", 0, ALREADY_THERE, 0, |root| install_bundle(root, id)), (Ok(()), vec![INSTALL_CALL.to_string()]));
+        let (result, calls) = with_fake_flatpak("1.14.6", 1, ALREADY_THERE, 0, |root| install_bundle(root, None));
+        assert_eq!((result, calls), (Err(refused.clone()), vec![INSTALL_CALL.to_string()]));
+
+        // A Flatpak that can reinstall a bundle meant the refusal, so the installed copy stays.
+        let (result, calls) = with_fake_flatpak("1.16.2", 1, ALREADY_THERE, 0, |root| install_bundle(root, id));
+        assert_eq!(result, Err(refused.clone()));
+        assert_eq!(calls, [INSTALL_CALL, "--version"]);
+
+        // Any other failure leaves it alone too.
+        let (result, calls) = with_fake_flatpak("1.14.6", 1, "Invalid checksum", 0, |root| install_bundle(root, id));
+        assert_eq!(result, Err("Flatpak couldn't install the bundle: Failed to install bundle: Invalid checksum".to_string()));
+        assert_eq!(calls, [INSTALL_CALL]);
+
+        // A failed uninstall reports the first error, and the app is still there.
+        let (result, calls) = with_fake_flatpak("1.14.6", 1, ALREADY_THERE, 1, |root| install_bundle(root, id));
+        assert_eq!(result, Err(refused.clone()));
+        assert_eq!(calls, [INSTALL_CALL, "--version", &uninstall]);
+
+        // A failure after the uninstall says what happened and what to do next.
+        let (result, calls) = with_fake_flatpak("1.14.6", 2, ALREADY_THERE, 0, |root| install_bundle(root, id));
+        let error = result.unwrap_err();
+        assert!(error.starts_with(&refused) && error.contains("uninstalled first") && error.contains("Install tries again"), "{error}");
+        assert_eq!(calls, [INSTALL_CALL, "--version", &uninstall, INSTALL_CALL]);
+    }
+
+    #[cfg(target_os = "linux")]
+    #[test]
+    fn a_self_update_never_uninstalls_the_app_it_runs_from() {
+        // The update landed earlier and never restarted, so only the restart is left.
+        let (result, calls) = with_fake_flatpak("1.14.6", 1, ALREADY_THERE, 0, |root| update_bundle(root, false));
+        assert_eq!((result, calls), (Ok(()), vec![INSTALL_CALL.to_string()]));
+
+        let (result, calls) = with_fake_flatpak("1.14.6", 0, ALREADY_THERE, 0, |root| update_bundle(root, false));
+        assert_eq!((result, calls), (Ok(()), vec![INSTALL_CALL.to_string()]));
+
+        // Anything else is still a failure: a broken bundle, or one holding another app.
+        for why in ["Invalid checksum", "ai.storyteller.photocraft already installed"] {
+            let (result, calls) = with_fake_flatpak("1.14.6", 1, why, 0, |root| update_bundle(root, false));
+            assert_eq!(result, Err(format!("Flatpak couldn't install the bundle: Failed to install bundle: {why}")));
+            assert_eq!(calls, [INSTALL_CALL]);
+        }
+    }
+
+    #[cfg(target_os = "linux")]
+    #[test]
+    fn only_flatpaks_already_installed_error_counts_as_installed() {
+        let id = FLATPAK_ID;
+        let failed = |why: &str| format!("Flatpak couldn't install the bundle: Failed to install bundle {id}: {why}");
+        assert!(linux::already_installed(&failed(ALREADY_THERE), id));
+        // A bundle Flatpak can't use, or one for another app, keeps the installed copy.
+        assert!(!linux::already_installed(&failed("Can't change remote during bundle install"), id));
+        assert!(!linux::already_installed(&failed("Invalid checksum of length 0 expected 32"), id));
+        assert!(!linux::already_installed(&failed("ai.storyteller.photocraft already installed"), id));
+    }
+
+    #[cfg(target_os = "linux")]
+    #[test]
+    fn only_a_flatpak_before_1_16_2_needs_a_bundle_reinstalled_by_hand() {
+        assert!(linux::ignores_bundle_reinstall("Flatpak 1.14.6\n"));
+        assert!(linux::ignores_bundle_reinstall("Flatpak 1.16.1"));
+        assert!(!linux::ignores_bundle_reinstall("Flatpak 1.16.2"));
+        assert!(!linux::ignores_bundle_reinstall("Flatpak 1.18.4"));
+        // An unreadable version leaves the failed install as it is.
+        assert!(!linux::ignores_bundle_reinstall(""));
+    }
+
     #[cfg(target_os = "linux")]
     #[test]
     fn adds_and_removes_only_its_own_menu_entry_and_icon() {

crates/update/src/process.rs

@@ -142,6 +142,19 @@ pub fn flatpak_command() -> Command {
     }
 }
 
+/// [`flatpak_command`] in the C locale, for output this code reads: Flatpak translates its messages.
+pub fn flatpak_command_c() -> Command {
+    if in_sandbox() {
+        let mut cmd = command("flatpak-spawn");
+        cmd.args(["--host", "--env=LC_ALL=C", "flatpak"]);
+        cmd
+    } else {
+        let mut cmd = command("flatpak");
+        cmd.env("LC_ALL", "C");
+        cmd
+    }
+}
+
 /// Runs a flatpak command and captures its standard output.
 pub fn capture_flatpak(args: &[&str], timeout: Duration) -> Option<String> {
     if in_sandbox() {
@@ -405,6 +418,15 @@ mod tests {
 mod run_tests {
     use super::*;
 
+    #[test]
+    fn flatpak_output_this_code_reads_stays_in_english() {
+        if in_sandbox() {
+            return;
+        }
+        let cmd = flatpak_command_c();
+        assert!(cmd.get_envs().any(|(name, value)| name == "LC_ALL" && value == Some(std::ffi::OsStr::new("C"))));
+    }
+
     /// A shell command line, for the platform's shell.
     fn shell(script: &str) -> (&'static str, [&str; 2]) {
         if cfg!(windows) { ("cmd", ["/C", script]) } else { ("/bin/sh", ["-c", script]) }

crates/update/src/self_update.rs

@@ -385,7 +385,7 @@ pub fn update(
             }
             #[cfg(target_os = "linux")]
             Install::Package(kind, exe) => {
-                let result = install::linux::install_package(*kind, &file, stop, phase);
+                let result = install::linux::install_package(*kind, &file, None, stop, phase);
                 let _ = std::fs::remove_file(&file);
                 result?;
                 relaunch_after_exit(app, pid, &[exe.as_os_str()])
@@ -399,7 +399,7 @@ pub fn update(
                     // would install a second one beside it.
                     None => return Err(format!("this copy of {} can't update itself; download the new version instead", app.name)),
                 };
-                let result = install::linux::install_flatpak_bundle(&file, system, stop, phase);
+                let result = install::linux::update_flatpak_bundle(&file, &app.bundle_id(), system, stop, phase);
                 let _ = std::fs::remove_file(&file);
                 result?;
                 relaunch_flatpak_after_exit(app)