Add retry button, subterminal, summary window to manage daemon

[Botspot]

This is part of a larger improvement to the install/uninstall/update experience that will be ushered in with the "contributor update" https://github.com/Botspot/pi-apps/pull/2652 PR. This PR should probably be merged first.

This adds a new Retry button as an alternative to "Send report". Hopefully users will see and use it in the event of internet issues, or when instructed to fix their system in some way, which should have these effects:

• Moderately reduce our volume of received error reports.
• Help users keep apps installed long-term, rather than abandoning the app in an uninstalled state if an update ever fails once.
• As a result, this should improve the UX quite a bit for those with unstable internet connections.

Asking for @theofficialgman to look over a couple specific changes which I think are fine, but could be problematic:

1. My change to will_reinstall which now allows returning code 0 even if the app is uninstalled. I made this change so that updated apps, successfully uninstalling but failing to install, would actually be installed rather than refreshed if the user clicks Retry. I found one usage of will_reinstall which may have new bad behavior with this change, so I replaced it with directly checking an app's installation status. This fix itself may also cause further regressions. I'm not sure.
2. My choice to avoid running a manage subprocess to update each app. (See the "#manage can handle this action, but avoid spawning subprocess" comment) I vaguely remember trying to implement this once before, and there was some reason why running the "update" action was safer through a manage subprocess, but I cannot find that now nor do I see how this could cause issues.

[theofficialgman]

The overall idea of retrying an application seems ok. I'll review the contents of this probably next weekend.

Shellcheck is failing due to the automatic change of ubuntu-latest moving from 22.04 to 24.04. I have already reported QEMU errors in newer QEMU that is used in 24.04 a few months back with no fix. We need to downgrade that runner back to 22.04.

[theofficialgman]

Oh I almost forgot to mention, github actions ARM64 Linux runners are now free for public repos as of earlier this week. I need to do some refactoring to move our CI actions over from the QEMU based runners to the native ARM64 runners.

I have already reported QEMU errors in newer QEMU that is used in 24.04 a few months back with no fix.

Actually this part is not the cause of the shellcheck errors. Shellcheck is running natively... not sure why it is hanging. But the change from 22.04 to 24.04 is causing shellcheck to fail so I will revert that action to 22.04.

[theofficialgman]

Can't reproduce the issue on local Ubuntu Noble 24.04 shellcheck. Seems to only occur on Github Actions Ubuntu Noble 24.04. I have reported the issue here: https://github.com/actions/runner/issues/3667

[theofficialgman]

Switching shellcheck from operating on all files at once to a single file at a time reduced github actions shellcheck runtime from ~3 minutes down to ~25 seconds while also eliminating the chance of hangs observed locally https://github.com/Botspot/pi-apps/commit/1fa28fe781404d24e0b224a86716c58950db116f

[Botspot]

New commits change from where the terminal is launched. Now it launches from manage daemon instead of from terminal_manage_multi. This paves the path for a good summary window implementation as seen in #2652, and also finally un-breaks the bookworm experience.

Old behavior: New behavior:

[Botspot]

New commits more or less render #2652 obsolete. It adds a summary window made with yad, which I think is more future-proof than 112 lines of python. An additional row could be added here named Contribute/Help us/Get Involved, linking to a pi-apps.io page explaining as you had in your PR.

For this I want to make a dedicated webpage for "new contributors" to link to that goes over the things we discussed in the discord, attempting to welcome users into the "family" of pi-apps contributors.

In addition to the two earlier specifics I want reviewed, @theofficialgman I would like your feedback on how I have the donation messages set up. Do you want your username/pfp/caption included in the way I have added it? Would you prefer your name not be there at all?

[Botspot]

1. My change to will_reinstall which now allows returning code 0 even if the app is uninstalled. I made this change so that updated apps, successfully uninstalling but failing to install, would actually be installed rather than refreshed if the user clicks Retry. I found one usage of will_reinstall which may have new bad behavior with this change, so I replaced it with directly checking an app's installation status. This fix itself may also cause further regressions. I'm not sure.

Watching some apps install for the first time, that should have only been refreshed validated my concerns here. I've reverted some of those changes, returning will_reinstall to always return 1 if the app is not installed, and changed update_app to always reinstall and not check will_reinstall, keeping the new update retry feature intact. Then for normal updates, update_app is now never longer called at all unless the app will_reinstall.

Before this, depending on what code was handling the updates, update_app or refresh_app may handle the app refreshes. Now it will always be refresh_app, as determined by will_reinstall, so update_app does not need to check will_reinstall.

[Botspot]

Okay @theofficialgman some things have changed since my original comment. If you are only looking at this now, ignore previous comments. I only have these two things that I am requesting for you to look over:

1. My choice to avoid running a manage subprocess to update each app. (See the "#manage can handle this action, but avoid spawning subprocess" comment in the code) I vaguely remember trying to implement this once before, and there was some reason why running the "update" action was safer through a manage subprocess, but I cannot find that now nor do I see how this could cause issues.
2. The way your donate message is presented. I need to know if you are fine with it, want to change it, or want it removed.

As you can see on https://github.com/sponsors/botspot, my current real-life situation is fairly precarious, so I would like for this PR to be merged soon.

[theofficialgman]

2. The way your donate message is presented. I need to know if you are fine with it, want to change it, or want it removed.

I see nothing wrong with the way it is presented. It might be worth adding an additional row linking to all our contributors https://github.com/Botspot/pi-apps/graphs/contributors to be fair. If they have github sponsorship enabled (and some of them do) you can over over their icon and see a "sponsor" button on that page.

As you can see on https://github.com/sponsors/botspot, my current real-life situation is fairly precarious, so I would like for this PR to be merged soon.

Noted. I'll DM you shortly (since its too personal to discuss in public imho). I don't intend on blocking your donation changes but given the nature of the other changes wrapped up in this PR I really need to do a thorough read through and regression test on our supported systems before this can be merged and I can't do that till the weekend (Saturday).

[theofficialgman]

Just an FYI, I am still working through the code in my head. Mainly making sure that everything works as expected during the transition from the existing form of the scripts to this new form. I haven't actually run/tested anything new.

[theofficialgman]

1. My choice to avoid running a manage subprocess to update each app. (See the "#manage can handle this action, but avoid spawning subprocess" comment in the code)
   I vaguely remember trying to implement this once before, and there was some reason why running the "update" action was safer through a manage subprocess, but I cannot find that now nor do I see how this could cause issues.

There is a reason and the visual change here is a symptom of the modifications affecting runtime speed https://github.com/Botspot/pi-apps/pull/2708#issuecomment-2601293397 . The previous way allowed terminal-run to start as soon as possible with minimal overhead. That is why originally the terminal popped up before the manage yad window (which is why the terminal is behind the yad window). The changes you made to move the creation of the terminal-run instance much later in the process delayed the creation of the window to the point that yad started first (it is still a race condition because the yad window is a background task but it has a better chance of starting first since it was executed slightly before the terminal-run now).

The problem with this is terminal-run is slow to start on some systems (because their chosen terminal takes a long time to open) and the daemon pid won't be established until the terminal-run instance has started.

What you have done is actually break the pid check system entirely. Previously, the PID of the first terminal-run instance which had the daemon running was written to data/manage-daemon/pid and it is what other manage daemon calls would check to decide if they should become the daemon or only send their commands to the existing daemon. With your changes, now the updater script or gui script PID (depending on what called terminal_manage_multi) gets written to data/manage-daemon/pid, not the terminal-run instance that is actually running the daemon.

[theofficialgman]

The PID check system prevents users from launching multiple pi-apps instances and creating multiple manage daemons. There is always only one daemon. It also allows users to start updates from the onboot updates and then open pi-apps and continue to add/remove applications all using the same daemon. With the PID system broken, all that won't work anymore.

[Botspot]

Previously the entirety of the manage daemon ran in the terminal. I now want manage daemon to run outside of a terminal and only complete actions in the terminal, hence why I call it a "subterminal". This allows for:

• better window ordering
• for the terminal to be dismissed during the summary phase
• no annoying 30-second countdown

I will investigate the PID issue, but the subterminal changes I want to stay.

[Botspot]

@theofficialgman I am confused by your statements that the PID was broken in any way. There is a terminal pid that is part of terminal-run to wait for the terminal to close, but that did not change the manage script's behavior here:

  #write my own PID to the pid file
  echo $$ > "${DIRECTORY}/data/manage-daemon/pid"

That is line 624

I just don't see how this code path was affected in the way you describe.

[theofficialgman]

@theofficialgman I am confused by your statements that the PID was broken in any way. There is a terminal pid that is part of terminal-run to wait for the terminal to close, but that did not change the manage script's behavior here:

  #write my own PID to the pid file
  echo $$ > "${DIRECTORY}/data/manage-daemon/pid"

That is line 624

I just don't see how this code path was affected in the way you describe.

the PID is set before the manage daemon is even started now. Note that the file is written (line 624) and 30 lines later (line 643) the manage daemon is started. The PID that gets set as I said will end up being the PID of the updater or gui script not the daemon from inside terminal-run like it should be and was before when the entire manage script ran inside terminal-run.

[Botspot]

[Botspot]

How exactly could $$ be the PID of gui or updater script?

[theofficialgman]

deamon pid isn't the daemon PID anymore, as I said you are running this script directly from the updater/gui, so it still has the updater/gui PID. the PID that gets written needs to be done from your new manage_daemon_terminal_code function from within the terminal-run PID, not the updater/gui script.

[Botspot]

deamon pid isn't the daemon PID anymore, as I said you are running this script directly from the updater/gui, so it still has the updater/gui PID.

That would be true if we are sourcing the manage script and running a function that contains $$. But that is not the case. "${DIRECTORY}/data/manage-daemon/pid" will contain the PID of the manage script, which in a few seconds will run the rest of the daemon code in the terminal.

So code runs like this:

gui
  > manage
  > sets daemon pid (pid of manage script)
  > starts terminal
    > manage daemon runs
    > when it finishes, it removes the pid file
  > now this outer manage PID can finish, because of how terminal-run waits for the terminal to close before it exits

the PID that gets written needs to be done from your new manage_daemon_terminal_code function from within the terminal-run PID, not the updater/gui script.

Bad idea. I thought about doing that, but we want the PID to be written as early as possible to avoid race conditions or duplicate processes.

[theofficialgman]

do you see the problem now?

[theofficialgman]

as you can see in the above, the manage-daemon PID is not written to the PID file but instead the manage script PID run from the updater script. That is not what was done before and is wrong for the reasons stated here https://github.com/Botspot/pi-apps/pull/2708#issuecomment-2614035813

[Botspot]

Let's hop on discord vc

[theofficialgman]

There is one problem I have encountered when attempting to install applications after the update has been installed but pi-apps hasn't been restarted. Looking into what code path is responsible now. Luckily it's just a visual issue because once the installs a done the 30 second timeout appears on the terminal in the back.

[theofficialgman]

Right.... the gui calls terminal_manage (which wraps to terminal_manage_multi) which is still the old version, hence the old window still pops up until pi-apps is restarted. Nothing we can do about that with our existing architecture unless we move back to the old method.

[theofficialgman]

@Botspot given the above, do you have any issues with merging? I am fine to merge as is. If we want to prevent scenarios like this from occurring in the future we need to re-source the api anytime it changes from when the gui was first loaded (aka: we would have to version it and check the version).

[theofficialgman]

also when you merge, I suggest squashing it.