From 22cab8e36792f28286c1f0c7a55e02a3a5aafd5b Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Mon, 6 Jan 2025 16:27:48 +0100 Subject: [PATCH 1/2] show readable error message when applying patch without (extracted) source When there is no source `self.src` is an empty list which leads to a rather generic error message. Improve that by showing that there was no source to apply the patch to. For extensions `self.src` is set to a string as only a single source is supported. Accessing `self.src[0].['finalpath']` leads to an error > TypeError: string indices must be integers This happens when the source didn't got extracted so `self.ext_dir` and hence `beginpath` will be `None`. Make the error show that the source was not extracted. --- easybuild/framework/easyblock.py | 24 +++--- test/framework/easyblock.py | 83 +++++++++++++++++-- .../easyblocks/generic/dummyextension.py | 16 ++++ 3 files changed, 104 insertions(+), 19 deletions(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index a515bc4c86..9aaebe9bd4 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -2607,8 +2607,6 @@ def patch_step(self, beginpath=None, patches=None): self.log.info("Applying patch %s" % patch['name']) trace_msg("applying patch %s" % patch['name']) - # patch source at specified index (first source if not specified) - srcind = patch.get('source', 0) # if patch level is specified, use that (otherwise let apply_patch derive patch level) level = patch.get('level', None) # determine suffix of source path to apply patch in (if any) @@ -2616,16 +2614,22 @@ def patch_step(self, beginpath=None, patches=None): # determine whether 'patch' file should be copied rather than applied copy_patch = 'copy' in patch and 'sourcepath' not in patch - self.log.debug("Source index: %s; patch level: %s; source path suffix: %s; copy patch: %s", - srcind, level, srcpathsuffix, copy_patch) + self.log.debug("Patch level: %s; source path suffix: %s; copy patch: %s", + level, srcpathsuffix, copy_patch) if beginpath is None: - try: - beginpath = self.src[srcind]['finalpath'] - self.log.debug("Determine begin path for patch %s: %s" % (patch['name'], beginpath)) - except IndexError as err: - raise EasyBuildError("Can't apply patch %s to source at index %s of list %s: %s", - patch['name'], srcind, self.src, err) + # If the src member is a string we have an extension with a single source. + # If that did extract the source beginpath would be set. + if isinstance(self.src, str): + raise EasyBuildError("Cannot apply patches if sources were not extracted. " + "Patch file: " + patch['name']) + if self.src: + # Use (extracted) location of first source. + # Other sources will likely not have a reasonable finalpath set. + beginpath = self.src[0]['finalpath'] + self.log.debug("Determined begin path for patch %s: %s" % (patch['name'], beginpath)) + else: + raise EasyBuildError("Can't apply patch %s when there are no sources!", patch['name']) else: self.log.debug("Using specified begin path for patch %s: %s" % (patch['name'], beginpath)) diff --git a/test/framework/easyblock.py b/test/framework/easyblock.py index c442b7f782..13225a9b50 100644 --- a/test/framework/easyblock.py +++ b/test/framework/easyblock.py @@ -2020,26 +2020,30 @@ def test_exclude_path_to_top_of_module_tree(self): def test_patch_step(self): """Test patch step.""" - test_easyconfigs = os.path.join(os.path.abspath(os.path.dirname(__file__)), 'easyconfigs', 'test_ecs') - ec = process_easyconfig(os.path.join(test_easyconfigs, 't', 'toy', 'toy-0.0.eb'))[0] - orig_sources = ec['ec']['sources'][:] + cwd = os.getcwd() + + testdir = os.path.abspath(os.path.dirname(__file__)) + test_easyconfigs = os.path.join(testdir, 'easyconfigs', 'test_ecs') + ec = process_easyconfig(os.path.join(test_easyconfigs, 't', 'toy', 'toy-0.0.eb'))[0]['ec'] + orig_sources = ec['sources'][:] toy_patches = [ 'toy-0.0_fix-silly-typo-in-printf-statement.patch', # test for applying patch ('toy-extra.txt', 'toy-0.0'), # test for patch-by-copy ] - self.assertEqual(ec['ec']['patches'], toy_patches) + self.assertEqual(ec['patches'], toy_patches) # test applying patches without sources - ec['ec']['sources'] = [] - eb = EasyBlock(ec['ec']) + ec['sources'] = [] + eb = EasyBlock(ec) eb.fetch_step() eb.extract_step() self.assertErrorRegex(EasyBuildError, '.*', eb.patch_step) # test actual patching of unpacked sources - ec['ec']['sources'] = orig_sources - eb = EasyBlock(ec['ec']) + ec['sources'] = orig_sources + change_dir(cwd) + eb = EasyBlock(ec) eb.fetch_step() eb.extract_step() eb.patch_step() @@ -2051,7 +2055,8 @@ def test_patch_step(self): # check again with backup of patched files enabled update_build_option('backup_patched_files', True) - eb = EasyBlock(ec['ec']) + change_dir(cwd) + eb = EasyBlock(ec) eb.fetch_step() eb.extract_step() eb.patch_step() @@ -2299,6 +2304,66 @@ def check_ext_start_dir(expected_start_dir, unpack_src=True, parent_startdir=Non check_ext_start_dir(self.test_prefix, parent_startdir=self.test_prefix) self.assertFalse(self.get_stderr()) + def test_extension_patch_step(self): + """Test start dir with extensions.""" + test_easyconfigs = os.path.join(os.path.abspath(os.path.dirname(__file__)), 'easyconfigs', 'test_ecs') + ec = process_easyconfig(os.path.join(test_easyconfigs, 't', 'toy', 'toy-0.0.eb'))[0]['ec'] + + cwd = os.getcwd() + self.assertExists(cwd) + + def run_extension_step(): + change_dir(cwd) + eb = EasyBlock(ec) + # Cleanup build directory + if os.path.exists(eb.builddir): + remove_dir(eb.builddir) + eb.make_builddir() + eb.update_config_template_run_step() + eb.extensions_step(fetch=True, install=True) + return os.path.join(eb.builddir) + + ec['exts_defaultclass'] = 'DummyExtension' + ec['exts_list'] = [('toy', '0.0', {'easyblock': 'DummyExtension'})] + + # No patches, no errors + with self.mocked_stdout_stderr(): + run_extension_step() + self.assertFalse(self.get_stderr()) + + # Patch present, source extracted + with ec.disable_templating(): + ec['exts_list'][0][2]['patches'] = [('toy-extra.txt', 'toy-0.0')] + ec['exts_list'][0][2]['unpack_source'] = True + with self.mocked_stdout_stderr(): + builddir = run_extension_step() + self.assertTrue(os.path.isfile(os.path.join(builddir, 'toy', 'toy-0.0', 'toy-extra.txt'))) + self.assertFalse(self.get_stderr()) + + # Patch but source not extracted + with ec.disable_templating(): + ec['exts_list'][0][2]['unpack_source'] = False + with self.mocked_stdout_stderr(): + self.assertErrorRegex(EasyBuildError, 'not extracted', run_extension_step) + self.assertFalse(self.get_stderr()) + + # Patch but no source + with ec.disable_templating(): + ec['exts_list'][0][2]['nosource'] = True + with self.mocked_stdout_stderr(): + self.assertErrorRegex(EasyBuildError, 'no sources', run_extension_step) + self.assertFalse(self.get_stderr()) + + # Patch without source is possible if the start_dir is set + with ec.disable_templating(): + ec['start_dir'] = '%(builddir)s' + ec['exts_list'][0][2]['nosource'] = True + ec['exts_list'][0][2]['patches'] = [('toy-extra.txt', '.')] + with self.mocked_stdout_stderr(): + builddir = run_extension_step() + self.assertTrue(os.path.isfile(os.path.join(builddir, 'toy-extra.txt'))) + self.assertFalse(self.get_stderr()) + def test_prepare_step(self): """Test prepare step (setting up build environment).""" test_easyconfigs = os.path.join(os.path.abspath(os.path.dirname(__file__)), 'easyconfigs', 'test_ecs') diff --git a/test/framework/sandbox/easybuild/easyblocks/generic/dummyextension.py b/test/framework/sandbox/easybuild/easyblocks/generic/dummyextension.py index e0429a450b..13ac862db9 100644 --- a/test/framework/sandbox/easybuild/easyblocks/generic/dummyextension.py +++ b/test/framework/sandbox/easybuild/easyblocks/generic/dummyextension.py @@ -28,12 +28,21 @@ @author: Kenneth Hoste (Ghent University) """ +from easybuild.framework.easyconfig import CUSTOM from easybuild.framework.extensioneasyblock import ExtensionEasyBlock class DummyExtension(ExtensionEasyBlock): """Support for building/installing dummy extensions.""" + @staticmethod + def extra_options(): + """Custom easyconfig parameters for dummy extensions.""" + extra_vars = { + 'unpack_source': [None, "Unpack sources", CUSTOM], + } + return ExtensionEasyBlock.extra_options(extra_vars=extra_vars) + def __init__(self, *args, **kwargs): super(DummyExtension, self).__init__(*args, **kwargs) @@ -41,3 +50,10 @@ def __init__(self, *args, **kwargs): # use lowercase name as default value for expected module name, and replace '-' with '_' if 'modulename' not in self.options: self.options['modulename'] = self.name.lower().replace('-', '_') + + def run(self, unpack_src=False): + """Install the dummy extension.""" + ec_unpack_source = self.cfg.get('unpack_source') + if ec_unpack_source is not None: + unpack_src = ec_unpack_source + super(DummyExtension, self).run(unpack_src) From 0876b63929ffede2a5ea69ec9965796c6e2bc5f8 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Wed, 19 Feb 2025 10:37:09 +0100 Subject: [PATCH 2/2] Restore environment after sub-test --- test/framework/easyblock.py | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/test/framework/easyblock.py b/test/framework/easyblock.py index 13225a9b50..16ec79e0b9 100644 --- a/test/framework/easyblock.py +++ b/test/framework/easyblock.py @@ -30,6 +30,7 @@ @author: Maxime Boissonneault (Compute Canada) @author: Jan Andre Reuter (Juelich Supercomputing Centre) """ +import copy import os import re import shutil @@ -49,6 +50,7 @@ from easybuild.tools import LooseVersion, config from easybuild.tools.build_log import EasyBuildError from easybuild.tools.config import get_module_syntax, update_build_option +from easybuild.tools.environment import modify_env from easybuild.tools.filetools import change_dir, copy_dir, copy_file, mkdir, read_file, remove_dir, remove_file from easybuild.tools.filetools import verify_checksum, write_file from easybuild.tools.module_generator import module_generator @@ -2311,17 +2313,23 @@ def test_extension_patch_step(self): cwd = os.getcwd() self.assertExists(cwd) + # Take environment with test-specific variable set up + orig_environ = copy.deepcopy(os.environ) def run_extension_step(): - change_dir(cwd) - eb = EasyBlock(ec) - # Cleanup build directory - if os.path.exists(eb.builddir): - remove_dir(eb.builddir) - eb.make_builddir() - eb.update_config_template_run_step() - eb.extensions_step(fetch=True, install=True) - return os.path.join(eb.builddir) + try: + change_dir(cwd) + eb = EasyBlock(ec) + # Cleanup build directory + if os.path.exists(eb.builddir): + remove_dir(eb.builddir) + eb.make_builddir() + eb.update_config_template_run_step() + eb.extensions_step(fetch=True, install=True) + return os.path.join(eb.builddir) + finally: + # restore original environment to continue testing with a clean slate + modify_env(os.environ, orig_environ, verbose=False) ec['exts_defaultclass'] = 'DummyExtension' ec['exts_list'] = [('toy', '0.0', {'easyblock': 'DummyExtension'})]