Naomi icon indicating copy to clipboard operation
Naomi copied to clipboard

Refactor bash scripts

Open rileyhawk1417 opened this issue 2 years ago • 13 comments

Refactoring Bash Scripts

Refactoring the bash install scripts to avoid repetition of commonly used code.

Description

Did some refactoring of the commonly used functions/variables and moved them to one file. So to avoid writing the same commands in different files.

Related Issue

This PR is linked to issue #372

Motivation and Context

I feel it's better to avoid repeating the same thing over and over if we can just reference what we need

How Has This Been Tested?

This has been tested with Distrobox Containers(currently still in testing)

Screenshots (if appropriate):

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [ ] My code follows the code style of this project.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [ ] I have added tests to cover my changes.
  • [ ] All new and existing tests passed.

rileyhawk1417 avatar Feb 11 '23 19:02 rileyhawk1417

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 11 '23 19:02 CLAassistant

Thanks @rileyhawk1417. I'm not sure why there are so many commits from two years ago included, or why there are conflicts with the current version of the files. I'm working on the install process also right now, so I'll download, merge and test your edits when I have a chance, sometime this week.

aaronchantrill avatar Feb 23 '23 16:02 aaronchantrill

Not sure why the commits are pulling back old dates. Unless am creating a new branch wrong. I usually do git checkout -b new_branch As for the install files still had some cleaning to do, but I can put it on hold if it's something that might conflict with what you are doing

rileyhawk1417 avatar Feb 23 '23 20:02 rileyhawk1417

@rileyhawk1417 no, that sounds like the right way to create a new branch. Sometimes git can be strange. I don't think anything you do will conflict with what I'm doing. I'm just trying to figure out the best combination of packages for installing Naomi with the minimum amount of compiling.

aaronchantrill avatar Feb 24 '23 02:02 aaronchantrill

Okay, I understand what you are doing with moving functions into files that can be sourced instead of requiring that the whole setup be copied and edited each time a new version of the installer is added. I like what you've done.

I did notice that you have both the original "python_requirements.txt" and a new "requirements.txt" file with pypi packages. You added pocketsphinx and phonetisaurus to the original "python-requirements.txt" file, and "requirements.txt" appears to be a copy of "python-requirements.txt" from before these changes. I know that "requirements.txt" is a more traditional name for a list of Pipy packages, so I wasn't sure if you were thinking of changing it. Anyway, I don't think we need it.

Instead of installing the Pypi "pocketsphinx" package, we currently build the pocketsphinx python package at the same time as pocketsphinx. In the new way of installing pocketsphinx that I am working on, we would be installing the apt "python3-pocketsphinx" and "python3-sphinxbase" packages along with "pocketsphinx", "sphinxtrain" and "sphinxbase". I do not actually see any pocketsphinx packages in the official Arch packages or in the AUR, so it may be that the best option for Arch is to continue to build pocketsphinx from source.

I'll work on resolving the conflicts in the next couple of days.

aaronchantrill avatar Feb 28 '23 02:02 aaronchantrill

Here is an issue that we have had for a while. Using custom colors with the install scripts but not checking or controlling the background. This is a terminal on a new Debian Bullseye installation using Mate desktop: image The background of the terminal window is white by default, making the writing illegible. I think we need to just keep returning to the default text color "\033[0m" instead of setting the color to white.

aaronchantrill avatar Feb 28 '23 16:02 aaronchantrill

@rileyhawk1417 The naomi_apt_requirements.sh script is missing. Is that something you are moving into a function? I am testing and had to restore it on my copy because the script.deb.sh still requires it. I'll make a pull request later. I'm just not sure what your goal is.

aaronchantrill avatar Feb 28 '23 16:02 aaronchantrill

Was fully working on the Arch side of things, was going to sort out the apt script. The goal was to have one file that checks if it has apt or pacman package manager. Then install the requirements from the relevant txt files

rileyhawk1417 avatar Feb 28 '23 16:02 rileyhawk1417

Also, can you change sklearn to scikit-learn and remove pocketsphinx from your modified version of python-requirements.txt?

That will allow you to address issue sklearn breaks installer #371 at the same time.

Thanks!

aaronchantrill avatar Feb 28 '23 18:02 aaronchantrill

@rileyhawk1417 Ah, I see that that the functionality from apt_requirements.sh has moved into naomi_requirements.sh which is a better way of doing it. Unfortunately the script.deb.sh still appears to reference the original file. I'll wait until you are done with your edits before testing in debian again. Thank you.

aaronchantrill avatar Mar 01 '23 02:03 aaronchantrill

Here is just a written list of how I would like to see the installer functionality go. The first script is one that could be downloaded and run independently (without downloading the whole Naomi repository). The second script would be downloaded by the first (if it doesn't exist yet) and then run. For this reason, the first script should be used for collecting information but not access any include files, and the second script could access include files but should not collect information, if that makes sense.

Here are my thoughts:

  1. if Naomi has been installed, (~/.config/naomi exists) present the following menu:

Welcome to the Naomi Installer. Pick one of the options below to get started:

'Uninstall': This will remove Naomi from your system.

'Update': This will update Naomi if there is a newer release for your installed version.

'Version': This will allow you to switch what version of Naomi you have installed.

'AutoUpdate': This will allow you to enable/disable Naomi auto updates.

'Quit'

Otherwise, if Naomi has not been installed, present this menu:

Welcome to the Naomi Installer. Pick one of the options below to get started:

'Install': This will fresh install & setup Naomi on your system.

'Quit'

There should probably also be a “reinstall” option. Both reinstall and update should run through the entire install process from the downloaded install script file.

  1. Ask which version the user wants to install. The versions are:
Version    Branch       Auto Update
Nightly     Naomi-dev Yes
Milestone   Naomi-dev No
Stable      master       No

If the ~/Naomi/installers directory exists already, add an option to use the currently downloaded version. If they want to keep the current version, I think they should be set to “milestone” settings in the .naomi_options.json file.

  1. If the user chooses “Install” and either does not have Naomi downloaded or has chosen not to use the currently downloaded version, back up the current ~/Naomi folder if there is one, then git clone the proper version.

  2. Check if the user wants to start virtualenvwrapper automatically.

  3. Check if the user wants to be prompted each time the installer uses sudo or not.

  4. Pass control to the os script (script.apt.sh, script.pacman.sh, etc.)

  5. Install system repository packages (apt, yum, zypper, pacman, etc)

  6. Download and build any software that needs to be compiled (this should only be for core functionality; plugins should handle this in the plugin’s init method).

  7. Install virtualenvwrapper and create a Naomi virtual environment.

  8. Activate the virtualenvwrapper and update the .bashrc script to start virtualenvwrapper automatically if requested above.

  9. Install Pypi pip packages.

  10. Print a “Success” message asking the user to run Naomi with the --repopulate flag.

aaronchantrill avatar Apr 15 '23 19:04 aaronchantrill

I concur with this. It does not change the way we currently do things too much but rather fixes some oversights and implements some minor adjustments.

AustinCasteel avatar Apr 15 '23 20:04 AustinCasteel

I agree @AustinCasteel it does plug some holes that we might have missed, but at the same time improves a few places where the installer can be handled better.

rileyhawk1417 avatar Apr 16 '23 10:04 rileyhawk1417