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

Add ability to detect and remove AppImageLauncher #52

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Conversation

thw26
Copy link
Collaborator

@thw26 thw26 commented Jan 25, 2024

Partial fix for #5. This adds the ability to remove AppImage Launcher.

Add config.PMC_REMOVE; remove yum
Add config.BADPACKAGES
Add query_package() "mode" var, add remove_packages(), add check_bad_packages()

I've also made sure to add a major warning label to this remove command to its destructive capability.

One thing we may need to add is an exit function to the AppImageLauncher detection. If it is found, Logos won't be able to launch, and the install should fail at that point, otherwise we will wind up having support requests.

If the user removes it, the install should exit and instruct the user to reboot, or offer to reboot the system for the user but with warning.

@thw26 thw26 requested a review from n8marti January 25, 2024 04:46
@thw26 thw26 mentioned this pull request Jan 25, 2024
14 tasks
utils.py Outdated Show resolved Hide resolved
utils.py Outdated Show resolved Hide resolved
utils.py Outdated Show resolved Hide resolved
@n8marti
Copy link
Collaborator

n8marti commented Jan 27, 2024

One thing we may need to add is an exit function to the AppImageLauncher detection. If it is found, Logos won't be able to launch, and the install should fail at that point, otherwise we will wind up having support requests.

I think this would fit logically around the same time as (or somehow incuded in?) when the dependency checks are run.

@n8marti
Copy link
Collaborator

n8marti commented Jan 27, 2024

If the user removes it, the install should exit and instruct the user to reboot, or offer to reboot the system for the user but with warning.

I don't know how portable this is, but in debian-based systems if you want to require a reboot you can create the file /run/reboot-required, which will (eventually?) pop up a window saying "Your system needs to be restarted", and giving the Restart/Not now/Cancel options.

@thw26
Copy link
Collaborator Author

thw26 commented Jan 27, 2024

I think this would fit logically around the same time as (or somehow incuded in?) when the dependency checks are run.

Yes, that makes the most sense.

At present I don't actually remove the packages because it can be destructive if the system thinks it needs to remove some lower dependency. Should we require the user to remove manually or automate it, since this is designed for automated install? We could just present the user with a string they need to run (e.g., "Please run XXX in your terminal.", or direct them to their software repos.

Personally I think we should do automated removal, but with warnings.

Also, I will need to wrap install_dependencies and the new check_conflicting_packages into one new function, or find a better way to combine them. I'm leaning toward the latter, as it should mean less overhead, especially for any SteamOS users. I could then also minimize user interaction by only having one request to proceed.

@thw26
Copy link
Collaborator Author

thw26 commented Jan 27, 2024

I don't know how portable this is, but in debian-based systems if you want to require a reboot you can create the file /run/reboot-required, which will (eventually?) pop up a window saying "Your system needs to be restarted", and giving the Restart/Not now/Cancel options.

That would be nice to do. The CLI would be easy to do this with. The GUI could probably do it, too. We would just need to determine a user's init system (e.g., systemd or otherwise) and then hardcode the command.

Would something like this play into #19?

@n8marti
Copy link
Collaborator

n8marti commented Jan 27, 2024

I think this would fit logically around the same time as (or somehow incuded in?) when the dependency checks are run.

Yes, that makes the most sense.

At present I don't actually remove the packages because it can be destructive if the system thinks it needs to remove some lower dependency. Should we require the user to remove manually or automate it, since this is designed for automated install? We could just present the user with a string they need to run (e.g., "Please run XXX in your terminal.", or direct them to their software repos.

Personally I think we should do automated removal, but with warnings.

Also, I will need to wrap install_dependencies and the new check_conflicting_packages into one new function, or find a better way to combine them. I'm leaning toward the latter, as it should mean less overhead, especially for any SteamOS users. I could then also minimize user interaction by only having one request to proceed.

I think I'd be fine with either warning the user to uninstall AppImage Launcher before running LogosLinuxInstaller again, or auto-uninstalling it using LogosLinuxInstaller. Surely there aren't other apps that are depending on it, except that I'd want [someone] to test distros like Manjaro that have in the OS ISO. For example, I know that if you install ubuntu server, then later install ubuntu-desktop on top of it, ubuntu-desktop "depends" on all the default desktop apps. So if you later try to uninstall one of the ubuntu-desktop dependencies, then it will also want to uninstall ubuntu-desktop itself, which will have a huge cascading effect. If that unwelcome scenario is possible for AppImage Launcher on distros like Manjaro, I'd be a little worried about removing AppImage Launcher. But again, that doesn't seem too likely, right?

@thw26
Copy link
Collaborator Author

thw26 commented Jan 27, 2024

But again, that doesn't seem too likely, right?

My sentiments exactly.

@n8marti
Copy link
Collaborator

n8marti commented Jan 27, 2024

I don't know how portable this is, but in debian-based systems if you want to require a reboot you can create the file /run/reboot-required, which will (eventually?) pop up a window saying "Your system needs to be restarted", and giving the Restart/Not now/Cancel options.

That would be nice to do. The CLI would be easy to do this with. The GUI could probably do it, too. We would just need to determine a user's init system (e.g., systemd or otherwise) and then hardcode the command.

Would something like this play into #19?

Hm, to accomplish resumable install, I'd be tempted to make the install steps very explicit in the script, then have each step simply check to see whether whatever it's supposed to do has already been done or not. If so, it skips on to the next one; if not, is runs that step. That would replace the current strategy of just checking for the INSTALLDIR and refusing to continue if that folder exists.

@thw26
Copy link
Collaborator Author

thw26 commented Jan 27, 2024

Hm, to accomplish resumable install, I'd be tempted to make the install steps very explicit in the script, then have each step simply check to see whether whatever it's supposed to do has already been done or not. If so, it skips on to the next one; if not, is runs that step. That would replace the current strategy of just checking for the INSTALLDIR and refusing to continue if that folder exists.

That's more or less what I was thinking for how to do this.

I would think this would be necessary if we are going to require a reboot for AIL. Though preferably dependencies would be the very first step in running the software and bypass some of the need for "resuming" an install, if it never began in the first place.

@thw26
Copy link
Collaborator Author

thw26 commented Jan 27, 2024

Given the above, I'll update the code to combine the two dependency check functions. I will probably stub the reboot functionality and I might just have it exit with a message for the time being as a later PR.

Combine install procedures
Install libfuse regardless of whether we use AppImage or not
Add `config.BADPACKAGES`, `config.REBOOT_REQUIRED`, `config.PMC_REMOVE`; remove yum
Add `query_package()` "mode" var, add `remove_packages()`
Add `reboot()`
@n8marti
Copy link
Collaborator

n8marti commented Jan 31, 2024

I haven't had time to do any further testing, but I don't have any further code comments after this most recent commit.

@thw26 thw26 merged commit 413167e into main Jan 31, 2024
@thw26 thw26 deleted the issue5-part-2 branch February 16, 2024 07:20
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.

None yet

2 participants