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

Update install.sh, deletes miniconda installer if interrupted #37

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pato-pan
Copy link

Potential errors could be avoided and resources can be saved if the miniconda installer is deleted before downloading it again.

This is in the case the script is interrupted before the download is completed.

I thought that it would be better if it instead checked if it was already there, just so it doesn't have to download it again, but that would cause issues in case the existing file is incomplete or corrupted

@deffcolony
Copy link
Collaborator

@pato-pan hi could you review the script again? currently there is a branch conflicts since major updates has been made these months.

currently the installer will get cleaned up after it has been installed:

rm /tmp/miniconda_installer.sh

do you want me to add the cleanup above:

wget "https://repo.anaconda.com/miniconda/$miniconda_installer" -O /tmp/miniconda_installer.sh
aswell?

@deffcolony deffcolony added 🚫 Merge Conflicts [PR] Submitted code needs rebasing 🚏 Awaiting User Response [ISSUE] Response from original author is pending labels Mar 29, 2024
@pato-pan
Copy link
Author

pato-pan commented Apr 4, 2024

@pato-pan hi could you review the script again? currently there is a branch conflicts since major updates has been made these months.

currently the installer will get cleaned up after it has been installed:

rm /tmp/miniconda_installer.sh

do you want me to add the cleanup above:

wget "https://repo.anaconda.com/miniconda/$miniconda_installer" -O /tmp/miniconda_installer.sh

aswell?

yes, it should be run before making the download unless you find a way to resume the download, fix it, reuse it if completed, or force it to overwrite the existing file.

Updated for new commits

added the -f (force) option. Helps whenever there's corruption or permissions issue.

wget will now resume existing downloads. Untested. Questions.
If the file is corrupted or different from what's expected, will it overwrite it?
If the file already exists and is completed, will it skip it?


Untested behavior in the case the file can't be resumed, I don't know if the rm -f is needed now. It might be a good idea to not delete it at all in case the install fails at a later part of the process. The user can resume the install without having to redownload the file if you don't delete it. Files in /tmp/ get removed by themselves after shutting down the device anyways.
@pato-pan pato-pan reopened this Apr 4, 2024
@pato-pan
Copy link
Author

pato-pan commented Apr 4, 2024

I am too tired to this test right now. It might be worth considering to not remove the file unless the install was successful, or to not delete the file at all and let the machine delete it (as it usually does after you restart)

I looked through the wget help page, and found it can resume downloads with -c.

I also added the -f option, because in my experience when something doesn't go as planned, the file is corrupted or you get a permissions issue. The -f or --force fixes that (in any other case, a file system check, fschk, but that's beyond the scope of this script).

For now, I uncommented the removals until I tested this and answered my questions on wget's behavior. I should add a commit to remove the miniconda installer only when the SillyTavern install is completed. You may want to consider a dedicated section or function to clean up install files at the end of the install if there's anything else. This will make it so the user spends less time since they can simply resume with the files they already have downloaded if something went wrong.

edit:

deleting the file was a good idea because it couldn't be used again(invalid now) and it was there just wasting space. Duplicates would be created every time the installar was run (invalid now). It's still valid that it wastes space, but it no longer stacks up

the tradeoff makes this waste worth it. Which is a better user experience by providing a faster reinstall when a failure occurs. Currently it only makes sense to remove it if the script is sure the install went well, but even then that's unnecessary and the major motivation would be cleanliness.

@deffcolony deffcolony added 🧑‍💻 In Progress [ISSUE] Assigned to an issue that is curretly being worked on and removed 🚫 Merge Conflicts [PR] Submitted code needs rebasing 🚏 Awaiting User Response [ISSUE] Response from original author is pending labels Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧑‍💻 In Progress [ISSUE] Assigned to an issue that is curretly being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants