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

add support for implementing pre- and post-step hooks #2343

Merged
merged 18 commits into from
Dec 12, 2017
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions easybuild/framework/easyblock.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,7 @@
from easybuild.tools.filetools import adjust_permissions, apply_patch, back_up_file, change_dir, convert_name
from easybuild.tools.filetools import compute_checksum, copy_file, derive_alt_pypi_url, diff_files, download_file
from easybuild.tools.filetools import encode_class_name, extract_file, is_alt_pypi_url, mkdir, move_logs, read_file
from easybuild.tools.filetools import remove_file, rmtree2, write_file
from easybuild.tools.filetools import verify_checksum, weld_paths
from easybuild.tools.filetools import remove_file, rmtree2, run_hook, write_file, verify_checksum, weld_paths
Copy link
Member

Choose a reason for hiding this comment

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

write_file at the end to preserve alphabetical order?

from easybuild.tools.run import run_cmd
from easybuild.tools.jenkins import write_to_xml
from easybuild.tools.module_generator import ModuleGeneratorLua, ModuleGeneratorTcl, module_generator, dependencies_for
Expand Down Expand Up @@ -136,7 +135,7 @@ def extra_options(extra=None):
#
# INIT
#
def __init__(self, ec):
def __init__(self, ec, hooks=None):
"""
Initialize the EasyBlock instance.
:param ec: a parsed easyconfig file (EasyConfig instance)
Expand All @@ -145,6 +144,9 @@ def __init__(self, ec):
# keep track of original working directory, so we can go back there
self.orig_workdir = os.getcwd()

# list of pre- and post-step hooks
self.hooks = hooks or []

# list of patch/source files, along with checksums
self.patches = []
self.src = []
Expand Down Expand Up @@ -2434,6 +2436,9 @@ def run_step(self, step, step_methods):
"""
self.log.info("Starting %s step", step)
self.update_config_template_run_step()

Copy link
Member

Choose a reason for hiding this comment

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

Calling run_hook when hooks = None would work, but seems odd.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather have the logic for dealing with not having any hooks defined in run_hook rather than having to copy-paste the logic to check whether any hooks are defined in multiple places.

Copy link
Member

Choose a reason for hiding this comment

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

Seems reasonable.

run_hook(step, self.hooks, pre_step_hook=True, args=[self])
Copy link
Member

Choose a reason for hiding this comment

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

I now understand why args is needed (in a previous comment on a separate commit I didn't :-P). Question is: Can we test it somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean, test that self is being passed as an argument?

That's already done in , see test_toy_build_hooks in toy_build.py where the post_install_hook there which uses self.name and self.version.

Copy link
Member

Choose a reason for hiding this comment

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

I mean that args seems like a kind of wildcard that I find trouble understanding. It is passed directly to the hook in all cases except the start and end hooks, where run_hook is executed without any of the optional arguments. And then the hook itself might do something with it, but being it either [] or [self] I don't find it very clear what is the purpose of it. I guess that now, with a slightly better understanding of what you are doing here, my comment is simply "can we rename args to something more descriptive?". With a different name I suppose I would understand it from the beginning.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, only the step hooks get passed an argument (i.e. self, the EasyBlock instance that gives access to all information).

This may change in the future though, if a reason pops up to pass additional arguments.

args is commonly used in Python code, see for example https://pythontips.com/2013/08/04/args-and-kwargs-in-python-explained/.

The idea is that the args list is passed down with *args to "unpack" it when the hook is called.

Hooks should be implemented like this, to be future-proof (i.e. keep working when we may pass down additional (even named) arguments):

def pre_install_hook(self, *args, **kwargs):
    # do something with self

def start_hook(*args, **kwargs):
    # do something (no (named) arguments passed, for now)

I'll clarify this in the yet-to-be-written documentation.


for step_method in step_methods:
self.log.info("Running method %s part of step %s" % ('_'.join(step_method.func_code.co_names), step))

Expand All @@ -2457,6 +2462,8 @@ def run_step(self, step, step_methods):
# and returns the actual method, so use () to execute it
step_method(self)()

run_hook(step, self.hooks, post_step_hook=True, args=[self])

if self.cfg['stop'] == step:
self.log.info("Stopping after %s step.", step)
raise StopException(step)
Expand Down Expand Up @@ -2601,7 +2608,7 @@ def print_dry_run_note(loc, silent=True):
dry_run_msg(msg, silent=silent)


def build_and_install_one(ecdict, init_env):
def build_and_install_one(ecdict, init_env, hooks=None):
"""
Build the software
:param ecdict: dictionary contaning parsed easyconfig + metadata
Expand Down Expand Up @@ -2639,7 +2646,7 @@ def build_and_install_one(ecdict, init_env):
try:
app_class = get_easyblock_class(easyblock, name=name)

app = app_class(ecdict['ec'])
app = app_class(ecdict['ec'], hooks=hooks)
_log.info("Obtained application instance of for %s (easyblock: %s)" % (name, easyblock))
except EasyBuildError, err:
print_error("Failed to get application instance for %s (easyblock: %s): %s" % (name, easyblock, err.msg),
Expand Down
26 changes: 20 additions & 6 deletions easybuild/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
from easybuild.framework.easyconfig.tweak import obtain_ec_for, tweak
from easybuild.tools.config import find_last_log, get_repository, get_repositorypath, build_option
from easybuild.tools.docs import list_software
from easybuild.tools.filetools import adjust_permissions, cleanup, write_file
from easybuild.tools.filetools import adjust_permissions, cleanup, load_hooks, run_hook, write_file
from easybuild.tools.github import check_github, find_easybuild_easyconfig, install_github_token
from easybuild.tools.github import new_pr, merge_pr, update_pr
from easybuild.tools.modules import modules_tool
Expand Down Expand Up @@ -104,18 +104,27 @@ def find_easyconfigs_by_specs(build_specs, robot_path, try_to_generate, testing=
return [(ec_file, generated)]


def build_and_install_software(ecs, init_session_state, exit_on_failure=True):
"""Build and install software for all provided parsed easyconfig files."""
def build_and_install_software(ecs, init_session_state, hooks=None, exit_on_failure=True):
"""
Build and install software for all provided parsed easyconfig files.

:param ecs: easyconfig files to install software with
:param init_session_state: initial session state, to use in test reports
:param hooks: list of defined pre- and post-step hooks
:param exit_on_failure: whether or not to exit on installation failure
"""
# obtain a copy of the starting environment so each build can start afresh
# we shouldn't use the environment from init_session_state, since relevant env vars might have been set since
# e.g. via easyconfig.handle_allowed_system_deps
init_env = copy.deepcopy(os.environ)

run_hook('start', hooks)
Copy link
Member

Choose a reason for hiding this comment

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

The labelling is at the moment "freeform". Nothing wrong with it, but I wonder if it makes sense to restrict it and create a list of valid labels? That way one can also inform the user in case of typos in the hooks list. Think of "why is my stat_hook not working?" kind of situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, good idea.

So, if someone includes stat_hook, they would get an error that stat is not a valid hook, and then spit out a list of known hooks?

Copy link
Member

Choose a reason for hiding this comment

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

Yep


res = []
for ec in ecs:
ec_res = {}
try:
(ec_res['success'], app_log, err) = build_and_install_one(ec, init_env)
(ec_res['success'], app_log, err) = build_and_install_one(ec, init_env, hooks=hooks)
ec_res['log_file'] = app_log
if not ec_res['success']:
ec_res['err'] = EasyBuildError(err)
Expand Down Expand Up @@ -154,6 +163,8 @@ def build_and_install_software(ecs, init_session_state, exit_on_failure=True):

res.append((ec, ec_res))

run_hook('end', hooks)

return res


Expand Down Expand Up @@ -453,9 +464,12 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None):
sys.exit(0)

# build software, will exit when errors occurs (except when testing)
exit_on_failure = not options.dump_test_report and not options.upload_test_report
if not testing or (testing and do_build):
ecs_with_res = build_and_install_software(ordered_ecs, init_session_state, exit_on_failure=exit_on_failure)
exit_on_failure = not options.dump_test_report and not options.upload_test_report
hooks = load_hooks(build_option('hooks'))

ecs_with_res = build_and_install_software(ordered_ecs, init_session_state, hooks=hooks,
exit_on_failure=exit_on_failure)
else:
ecs_with_res = [(ec, {}) for ec in ordered_ecs]

Expand Down
9 changes: 5 additions & 4 deletions easybuild/tools/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,7 @@ def mk_full_default_path(name, prefix=DEFAULT_PREFIX):
'force_download',
'from_pr',
'git_working_dirs_path',
'pr_branch_name',
'pr_target_account',
'pr_target_branch',
'pr_target_repo',
'hooks',
'github_user',
'github_org',
'group',
Expand All @@ -142,6 +139,10 @@ def mk_full_default_path(name, prefix=DEFAULT_PREFIX):
'optarch',
'package_tool_options',
'parallel',
'pr_branch_name',
'pr_target_account',
'pr_target_branch',
'pr_target_repo',
'rpath_filter',
'regtest_output_dir',
'skip',
Expand Down
79 changes: 79 additions & 0 deletions easybuild/tools/filetools.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import fileinput
import glob
import hashlib
import imp
Copy link
Member

Choose a reason for hiding this comment

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

No longer needed

import os
import re
import shutil
Expand Down Expand Up @@ -1651,3 +1652,81 @@ def diff_files(path1, path2):
file1_lines = ['%s\n' % l for l in read_file(path1).split('\n')]
file2_lines = ['%s\n' % l for l in read_file(path2).split('\n')]
return ''.join(difflib.unified_diff(file1_lines, file2_lines, fromfile=path1, tofile=path2))


def load_hooks(hooks_path):
"""Load defined hooks (if any)."""
hooks = []
if hooks_path:
(hooks_dir, hooks_filename) = os.path.split(hooks_path)
(hooks_mod_name, hooks_file_ext) = os.path.splitext(hooks_filename)
if hooks_file_ext == '.py':
_log.info("Importing hooks implementation from %s...", hooks_path)
(fh, pathname, descr) = imp.find_module(hooks_mod_name, [hooks_dir])
try:
# import module that defines hooks, and collect all functions of which name ends with '_hook'
imported_hooks = imp.load_module(hooks_mod_name, fh, pathname, descr)
for attr in dir(imported_hooks):
if attr.endswith('_hook'):
hook = getattr(imported_hooks, attr)
if callable(hook):
hooks.append(hook)
else:
_log.debug("Skipping non-callable attribute '%s' when loading hooks", attr)
_log.debug("Found hooks: %s", hooks)
except ImportError as err:
raise EasyBuildError("Failed to import hooks implementation from %s: %s", hooks_path, err)
else:
raise EasyBuildError("Provided path for hooks implementation should be location of a Python file (*.py)")
else:
_log.info("No location for hooks implementation provided, no hooks defined")

return hooks


def find_hook(label, known_hooks, pre_step_hook=False, post_step_hook=False):
"""
Find hook with specified label.

:param label: name of hook
:param known_hooks: list of known hooks
:param pre_step_hook: indicates whether hook to run is a pre-step hook
:param post_step_hook: indicates whether hook to run is a post-step hook
"""
res = None

Copy link
Member

Choose a reason for hiding this comment

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

The if below means that all hooks, except start and end need to have prefixes. Conceptually, I find it odd. If I create a install_hook I expect it to be executed after the install step. But it needs to be called post_install_hook. That way it is clear what it does, but I wonder if it makes sense to consider too install_hook.

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel install_hook is ambiguous. Some may assume it would be called as soon as the install step is triggered (equivalent with the current pre_install_hook), while your assumption is different.
To avoid guessing & confusion, I feel it's better to be explicit...

With your suggestion above in mind, you'd get an error like this when you define install_hook:

ERROR: 'install_hook' is not a valid hook; available hooks are: ..., pre_install_hook, post_install_hook, ...

Copy link
Member

Choose a reason for hiding this comment

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

I think that's a reasonable approach.

if pre_step_hook:
hook_prefix = 'pre_'
elif post_step_hook:
hook_prefix = 'post_'
else:
hook_prefix = ''

hook_name = hook_prefix + label + '_hook'

for hook in known_hooks:
if hook.__name__ == hook_name:
_log.info("Found %s hook", hook_name)
res = hook
break

return res


def run_hook(label, known_hooks, pre_step_hook=False, post_step_hook=False, args=None):
"""
Run hook with specified label.

:param label: name of hook
:param known_hooks: list of known hooks
:param pre_step_hook: indicates whether hook to run is a pre-step hook
:param post_step_hook: indicates whether hook to run is a post-step hook
:param args: arguments to pass to hook function
"""
hook = find_hook(label, known_hooks, pre_step_hook=pre_step_hook, post_step_hook=post_step_hook)
if hook:
if args is None:
args = []

_log.info("Running %s hook (arguments: %s)...", hook.__name__, args)
hook(*args)
7 changes: 6 additions & 1 deletion easybuild/tools/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@
from easybuild.tools.docs import avail_toolchain_opts, avail_easyconfig_params, avail_easyconfig_templates
from easybuild.tools.docs import list_easyblocks, list_toolchains
from easybuild.tools.environment import restore_env, unset_env_vars
from easybuild.tools.filetools import CHECKSUM_TYPE_SHA256, CHECKSUM_TYPES, mkdir
from easybuild.tools.filetools import CHECKSUM_TYPE_SHA256, CHECKSUM_TYPES, mkdir, resolve_path
from easybuild.tools.github import GITHUB_EB_MAIN, GITHUB_EASYCONFIGS_REPO, HAVE_GITHUB_API, HAVE_KEYRING
from easybuild.tools.github import fetch_github_token
from easybuild.tools.include import include_easyblocks, include_module_naming_schemes, include_toolchains
Expand Down Expand Up @@ -433,6 +433,7 @@ def config_options(self):
'buildpath': ("Temporary build path", None, 'store', mk_full_default_path('buildpath')),
'external-modules-metadata': ("List of files specifying metadata for external modules (INI format)",
'strlist', 'store', None),
'hooks': ("Location of Python module with hook implementations", 'str', 'store', None),
'ignore-dirs': ("Directory names to ignore when searching for files/dirs",
'strlist', 'store', ['.git', '.svn']),
'include-easyblocks': ("Location(s) of extra or customized easyblocks", 'strlist', 'store', []),
Expand Down Expand Up @@ -779,6 +780,10 @@ def postprocess(self):
# handle configuration options that affect other configuration options
self._postprocess_config()

# make sure absolute path is used for location of hooks
if self.options.hooks:
self.options.hooks = resolve_path(self.options.hooks)

# show current configuration and exit, if requested
if self.options.show_config or self.options.show_full_config:
self.show_config()
Expand Down
68 changes: 68 additions & 0 deletions test/framework/filetools.py
Original file line number Diff line number Diff line change
Expand Up @@ -1542,6 +1542,74 @@ def test_diff_files(self):
regex = re.compile('^--- .*/foo\s*\n\+\+\+ .*/bar\s*$', re.M)
self.assertTrue(regex.search(res), "Pattern '%s' found in: %s" % (regex.pattern, res))

def test_hooks(self):
"""Test for functions that support use of hooks."""
test_hooks_pymod = os.path.join(self.test_prefix, 'test_hooks.py')
test_hooks_pymod_txt = '\n'.join([
'def start_hook():',
' print("running start hook")',
'',
'def foo():',
' print("running foo helper method")',
'',
'def post_configure_hook(self):',
' print("running post-configure hook")',
' foo()',
'',
'def pre_install_hook(self):',
' print("running pre-install hook")',
])
ft.write_file(test_hooks_pymod, test_hooks_pymod_txt)

hooks = ft.load_hooks(test_hooks_pymod)
self.assertEqual(len(hooks), 3)
self.assertEqual(sorted(h.__name__ for h in hooks), ['post_configure_hook', 'pre_install_hook', 'start_hook'])
self.assertTrue(all(callable(h) for h in hooks))

post_configure_hook = [h for h in hooks if h.__name__ == 'post_configure_hook'][0]
pre_install_hook = [h for h in hooks if h.__name__ == 'pre_install_hook'][0]
start_hook = [h for h in hooks if h.__name__ == 'start_hook'][0]

self.assertEqual(ft.find_hook('configure', hooks), None)
self.assertEqual(ft.find_hook('configure', hooks, pre_step_hook=True), None)
self.assertEqual(ft.find_hook('configure', hooks, post_step_hook=True), post_configure_hook)

self.assertEqual(ft.find_hook('install', hooks), None)
self.assertEqual(ft.find_hook('install', hooks, pre_step_hook=True), pre_install_hook)
self.assertEqual(ft.find_hook('install', hooks, post_step_hook=True), None)

self.assertEqual(ft.find_hook('build', hooks), None)
self.assertEqual(ft.find_hook('build', hooks, pre_step_hook=True), None)
self.assertEqual(ft.find_hook('build', hooks, post_step_hook=True), None)

self.assertEqual(ft.find_hook('start', hooks), start_hook)
self.assertEqual(ft.find_hook('start', hooks, pre_step_hook=True), None)
self.assertEqual(ft.find_hook('start', hooks, post_step_hook=True), None)

self.mock_stdout(True)
self.mock_stderr(True)
ft.run_hook('start', hooks)
ft.run_hook('configure', hooks, pre_step_hook=True, args=[None])
ft.run_hook('configure', hooks, post_step_hook=True, args=[None])
ft.run_hook('build', hooks, pre_step_hook=True, args=[None])
ft.run_hook('build', hooks, post_step_hook=True, args=[None])
ft.run_hook('install', hooks, pre_step_hook=True, args=[None])
ft.run_hook('install', hooks, post_step_hook=True, args=[None])
stdout = self.get_stdout()
stderr = self.get_stderr()
self.mock_stdout(False)
self.mock_stderr(False)

expected_stdout = '\n'.join([
"running start hook",
"running post-configure hook",
"running foo helper method",
"running pre-install hook",
])

self.assertEqual(stdout.strip(), expected_stdout)
self.assertEqual(stderr, '')


def suite():
""" returns all the testcases in this module """
Expand Down
43 changes: 43 additions & 0 deletions test/framework/toy_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -1743,6 +1743,49 @@ def test_toy_build_trace(self):
regex = re.compile(pattern, re.M)
self.assertTrue(regex.search(stdout), "Pattern '%s' found in: %s" % (regex.pattern, stdout))

def test_toy_build_hooks(self):
"""Test use of --hooks."""
hooks_file = os.path.join(self.test_prefix, 'my_hooks.py')
hooks_file_txt = '\n'.join([
"import os",
'',
"def start_hook():",
" print('start hook triggered')",
'',
"def pre_configure_hook(self):",
" print('pre-configure: toy.source: %s' % os.path.exists('toy.source'))",
'',
"def post_configure_hook(self):",
" print('post-configure: toy.source: %s' % os.path.exists('toy.source'))",
'',
"def post_install_hook(self):",
" print('in post-install hook for %s v%s' % (self.name, self.version))",
" print(', '.join(sorted(os.listdir(self.installdir))))",
'',
"def end_hook():",
" print('end hook triggered, all done!')",
])
write_file(hooks_file, hooks_file_txt)

self.mock_stderr(True)
self.mock_stdout(True)
self.test_toy_build(extra_args=['--hooks=%s' % hooks_file])
stderr = self.get_stderr()
stdout = self.get_stdout()
self.mock_stderr(False)
self.mock_stdout(False)

self.assertEqual(stderr, '')
expected_output = '\n'.join([
"start hook triggered",
"pre-configure: toy.source: True",
"post-configure: toy.source: False",
"in post-install hook for toy v0.0",
"bin, lib",
"end hook triggered, all done!",
])
self.assertEqual(stdout.strip(), expected_output)


def suite():
""" return all the tests in this file """
Expand Down