diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index 80a6ba6560..fd03cea091 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -62,11 +62,11 @@ import urllib.request as std_urllib from easybuild.base import fancylogger -from easybuild.tools import run # import build_log must stay, to use of EasyBuildLog from easybuild.tools.build_log import EasyBuildError, dry_run_msg, print_msg, print_warning from easybuild.tools.config import ERROR, GENERIC_EASYBLOCK_PKG, IGNORE, WARN, build_option, install_path from easybuild.tools.output import PROGRESS_BAR_DOWNLOAD_ONE, start_progress_bar, stop_progress_bar, update_progress_bar +from easybuild.tools.run import run from easybuild.tools.utilities import natural_keys, nub, remove_unwanted_chars, trace_msg try: @@ -445,7 +445,7 @@ def extract_file(fn, dest, cmd=None, extra_options=None, overwrite=False, forced """ if not os.path.isfile(fn) and not build_option('extended_dry_run'): - raise EasyBuildError("Can't extract file %s: no such file", fn) + raise EasyBuildError(f"Can't extract file {fn}: no such file") mkdir(dest, parents=True) @@ -453,24 +453,24 @@ def extract_file(fn, dest, cmd=None, extra_options=None, overwrite=False, forced abs_dest = os.path.abspath(dest) # change working directory - _log.debug("Unpacking %s in directory %s", fn, abs_dest) + _log.debug(f"Unpacking {fn} in directory {abs_dest}") cwd = change_dir(abs_dest) if cmd: # complete command template with filename cmd = cmd % fn - _log.debug("Using specified command to unpack %s: %s", fn, cmd) + _log.debug("Using specified command to unpack {fn}: {cmd}") else: cmd = extract_cmd(fn, overwrite=overwrite) - _log.debug("Using command derived from file extension to unpack %s: %s", fn, cmd) + _log.debug("Using command derived from file extension to unpack {fn}: {cmd}") if not cmd: - raise EasyBuildError("Can't extract file %s with unknown filetype", fn) + raise EasyBuildError("Can't extract file {fn} with unknown filetype") if extra_options: - cmd = "%s %s" % (cmd, extra_options) + cmd = f"{cmd} {extra_options}" - run.run_cmd(cmd, simple=True, force_in_dry_run=forced, trace=trace) + run(cmd, in_dry_run=forced, hidden=not trace) # note: find_base_dir also changes into the base dir! base_dir = find_base_dir() @@ -479,7 +479,7 @@ def extract_file(fn, dest, cmd=None, extra_options=None, overwrite=False, forced # change back to where we came from (unless that was a non-existing directory) if not change_into_dir: if cwd is None: - raise EasyBuildError("Can't change back to non-existing directory after extracting %s in %s", fn, dest) + raise EasyBuildError(f"Can't change back to non-existing directory after extracting {fn} in {dest}") else: change_dir(cwd) @@ -1547,27 +1547,27 @@ def apply_patch(patch_file, dest, fn=None, copy=False, level=None, use_git=False if build_option('extended_dry_run'): # skip checking of files in dry run mode patch_filename = os.path.basename(patch_file) - dry_run_msg("* applying patch file %s" % patch_filename, silent=build_option('silent')) + dry_run_msg(f"* applying patch file {patch_filename}", silent=build_option('silent')) elif not os.path.isfile(patch_file): - raise EasyBuildError("Can't find patch %s: no such file", patch_file) + raise EasyBuildError(f"Can't find patch {patch_file}: no such file") elif fn and not os.path.isfile(fn): - raise EasyBuildError("Can't patch file %s: no such file", fn) + raise EasyBuildError(f"Can't patch file {fn}: no such file") # copy missing files if copy: if build_option('extended_dry_run'): - dry_run_msg(" %s copied to %s" % (patch_file, dest), silent=build_option('silent')) + dry_run_msg(f" {patch_file} copied to {dest}", silent=build_option('silent')) else: copy_file(patch_file, dest) - _log.debug("Copied patch %s to dir %s" % (patch_file, dest)) + _log.debug(f"Copied patch {patch_file} to dir {dest}") # early exit, work is done after copying return True elif not os.path.isdir(dest): - raise EasyBuildError("Can't patch directory %s: no such directory", dest) + raise EasyBuildError(f"Can't patch directory {dest}: no such directory") # use absolute paths abs_patch_file = os.path.abspath(patch_file) @@ -1582,14 +1582,14 @@ def apply_patch(patch_file, dest, fn=None, copy=False, level=None, use_git=False patch_subextension = os.path.splitext(patch_stem)[1] if patch_subextension == ".patch": workdir = tempfile.mkdtemp(prefix='eb-patch-') - _log.debug("Extracting the patch to: %s", workdir) + _log.debug(f"Extracting the patch to: {workdir}") # extracting the patch extracted_dir = extract_file(abs_patch_file, workdir, change_into_dir=False) abs_patch_file = os.path.join(extracted_dir, patch_stem) if use_git: verbose = '--verbose ' if build_option('debug') else '' - patch_cmd = "git apply %s%s" % (verbose, abs_patch_file) + patch_cmd = f"git apply {verbose}{abs_patch_file}" else: if level is None and build_option('extended_dry_run'): level = '' @@ -1602,27 +1602,30 @@ def apply_patch(patch_file, dest, fn=None, copy=False, level=None, use_git=False patched_files = det_patched_files(path=abs_patch_file) if not patched_files: - raise EasyBuildError("Can't guess patchlevel from patch %s: no testfile line found in patch", - abs_patch_file) + msg = f"Can't guess patchlevel from patch {abs_patch_file}: no testfile line found in patch" + raise EasyBuildError(msg) level = guess_patch_level(patched_files, abs_dest) if level is None: # level can also be 0 (zero), so don't use "not level" # no match - raise EasyBuildError("Can't determine patch level for patch %s from directory %s", patch_file, abs_dest) + raise EasyBuildError(f"Can't determine patch level for patch {patch_file} from directory {abs_dest}") else: - _log.debug("Guessed patch level %d for patch %s" % (level, patch_file)) + _log.debug(f"Guessed patch level {level} for patch {patch_file}") else: - _log.debug("Using specified patch level %d for patch %s" % (level, patch_file)) + _log.debug(f"Using specified patch level {level} for patch {patch_file}") backup_option = '-b ' if build_option('backup_patched_files') else '' - patch_cmd = "patch " + backup_option + "-p%s -i %s" % (level, abs_patch_file) + patch_cmd = f"patch {backup_option} -p{level} -i {abs_patch_file}" - out, ec = run.run_cmd(patch_cmd, simple=False, path=abs_dest, log_ok=False, trace=False) + res = run(patch_cmd, fail_on_error=False, hidden=True, work_dir=abs_dest) + + if res.exit_code: + msg = f"Couldn't apply patch file {patch_file}. " + msg += f"Process exited with code {res.exit_code}: {res.output}" + raise EasyBuildError(msg) - if ec: - raise EasyBuildError("Couldn't apply patch file %s. Process exited with code %s: %s", patch_file, ec, out) return True @@ -2148,7 +2151,7 @@ def move_logs(src_logfile, target_logfile): try: # there may be multiple log files, due to log rotation - app_logs = glob.glob('%s*' % src_logfile) + app_logs = glob.glob(f'{src_logfile}*') for app_log in app_logs: # retain possible suffix new_log_path = target_logfile + app_log[src_logfile_len:] @@ -2159,11 +2162,11 @@ def move_logs(src_logfile, target_logfile): # move log to target path move_file(app_log, new_log_path) - _log.info("Moved log file %s to %s" % (src_logfile, new_log_path)) + _log.info(f"Moved log file {src_logfile} to {new_log_path}") if zip_log_cmd: - run.run_cmd("%s %s" % (zip_log_cmd, new_log_path)) - _log.info("Zipped log %s using '%s'", new_log_path, zip_log_cmd) + run(f"{zip_log_cmd} {new_log_path}") + _log.info(f"Zipped log {new_log_path} using '{zip_log_cmd}'") except (IOError, OSError) as err: raise EasyBuildError("Failed to move log file(s) %s* to new log file %s*: %s", @@ -2253,21 +2256,6 @@ def decode_class_name(name): return decode_string(name) -def run_cmd(cmd, log_ok=True, log_all=False, simple=False, inp=None, regexp=True, log_output=False, path=None): - """NO LONGER SUPPORTED: use run_cmd from easybuild.tools.run instead""" - _log.nosupport("run_cmd was moved from easybuild.tools.filetools to easybuild.tools.run", '2.0') - - -def run_cmd_qa(cmd, qa, no_qa=None, log_ok=True, log_all=False, simple=False, regexp=True, std_qa=None, path=None): - """NO LONGER SUPPORTED: use run_cmd_qa from easybuild.tools.run instead""" - _log.nosupport("run_cmd_qa was moved from easybuild.tools.filetools to easybuild.tools.run", '2.0') - - -def parse_log_for_error(txt, regExp=None, stdout=True, msg=None): - """NO LONGER SUPPORTED: use parse_log_for_error from easybuild.tools.run instead""" - _log.nosupport("parse_log_for_error was moved from easybuild.tools.filetools to easybuild.tools.run", '2.0') - - def det_size(path): """ Determine total size of given filepath (in bytes). @@ -2592,7 +2580,7 @@ def get_source_tarball_from_git(filename, targetdir, git_config): """ # sanity check on git_config value being passed if not isinstance(git_config, dict): - raise EasyBuildError("Found unexpected type of value for 'git_config' argument: %s" % type(git_config)) + raise EasyBuildError("Found unexpected type of value for 'git_config' argument: {type(git_config)}") # Making a copy to avoid modifying the object with pops git_config = git_config.copy() @@ -2606,7 +2594,7 @@ def get_source_tarball_from_git(filename, targetdir, git_config): # input validation of git_config dict if git_config: - raise EasyBuildError("Found one or more unexpected keys in 'git_config' specification: %s", git_config) + raise EasyBuildError("Found one or more unexpected keys in 'git_config' specification: {git_config}") if not repo_name: raise EasyBuildError("repo_name not specified in git_config parameter") @@ -2643,14 +2631,14 @@ def get_source_tarball_from_git(filename, targetdir, git_config): # checkout is done separately below for specific commits clone_cmd.append('--no-checkout') - clone_cmd.append('%s/%s.git' % (url, repo_name)) + clone_cmd.append(f'{url}/{repo_name}.git') if clone_into: - clone_cmd.append('%s' % clone_into) + clone_cmd.append(clone_into) tmpdir = tempfile.mkdtemp() - cwd = change_dir(tmpdir) - run.run_cmd(' '.join(clone_cmd), log_all=True, simple=True, regexp=False, trace=False) + + run(' '.join(clone_cmd), hidden=True, verbose_dry_run=True, work_dir=tmpdir) # If the clone is done into a specified name, change repo_name if clone_into: @@ -2662,43 +2650,45 @@ def get_source_tarball_from_git(filename, targetdir, git_config): if recursive: checkout_cmd.extend(['&&', 'git', 'submodule', 'update', '--init', '--recursive']) - run.run_cmd(' '.join(checkout_cmd), log_all=True, simple=True, regexp=False, path=repo_name, trace=False) + work_dir = os.path.join(tmpdir, repo_name) if repo_name else tmpdir + run(' '.join(checkout_cmd), work_dir=work_dir, hidden=True, verbose_dry_run=True) elif not build_option('extended_dry_run'): # If we wanted to get a tag make sure we actually got a tag and not a branch with the same name # This doesn't make sense in dry-run mode as we don't have anything to check - cmd = 'git describe --exact-match --tags HEAD' - # Note: Disable logging to also disable the error handling in run_cmd - (out, ec) = run.run_cmd(cmd, log_ok=False, log_all=False, regexp=False, path=repo_name, trace=False) - if ec != 0 or tag not in out.splitlines(): - print_warning('Tag %s was not downloaded in the first try due to %s/%s containing a branch' - ' with the same name. You might want to alert the maintainers of %s about that issue.', - tag, url, repo_name, repo_name) + cmd = "git describe --exact-match --tags HEAD" + work_dir = os.path.join(tmpdir, repo_name) if repo_name else tmpdir + res = run(cmd, fail_on_error=False, work_dir=work_dir, hidden=True, verbose_dry_run=True) + + if res.exit_code != 0 or tag not in res.output.splitlines(): + msg = f"Tag {tag} was not downloaded in the first try due to {url}/{repo_name} containing a branch" + msg += f" with the same name. You might want to alert the maintainers of {repo_name} about that issue." + print_warning(msg) + cmds = [] if not keep_git_dir: # make the repo unshallow first; # this is equivalent with 'git fetch -unshallow' in Git 1.8.3+ # (first fetch seems to do nothing, unclear why) - cmds.append('git fetch --depth=2147483647 && git fetch --depth=2147483647') + cmds.append("git fetch --depth=2147483647 && git fetch --depth=2147483647") - cmds.append('git checkout refs/tags/' + tag) + cmds.append(f"git checkout refs/tags/{tag}") # Clean all untracked files, e.g. from left-over submodules - cmds.append('git clean --force -d -x') + cmds.append("git clean --force -d -x") if recursive: - cmds.append('git submodule update --init --recursive') + cmds.append("git submodule update --init --recursive") for cmd in cmds: - run.run_cmd(cmd, log_all=True, simple=True, regexp=False, path=repo_name, trace=False) + run(cmd, work_dir=work_dir, hidden=True, verbose_dry_run=True) # create an archive and delete the git repo directory if keep_git_dir: tar_cmd = ['tar', 'cfvz', targetpath, repo_name] else: tar_cmd = ['tar', 'cfvz', targetpath, '--exclude', '.git', repo_name] - run.run_cmd(' '.join(tar_cmd), log_all=True, simple=True, regexp=False, trace=False) + run(' '.join(tar_cmd), work_dir=tmpdir, hidden=True, verbose_dry_run=True) # cleanup (repo_name dir does not exist in dry run mode) - change_dir(cwd) remove(tmpdir) return targetpath diff --git a/easybuild/tools/run.py b/easybuild/tools/run.py index c972277b00..76c6883eb9 100644 --- a/easybuild/tools/run.py +++ b/easybuild/tools/run.py @@ -108,7 +108,7 @@ def cache_aware_func(cmd, *args, **kwargs): @run_cache def run(cmd, fail_on_error=True, split_stderr=False, stdin=None, env=None, - hidden=False, in_dry_run=False, work_dir=None, shell=True, + hidden=False, in_dry_run=False, verbose_dry_run=False, work_dir=None, shell=True, output_file=False, stream_output=False, asynchronous=False, qa_patterns=None, qa_wait_patterns=None): """ @@ -120,6 +120,7 @@ def run(cmd, fail_on_error=True, split_stderr=False, stdin=None, env=None, :param env: environment to use to run command (if None, inherit current process environment) :param hidden: do not show command in terminal output (when using --trace, or with --extended-dry-run / -x) :param in_dry_run: also run command in dry run mode + :param verbose_dry_run: show that command is run in dry run mode (overrules 'hidden') :param work_dir: working directory to run command in (current working directory if None) :param shell: execute command through bash shell (enabled by default) :param output_file: collect command output in temporary output file @@ -135,7 +136,7 @@ def run(cmd, fail_on_error=True, split_stderr=False, stdin=None, env=None, """ # temporarily raise a NotImplementedError until all options are implemented - if any((work_dir, stream_output, asynchronous)): + if any((stream_output, asynchronous)): raise NotImplementedError if qa_patterns or qa_wait_patterns: @@ -162,7 +163,7 @@ def run(cmd, fail_on_error=True, split_stderr=False, stdin=None, env=None, # early exit in 'dry run' mode, after printing the command that would be run (unless 'hidden' is enabled) if not in_dry_run and build_option('extended_dry_run'): - if not hidden: + if not hidden or verbose_dry_run: silent = build_option('silent') msg = f" running command \"{cmd_str}\"\n" msg += f" (in {work_dir})" @@ -187,7 +188,7 @@ def run(cmd, fail_on_error=True, split_stderr=False, stdin=None, env=None, _log.info(f"Running command '{cmd_str}' in {work_dir}") proc = subprocess.run(cmd, stdout=subprocess.PIPE, stderr=stderr, check=fail_on_error, - env=env, input=stdin, shell=shell, executable=executable) + cwd=work_dir, env=env, input=stdin, shell=shell, executable=executable) # return output as a regular string rather than a byte sequence (and non-UTF-8 characters get stripped out) output = proc.stdout.decode('utf-8', 'ignore') diff --git a/test/framework/filetools.py b/test/framework/filetools.py index fcaebe16d4..d4eb45b7c9 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -2831,7 +2831,7 @@ def run_check(): r' running command "git clone --no-checkout %(git_repo)s"', r" \(in /.*\)", r' running command "git checkout 8456f86 && git submodule update --init --recursive"', - r" \(in testrepository\)", + r" \(in /.*/testrepository\)", r' running command "tar cfvz .*/target/test.tar.gz --exclude .git testrepository"', r" \(in /.*\)", ]) % git_repo @@ -2842,7 +2842,7 @@ def run_check(): r' running command "git clone --no-checkout %(git_repo)s"', r" \(in /.*\)", r' running command "git checkout 8456f86"', - r" \(in testrepository\)", + r" \(in /.*/testrepository\)", r' running command "tar cfvz .*/target/test.tar.gz --exclude .git testrepository"', r" \(in /.*\)", ]) % git_repo diff --git a/test/framework/lib.py b/test/framework/lib.py index 9eed88632f..dd1d27c41b 100644 --- a/test/framework/lib.py +++ b/test/framework/lib.py @@ -43,7 +43,7 @@ from easybuild.tools.options import set_up_configuration from easybuild.tools.filetools import mkdir from easybuild.tools.modules import modules_tool -from easybuild.tools.run import run_cmd +from easybuild.tools.run import run, run_cmd class EasyBuildLibTest(TestCase): @@ -67,14 +67,7 @@ def tearDown(self): def configure(self): """Utility function to set up EasyBuild configuration.""" - - # wipe BuildOption singleton instance, so it gets re-created when set_up_configuration is called - if BuildOptions in BuildOptions._instances: - del BuildOptions._instances[BuildOptions] - - self.assertNotIn(BuildOptions, BuildOptions._instances) - set_up_configuration(silent=True) - self.assertIn(BuildOptions, BuildOptions._instances) + set_up_configuration(silent=True, reconfigure=True) def test_run_cmd(self): """Test use of run_cmd function in the context of using EasyBuild framework as a library.""" @@ -92,8 +85,24 @@ def test_run_cmd(self): self.assertEqual(ec, 0) self.assertEqual(out, 'hello\n') + def test_run(self): + """Test use of run function in the context of using EasyBuild framework as a library.""" + + error_pattern = r"Undefined build option: .*" + error_pattern += r" Make sure you have set up the EasyBuild configuration using set_up_configuration\(\)" + self.assertErrorRegex(EasyBuildError, error_pattern, run, "echo hello") + + self.configure() + + # runworks fine if set_up_configuration was called first + self.mock_stdout(True) + res = run("echo hello") + self.mock_stdout(False) + self.assertEqual(res.exit_code, 0) + self.assertEqual(res.output, 'hello\n') + def test_mkdir(self): - """Test use of run_cmd function in the context of using EasyBuild framework as a library.""" + """Test use of mkdir function in the context of using EasyBuild framework as a library.""" test_dir = os.path.join(self.tmpdir, 'test123') diff --git a/test/framework/run.py b/test/framework/run.py index 4cd6821569..c8195cc3a7 100644 --- a/test/framework/run.py +++ b/test/framework/run.py @@ -342,6 +342,48 @@ def test_run_bis(self): self.assertEqual(len(res.output), len("hello\n" * 300)) self.assertEqual(res.exit_code, 0) + def test_run_cmd_work_dir(self): + """ + Test running command in specific directory with run_cmd function. + """ + orig_wd = os.getcwd() + self.assertFalse(os.path.samefile(orig_wd, self.test_prefix)) + + test_dir = os.path.join(self.test_prefix, 'test') + for fn in ('foo.txt', 'bar.txt'): + write_file(os.path.join(test_dir, fn), 'test') + + with self.mocked_stdout_stderr(): + (out, ec) = run_cmd("ls | sort", path=test_dir) + + self.assertEqual(ec, 0) + self.assertEqual(out, 'bar.txt\nfoo.txt\n') + + self.assertTrue(os.path.samefile(orig_wd, os.getcwd())) + + def test_run_work_dir(self): + """ + Test running command in specific directory with run function. + """ + orig_wd = os.getcwd() + self.assertFalse(os.path.samefile(orig_wd, self.test_prefix)) + + test_dir = os.path.join(self.test_prefix, 'test') + for fn in ('foo.txt', 'bar.txt'): + write_file(os.path.join(test_dir, fn), 'test') + + cmd = "ls | sort" + with self.mocked_stdout_stderr(): + res = run(cmd, work_dir=test_dir) + + self.assertEqual(res.cmd, cmd) + self.assertEqual(res.exit_code, 0) + self.assertEqual(res.output, 'bar.txt\nfoo.txt\n') + self.assertEqual(res.stderr, None) + self.assertEqual(res.work_dir, test_dir) + + self.assertTrue(os.path.samefile(orig_wd, os.getcwd())) + def test_run_cmd_log_output(self): """Test run_cmd with log_output enabled""" with self.mocked_stdout_stderr(): diff --git a/test/framework/toy_build.py b/test/framework/toy_build.py index 2192b0589c..eb8a417b24 100644 --- a/test/framework/toy_build.py +++ b/test/framework/toy_build.py @@ -1429,10 +1429,12 @@ def test_toy_extension_extract_cmd(self): ]) write_file(test_ec, test_ec_txt) - error_pattern = "unzip .*/bar-0.0.tar.gz.* exited with exit code [1-9]" + error_pattern = "unzip .*/bar-0.0.tar.gz.* returned non-zero exit status" with self.mocked_stdout_stderr(): - self.assertErrorRegex(EasyBuildError, error_pattern, self._test_toy_build, ec_file=test_ec, - raise_error=True, verbose=False) + # for now, we expect subprocess.CalledProcessError, but eventually 'run' function will + # do proper error reporting + self.assertErrorRegex(EasyBuildError, error_pattern, + self._test_toy_build, ec_file=test_ec, raise_error=True, verbose=False) def test_toy_extension_sources_git_config(self): """Test install toy that includes extensions with 'sources' spec including 'git_config'."""