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

Modules installer #174

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
149 changes: 149 additions & 0 deletions client/modinstaller.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
#!/usr/bin/env python2
import argparse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't argparse a 3rd party module? Not sure. If we're expecting people to use at least Python 2.7, it's not, but then we should remove it from requirements.txt.

Anyway, I'd rather put this import some lines below (i.e. between urllib and pip).

import os
import shutil
import subprocess
import tempfile
import urllib

import pip
import requests

import jasperpath


Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From PEP8

Separate top-level function and class definitions with two blank lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I installed pymode on vim, and it kept complaining about that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, Flake8 complains, too. @crm416 @shbhrsaha Will we adhere to PEP008 in this matter or do we stick with current coding style?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've generally tried to adhere to that (I use an auto-PEP8 formatter in Sublime, so it gets added automatically). In the future, yes, lets follow PEP8.

MODULES_URL = 'http://jaspermoduleshub.herokuapp.com/plugins/%s.json'


class ModuleInstallationError(Exception):
def __init__(self, message, module):
self.message = message
self.module = module

def __str__(self):
return "%s: %s" % (self.message, self.module)


def install(module, install_location, install_dependencies):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make install_dependencies default to False and install_location default to jasperpath.PLUGIN_PATH?

Or even better: Can you'add jasperpath.config('plugins') to get_modules in brain.py and then use jasperpath.config('plugins') as default here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are defaulted in the argparse options.

temp_path = _get_temp_module_folder(module)
try:
_check_installation_path(module, install_location)
_download_module_files(temp_path, module)
if install_dependencies:
_install_requirements(temp_path)
else:
_list_unmet_requirements(module, temp_path)
_copy_module_directory(temp_path, module, install_location)
finally:
_remove_temp_directory(temp_path)


def _module_url(module):
return MODULES_URL % urllib.quote(module)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really have to be wrapped in a custom function? I'd combine _module_exists and _get_module_metadata into one function that either returns metadata or None (alternative: raises an error that'll be catched by install).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: unneccessary newline


def _check_installation_path(module, install_location):
module_path = _get_module_folder(module, install_location)
if os.path.exists(module_path):
raise ModuleInstallationError('Module folder already exists', module)
if not os.access(install_location, os.W_OK):
raise ModuleInstallationError('Location is not writable', module)


def _get_module_folder(module, install_location):
return os.path.join(install_location, module)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really have to be wrapped in a custom function?



def _get_module_metadata(module):
try:
response = requests.get(_module_url(module))
response.raise_for_status()
except requests.exceptions.HTTPError:
raise ModuleInstallationError("The module could not be found", module)
else:
return response.json()


def _get_temp_module_folder(module):
return tempfile.mkdtemp(prefix=module)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really have to be wrapped in a custom function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to maintain a similar level of abstraction inside the install function. Therefore, that module calls the function that depict the inner working. You'll sometimes end up with functions that wrap only one statement, but it's a price you pay for the simplicity of the final code. Anyway, I don't think inlining this statement would entail any benefits, other than having one function less in the module.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I'd rather use a contextmanager.



def _copy_module_directory(tmp_path, module, install_location):
module_path = _get_module_folder(module, install_location)
shutil.copytree(tmp_path, module_path)


def _download_module_files(module_path, module):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to _download to make clear that this one is the general function that calls the two special ones (_download_git, _download_single_file)

metadata = _get_module_metadata(module)
file_type = metadata['last_version']['file_type']
if file_type == 'git':
_download_git(metadata, module_path)
elif file_type == 'file':
_download_single_file(metadata, module_path, module)
else:
raise ModuleInstallationError("Invalid file type", module)


def _download_git(metadata, module_path):
filename = metadata['last_version']['file']
subprocess.call(['git', 'clone', filename, module_path])


def _download_single_file(metadata, module_path, module):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you merge _download_single_file and _download_file? These don't need to be 2 separate functions IMHO.

_create_module_folder(module_path)
_download_file(metadata, module_path, module, '.py')


def _create_module_folder(module_path):
if not os.path.exists(module_path):
os.makedirs(module_path)
open(os.path.join(module_path, '__init__.py'), 'a').close()


def _download_file(metadata, module_path, module, extension):
filename = metadata['last_version']['file']
module_file = os.path.join(module_path, module + extension)
urllib.urlretrieve(filename, module_file)


def _install_requirements(module_path):
reqs_file = os.path.join(module_path, 'requirements.txt')
if os.path.isfile(reqs_file):
pip.main(['install', '-r', reqs_file])


def _list_unmet_requirements(module, module_path):
req_file = os.path.join(module_path, 'requirements.txt')
if os.access(req_file, os.R_OK):
missing_reqs = [req
for req in pip.req.parse_requirements(req_file)
if not req.check_if_exists()]
if missing_reqs:
print("PIP requirements missing for module %s:" % module)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I know I did the same, but somehow I think i'd be cooler if this function returned unmet dependencies as a list. Nevertheless, that probably can be postponed until v2.

for req in missing_reqs:
print(" - %s" % req.name)


def _remove_temp_directory(temp_path):
shutil.rmtree(temp_path)


if __name__ == '__main__':
parser = argparse.ArgumentParser(description='Jasper modules installer')
parser.add_argument('--install', nargs='+',
help='Modules to install')
parser.add_argument('--location', default=jasperpath.PLUGIN_PATH,
help="The location to install the modules to")
parser.add_argument('--install-dependencies', default=False,
help="Install required dependencies")

args = parser.parse_args()
for module in args.install:
try:
install(module,
install_location=args.location,
install_dependencies=args.install_dependencies)
except ModuleInstallationError as e:
print e
else:
print "Installed module: %s" % module