Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change manage daemon to allow retrying failed actions #2708

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Botspot
Copy link
Owner

@Botspot Botspot commented Jan 19, 2025

This is part of a larger improvement to the install/uninstall/update experience that will be ushered in with the "contributor update" #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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

theofficialgman commented Jan 20, 2025

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: actions/runner#3667

@theofficialgman
Copy link
Collaborator

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 1fa28fe

This has bothered me ever since bookworm dropped. Now terminal appears
after queueviewer window, thus, on top of it. No more half occluded
terminal. This also prepares for a summary window from "Contributor
update" to appear after the terminal closes.
@Botspot
Copy link
Owner Author

Botspot commented Jan 20, 2025

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:
20250119_22h01m09s_grim
New behavior:
20250119_22h01m08s_grim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants