-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
Refactor bash scripts #370
base: naomi-dev
Are you sure you want to change the base?
Refactor bash scripts #370
Conversation
Bumping Master
bumping master
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. |
Not sure why the commits are pulling back old dates. Unless am creating a new branch wrong. |
@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. |
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. |
@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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now there are some other functionality missing when I get to the actual --repopulate part of things, so I'm curious what version you started with. This is one of the reasons I don't ever let Naomi autoupdate, so that I'm always developing on a clean copy of the current state on git. This brings me back to the problem of the extra Austin commits that got pulled in. This usually happens when I make a pull request and then start developing my next submission without fetching from Github first. When you merge a pull request, Github likes to flatten all the commits in that pull request into just one commit representing the whole pull request. This is a lot easier to read than all the commits a person has done while developing a feature, especially since Git recommends that you commit often. It does lead to confusing situations, though. It looks like you may have started from a version with new commits by Austin before the pull request was merged.
Could you back up your current development environment, clone a fresh copy of the naomi-dev branch of Naomi, then copy the changed files into it? I tried doing this and it makes it much clearer what has changed. I think we can also merge your changes even with the duplicate commits from Austin and they will all basically just merge down, but we will need to deal with the conflicts. I think if you re-implement the changes in a clean branch, we won't have to worry about those.
This probably will require submitting a new pull request. I'm not sure if you can copy the changes into a fresh clone, then create a new "refactor_bash_scripts" branch, commit your changes, and push to your account and just have it update correctly.
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! |
@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. |
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:
Welcome to the Naomi Installer. Pick one of the options below to get started: 'Uninstall': 'Update': 'Version': 'AutoUpdate': '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': '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.
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.
|
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. |
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. |
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
Checklist: