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

Copy build log and artifacts to a permanent location after failures #4601

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
59 changes: 49 additions & 10 deletions easybuild/framework/easyblock.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,13 @@
from easybuild.tools.config import FORCE_DOWNLOAD_ALL, FORCE_DOWNLOAD_PATCHES, FORCE_DOWNLOAD_SOURCES
from easybuild.tools.config import EASYBUILD_SOURCES_URL # noqa
from easybuild.tools.config import build_option, build_path, get_log_filename, get_repository, get_repositorypath
from easybuild.tools.config import install_path, log_path, package_path, source_paths
from easybuild.tools.config import install_path, log_path, package_path, source_paths, error_log_path
from easybuild.tools.environment import restore_env, sanitize_env
from easybuild.tools.filetools import CHECKSUM_TYPE_MD5, CHECKSUM_TYPE_SHA256
from easybuild.tools.filetools import adjust_permissions, apply_patch, back_up_file, change_dir, check_lock
from easybuild.tools.filetools import compute_checksum, convert_name, copy_file, create_lock, create_patch_info
from easybuild.tools.filetools import convert_name, copy_file, copy_dir, create_lock, create_patch_info, is_readable
from easybuild.tools.filetools import derive_alt_pypi_url, diff_files, dir_contains_files, download_file
from easybuild.tools.filetools import encode_class_name, extract_file
from easybuild.tools.filetools import encode_class_name, extract_file, compute_checksum
from easybuild.tools.filetools import find_backup_name_candidate, get_source_tarball_from_git, is_alt_pypi_url
from easybuild.tools.filetools import is_binary, is_sha256_checksum, mkdir, move_file, move_logs, read_file, remove_dir
from easybuild.tools.filetools import remove_file, remove_lock, verify_checksum, weld_paths, write_file, symlink
Expand Down Expand Up @@ -1045,16 +1045,23 @@ def moduleGenerator(self):
#
# DIRECTORY UTILITY FUNCTIONS
#
def gen_builddir(self):
"""Generate the (unique) name for the builddir"""
def get_relative_builddir_base_path(self):
"""Generate builddir base name relative to build_path"""
clean_name = remove_unwanted_chars(self.name)

# if a toolchain version starts with a -, remove the - so prevent a -- in the path name
tc = self.cfg['toolchain']
tcversion = tc['version'].lstrip('-')
lastdir = "%s%s-%s%s" % (self.cfg['versionprefix'], tc['name'], tcversion, self.cfg['versionsuffix'])

builddir = os.path.join(os.path.abspath(build_path()), clean_name, self.version, lastdir)
relative_builddir = os.path.join(clean_name, self.version, lastdir)

return relative_builddir

def gen_builddir(self):
"""Generate the (unique) name for the builddir"""
relative_builddir = self.get_relative_builddir_base_path()
builddir = os.path.join(os.path.abspath(build_path()), relative_builddir)

# make sure build dir is unique if cleanupoldbuild is False or not set
if not self.cfg.get('cleanupoldbuild', False):
Expand Down Expand Up @@ -4198,6 +4205,40 @@ def print_dry_run_note(loc, silent=True):
dry_run_msg(msg, silent=silent)


def persists_failed_compilation_log_and_artifacts(build_successful, application_log, silent, app, err_log_path):
def do_if_paths_distinct(operation, source, destination):
if not os.path.exists(source):
return
if os.path.realpath(source) == os.path.realpath(destination):
return
operation(source, destination)

if application_log:
# there may be multiple log files, or the file name may be different due to zipping
logs = glob.glob('%s*' % application_log)
print_msg(
"Results of the build can be found in the temporary log file(s) %s" % ', '.join(logs),
log=_log,
silent=silent
)

if err_log_path and not build_successful:
for log_file in logs:
target_file = os.path.join(err_log_path, os.path.basename(log_file))
do_if_paths_distinct(copy_file, log_file, target_file)

builddir = app.builddir
if is_readable(builddir):
build_artifact_log_path = os.path.join(err_log_path, app.get_relative_builddir_base_path())
do_if_paths_distinct(copy_dir, builddir, build_artifact_log_path)

print_msg(
"Build log and any output artifacts copied to permanent storage: %s" % err_log_path,
log=_log,
silent=silent
)


def build_and_install_one(ecdict, init_env):
"""
Build the software
Expand Down Expand Up @@ -4456,10 +4497,8 @@ def ensure_writable_log_dir(log_dir):
else:
dry_run_msg("(no ignored errors during dry run)\n", silent=silent)

if application_log:
# there may be multiple log files, or the file name may be different due to zipping
logs = glob.glob('%s*' % application_log)
print_msg("Results of the build can be found in the log file(s) %s" % ', '.join(logs), log=_log, silent=silent)
err_log_path = error_log_path(ec=ecdict['ec'])
persists_failed_compilation_log_and_artifacts(success, application_log, silent, app, err_log_path)

del app

Expand Down
29 changes: 29 additions & 0 deletions easybuild/tools/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
DEFAULT_PATH_SUBDIRS = {
'buildpath': 'build',
'containerpath': 'containers',
'errorlogpath': 'error_log',
'installpath': '',
'packagepath': 'packages',
'repositorypath': 'ebfiles_repo',
Expand Down Expand Up @@ -478,6 +479,7 @@ class ConfigurationVariables(BaseConfigurationVariables):
'buildpath',
'config',
'containerpath',
'errorlogpath',
'installpath',
'installpath_modules',
'installpath_software',
Expand Down Expand Up @@ -843,6 +845,33 @@ def log_path(ec=None):
return log_file_format(return_directory=True, ec=ec, date=date, timestamp=timestamp)


def error_log_path(ec=None):
"""
Return the default error log path

This is a path where file from the build_log_path can be stored permanently
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This is a path where file from the build_log_path can be stored permanently
This is a path where files from the build_log_path can be stored permanently

:param ec: dict-like value that provides values for %(name)s and %(version)s template values
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't match what is used. It expects name and version keys, not templates

Copy link
Author

Choose a reason for hiding this comment

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

I used the %(name)s and %(version)s template strings as default values. I have seen this use in other cases, log_file_format function on the same file (they indeed appear in file paths). It seems like a nice way to inform the user about a missing value without a hard failure.

Should we avoid using template names as default values or explain their use in a better way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. Isn't trivial to understand for me what is meant by that. I'd change the documentation to

:param ec: dict-like value with at least the keys 'name' and 'version'

I would not use a default and just fail hard if the keys do not exist because that is what is documented: The function expects a dict with those keys. If you don't provide one it is the fault of the caller. If we do it right in easybuild the keys will always exist, so no need for fallbacks. Or am I missing anything?

"""
error_log_path = ConfigurationVariables()['errorlogpath']
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a good idea to have a variable with the same name as a function. Maybe rename the function to get_.... or default_...?


if ec is None:
ec = {}

name, version = ec.get('name', '%(name)s'), ec.get('version', '%(version)s')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would you want %(name)s in the folder name?

date = time.strftime("%Y%m%d")
timestamp = time.strftime("%H%M%S")

base_path = '/'.join([error_log_path, name + '-' + version, date + '-' + timestamp])

path = base_path
inc_no = 1
while os.path.exists(path):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rather use filetools.create_unused_dir? This avoids duplicating the logic and a possible conflict when another eb instance runs in parallel

Copy link
Author

Choose a reason for hiding this comment

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

I need just the directory name to copy a directory to a new location, not to create a directory. The filetools.create_unused_dir couples closely the 2 functions, creating the directory name and creating the directory.

I will try to decouple the 2 functions and extract a function for creating a file name in a different commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

That "coupling" is intentional: If you don't create the directory you have a race condition for process running in parallel defeating the purpose of this function. Why don't you want that directory created? Can't you just fill it later using the created directory?

Copy link
Author

Choose a reason for hiding this comment

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

You couple through the file system, got it.

path = base_path + '_' + str(inc_no)
inc_no += 1

return path


def get_build_log_path():
"""
Return (temporary) directory for build log
Expand Down
8 changes: 5 additions & 3 deletions easybuild/tools/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,8 @@ def config_options(self):
'envvars-user-modules': ("List of environment variables that hold the base paths for which user-specific "
"modules will be installed relative to", 'strlist', 'store',
[DEFAULT_ENVVAR_USERS_MODULES]),
'errorlogpath': ("Location where logs and artifacts are copied in case of an error",
None, 'store', mk_full_default_path('errorlogpath')),
'external-modules-metadata': ("List of (glob patterns for) paths to files specifying metadata "
"for external modules (INI format)", 'strlist', 'store', None),
'hooks': ("Location of Python module with hook implementations", 'str', 'store', None),
Expand Down Expand Up @@ -1153,7 +1155,7 @@ def _postprocess_config(self):
# - the <path> could also specify the location of a *remote* (Git( repository,
# which can be done in variety of formats (git@<url>:<org>/<repo>), https://<url>, etc.)
# (see also https://github.com/easybuilders/easybuild-framework/issues/3892);
path_opt_names = ['buildpath', 'containerpath', 'git_working_dirs_path', 'installpath',
path_opt_names = ['buildpath', 'containerpath', 'errorlogpath', 'git_working_dirs_path', 'installpath',
'installpath_modules', 'installpath_software', 'prefix', 'packagepath',
'robot_paths', 'sourcepath']

Expand All @@ -1163,8 +1165,8 @@ def _postprocess_config(self):
if self.options.prefix is not None:
# prefix applies to all paths, and repository has to be reinitialised to take new repositorypath in account
# in the legacy-style configuration, repository is initialised in configuration file itself
path_opts = ['buildpath', 'containerpath', 'installpath', 'packagepath', 'repository', 'repositorypath',
'sourcepath']
path_opts = ['buildpath', 'containerpath', 'errorlogpath', 'installpath', 'packagepath', 'repository',
'repositorypath', 'sourcepath']
for dest in path_opts:
if not self.options._action_taken.get(dest, False):
if dest == 'repository':
Expand Down
2 changes: 2 additions & 0 deletions test/framework/docs.py
Original file line number Diff line number Diff line change
Expand Up @@ -724,6 +724,7 @@ def test_list_software(self):
'homepage: https://easybuilders.github.io/easybuild',
'',
" * toy v0.0: gompi/2018a, system",
" * toy v0.0 (versionsuffix: '-buggy'): system",
" * toy v0.0 (versionsuffix: '-deps'): system",
" * toy v0.0 (versionsuffix: '-iter'): system",
" * toy v0.0 (versionsuffix: '-multiple'): system",
Expand All @@ -746,6 +747,7 @@ def test_list_software(self):
'version versionsuffix toolchain',
'======= ============= ===========================',
'``0.0`` ``gompi/2018a``, ``system``',
'``0.0`` ``-buggy`` ``system``',
'``0.0`` ``-deps`` ``system``',
'``0.0`` ``-iter`` ``system``',
'``0.0`` ``-multiple`` ``system``',
Expand Down
33 changes: 33 additions & 0 deletions test/framework/easyconfigs/test_ecs/t/toy/toy-0.0-buggy.eb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
name = 'toy'
Copy link
Contributor

Choose a reason for hiding this comment

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

What we usually do instead of adding yet another test easyconfig is read_file it and write with modifications. See self.contents and self.eb_file

version = '0.0'
versionsuffix = '-buggy'

homepage = 'https://easybuilders.github.io/easybuild'
description = "Toy C program, 100% toy."

toolchain = SYSTEM

sources = [SOURCE_TAR_GZ]
checksums = [[
'be662daa971a640e40be5c804d9d7d10', # default (MD5)
'44332000aa33b99ad1e00cbd1a7da769220d74647060a10e807b916d73ea27bc', # default (SHA256)
('adler32', '0x998410035'),
('crc32', '0x1553842328'),
('md5', 'be662daa971a640e40be5c804d9d7d10'),
('sha1', 'f618096c52244539d0e89867405f573fdb0b55b0'),
('size', 273),
]]
patches = [
'toy-0.0_add-bug.patch',
('toy-extra.txt', 'toy-0.0'),
]

sanity_check_paths = {
'files': [('bin/yot', 'bin/toy')],
'dirs': ['bin'],
}

postinstallcmds = ["echo TOY > %(installdir)s/README"]

moduleclass = 'tools'
# trailing comment, leave this here, it may trigger bugs with extract_comments()
2 changes: 1 addition & 1 deletion test/framework/easyconfigs/test_ecs/t/toy/toy-0.0-test.eb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ checksums = [[
('size', 273),
]]
patches = [
'toy-0.0_fix-silly-typo-in-printf-statement.patch',
'toy-0.0_add-bug.patch',
gkaf89 marked this conversation as resolved.
Show resolved Hide resolved
('toy-extra.txt', 'toy-0.0'),
]

Expand Down
2 changes: 1 addition & 1 deletion test/framework/filetools.py
Original file line number Diff line number Diff line change
Expand Up @@ -2474,7 +2474,7 @@ def test_index_functions(self):
# test with specified path with and without trailing '/'s
for path in [test_ecs, test_ecs + '/', test_ecs + '//']:
index = ft.create_index(path)
self.assertEqual(len(index), 94)
self.assertEqual(len(index), 95)

expected = [
os.path.join('b', 'bzip2', 'bzip2-1.0.6-GCC-4.9.2.eb'),
Expand Down
10 changes: 9 additions & 1 deletion test/framework/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -5283,7 +5283,15 @@ def test_prefix_option(self):

regex = re.compile(r"(?P<cfg_opt>\S*).*%s.*" % self.test_prefix, re.M)

expected = ['buildpath', 'containerpath', 'installpath', 'packagepath', 'prefix', 'repositorypath']
expected = [
'buildpath',
'containerpath',
'errorlogpath',
'installpath',
'packagepath',
'prefix',
'repositorypath',
]
self.assertEqual(sorted(regex.findall(txt)), expected)

def test_dump_env_script(self):
Expand Down
10 changes: 10 additions & 0 deletions test/framework/sandbox/sources/toy/toy-0.0_add-bug.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
--- a/toy-0.0.orig/toy.source 2014-03-06 18:48:16.000000000 +0100
+++ b/toy-0.0/toy.source 2020-08-18 12:19:35.000000000 +0200
@@ -2,6 +2,6 @@

int main(int argc, char* argv[]){

- printf("I'm a toy, and proud of it.\n");
+ printf("I'm a toy, and proud of it.\n")
return 0;
}
81 changes: 77 additions & 4 deletions test/framework/toy_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,10 @@
import sys
import tempfile
import textwrap
import pathlib
import filecmp
from easybuild.tools import LooseVersion
from test.framework.utilities import EnhancedTestCase, TestLoaderFiltered
from test.framework.utilities import EnhancedTestCase, TestLoaderFiltered, TempDirectory
from test.framework.package import mock_fpm
from unittest import TextTestRunner

Expand Down Expand Up @@ -156,9 +158,9 @@ def check_toy(self, installpath, outtxt, name='toy', version='0.0', versionprefi
devel_module_path = os.path.join(software_path, 'easybuild', '%s-%s-easybuild-devel' % (name, full_version))
self.assertExists(devel_module_path)

def test_toy_build(self, extra_args=None, ec_file=None, tmpdir=None, verify=True, fails=False, verbose=True,
raise_error=False, test_report=None, name='toy', versionsuffix='', testing=True,
raise_systemexit=False, force=True, test_report_regexs=None, debug=True):
def test_toy_build(self, extra_args=None, ec_file=None, tmpdir=None, tmp_logdir=None, verify=True, fails=False,
verbose=True, raise_error=False, test_report=None, name='toy', versionsuffix='', testing=True,
raise_systemexit=False, force=True, test_report_regexs=None, debug=True, check_errorlog=None):
"""Perform a toy build."""
if extra_args is None:
extra_args = []
Expand All @@ -172,6 +174,7 @@ def test_toy_build(self, extra_args=None, ec_file=None, tmpdir=None, verify=True
'--sourcepath=%s' % self.test_sourcepath,
'--buildpath=%s' % self.test_buildpath,
'--installpath=%s' % self.test_installpath,
'--errorlogpath=%s' % self.test_errorlogpath,
'--unittest-file=%s' % self.logfile,
'--robot=%s' % os.pathsep.join([self.test_buildpath, os.path.dirname(__file__)]),
]
Expand All @@ -183,6 +186,8 @@ def test_toy_build(self, extra_args=None, ec_file=None, tmpdir=None, verify=True
args.append('--tmpdir=%s' % tmpdir)
if test_report is not None:
args.append('--dump-test-report=%s' % test_report)
if tmp_logdir is not None:
args.append('--tmp-logdir=%s' % tmp_logdir)
args.extend(extra_args)
myerr = None
try:
Expand Down Expand Up @@ -228,6 +233,9 @@ def test_toy_build(self, extra_args=None, ec_file=None, tmpdir=None, verify=True
msg = "Pattern %s found in full test report: %s" % (regex.pattern, test_report_txt)
self.assertTrue(regex.search(test_report_txt), msg)

if check_errorlog is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this better done outside? Doesn't need to be in this function, does it?

check_errorlog(outtxt, tmp_logdir, self.test_buildpath, self.test_errorlogpath)

return outtxt

def run_test_toy_build_with_output(self, *args, **kwargs):
Expand Down Expand Up @@ -271,6 +279,71 @@ def test_toy_broken(self):
# cleanup
shutil.rmtree(tmpdir)

def detect_log_file(self, tmp_logpath):
log_files = list(pathlib.Path(tmp_logpath).glob("**/*.log"))

self.assertTrue(len(log_files) >= 1, 'Log files generated')
log_file = log_files[0]
self.assertTrue(len(log_files) == 1, f"Log file '{log_file}' is unique")

return log_file

def assert_build_files_copied(self, buildpath, errorlogpath):
buildir = pathlib.Path(buildpath)
errorlogdir = pathlib.Path(errorlogpath)
iso_date_pattern = r'????????-??????'
for file in buildir.iterdir():
file_relative_path = file.relative_to(buildir)
file_copies = list(errorlogdir.glob(f"toy-0.0/{iso_date_pattern}/{file_relative_path}"))
self.assertTrue(len(file_copies) == 1, f"Unique copy of toy build file '{file}' made")
file_copy = file_copies[0]

if file_copy.is_file():
msg = f"File '{file}' copied succesfully"
self.assertTrue(filecmp.cmp(str(file), str(file_copy), shallow=False), msg)

def assert_log_files_copied(self, log_file, errorlogpath):
file_name = log_file.name
saved_log_files = list(pathlib.Path(errorlogpath).glob(f"**/{file_name}"))
self.assertTrue(len(saved_log_files) == 1, f"Unique copy of log file '{log_file}' made")
for saved_log_file in saved_log_files:
msg = f"Log file '{log_file}' copied succesfully"
self.assertTrue(filecmp.cmp(str(log_file), str(saved_log_file), shallow=False), msg)

def assert_error_reported(self, outtxt, output_regexs):
for regex_pattern in output_regexs:
regex = re.compile(regex_pattern, re.M)
msg_stdout = "Pattern %s found in full test report: %s" % (regex.pattern, outtxt)
self.assertTrue(regex.search(outtxt), msg_stdout)

def check_errorlog(self, output_regexs, outtxt, tmp_logpath, buildpath, errorlogpath):
log_file = self.detect_log_file(tmp_logpath)

self.assert_build_files_copied(buildpath, errorlogpath)
self.assert_log_files_copied(log_file, errorlogpath)
self.assert_error_reported(outtxt, output_regexs)

with open(f"{log_file}", 'r') as p_log_file:
self.assert_error_reported(p_log_file.read(), output_regexs)

def test_toy_broken_compilation(self):
"""Test whether log files and the build directory are copied to a permanent location after a failed
compilation."""
tmpdir = TempDirectory()
tmp_logdir = TempDirectory()
broken_compilation_ec = os.path.join(os.path.dirname(__file__),
'easyconfigs', 'test_ecs', 't', 'toy', 'toy-0.0-buggy.eb')

output_regexs = [r"^\s+toy\.c:5:44: error: expected (;|';') before"]

def check_errorlog(outtxt, tmp_logpath, buildpath, errorlogpath):
self.check_errorlog(output_regexs, outtxt, tmp_logpath, buildpath, errorlogpath)

self.run_test_toy_build_with_output(
ec_file=broken_compilation_ec, tmpdir=tmpdir.get_path(), tmp_logdir=tmp_logdir.get_path(),
verify=False, fails=True, verbose=False, raise_error=False,
name='toy', versionsuffix='-buggy', check_errorlog=check_errorlog)

def test_toy_tweaked(self):
"""Test toy build with tweaked easyconfig, for testing extra easyconfig parameters."""
test_ecs_dir = os.path.join(os.path.abspath(os.path.dirname(__file__)), 'easyconfigs')
Expand Down
Loading
Loading