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

Implemented TerminalCommand and a update mechanism. #369

Merged
merged 8 commits into from
Feb 28, 2019

Conversation

LexiconCode
Copy link
Member

@LexiconCode LexiconCode commented Jan 11, 2019

This pull request implements the following

  • TerminalCommand in terminal.py - runs trusted and untrusted(ConfirmAction for safety) CMD/terminal commands suppressing the terminal window.
  • DragonflyCheck - checks if dragonfly is up-to-date.
  • CasterCheck - checks if Caster is up-to-date.
  • InternetCheck - checks Casters network connection via cloudfire DNS
  • online_mode in settings.py - Toggles features that require a network connection

ToDo

A big thanks to @Danesprite for contributing code towards TerminalCommand and implementing RunCommand in Dragonfly that makes this Pull request possible.

@LexiconCode LexiconCode added the New Feature A new feature that is not currently implemented. label Jan 11, 2019
@LexiconCode LexiconCode self-assigned this Jan 11, 2019
@LexiconCode LexiconCode changed the title Implemented TerminalCommand and DependencyCheck Implemented TerminalCommand and a update mechanism. Jan 30, 2019
@LexiconCode LexiconCode added the Complete Pull request is complete and tested as defined by Contributor label Feb 1, 2019
@LexiconCode LexiconCode force-pushed the dependency_check branch 2 times, most recently from 552570d to e697351 Compare February 7, 2019 03:35
LexiconCode added 5 commits February 12, 2019 09:08
+ TerminalCommand in terminal.py - runs trusted and untrusted(ConfirmAction for safety) CMD/terminal commands
+ DependencyCheck - checks of dragonfly's up-to-date.
+ internetcheck - checks Casters network connection via cloudfire DNS
+ online_mode in settings.py - toggles features that require a network connection

ToDo
+ Document Features
@drmfinlay
Copy link
Member

I've done some testing with this. At first the pip commands didn't work properly for me because 'pip' was using Python 3.5 instead. They worked correctly after I fixed that by adjusting the Windows PATH variable.

Unfortunately if the pip commands fail, Dragon will still be restarted. Perhaps the "update X" commands should use a TerminalCommand sub-class which reboots dragon with Playback([(["reboot", "dragon"], 0.0)]) only if the subprocess exited successfully with a return code of 0?

I tried doing that and discovered a bug in Caster's TerminalCommand class: the synchronous parameter is ignored if not specified at the class level. So if the Playback action is executed in process_command(), it will run on a separate thread and natlink will raise an error.

I can fix these things in this PR if you like :-)

I reckon dragonfly's RunCommand action should probably log an error if the subprocess exits with a non-zero return code. I'll update it to do that.

@LexiconCode
Copy link
Member Author

LexiconCode commented Feb 16, 2019

Thank you for testing. I wondered how it would work with side by side versions of python depending on the path. I'll see if I can figure out some sort of clever way to handle that. It will involve checking the python version number and name that uses for execution (py, py2, python and so on). I agree that it should only return code of 0 for now. Oh I could have swore I thought I fixed that issue by adding the synchronous parameter on the class level defaulting to false.

Edit: This is what I thought fix the issue but apparently not. :/
def __init__(self, trusted=False, command=None, synchronous=False, process_command=None):

I appreciate that, feel free to make any changes to the pull request. I wish GitHub made that easier for co-contributors.

@LexiconCode
Copy link
Member Author

LexiconCode commented Feb 16, 2019

For now we could put this in the settings.py.

    # python settings
    "python": {
        "automatic_settings": True,  # Set to false to manually set "version" and "pip" below.
        "version": "python",  # Depending Python setup (python, python2, python2.7, py, py -2)
        "pip": "pip"  # Depending on PIP setup (pip, pip2, pip2.7)
    },

I'll see if I can come up of the function that illiterate through until a valid setting is found for 2.7. unless "automatic_settings" is set to false.

This way we don't have to have specific setup instructions for python which can vary based on default path, Python install method, and 2.X and 3.X congruent. Ideally I wouldn't want somebody to have to change there Python set up especially if they're already a Python developer. Usually a lot of other software that have settings that need to be adjusted as well in that case.

@drmfinlay
Copy link
Member

I think instead of having a setting for it, the absolute path to 'Python27\scripts\pip.exe' could be retrieved from os.environ["PATH"]. That way the correct script can be executed directly. I think we can rely on the PATH variable containing the 'Python27\scripts' directory and log an error otherwise. In the future, sys.version_info can be checked to call the correct script depending on the Python version used.

Yep, that allows the synchronous parameter to be specified for TerminalCommand, but the parameter wasn't passed to the superclass's constructor with:

RunCommand.__init__(self, command, process_command, synchronous)

I can add the PATH checking too if you want. I noticed that RunCommand doesn't actually support Windows/non-POSIX paths properly. I can release a patch version to fix that, it's a simple fix.

@LexiconCode
Copy link
Member Author

Sounds like a plan :)

- Pass missing arguments to RunCommand.__init__ from the
  TerminalCommand class.
- Use the pip.exe script for Python 2.7 directly by parsing the
  Windows PATH variable, falling back on pip.exe if the file
  isn't in the usual place.
- Stop Caster's update commands from restarting if the commands
  fail.
- Update minimum required dragonfly2 version to 0.11.1.
@drmfinlay
Copy link
Member

I've added the changes I mentioned. Sorry it took me a while.

@LexiconCode LexiconCode merged commit 7f6a43b into dictation-toolbox:develop Feb 28, 2019
@LexiconCode LexiconCode deleted the dependency_check branch March 1, 2019 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Pull request is complete and tested as defined by Contributor New Feature A new feature that is not currently implemented.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants