From 43031e4ee76ea753a910fa332273ac17a9ae52f8 Mon Sep 17 00:00:00 2001 From: Georgios Kafanas Date: Sun, 19 Jan 2025 15:27:55 +0100 Subject: [PATCH 01/25] Add function determining if path is ancestor of another path --- easybuild/tools/filetools.py | 13 +++++++++++++ test/framework/filetools.py | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index 033c88a65d..a16e2720d6 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -603,6 +603,19 @@ def normalize_path(path): return start_slashes + os.path.sep.join(filtered_comps) +def is_parent_path(ancestor, path): + """ + Return true if ancestor is prefix of path + + :param ancestor: absolute or relative path + :param path: absolute or relative path + """ + ancestor = os.path.realpath(ancestor) + path = os.path.realpath(path) + common_path = os.path.commonprefix([ancestor, path]) + return common_path == ancestor + + def is_alt_pypi_url(url): """Determine whether specified URL is already an alternative PyPI URL, i.e. whether it contains a hash.""" # example: .../packages/5b/03/e135b19fadeb9b1ccb45eac9f60ca2dc3afe72d099f6bd84e03cb131f9bf/easybuild-2.7.0.tar.gz diff --git a/test/framework/filetools.py b/test/framework/filetools.py index b72f3ded25..56a4c45809 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -476,6 +476,42 @@ def test_normalize_path(self): self.assertEqual(ft.normalize_path('/././foo//bar/././baz/'), '/foo/bar/baz') self.assertEqual(ft.normalize_path('//././foo//bar/././baz/'), '//foo/bar/baz') + def test_is_parent_path(self): + """Test is_parent_path""" + self.assertTrue(ft.is_parent_path('/foo/bar', '/foo/bar/test0')) + self.assertTrue(ft.is_parent_path('/foo/bar', '/foo/bar/test0/test1')) + self.assertTrue(ft.is_parent_path('/foo/bar', '/foo/bar')) + self.assertFalse(ft.is_parent_path('/foo/bar', '/foo/test')) + + # Check that trailing slashes are ignored + self.assertTrue(ft.is_parent_path('/foo/bar/', '/foo/bar')) + self.assertTrue(ft.is_parent_path('/foo/bar', '/foo/bar/')) + self.assertTrue(ft.is_parent_path('/foo/bar/', '/foo/bar/')) + + # Check that is also accepts relative paths + self.assertTrue(ft.is_parent_path('foo/bar', 'foo/bar/test0')) + self.assertTrue(ft.is_parent_path('foo/bar', 'foo/bar/test0/test1')) + self.assertTrue(ft.is_parent_path('foo/bar', 'foo/bar')) + self.assertFalse(ft.is_parent_path('foo/bar', 'foo/test')) + + # Check that relative paths are accounted + self.assertTrue(ft.is_parent_path('foo/../baz', 'bar/../baz')) + + # Check that symbolic links are accounted + os.mkdir(os.path.join(self.test_prefix, 'base')) + os.mkdir(os.path.join(self.test_prefix, 'base', 'concrete')) + os.symlink( + os.path.join(self.test_prefix, 'base', 'concrete'), + os.path.join(self.test_prefix, 'base', 'link') + ) + self.assertTrue( + ft.is_parent_path( + os.path.join(self.test_prefix, 'base', 'link'), + os.path.join(self.test_prefix, 'base', 'concrete', 'file') + ) + ) + shutil.rmtree(os.path.join(self.test_prefix, 'base')) + def test_det_file_size(self): """Test det_file_size function.""" From 5af0637c73743495f65dbedbbdc51796818ceb0b Mon Sep 17 00:00:00 2001 From: Georgios Kafanas Date: Tue, 21 Jan 2025 23:45:16 +0100 Subject: [PATCH 02/25] Add assertion about the existence of a list of paths --- easybuild/base/testing.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/easybuild/base/testing.py b/easybuild/base/testing.py index 55275c5875..297cd01ccd 100644 --- a/easybuild/base/testing.py +++ b/easybuild/base/testing.py @@ -117,6 +117,11 @@ def assertNotExists(self, path, msg=None): msg = "'%s' should not exist" % path self.assertFalse(os.path.exists(path), msg) + def assertAllExist(self, paths, msg=None): + """Assert that all paths in the given list exist""" + for p in paths: + self.assertExists(p, msg) + def setUp(self): """Prepare test case.""" super(TestCase, self).setUp() From e92e46907d6e65298b6a5e59750c6df50ff36498 Mon Sep 17 00:00:00 2001 From: Georgios Kafanas Date: Sat, 11 Jan 2025 23:34:06 +0100 Subject: [PATCH 03/25] Add function generating unused directories given a list of paths The create_unused_dir function distinguishes between the parent directory and the name of the target subdirectory in 2 separate arguments. This interface requires additional parameter management when combined with newer path management interfaces such as pathlib. - Add a function create_unused_dirs accepting the target paths as a single argument. - Support the simultaneous creation of multiple unused paths with all of them having the same suffix if any of the actually requested paths is unavailable. --- easybuild/tools/filetools.py | 70 ++++++++++ test/framework/filetools.py | 247 +++++++++++++++++++++++++++++++++++ 2 files changed, 317 insertions(+) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index a16e2720d6..bfd1498645 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -3109,3 +3109,73 @@ def create_unused_dir(parent_folder, name): set_gid_sticky_bits(path, recursive=True) return path + + +def get_first_nonexisting_parent_path(path): + """ + Get first directory that does not exist, starting at path and going up. + """ + path = os.path.abspath(path) + + nonexisting_parent = None + while not os.path.exists(path): + nonexisting_parent = path + path = os.path.dirname(path) + + return nonexisting_parent + + +def _delete_directories(paths): + for p in paths: + shutil.rmtree(p, ignore_errors=True) + + +def create_unused_dirs(paths, index_upper_bound=10000): + """ + Create directories with given paths, including the parent directories + When a directory in the same path for any of the path in the path list already exists, then the suffix '_' is + appended for i=0..(index_upper_bound-1) until an index is found so that all required path as unused. All created + directories have the same suffix. + + :param paths: list of directory paths to be created + :param index_upper_bound: maximum index that will be tried before failing + """ + paths = [os.path.abspath(p) for p in paths] + for i_path, path in enumerate(paths): + for i_parent, parent in enumerate(paths): + if i_parent != i_path and is_parent_path(parent, path): + raise EasyBuildError(f"Path '{parent}' is a parent path of '{path}'.") + + first_nonexisting_parent_paths = [get_first_nonexisting_parent_path(p) for p in paths] + + final_paths = paths + all_paths_created = False + number = -1 + while number < index_upper_bound and not all_paths_created: + tried_paths = [] + if number >= 0: + final_paths = [f"{p}_{number}" for p in paths] + try: + for p in final_paths: + tried_paths.append(p) + os.makedirs(p) + all_paths_created = True + except OSError as err: + # Distinguish between error due to existing folder and anything else + if not os.path.exists(tried_paths[-1]): + _delete_directories(tried_paths[0:-1]) + raise EasyBuildError("Failed to create directory %s: %s", tried_paths[-1], err) + _delete_directories(tried_paths[0:-1]) + except BaseException as err: + _delete_directories(tried_paths) + raise err + number += 1 + + if not all_paths_created: + raise EasyBuildError(f"Exceeded maximum number of attempts ({number}) to generate unique directories.") + + # set group ID and sticky bits, if desired + for p in first_nonexisting_parent_paths: + set_gid_sticky_bits(p, recursive=True) + + return final_paths diff --git a/test/framework/filetools.py b/test/framework/filetools.py index 56a4c45809..b232a6becc 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -3743,6 +3743,30 @@ def test_set_gid_sticky_bits(self): self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID) self.assertEqual(dir_perms & stat.S_ISVTX, stat.S_ISVTX) + def test_get_first_nonexisting_parent_path(self): + """Test get_first_nonexisting_parent_path function.""" + root_path = os.path.join(self.test_prefix, 'a') + target_path = os.path.join(self.test_prefix, 'a', 'b', 'c') + os.makedirs(root_path) + first_nonexisting_parent = ft.get_first_nonexisting_parent_path(target_path) + self.assertEqual(first_nonexisting_parent, os.path.join(self.test_prefix, 'a', 'b')) + shutil.rmtree(root_path) + + # Use a reflexive parent relation + root_path = os.path.join(self.test_prefix, 'a', 'b') + target_path = os.path.join(self.test_prefix, 'a', 'b', 'c') + os.makedirs(root_path) + first_nonexisting_parent = ft.get_first_nonexisting_parent_path(target_path) + self.assertEqual(first_nonexisting_parent, os.path.join(self.test_prefix, 'a', 'b', 'c')) + shutil.rmtree(root_path) + + root_path = os.path.join(self.test_prefix, 'a', 'b', 'c') + target_path = os.path.join(self.test_prefix, 'a', 'b', 'c') + os.makedirs(root_path) + first_nonexisting_parent = ft.get_first_nonexisting_parent_path(target_path) + self.assertEqual(first_nonexisting_parent, None) + shutil.rmtree(root_path) + def test_create_unused_dir(self): """Test create_unused_dir function.""" path = ft.create_unused_dir(self.test_prefix, 'folder') @@ -3781,6 +3805,229 @@ def test_create_unused_dir(self): self.assertEqual(path, os.path.join(self.test_prefix, 'file_0')) self.assertExists(path) + def test_create_unused_dirs(self): + """Test create_unused_dirs function.""" + test_root = os.path.join(self.test_prefix, 'test_create_unused_dirs') + + os.mkdir(test_root) + requested_paths = [ + os.path.join(test_root, 'folder_a'), + os.path.join(test_root, 'folder_b'), + ] + paths = ft.create_unused_dirs(requested_paths) + self.assertEqual(paths, requested_paths) + self.assertAllExist(paths) + shutil.rmtree(test_root) + + # Repeat with existing folder(s) should create new ones + os.mkdir(test_root) + requested_paths = [ + os.path.join(test_root, 'folder_a'), + os.path.join(test_root, 'folder_b'), + ] + for p in requested_paths: + os.mkdir(p) + for i in range(10): + paths = ft.create_unused_dirs(requested_paths) + self.assertEqual(paths, [f"{p}_{i}" for p in requested_paths]) + self.assertAllExist(paths) + shutil.rmtree(test_root) + + # Add a suffix in both directories if a suffix already exists + os.mkdir(test_root) + requested_paths = [ + os.path.join(test_root, "existing_a"), + os.path.join(test_root, "existing_b"), + ] + os.mkdir(os.path.join(test_root, "existing_b")) + paths = ft.create_unused_dirs(requested_paths) + self.assertEqual(paths, [f"{p}_0" for p in requested_paths]) + self.assertNotExists(os.path.join(test_root, "existing_a")) + self.assertAllExist(paths) + shutil.rmtree(test_root) + + # Skip suffix if a directory with the suffix already exists + os.mkdir(test_root) + existing_idx = 1 + requested_paths = [ + os.path.join(test_root, "existing_idx_a"), + os.path.join(test_root, "existing_idx_b"), + ] + + os.mkdir(os.path.join(test_root, f"existing_idx_b_{existing_idx}")) + + def expected_idx(n): + if n == 0: + return "" + new_idx = n - 1 + if n > existing_idx: + new_idx += 1 + return f"_{new_idx}" + + for i in range(3): + paths = ft.create_unused_dirs(requested_paths) + self.assertEqual(paths, [p + expected_idx(i) for p in requested_paths]) + self.assertAllExist(paths) + self.assertNotExists(os.path.join(test_root, f"existing_idx_a_{existing_idx}")) + self.assertExists(os.path.join(test_root, f"existing_idx_b_{existing_idx}")) + + shutil.rmtree(test_root) + + # Support creation of parent directories + os.mkdir(test_root) + requested_paths = [os.path.join(test_root, "parent_folder", "folder")] + paths = ft.create_unused_dirs(requested_paths) + self.assertEqual(paths, requested_paths) + self.assertAllExist(paths) + shutil.rmtree(test_root) + + # Not influenced by similar folder + os.mkdir(test_root) + requested_paths = [os.path.join(test_root, "folder_a2")] + paths = ft.create_unused_dirs(requested_paths) + self.assertEqual(paths, requested_paths) + self.assertAllExist(paths) + for i in range(10): + paths = ft.create_unused_dirs(requested_paths) + self.assertEqual(paths, [f"{p}_{i}" for p in requested_paths]) + self.assertAllExist(paths) + shutil.rmtree(test_root) + + # Fail cleanly if passed a readonly folder + os.mkdir(test_root) + readonly_dir = os.path.join(test_root, "ro_folder") + ft.mkdir(readonly_dir) + old_perms = os.lstat(readonly_dir)[stat.ST_MODE] + ft.adjust_permissions(readonly_dir, stat.S_IREAD | stat.S_IEXEC, relative=False) + requested_path = [os.path.join(readonly_dir, "new_folder")] + try: + self.assertErrorRegex(EasyBuildError, "Failed to create directory", + ft.create_unused_dirs, requested_path) + finally: + ft.adjust_permissions(readonly_dir, old_perms, relative=False) + shutil.rmtree(test_root) + + # Fail if the number of attempts to create the directory is exceeded + os.mkdir(test_root) + requested_paths = [os.path.join(test_root, "attempt")] + os.mkdir(os.path.join(test_root, "attempt")) + os.mkdir(os.path.join(test_root, "attempt_0")) + os.mkdir(os.path.join(test_root, "attempt_1")) + os.mkdir(os.path.join(test_root, "attempt_2")) + os.mkdir(os.path.join(test_root, "attempt_3")) + idx_ub = 4 + self.assertErrorRegex( + EasyBuildError, + rf"Exceeded maximum number of attempts \({idx_ub}\) to generate unique directories.", + ft.create_unused_dirs, + requested_paths, index_upper_bound=idx_ub + ) + shutil.rmtree(test_root) + + # Ignore files same as folders. So first just create a file with no contents + os.mkdir(test_root) + requested_path = os.path.join(test_root, "file") + ft.write_file(requested_path, '') + paths = ft.create_unused_dirs([requested_path]) + self.assertEqual(paths, [requested_path + "_0"]) + self.assertAllExist(paths) + shutil.rmtree(test_root) + + # Deny creation of nested directories + requested_paths = [ + os.path.join(test_root, "foo/bar"), + os.path.join(test_root, "foo/bar/baz"), + ] + self.assertErrorRegex( + EasyBuildError, + "Path '.*/foo/bar' is a parent path of '.*/foo/bar/baz'", + ft.create_unused_dirs, + requested_paths + ) + self.assertNotExists(test_root) # Fail early, do not create intermediate directories + + requested_paths = [ + os.path.join(test_root, "foo/bar/baz"), + os.path.join(test_root, "foo/bar"), + ] + self.assertErrorRegex( + EasyBuildError, + "Path '.*/foo/bar' is a parent path of '.*/foo/bar/baz'", + ft.create_unused_dirs, + requested_paths + ) + self.assertNotExists(test_root) # Fail early, do not create intermediate directories + + requested_paths = [ + os.path.join(test_root, "foo/bar"), + os.path.join(test_root, "foo/bar"), + ] + self.assertErrorRegex( + EasyBuildError, + "Path '.*/foo/bar' is a parent path of '.*/foo/bar'", + ft.create_unused_dirs, + requested_paths + ) + self.assertNotExists(test_root) # Fail early, do not create intermediate directories + + # Allow creation of non-nested directories + os.mkdir(test_root) + requested_paths = [ + os.path.join(test_root, "nested/foo/bar"), + os.path.join(test_root, "nested/foo/baz"), + os.path.join(test_root, "nested/buz"), + ] + paths = ft.create_unused_dirs(requested_paths) + self.assertEqual(paths, requested_paths) + self.assertAllExist(paths) + shutil.rmtree(test_root) + + # Test that permissions are set in single directories + os.mkdir(test_root) + init_config(build_options={'set_gid_bit': True, 'sticky_bit': True}) + requested_path = os.path.join(test_root, "directory") + paths = ft.create_unused_dirs([requested_path]) + self.assertEqual(len(paths), 1) + dir_perms = os.lstat(paths[0])[stat.ST_MODE] + self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID) + self.assertEqual(dir_perms & stat.S_ISVTX, stat.S_ISVTX) + init_config(build_options={'set_gid_bit': None, 'sticky_bit': None}) + shutil.rmtree(test_root) + + # Test that permissions are set correctly across a whole path + os.mkdir(test_root) + init_config(build_options={'set_gid_bit': True, 'sticky_bit': True}) + requested_path = os.path.join(test_root, "directory", "subdirectory") + paths = ft.create_unused_dirs([requested_path]) + self.assertEqual(len(paths), 1) + tested_paths = [ + os.path.join(test_root, "directory"), + os.path.join(test_root, "directory", "subdirectory"), + ] + for path in tested_paths: + dir_perms = os.lstat(path)[stat.ST_MODE] + self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID) + self.assertEqual(dir_perms & stat.S_ISVTX, stat.S_ISVTX) + init_config(build_options={'set_gid_bit': None, 'sticky_bit': None}) + shutil.rmtree(test_root) + + # Test that existing directory permissions are not modified + os.mkdir(test_root) + init_config(build_options={'set_gid_bit': True, 'sticky_bit': True}) + requested_path = os.path.join(test_root, "directory", "subdirectory") + existing_parent = os.path.join(test_root, "directory") + os.mkdir(existing_parent) + paths = ft.create_unused_dirs([requested_path]) + self.assertEqual(len(paths), 1) + dir_perms = os.lstat(paths[0])[stat.ST_MODE] + self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID) + self.assertEqual(dir_perms & stat.S_ISVTX, stat.S_ISVTX) + dir_perms = os.lstat(existing_parent)[stat.ST_MODE] + self.assertEqual(dir_perms & stat.S_ISGID, 0) + self.assertEqual(dir_perms & stat.S_ISVTX, 0) + init_config(build_options={'set_gid_bit': None, 'sticky_bit': None}) + shutil.rmtree(test_root) + def suite(): """ returns all the testcases in this module """ From 460fe88d0789920f03176523f1dc0f7b8d3ecb01 Mon Sep 17 00:00:00 2001 From: Georgios Kafanas Date: Sat, 3 Aug 2024 01:48:34 +0200 Subject: [PATCH 04/25] Copy build log and artifacts to permanents locations after a failure Packages are be built in some selected build path (--buildpath), and the logs of successful compilation are then concentrated to some other location for permanent storage (--logfile-format). Logs of failed builds remain in the build path location so that they can be inspected. However, this setup is problematic when building software in HPC jobs. Quite often in HPC systems the build path is set to some fast storage local to the node, like NVME raid mounted on `/tmp` or `/dev/shm` (as suggested in the documentation: https://docs.easybuild.io/configuration/#buildpath). The node storage is often wiped out after the end of a job, so the log files and the artifacts are no longer available after the termination of the job. This commit adds options (--log-error-path and --artifact-error-path) to accumulate error logs and artifacts in some more permanent locations, so that the can be easily inspected after a failed build. By default, neither the logs nor the build artifacts are copied in the permanent location. --- easybuild/framework/easyblock.py | 87 ++++++- easybuild/tools/config.py | 46 ++++ easybuild/tools/options.py | 14 +- test/framework/options.py | 11 +- .../sandbox/sources/toy/toy-0.0_add-bug.patch | 10 + test/framework/toy_build.py | 220 ++++++++++++++++++ 6 files changed, 379 insertions(+), 9 deletions(-) create mode 100644 test/framework/sandbox/sources/toy/toy-0.0_add-bug.patch diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index 1eb610b2c8..9b71f40af1 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -81,6 +81,7 @@ from easybuild.tools.config import PYTHONPATH, SEARCH_PATH_BIN_DIRS, SEARCH_PATH_LIB_DIRS 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 get_log_error_path, get_artifact_error_path from easybuild.tools.environment import restore_env, sanitize_env from easybuild.tools.filetools import CHECKSUM_TYPE_SHA256 from easybuild.tools.filetools import adjust_permissions, apply_patch, back_up_file, change_dir, check_lock @@ -90,6 +91,7 @@ from easybuild.tools.filetools import get_cwd, 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 +from easybuild.tools.filetools import copy_dir, is_parent_path, create_unused_dirs from easybuild.tools.hooks import ( BUILD_STEP, CLEANUP_STEP, CONFIGURE_STEP, EXTENSIONS_STEP, EXTRACT_STEP, FETCH_STEP, INSTALL_STEP, MODULE_STEP, MODULE_WRITE, PACKAGE_STEP, PATCH_STEP, PERMISSIONS_STEP, POSTITER_STEP, POSTPROC_STEP, PREPARE_STEP, READY_STEP, @@ -4422,6 +4424,86 @@ def print_dry_run_note(loc, silent=True): dry_run_msg(msg, silent=silent) +def create_persistence_paths(operation_args): + persistence_paths = [target_path for (_, _, target_path, _) in operation_args] + persistence_paths = create_unused_dirs(persistence_paths) + + for i, (operation, source, _, msg) in enumerate(operation_args): + operation_args[i] = (operation, source, persistence_paths[i], msg) + + return operation_args + + +def execute_and_log_persistence_operation(operation, source_paths, target_dir, msg, silent): + for p in source_paths: + if os.path.exists(p): + operation(p, target_dir) + print_msg(msg, log=_log, silent=silent) + + +def persist_failed_compilation_log_and_artifacts(build_successful, application_log, silent, app, easyconfig): + if not application_log: + return + + # there may be multiple log files, or the file name may be different due to zipping + logs = glob.glob(f"{application_log}*") + print_msg( + "Results of the build can be found in the temporary log file(s): " + ", ".join(logs), + log=_log, + silent=silent + ) + + if build_successful: + return + + datetime_stamp = time.strftime("%Y%m%d") + '-' + time.strftime("%H%M%S") + operation_args = [] + + log_error_path = get_log_error_path(easyconfig) + if log_error_path: + log_error_path = os.path.join(log_error_path, datetime_stamp) + + if is_parent_path(app.builddir, log_error_path): + print_warning( + "Persistent log directory is subdirectory of build directory; not copying logs.", + log=_log, + silent=silent + ) + else: + operation_args.append( + ( + copy_file, + logs, + log_error_path, + f"Logs of failed build copied to permanent storage: {log_error_path}" + ) + ) + + artifact_error_path = get_artifact_error_path(easyconfig) + if artifact_error_path and os.path.isdir(app.builddir): + artifact_error_path = os.path.join(artifact_error_path, datetime_stamp) + + if is_parent_path(app.builddir, artifact_error_path): + print_warning( + "Persistent artifact directory is subdirectory of build directory; not copying artifacts.", + log=_log, + silent=silent + ) + else: + operation_args.append( + ( + lambda source, destination: copy_dir(source, destination, dirs_exist_ok=True), + [app.builddir], + artifact_error_path, + f"Artifacts of failed build copied to permanent storage: {artifact_error_path}" + ) + ) + + operation_args = create_persistence_paths(operation_args) + for args in operation_args: + execute_and_log_persistence_operation(*args, silent=silent) + + def build_and_install_one(ecdict, init_env): """ Build the software @@ -4675,10 +4757,7 @@ 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) + persist_failed_compilation_log_and_artifacts(success, application_log, silent, app, ecdict['ec']) del app diff --git a/easybuild/tools/config.py b/easybuild/tools/config.py index ecce1a7f0d..ebae57ad66 100644 --- a/easybuild/tools/config.py +++ b/easybuild/tools/config.py @@ -105,9 +105,11 @@ DEFAULT_MODULE_SYNTAX = 'Lua' DEFAULT_MODULES_TOOL = 'Lmod' DEFAULT_PATH_SUBDIRS = { + 'artifact_error_path': 'error_artifacts', 'buildpath': 'build', 'containerpath': 'containers', 'installpath': '', + 'log_error_path': 'error_logs', 'packagepath': 'packages', 'repositorypath': 'ebfiles_repo', 'sourcepath': 'sources', @@ -490,6 +492,7 @@ class ConfigurationVariables(BaseConfigurationVariables): # list of known/required keys REQUIRED = [ + 'artifact_error_path', 'buildpath', 'config', 'containerpath', @@ -497,6 +500,7 @@ class ConfigurationVariables(BaseConfigurationVariables): 'installpath_modules', 'installpath_software', 'job_backend', + 'log_error_path', 'logfile_format', 'moduleclasses', 'module_naming_scheme', @@ -865,6 +869,48 @@ def log_path(ec=None): return log_file_format(return_directory=True, ec=ec, date=date, timestamp=timestamp) +def get_log_error_path(ec): + """ + Return the 'log_error_path', the location where logs are copied in case of failure + + :param ec: dict-like value with 'name' and 'version' keys defined + """ + log_error_path = ConfigurationVariables()['log_error_path'] + + if not log_error_path: + return None + + try: + name, version = ec['name'], ec['version'] + except KeyError: + raise EasyBuildError("The 'name' and 'version' keys are required.") + + path = os.path.join(log_error_path, name + '-' + version) + + return path + + +def get_artifact_error_path(ec): + """ + Return the 'artifact_error_path', the location where build directories are copied in case of failure + + :param ec: dict-like value with 'name' and 'version' keys defined + """ + artifact_error_path = ConfigurationVariables()['artifact_error_path'] + + if not artifact_error_path: + return None + + try: + name, version = ec['name'], ec['version'] + except KeyError: + raise EasyBuildError("The 'name' and 'version' keys are required.") + + path = os.path.join(artifact_error_path, name + '-' + version) + + return path + + def get_build_log_path(): """ Return (temporary) directory for build log diff --git a/easybuild/tools/options.py b/easybuild/tools/options.py index 9c4ad3c734..0a9babe955 100644 --- a/easybuild/tools/options.py +++ b/easybuild/tools/options.py @@ -576,6 +576,9 @@ def config_options(self): descr = ("Configuration options", "Configure EasyBuild behavior.") opts = OrderedDict({ + 'artifact-error-path': ("Location where artifacts are copied in case of an error; " + "an empty value disables copying the logs.", + None, 'store', None, {'metavar': "PATH"}), 'avail-module-naming-schemes': ("Show all supported module naming schemes", None, 'store_true', False,), 'avail-modules-tools': ("Show all supported module tools", @@ -606,6 +609,9 @@ def config_options(self): None, 'store', None), 'job-backend': ("Backend to use for submitting jobs", 'choice', 'store', DEFAULT_JOB_BACKEND, sorted(avail_job_backends().keys())), + 'log-error-path': ("Location where logs are copied in case of an error; " + "an empty value disables copying the logs.", + None, 'store', None, {'metavar': "PATH"}), # purposely take a copy for the default logfile format 'logfile-format': ("Directory name and format of the log file", 'strtuple', 'store', DEFAULT_LOGFILE_FORMAT[:], {'metavar': 'DIR,FORMAT'}), @@ -1218,8 +1224,8 @@ def _postprocess_config(self): # - the could also specify the location of a *remote* (Git( repository, # which can be done in variety of formats (git@:/), https://, etc.) # (see also https://github.com/easybuilders/easybuild-framework/issues/3892); - path_opt_names = ['buildpath', 'containerpath', 'git_working_dirs_path', 'installpath', - 'installpath_modules', 'installpath_software', 'prefix', 'packagepath', + path_opt_names = ['artifact_error_path', 'buildpath', 'containerpath', 'git_working_dirs_path', 'installpath', + 'installpath_modules', 'installpath_software', 'log_error_path', 'prefix', 'packagepath', 'robot_paths', 'sourcepath'] for opt_name in path_opt_names: @@ -1228,8 +1234,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 = ['artifact_error_path', 'buildpath', 'containerpath', 'installpath', 'log_error_path', + 'packagepath', 'repository', 'repositorypath', 'sourcepath'] for dest in path_opts: if not self.options._action_taken.get(dest, False): if dest == 'repository': diff --git a/test/framework/options.py b/test/framework/options.py index 0721380630..df0c29e247 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -5413,7 +5413,16 @@ def test_prefix_option(self): regex = re.compile(r"(?P\S*).*%s.*" % self.test_prefix, re.M) - expected = ['buildpath', 'containerpath', 'installpath', 'packagepath', 'prefix', 'repositorypath'] + expected = [ + 'artifact-error-path', + 'buildpath', + 'containerpath', + 'installpath', + 'log-error-path', + 'packagepath', + 'prefix', + 'repositorypath', + ] self.assertEqual(sorted(regex.findall(txt)), expected) def test_dump_env_script(self): diff --git a/test/framework/sandbox/sources/toy/toy-0.0_add-bug.patch b/test/framework/sandbox/sources/toy/toy-0.0_add-bug.patch new file mode 100644 index 0000000000..7512447168 --- /dev/null +++ b/test/framework/sandbox/sources/toy/toy-0.0_add-bug.patch @@ -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; + } diff --git a/test/framework/toy_build.py b/test/framework/toy_build.py index cd9aefb26f..4d142edf24 100644 --- a/test/framework/toy_build.py +++ b/test/framework/toy_build.py @@ -40,6 +40,8 @@ import sys import tempfile import textwrap +import pathlib +import filecmp from easybuild.tools import LooseVersion from importlib import reload from test.framework.utilities import EnhancedTestCase, TestLoaderFiltered, cleanup @@ -158,6 +160,7 @@ def check_toy(self, installpath, outtxt, name='toy', version='0.0', versionprefi self.assertExists(devel_module_path) def _test_toy_build(self, extra_args=None, ec_file=None, tmpdir=None, verify=True, fails=False, verbose=True, + tmp_logdir=None, log_error_dir=None, artifact_error_dir=None, raise_error=False, test_report=None, name='toy', versionsuffix='', testing=True, raise_systemexit=False, force=True, test_report_regexs=None, debug=True): """Perform a toy build.""" @@ -181,6 +184,12 @@ def _test_toy_build(self, extra_args=None, ec_file=None, tmpdir=None, verify=Tru 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) + if log_error_dir is not None: + args.append('--log-error-path=%s' % log_error_dir) + if artifact_error_dir is not None: + args.append('--artifact-error-path=%s' % artifact_error_dir) args.extend(extra_args) myerr = None try: @@ -281,6 +290,217 @@ def test_toy_broken(self): # cleanup shutil.rmtree(tmpdir) + def detect_log_file(self, tmp_log_path): + log_files = list(pathlib.Path(tmp_log_path).glob("**/*.log")) + + self.assertTrue(len(log_files) >= 1, "Log files generated") + log_file = log_files[0] + self.assertTrue(len(log_files) == 1, f"Single log file expected: {log_files}") + + return log_file + + def assert_build_files_copied(self, buildpath, artifact_error_path): + build_dir = pathlib.Path(buildpath) + storage_dir = pathlib.Path(artifact_error_path) + + app_build_dir = build_dir / "toy/0.0/system-system" + iso_date_pattern = "????????-??????" + + for file in app_build_dir.glob("**/*"): + file_relative_path = file.relative_to(app_build_dir) + file_copies = list(storage_dir.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 successfully" + self.assertTrue(filecmp.cmp(str(file), str(file_copy), shallow=False), msg) + + def assert_log_files_copied(self, log_file, log_error_path): + file_name = log_file.name + saved_log_files = list(pathlib.Path(log_error_path).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 successfully" + 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) + self.assertRegex(outtxt, regex) + + def check_errorlog(self, output_regexs, outtxt, tmp_logpath, buildpath, log_error_path, artifact_error_path): + log_file = self.detect_log_file(tmp_logpath) + + self.assert_log_files_copied(log_file, log_error_path) + self.assert_build_files_copied(buildpath, artifact_error_path) + 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.""" + with tempfile.TemporaryDirectory() as base_tmp_dir: + base_tmp_path = pathlib.Path(base_tmp_dir) + + tmpdir = base_tmp_path / 'tmp' + tmp_log_dir = base_tmp_path / 'log_dir' + tmp_log_error_dir = base_tmp_path / 'log_error_dir' + tmp_artifact_error_dir = base_tmp_path / 'artifact_error_dir' + tmp_easyconfig_dir = base_tmp_path / 'easyconfig_dir' + + base_ec = os.path.join( + os.path.dirname(__file__), + 'easyconfigs', 'test_ecs', 't', 'toy', 'toy-0.0.eb') + + base_ec_txt = read_file(base_ec) + broken_compilation_ec_txt = re.sub( + r'toy-0\.0_fix-silly-typo-in-printf-statement\.patch', + r'toy-0.0_add-bug.patch', + base_ec_txt) + broken_compilation_ec = os.path.join(tmp_easyconfig_dir, 'toy-0.0-buggy.eb') + write_file(broken_compilation_ec, broken_compilation_ec_txt) + + self.mock_stderr(True) + self.mock_stdout(True) + outtxt = self._test_toy_build( + ec_file=broken_compilation_ec, tmpdir=tmpdir, + verify=False, fails=True, verbose=False, raise_error=False, name='toy', versionsuffix='-buggy', + tmp_logdir=tmp_log_dir, log_error_dir=tmp_log_error_dir, artifact_error_dir=tmp_artifact_error_dir + ) + self.mock_stderr(False) + self.mock_stdout(False) + + output_regexs = [r"^\s*toy\.c:5:44: error: expected (;|.;.) before"] + + self.check_errorlog(output_regexs, outtxt, tmp_log_dir, + self.test_buildpath, tmp_log_error_dir, tmp_artifact_error_dir) + + def assert_persistence_not_overwriting_build_directory(self, outtxt, patterns_presence): + for pattern_presence in patterns_presence: + regex_pattern, present = pattern_presence + regex = re.compile(regex_pattern, re.M) + msg_stdout = "Pattern '%s' found in full test report:\n%s" % (regex.pattern, outtxt) + if present: + self.assertTrue(regex.search(outtxt), msg_stdout) + else: + self.assertFalse(regex.search(outtxt), msg_stdout) + + def test_toy_persistence_copying_restrictions(self): + """ + Test that EasyBuild refuses to move logs and artifacts inside the build directory in case of failed + compilations. + + Moving log files or artifacts inside the build directory modifies the build artifacts, and in the case of + build is also copying directories into themselves. + """ + with tempfile.TemporaryDirectory() as base_tmp_dir: + base_tmp_path = pathlib.Path(base_tmp_dir) + + tmpdir = base_tmp_path / 'tmp' + tmp_log_dir = base_tmp_path / 'log_dir' + tmp_easyconfig_dir = base_tmp_path / 'easyconfig_dir' + + base_ec = os.path.join( + os.path.dirname(__file__), + 'easyconfigs', 'test_ecs', 't', 'toy', 'toy-0.0.eb') + + base_ec_txt = read_file(base_ec) + broken_compilation_ec_txt = re.sub( + r'toy-0\.0_fix-silly-typo-in-printf-statement\.patch', + r'toy-0.0_add-bug.patch', + base_ec_txt) + broken_compilation_ec = os.path.join(tmp_easyconfig_dir, 'toy-0.0-buggy.eb') + write_file(broken_compilation_ec, broken_compilation_ec_txt) + + def check_overwriting_of_build_directory(tmp_log_error_dir, + tmp_artifact_error_dir, + copied_log_error_dir, + copied_artifact_error_dir): + self.mock_stderr(True) + self.mock_stdout(True) + + outtxt = self.test_toy_build( + ec_file=broken_compilation_ec, tmpdir=tmpdir, + tmp_logdir=tmp_log_dir, log_error_dir=tmp_log_error_dir, artifact_error_dir=tmp_artifact_error_dir, + verify=False, fails=True, verbose=False, raise_error=False, + name='toy', versionsuffix='-buggy') + + self.assert_persistence_not_overwriting_build_directory( + outtxt=outtxt, + patterns_presence=[ + ( + "Logs of failed build copied to permanent storage", + copied_log_error_dir + ), + ( + "Artifacts of failed build copied to permanent storage", + copied_artifact_error_dir + ), + ( + "Persistent log directory is subdirectory of build directory; not copying logs.", + not copied_log_error_dir + ), + ( + "Persistent artifact directory is subdirectory of build directory; not copying artifacts.", + not copied_artifact_error_dir + ), + ] + ) + + shutil.rmtree(tmpdir) + shutil.rmtree(tmp_log_dir) + shutil.rmtree(self.test_buildpath) + if copied_log_error_dir: + shutil.rmtree(tmp_log_error_dir) + if copied_artifact_error_dir: + shutil.rmtree(tmp_artifact_error_dir) + + self.mock_stderr(False) + self.mock_stdout(False) + + old_buildpath = self.test_buildpath + self.test_buildpath = str(base_tmp_path / 'build_dir') + + log_path_outside_build_directory = os.path.join(str(base_tmp_path), 'log_error_dir') + log_path_inside_build_directory = os.path.join(self.test_buildpath, + 'toy', '0.0', 'system-system', 'log_error_dir') + artifact_path_outside_build_directory = os.path.join(str(base_tmp_path), 'artifact_error_dir') + artifact_path_inside_build_directory = os.path.join(self.test_buildpath, + 'toy', '0.0', 'system-system', 'artifact_error_dir') + + check_overwriting_of_build_directory( + tmp_log_error_dir=log_path_outside_build_directory, + tmp_artifact_error_dir=artifact_path_outside_build_directory, + copied_log_error_dir=True, + copied_artifact_error_dir=True + ) + + check_overwriting_of_build_directory( + tmp_log_error_dir=log_path_inside_build_directory, + tmp_artifact_error_dir=artifact_path_outside_build_directory, + copied_log_error_dir=False, + copied_artifact_error_dir=True + ) + + check_overwriting_of_build_directory( + tmp_log_error_dir=log_path_outside_build_directory, + tmp_artifact_error_dir=artifact_path_inside_build_directory, + copied_log_error_dir=True, + copied_artifact_error_dir=False + ) + + check_overwriting_of_build_directory( + tmp_log_error_dir=log_path_inside_build_directory, + tmp_artifact_error_dir=artifact_path_inside_build_directory, + copied_log_error_dir=False, + copied_artifact_error_dir=False + ) + + self.test_buildpath = old_buildpath + 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') From 4750fcac7fce3739dfeea34fdc72c10dae7b2b8e Mon Sep 17 00:00:00 2001 From: Georgios Kafanas Date: Fri, 24 Jan 2025 01:54:17 +0100 Subject: [PATCH 05/25] Enforce persistence copying restrictions during input option validation Flags for persisting logs and artifacts are now checked during the validation of input options. In case of invalid settings the program terminates early with an error. Integration tests for internal logging functions are deactivated as the logging functions are never reached in normal operation. --- easybuild/tools/options.py | 17 ++++- test/framework/options.py | 46 ++++++++++++++ test/framework/toy_build.py | 123 ------------------------------------ 3 files changed, 62 insertions(+), 124 deletions(-) diff --git a/easybuild/tools/options.py b/easybuild/tools/options.py index 0a9babe955..f3f77c0bac 100644 --- a/easybuild/tools/options.py +++ b/easybuild/tools/options.py @@ -86,7 +86,7 @@ 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, expand_glob_paths, get_cwd -from easybuild.tools.filetools import install_fake_vsc, move_file, which +from easybuild.tools.filetools import install_fake_vsc, move_file, which, is_parent_path from easybuild.tools.github import GITHUB_PR_DIRECTION_DESC, GITHUB_PR_ORDER_CREATED from easybuild.tools.github import GITHUB_PR_STATE_OPEN, GITHUB_PR_STATES, GITHUB_PR_ORDERS, GITHUB_PR_DIRECTIONS from easybuild.tools.github import HAVE_GITHUB_API, HAVE_KEYRING, VALID_CLOSE_PR_REASONS @@ -1294,6 +1294,21 @@ def _postprocess_config(self): if self.options.inject_checksums or self.options.inject_checksums_to_json: self.options.pre_create_installdir = False + # Prevent permanent storage of logs and artifacts from modifying the build directory + if self.options.buildpath and self.options.log_error_path: + if is_parent_path(self.options.buildpath, self.options.log_error_path): + raise EasyBuildError( + "The --log-error-path ('%s') cannot reside on a subdirectory of the --buildpath ('%s')" + % (self.options.log_error_path, self.options.buildpath) + ) + + if self.options.buildpath and self.options.artifact_error_path: + if is_parent_path(self.options.buildpath, self.options.artifact_error_path): + raise EasyBuildError( + "The --artifact-error-path ('%s') cannot reside on a subdirectory of the --buildpath ('%s')" + % (self.options.log_error_path, self.options.buildpath) + ) + def _postprocess_list_avail(self): """Create all the additional info that can be requested (exit at the end)""" msg = '' diff --git a/test/framework/options.py b/test/framework/options.py index df0c29e247..f2ee6e966e 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1630,6 +1630,52 @@ def test_dry_run(self): regex = re.compile(r" \* \[%s\] \S+%s \(module: %s\)" % (mark, ec, mod), re.M) self.assertTrue(regex.search(logtxt), "Found match for pattern %s in '%s'" % (regex.pattern, logtxt)) + def test_persistence_copying_restrictions(self): + """ + Test that EasyBuild fails when instructed to move logs or artifacts inside the build directory + + Moving log files or artifacts inside the build directory modifies the build artifacts, and in the case of + build artifacts it is also copying directories into themselves. + """ + base_args = [ + 'gzip-1.4-GCC-4.6.3.eb', + '--dry-run', + '--robot', + ] + + def test_eb_with(option_flag, is_valid): + with tempfile.TemporaryDirectory() as root_dir: + build_dir = os.path.join(root_dir, 'build_dir') + if is_valid: + persist_path = os.path.join(root_dir, 'persist_dir') + else: + persist_path = os.path.join(root_dir, 'build_dir', 'persist_dir') + + extra_args = [ + f"--buildpath={build_dir}", + f"{option_flag}={persist_path}", + ] + + pattern = rf"The {option_flag} \(.*\) cannot reside on a subdirectory of the --buildpath \(.*\)" + + args = base_args + args.extend(extra_args) + + if is_valid: + try: + self.eb_main(args, raise_error=True) + except EasyBuildError: + self.fail( + "Should not fail with --buildpath='{build_dir}' and {option_flag}='{persist_path}'." + ) + else: + self.assertErrorRegex(EasyBuildError, pattern, self.eb_main, args, raise_error=True) + + test_eb_with(option_flag='--log-error-path', is_valid=True) + test_eb_with(option_flag='--log-error-path', is_valid=False) + test_eb_with(option_flag='--artifact-error-path', is_valid=True) + test_eb_with(option_flag='--artifact-error-path', is_valid=False) + def test_missing(self): """Test use of --missing/-M.""" diff --git a/test/framework/toy_build.py b/test/framework/toy_build.py index 4d142edf24..ff0905f2d8 100644 --- a/test/framework/toy_build.py +++ b/test/framework/toy_build.py @@ -378,129 +378,6 @@ def test_toy_broken_compilation(self): self.check_errorlog(output_regexs, outtxt, tmp_log_dir, self.test_buildpath, tmp_log_error_dir, tmp_artifact_error_dir) - def assert_persistence_not_overwriting_build_directory(self, outtxt, patterns_presence): - for pattern_presence in patterns_presence: - regex_pattern, present = pattern_presence - regex = re.compile(regex_pattern, re.M) - msg_stdout = "Pattern '%s' found in full test report:\n%s" % (regex.pattern, outtxt) - if present: - self.assertTrue(regex.search(outtxt), msg_stdout) - else: - self.assertFalse(regex.search(outtxt), msg_stdout) - - def test_toy_persistence_copying_restrictions(self): - """ - Test that EasyBuild refuses to move logs and artifacts inside the build directory in case of failed - compilations. - - Moving log files or artifacts inside the build directory modifies the build artifacts, and in the case of - build is also copying directories into themselves. - """ - with tempfile.TemporaryDirectory() as base_tmp_dir: - base_tmp_path = pathlib.Path(base_tmp_dir) - - tmpdir = base_tmp_path / 'tmp' - tmp_log_dir = base_tmp_path / 'log_dir' - tmp_easyconfig_dir = base_tmp_path / 'easyconfig_dir' - - base_ec = os.path.join( - os.path.dirname(__file__), - 'easyconfigs', 'test_ecs', 't', 'toy', 'toy-0.0.eb') - - base_ec_txt = read_file(base_ec) - broken_compilation_ec_txt = re.sub( - r'toy-0\.0_fix-silly-typo-in-printf-statement\.patch', - r'toy-0.0_add-bug.patch', - base_ec_txt) - broken_compilation_ec = os.path.join(tmp_easyconfig_dir, 'toy-0.0-buggy.eb') - write_file(broken_compilation_ec, broken_compilation_ec_txt) - - def check_overwriting_of_build_directory(tmp_log_error_dir, - tmp_artifact_error_dir, - copied_log_error_dir, - copied_artifact_error_dir): - self.mock_stderr(True) - self.mock_stdout(True) - - outtxt = self.test_toy_build( - ec_file=broken_compilation_ec, tmpdir=tmpdir, - tmp_logdir=tmp_log_dir, log_error_dir=tmp_log_error_dir, artifact_error_dir=tmp_artifact_error_dir, - verify=False, fails=True, verbose=False, raise_error=False, - name='toy', versionsuffix='-buggy') - - self.assert_persistence_not_overwriting_build_directory( - outtxt=outtxt, - patterns_presence=[ - ( - "Logs of failed build copied to permanent storage", - copied_log_error_dir - ), - ( - "Artifacts of failed build copied to permanent storage", - copied_artifact_error_dir - ), - ( - "Persistent log directory is subdirectory of build directory; not copying logs.", - not copied_log_error_dir - ), - ( - "Persistent artifact directory is subdirectory of build directory; not copying artifacts.", - not copied_artifact_error_dir - ), - ] - ) - - shutil.rmtree(tmpdir) - shutil.rmtree(tmp_log_dir) - shutil.rmtree(self.test_buildpath) - if copied_log_error_dir: - shutil.rmtree(tmp_log_error_dir) - if copied_artifact_error_dir: - shutil.rmtree(tmp_artifact_error_dir) - - self.mock_stderr(False) - self.mock_stdout(False) - - old_buildpath = self.test_buildpath - self.test_buildpath = str(base_tmp_path / 'build_dir') - - log_path_outside_build_directory = os.path.join(str(base_tmp_path), 'log_error_dir') - log_path_inside_build_directory = os.path.join(self.test_buildpath, - 'toy', '0.0', 'system-system', 'log_error_dir') - artifact_path_outside_build_directory = os.path.join(str(base_tmp_path), 'artifact_error_dir') - artifact_path_inside_build_directory = os.path.join(self.test_buildpath, - 'toy', '0.0', 'system-system', 'artifact_error_dir') - - check_overwriting_of_build_directory( - tmp_log_error_dir=log_path_outside_build_directory, - tmp_artifact_error_dir=artifact_path_outside_build_directory, - copied_log_error_dir=True, - copied_artifact_error_dir=True - ) - - check_overwriting_of_build_directory( - tmp_log_error_dir=log_path_inside_build_directory, - tmp_artifact_error_dir=artifact_path_outside_build_directory, - copied_log_error_dir=False, - copied_artifact_error_dir=True - ) - - check_overwriting_of_build_directory( - tmp_log_error_dir=log_path_outside_build_directory, - tmp_artifact_error_dir=artifact_path_inside_build_directory, - copied_log_error_dir=True, - copied_artifact_error_dir=False - ) - - check_overwriting_of_build_directory( - tmp_log_error_dir=log_path_inside_build_directory, - tmp_artifact_error_dir=artifact_path_inside_build_directory, - copied_log_error_dir=False, - copied_artifact_error_dir=False - ) - - self.test_buildpath = old_buildpath - 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') From d52d74485470058ba5e502a778d562ceb78b9c5d Mon Sep 17 00:00:00 2001 From: Georgios Kafanas Date: Thu, 30 Jan 2025 10:59:41 +0100 Subject: [PATCH 06/25] [STYLE] Follow style conventions in variable names and functions calls - Avoid single character variables names outside list comprehension. - Use a consistent pattern in function calls throughout the pull request. - Use context manager to mock stdout and stderr. --- easybuild/base/testing.py | 4 ++-- easybuild/framework/easyblock.py | 6 +++--- easybuild/tools/filetools.py | 8 +++---- test/framework/filetools.py | 36 +++++++++++++++++--------------- test/framework/toy_build.py | 27 ++++++++++++------------ 5 files changed, 42 insertions(+), 39 deletions(-) diff --git a/easybuild/base/testing.py b/easybuild/base/testing.py index 297cd01ccd..509f59e63e 100644 --- a/easybuild/base/testing.py +++ b/easybuild/base/testing.py @@ -119,8 +119,8 @@ def assertNotExists(self, path, msg=None): def assertAllExist(self, paths, msg=None): """Assert that all paths in the given list exist""" - for p in paths: - self.assertExists(p, msg) + for path in paths: + self.assertExists(path, msg) def setUp(self): """Prepare test case.""" diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index 9b71f40af1..4c85b859ef 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -4435,9 +4435,9 @@ def create_persistence_paths(operation_args): def execute_and_log_persistence_operation(operation, source_paths, target_dir, msg, silent): - for p in source_paths: - if os.path.exists(p): - operation(p, target_dir) + for path in source_paths: + if os.path.exists(path): + operation(path, target_dir) print_msg(msg, log=_log, silent=silent) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index bfd1498645..9cc76dc197 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -3126,8 +3126,8 @@ def get_first_nonexisting_parent_path(path): def _delete_directories(paths): - for p in paths: - shutil.rmtree(p, ignore_errors=True) + for path in paths: + shutil.rmtree(path, ignore_errors=True) def create_unused_dirs(paths, index_upper_bound=10000): @@ -3175,7 +3175,7 @@ def create_unused_dirs(paths, index_upper_bound=10000): raise EasyBuildError(f"Exceeded maximum number of attempts ({number}) to generate unique directories.") # set group ID and sticky bits, if desired - for p in first_nonexisting_parent_paths: - set_gid_sticky_bits(p, recursive=True) + for path in first_nonexisting_parent_paths: + set_gid_sticky_bits(path, recursive=True) return final_paths diff --git a/test/framework/filetools.py b/test/framework/filetools.py index b232a6becc..aca647a020 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -3825,8 +3825,8 @@ def test_create_unused_dirs(self): os.path.join(test_root, 'folder_a'), os.path.join(test_root, 'folder_b'), ] - for p in requested_paths: - os.mkdir(p) + for path in requested_paths: + os.mkdir(path) for i in range(10): paths = ft.create_unused_dirs(requested_paths) self.assertEqual(paths, [f"{p}_{i}" for p in requested_paths]) @@ -3848,28 +3848,28 @@ def test_create_unused_dirs(self): # Skip suffix if a directory with the suffix already exists os.mkdir(test_root) - existing_idx = 1 + existing_suffix = 1 requested_paths = [ - os.path.join(test_root, "existing_idx_a"), - os.path.join(test_root, "existing_idx_b"), + os.path.join(test_root, "existing_suffix_a"), + os.path.join(test_root, "existing_suffix_b"), ] - os.mkdir(os.path.join(test_root, f"existing_idx_b_{existing_idx}")) + os.mkdir(os.path.join(test_root, f"existing_suffix_b_{existing_suffix}")) - def expected_idx(n): - if n == 0: + def expected_suffix(n_calls_to_create_unused_dirs): + if n_calls_to_create_unused_dirs == 0: return "" - new_idx = n - 1 - if n > existing_idx: - new_idx += 1 - return f"_{new_idx}" + new_suffix = n_calls_to_create_unused_dirs - 1 + if n_calls_to_create_unused_dirs > existing_suffix: + new_suffix += 1 + return f"_{new_suffix}" for i in range(3): paths = ft.create_unused_dirs(requested_paths) - self.assertEqual(paths, [p + expected_idx(i) for p in requested_paths]) + self.assertEqual(paths, [p + expected_suffix(i) for p in requested_paths]) self.assertAllExist(paths) - self.assertNotExists(os.path.join(test_root, f"existing_idx_a_{existing_idx}")) - self.assertExists(os.path.join(test_root, f"existing_idx_b_{existing_idx}")) + self.assertNotExists(os.path.join(test_root, f"existing_suffix_a_{existing_suffix}")) + self.assertExists(os.path.join(test_root, f"existing_suffix_b_{existing_suffix}")) shutil.rmtree(test_root) @@ -3901,8 +3901,10 @@ def expected_idx(n): ft.adjust_permissions(readonly_dir, stat.S_IREAD | stat.S_IEXEC, relative=False) requested_path = [os.path.join(readonly_dir, "new_folder")] try: - self.assertErrorRegex(EasyBuildError, "Failed to create directory", - ft.create_unused_dirs, requested_path) + self.assertErrorRegex( + EasyBuildError, "Failed to create directory", + ft.create_unused_dirs, requested_path + ) finally: ft.adjust_permissions(readonly_dir, old_perms, relative=False) shutil.rmtree(test_root) diff --git a/test/framework/toy_build.py b/test/framework/toy_build.py index ff0905f2d8..a3e90d96b9 100644 --- a/test/framework/toy_build.py +++ b/test/framework/toy_build.py @@ -353,30 +353,31 @@ def test_toy_broken_compilation(self): base_ec = os.path.join( os.path.dirname(__file__), - 'easyconfigs', 'test_ecs', 't', 'toy', 'toy-0.0.eb') + 'easyconfigs', 'test_ecs', 't', 'toy', 'toy-0.0.eb' + ) base_ec_txt = read_file(base_ec) broken_compilation_ec_txt = re.sub( r'toy-0\.0_fix-silly-typo-in-printf-statement\.patch', r'toy-0.0_add-bug.patch', - base_ec_txt) + base_ec_txt + ) broken_compilation_ec = os.path.join(tmp_easyconfig_dir, 'toy-0.0-buggy.eb') write_file(broken_compilation_ec, broken_compilation_ec_txt) - self.mock_stderr(True) - self.mock_stdout(True) - outtxt = self._test_toy_build( - ec_file=broken_compilation_ec, tmpdir=tmpdir, - verify=False, fails=True, verbose=False, raise_error=False, name='toy', versionsuffix='-buggy', - tmp_logdir=tmp_log_dir, log_error_dir=tmp_log_error_dir, artifact_error_dir=tmp_artifact_error_dir - ) - self.mock_stderr(False) - self.mock_stdout(False) + with self.mocked_stdout_stderr(): + outtxt = self._test_toy_build( + ec_file=broken_compilation_ec, tmpdir=tmpdir, + verify=False, fails=True, verbose=False, raise_error=False, name='toy', versionsuffix='-buggy', + tmp_logdir=tmp_log_dir, log_error_dir=tmp_log_error_dir, artifact_error_dir=tmp_artifact_error_dir + ) output_regexs = [r"^\s*toy\.c:5:44: error: expected (;|.;.) before"] - self.check_errorlog(output_regexs, outtxt, tmp_log_dir, - self.test_buildpath, tmp_log_error_dir, tmp_artifact_error_dir) + self.check_errorlog( + output_regexs, outtxt, tmp_log_dir, + self.test_buildpath, tmp_log_error_dir, tmp_artifact_error_dir + ) def test_toy_tweaked(self): """Test toy build with tweaked easyconfig, for testing extra easyconfig parameters.""" From ea723ddfd9988e97d8749016d26849631448820c Mon Sep 17 00:00:00 2001 From: Georgios Kafanas Date: Fri, 7 Feb 2025 14:26:13 +0100 Subject: [PATCH 07/25] Mention individual files copied during error artifact persistence When copying logs and artifacts from failed builds, mention the full target path instead of its parent. This reduces the number of actions required to access the artifacts (select, paste, and enter). --- easybuild/framework/easyblock.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index 4c85b859ef..b7a7de8d21 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -4436,8 +4436,8 @@ def create_persistence_paths(operation_args): def execute_and_log_persistence_operation(operation, source_paths, target_dir, msg, silent): for path in source_paths: - if os.path.exists(path): - operation(path, target_dir) + operation(path, target_dir) + print_msg(msg, log=_log, silent=silent) @@ -4470,12 +4470,13 @@ def persist_failed_compilation_log_and_artifacts(build_successful, application_l silent=silent ) else: + log_file_copies = [os.path.join(log_error_path, os.path.basename(log)) for log in logs] operation_args.append( ( copy_file, logs, log_error_path, - f"Logs of failed build copied to permanent storage: {log_error_path}" + "Logs of failed build copied to permanent storage: " + ', '.join(log_file_copies) ) ) From 71b761cc15cf891f6845d47b73ce56871088bc98 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 1 Mar 2025 19:37:52 +0100 Subject: [PATCH 08/25] simplify test for copying of logs/build dirs for failing installations --- test/framework/toy_build.py | 145 +++++++++++++++--------------------- 1 file changed, 61 insertions(+), 84 deletions(-) diff --git a/test/framework/toy_build.py b/test/framework/toy_build.py index a3e90d96b9..7958876a15 100644 --- a/test/framework/toy_build.py +++ b/test/framework/toy_build.py @@ -40,7 +40,6 @@ import sys import tempfile import textwrap -import pathlib import filecmp from easybuild.tools import LooseVersion from importlib import reload @@ -160,7 +159,6 @@ def check_toy(self, installpath, outtxt, name='toy', version='0.0', versionprefi self.assertExists(devel_module_path) def _test_toy_build(self, extra_args=None, ec_file=None, tmpdir=None, verify=True, fails=False, verbose=True, - tmp_logdir=None, log_error_dir=None, artifact_error_dir=None, raise_error=False, test_report=None, name='toy', versionsuffix='', testing=True, raise_systemexit=False, force=True, test_report_regexs=None, debug=True): """Perform a toy build.""" @@ -184,12 +182,6 @@ def _test_toy_build(self, extra_args=None, ec_file=None, tmpdir=None, verify=Tru 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) - if log_error_dir is not None: - args.append('--log-error-path=%s' % log_error_dir) - if artifact_error_dir is not None: - args.append('--artifact-error-path=%s' % artifact_error_dir) args.extend(extra_args) myerr = None try: @@ -290,94 +282,79 @@ def test_toy_broken(self): # cleanup shutil.rmtree(tmpdir) - def detect_log_file(self, tmp_log_path): - log_files = list(pathlib.Path(tmp_log_path).glob("**/*.log")) + def test_toy_broken_copy_log_build_dir(self): + """ + Test whether log files and the build directory are copied to a permanent location + after a failed installation. + """ + toy_ec = os.path.join(os.path.dirname(__file__), 'easyconfigs', 'test_ecs', 't', 'toy', 'toy-0.0.eb') + toy_ec_txt = read_file(toy_ec) + + test_ec_txt = re.sub( + r'toy-0\.0_fix-silly-typo-in-printf-statement\.patch', + r'toy-0.0_add-bug.patch', + toy_ec_txt + ) + test_ec = os.path.join(self.test_prefix, 'toy-0.0-buggy.eb') + write_file(test_ec, test_ec_txt) + + # set up subdirectories where stuff should go + tmpdir = os.path.join(self.test_prefix, 'tmp') + tmp_log_dir = os.path.join(self.test_prefix, 'tmp-logs') + tmp_log_error_dir = os.path.join(self.test_prefix, 'logs-failures') + build_dirs_failures_path = os.path.join(self.test_prefix, 'build-dirs-failures') + + extra_args = [ + f'--artifact-error-path={build_dirs_failures_path}', + f'--log-error-path={tmp_log_error_dir}', + f'--tmp-logdir={tmp_log_dir}', + ] + with self.mocked_stdout_stderr(): + outtxt = self._test_toy_build(ec_file=test_ec, extra_args=extra_args, tmpdir=tmpdir, + verify=False, fails=True, verbose=False) - self.assertTrue(len(log_files) >= 1, "Log files generated") + # find path to temporary log file + log_files = glob.glob(os.path.join(tmp_log_dir, '*.log')) + self.assertTrue(len(log_files) == 1, f"Expected exactly one log file, found {len(log_files)}: {log_files}") log_file = log_files[0] - self.assertTrue(len(log_files) == 1, f"Single log file expected: {log_files}") - return log_file + # check that log files were copied + saved_log_files = glob.glob(os.path.join(tmp_log_error_dir, log_file)) + self.assertTrue(len(saved_log_files) == 1, f"Unique copy of log file '{log_file}' made") + saved_log_file = saved_log_files[0] + self.assertTrue(filecmp.cmp(log_file, saved_log_file, shallow=False), + f"Log file '{log_file}' copied successfully") - def assert_build_files_copied(self, buildpath, artifact_error_path): - build_dir = pathlib.Path(buildpath) - storage_dir = pathlib.Path(artifact_error_path) + # check that build directories were copied + build_dir = self.test_buildpath + topdir = build_dirs_failures_path - app_build_dir = build_dir / "toy/0.0/system-system" - iso_date_pattern = "????????-??????" + app_build_dir = os.path.join(build_dir, 'toy', '0.0', 'system-system') + subdir_pattern = '????????-??????' - for file in app_build_dir.glob("**/*"): - file_relative_path = file.relative_to(app_build_dir) - file_copies = list(storage_dir.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] + # find path to toy.c + toy_c_files = glob.glob(os.path.join(app_build_dir, '**', 'toy.c')) + self.assertTrue(len(toy_c_files) == 1, f"Exactly one toy.c file found: {toy_c_files}") + toy_c_file = toy_c_files[0] - if file_copy.is_file(): - msg = f"File '{file}' copied successfully" - self.assertTrue(filecmp.cmp(str(file), str(file_copy), shallow=False), msg) + res = glob.glob(os.path.join(topdir, 'toy-0.0', subdir_pattern, 'toy-0.0', os.path.basename(toy_c_file))) + self.assertTrue(len(res) == 1, f"Exactly one build dir found in {app_build_dir}: {res}") + copied_toy_c_file = res[0] + self.assertTrue(filecmp.cmp(toy_c_file, copied_toy_c_file, shallow=False), + f"Copy of {toy_c_file} should be found under {topdir}") - def assert_log_files_copied(self, log_file, log_error_path): - file_name = log_file.name - saved_log_files = list(pathlib.Path(log_error_path).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 successfully" - self.assertTrue(filecmp.cmp(str(log_file), str(saved_log_file), shallow=False), msg) + # check whether compiler error messages are present in build log - def assert_error_reported(self, outtxt, output_regexs): + # compiler error because of missing semicolon at end of line, could be: + # "error: expected ; before ..." + # "error: expected ';' after expression" + output_regexs = [r"^\s*toy\.c:5:44: error: expected (;|.;.)"] + + log_txt = read_file(log_file) for regex_pattern in output_regexs: regex = re.compile(regex_pattern, re.M) self.assertRegex(outtxt, regex) - - def check_errorlog(self, output_regexs, outtxt, tmp_logpath, buildpath, log_error_path, artifact_error_path): - log_file = self.detect_log_file(tmp_logpath) - - self.assert_log_files_copied(log_file, log_error_path) - self.assert_build_files_copied(buildpath, artifact_error_path) - 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.""" - with tempfile.TemporaryDirectory() as base_tmp_dir: - base_tmp_path = pathlib.Path(base_tmp_dir) - - tmpdir = base_tmp_path / 'tmp' - tmp_log_dir = base_tmp_path / 'log_dir' - tmp_log_error_dir = base_tmp_path / 'log_error_dir' - tmp_artifact_error_dir = base_tmp_path / 'artifact_error_dir' - tmp_easyconfig_dir = base_tmp_path / 'easyconfig_dir' - - base_ec = os.path.join( - os.path.dirname(__file__), - 'easyconfigs', 'test_ecs', 't', 'toy', 'toy-0.0.eb' - ) - - base_ec_txt = read_file(base_ec) - broken_compilation_ec_txt = re.sub( - r'toy-0\.0_fix-silly-typo-in-printf-statement\.patch', - r'toy-0.0_add-bug.patch', - base_ec_txt - ) - broken_compilation_ec = os.path.join(tmp_easyconfig_dir, 'toy-0.0-buggy.eb') - write_file(broken_compilation_ec, broken_compilation_ec_txt) - - with self.mocked_stdout_stderr(): - outtxt = self._test_toy_build( - ec_file=broken_compilation_ec, tmpdir=tmpdir, - verify=False, fails=True, verbose=False, raise_error=False, name='toy', versionsuffix='-buggy', - tmp_logdir=tmp_log_dir, log_error_dir=tmp_log_error_dir, artifact_error_dir=tmp_artifact_error_dir - ) - - output_regexs = [r"^\s*toy\.c:5:44: error: expected (;|.;.) before"] - - self.check_errorlog( - output_regexs, outtxt, tmp_log_dir, - self.test_buildpath, tmp_log_error_dir, tmp_artifact_error_dir - ) + self.assertRegex(log_txt, regex) def test_toy_tweaked(self): """Test toy build with tweaked easyconfig, for testing extra easyconfig parameters.""" From b2c2193a67bbd32bb8ce90b2ce72e180f0d20b5a Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 1 Mar 2025 19:55:54 +0100 Subject: [PATCH 09/25] use mkdir & remove_dir functions in additional filetools tests --- test/framework/filetools.py | 156 ++++++++++++++++++------------------ 1 file changed, 79 insertions(+), 77 deletions(-) diff --git a/test/framework/filetools.py b/test/framework/filetools.py index aca647a020..8561fcacd4 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -498,9 +498,9 @@ def test_is_parent_path(self): self.assertTrue(ft.is_parent_path('foo/../baz', 'bar/../baz')) # Check that symbolic links are accounted - os.mkdir(os.path.join(self.test_prefix, 'base')) - os.mkdir(os.path.join(self.test_prefix, 'base', 'concrete')) - os.symlink( + ft.mkdir(os.path.join(self.test_prefix, 'base')) + ft.mkdir(os.path.join(self.test_prefix, 'base', 'concrete')) + ft.symlink( os.path.join(self.test_prefix, 'base', 'concrete'), os.path.join(self.test_prefix, 'base', 'link') ) @@ -510,7 +510,6 @@ def test_is_parent_path(self): os.path.join(self.test_prefix, 'base', 'concrete', 'file') ) ) - shutil.rmtree(os.path.join(self.test_prefix, 'base')) def test_det_file_size(self): """Test det_file_size function.""" @@ -3747,25 +3746,25 @@ def test_get_first_nonexisting_parent_path(self): """Test get_first_nonexisting_parent_path function.""" root_path = os.path.join(self.test_prefix, 'a') target_path = os.path.join(self.test_prefix, 'a', 'b', 'c') - os.makedirs(root_path) + ft.mkdir(root_path, parents=True) first_nonexisting_parent = ft.get_first_nonexisting_parent_path(target_path) self.assertEqual(first_nonexisting_parent, os.path.join(self.test_prefix, 'a', 'b')) - shutil.rmtree(root_path) + ft.remove_dir(root_path) # Use a reflexive parent relation root_path = os.path.join(self.test_prefix, 'a', 'b') target_path = os.path.join(self.test_prefix, 'a', 'b', 'c') - os.makedirs(root_path) + ft.mkdir(root_path, parents=True) first_nonexisting_parent = ft.get_first_nonexisting_parent_path(target_path) self.assertEqual(first_nonexisting_parent, os.path.join(self.test_prefix, 'a', 'b', 'c')) - shutil.rmtree(root_path) + ft.remove_dir(root_path) root_path = os.path.join(self.test_prefix, 'a', 'b', 'c') target_path = os.path.join(self.test_prefix, 'a', 'b', 'c') - os.makedirs(root_path) + ft.mkdir(root_path, parents=True) first_nonexisting_parent = ft.get_first_nonexisting_parent_path(target_path) self.assertEqual(first_nonexisting_parent, None) - shutil.rmtree(root_path) + ft.remove_dir(root_path) def test_create_unused_dir(self): """Test create_unused_dir function.""" @@ -3809,7 +3808,7 @@ def test_create_unused_dirs(self): """Test create_unused_dirs function.""" test_root = os.path.join(self.test_prefix, 'test_create_unused_dirs') - os.mkdir(test_root) + ft.mkdir(test_root) requested_paths = [ os.path.join(test_root, 'folder_a'), os.path.join(test_root, 'folder_b'), @@ -3817,44 +3816,44 @@ def test_create_unused_dirs(self): paths = ft.create_unused_dirs(requested_paths) self.assertEqual(paths, requested_paths) self.assertAllExist(paths) - shutil.rmtree(test_root) + ft.remove_dir(test_root) # Repeat with existing folder(s) should create new ones - os.mkdir(test_root) + ft.mkdir(test_root) requested_paths = [ os.path.join(test_root, 'folder_a'), os.path.join(test_root, 'folder_b'), ] for path in requested_paths: - os.mkdir(path) + ft.mkdir(path) for i in range(10): paths = ft.create_unused_dirs(requested_paths) - self.assertEqual(paths, [f"{p}_{i}" for p in requested_paths]) + self.assertEqual(paths, [f'{p}_{i}' for p in requested_paths]) self.assertAllExist(paths) - shutil.rmtree(test_root) + ft.remove_dir(test_root) # Add a suffix in both directories if a suffix already exists - os.mkdir(test_root) + ft.mkdir(test_root) requested_paths = [ - os.path.join(test_root, "existing_a"), - os.path.join(test_root, "existing_b"), + os.path.join(test_root, 'existing_a'), + os.path.join(test_root, 'existing_b'), ] - os.mkdir(os.path.join(test_root, "existing_b")) + ft.mkdir(os.path.join(test_root, 'existing_b')) paths = ft.create_unused_dirs(requested_paths) - self.assertEqual(paths, [f"{p}_0" for p in requested_paths]) - self.assertNotExists(os.path.join(test_root, "existing_a")) + self.assertEqual(paths, [f'{p}_0' for p in requested_paths]) + self.assertNotExists(os.path.join(test_root, 'existing_a')) self.assertAllExist(paths) - shutil.rmtree(test_root) + ft.remove_dir(test_root) # Skip suffix if a directory with the suffix already exists - os.mkdir(test_root) + ft.mkdir(test_root) existing_suffix = 1 requested_paths = [ - os.path.join(test_root, "existing_suffix_a"), - os.path.join(test_root, "existing_suffix_b"), + os.path.join(test_root, 'existing_suffix_a'), + os.path.join(test_root, 'existing_suffix_b'), ] - os.mkdir(os.path.join(test_root, f"existing_suffix_b_{existing_suffix}")) + ft.mkdir(os.path.join(test_root, f'existing_suffix_b_{existing_suffix}')) def expected_suffix(n_calls_to_create_unused_dirs): if n_calls_to_create_unused_dirs == 0: @@ -3868,38 +3867,38 @@ def expected_suffix(n_calls_to_create_unused_dirs): paths = ft.create_unused_dirs(requested_paths) self.assertEqual(paths, [p + expected_suffix(i) for p in requested_paths]) self.assertAllExist(paths) - self.assertNotExists(os.path.join(test_root, f"existing_suffix_a_{existing_suffix}")) - self.assertExists(os.path.join(test_root, f"existing_suffix_b_{existing_suffix}")) + self.assertNotExists(os.path.join(test_root, f'existing_suffix_a_{existing_suffix}')) + self.assertExists(os.path.join(test_root, f'existing_suffix_b_{existing_suffix}')) - shutil.rmtree(test_root) + ft.remove_dir(test_root) # Support creation of parent directories - os.mkdir(test_root) - requested_paths = [os.path.join(test_root, "parent_folder", "folder")] + ft.mkdir(test_root) + requested_paths = [os.path.join(test_root, 'parent_folder', 'folder')] paths = ft.create_unused_dirs(requested_paths) self.assertEqual(paths, requested_paths) self.assertAllExist(paths) - shutil.rmtree(test_root) + ft.remove_dir(test_root) # Not influenced by similar folder - os.mkdir(test_root) - requested_paths = [os.path.join(test_root, "folder_a2")] + ft.mkdir(test_root) + requested_paths = [os.path.join(test_root, 'folder_a2')] paths = ft.create_unused_dirs(requested_paths) self.assertEqual(paths, requested_paths) self.assertAllExist(paths) for i in range(10): paths = ft.create_unused_dirs(requested_paths) - self.assertEqual(paths, [f"{p}_{i}" for p in requested_paths]) + self.assertEqual(paths, [f'{p}_{i}' for p in requested_paths]) self.assertAllExist(paths) - shutil.rmtree(test_root) + ft.remove_dir(test_root) # Fail cleanly if passed a readonly folder - os.mkdir(test_root) - readonly_dir = os.path.join(test_root, "ro_folder") + ft.mkdir(test_root) + readonly_dir = os.path.join(test_root, 'ro_folder') ft.mkdir(readonly_dir) old_perms = os.lstat(readonly_dir)[stat.ST_MODE] ft.adjust_permissions(readonly_dir, stat.S_IREAD | stat.S_IEXEC, relative=False) - requested_path = [os.path.join(readonly_dir, "new_folder")] + requested_path = [os.path.join(readonly_dir, 'new_folder')] try: self.assertErrorRegex( EasyBuildError, "Failed to create directory", @@ -3907,16 +3906,16 @@ def expected_suffix(n_calls_to_create_unused_dirs): ) finally: ft.adjust_permissions(readonly_dir, old_perms, relative=False) - shutil.rmtree(test_root) + ft.remove_dir(test_root) # Fail if the number of attempts to create the directory is exceeded - os.mkdir(test_root) - requested_paths = [os.path.join(test_root, "attempt")] - os.mkdir(os.path.join(test_root, "attempt")) - os.mkdir(os.path.join(test_root, "attempt_0")) - os.mkdir(os.path.join(test_root, "attempt_1")) - os.mkdir(os.path.join(test_root, "attempt_2")) - os.mkdir(os.path.join(test_root, "attempt_3")) + ft.mkdir(test_root) + requested_paths = [os.path.join(test_root, 'attempt')] + ft.mkdir(os.path.join(test_root, 'attempt')) + ft.mkdir(os.path.join(test_root, 'attempt_0')) + ft.mkdir(os.path.join(test_root, 'attempt_1')) + ft.mkdir(os.path.join(test_root, 'attempt_2')) + ft.mkdir(os.path.join(test_root, 'attempt_3')) idx_ub = 4 self.assertErrorRegex( EasyBuildError, @@ -3924,21 +3923,21 @@ def expected_suffix(n_calls_to_create_unused_dirs): ft.create_unused_dirs, requested_paths, index_upper_bound=idx_ub ) - shutil.rmtree(test_root) + ft.remove_dir(test_root) # Ignore files same as folders. So first just create a file with no contents - os.mkdir(test_root) - requested_path = os.path.join(test_root, "file") + ft.mkdir(test_root) + requested_path = os.path.join(test_root, 'file') ft.write_file(requested_path, '') paths = ft.create_unused_dirs([requested_path]) - self.assertEqual(paths, [requested_path + "_0"]) + self.assertEqual(paths, [requested_path + '_0']) self.assertAllExist(paths) - shutil.rmtree(test_root) + ft.remove_dir(test_root) # Deny creation of nested directories requested_paths = [ - os.path.join(test_root, "foo/bar"), - os.path.join(test_root, "foo/bar/baz"), + os.path.join(test_root, 'foo/bar'), + os.path.join(test_root, 'foo/bar/baz'), ] self.assertErrorRegex( EasyBuildError, @@ -3949,8 +3948,8 @@ def expected_suffix(n_calls_to_create_unused_dirs): self.assertNotExists(test_root) # Fail early, do not create intermediate directories requested_paths = [ - os.path.join(test_root, "foo/bar/baz"), - os.path.join(test_root, "foo/bar"), + os.path.join(test_root, 'foo/bar/baz'), + os.path.join(test_root, 'foo/bar'), ] self.assertErrorRegex( EasyBuildError, @@ -3961,8 +3960,8 @@ def expected_suffix(n_calls_to_create_unused_dirs): self.assertNotExists(test_root) # Fail early, do not create intermediate directories requested_paths = [ - os.path.join(test_root, "foo/bar"), - os.path.join(test_root, "foo/bar"), + os.path.join(test_root, 'foo/bar'), + os.path.join(test_root, 'foo/bar'), ] self.assertErrorRegex( EasyBuildError, @@ -3973,62 +3972,65 @@ def expected_suffix(n_calls_to_create_unused_dirs): self.assertNotExists(test_root) # Fail early, do not create intermediate directories # Allow creation of non-nested directories - os.mkdir(test_root) + ft.mkdir(test_root) requested_paths = [ - os.path.join(test_root, "nested/foo/bar"), - os.path.join(test_root, "nested/foo/baz"), - os.path.join(test_root, "nested/buz"), + os.path.join(test_root, 'nested/foo/bar'), + os.path.join(test_root, 'nested/foo/baz'), + os.path.join(test_root, 'nested/buz'), ] paths = ft.create_unused_dirs(requested_paths) self.assertEqual(paths, requested_paths) self.assertAllExist(paths) - shutil.rmtree(test_root) + ft.remove_dir(test_root) # Test that permissions are set in single directories - os.mkdir(test_root) + ft.mkdir(test_root) init_config(build_options={'set_gid_bit': True, 'sticky_bit': True}) - requested_path = os.path.join(test_root, "directory") + requested_path = os.path.join(test_root, 'directory') paths = ft.create_unused_dirs([requested_path]) self.assertEqual(len(paths), 1) dir_perms = os.lstat(paths[0])[stat.ST_MODE] self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID) self.assertEqual(dir_perms & stat.S_ISVTX, stat.S_ISVTX) init_config(build_options={'set_gid_bit': None, 'sticky_bit': None}) - shutil.rmtree(test_root) + ft.remove_dir(test_root) # Test that permissions are set correctly across a whole path - os.mkdir(test_root) + ft.mkdir(test_root) init_config(build_options={'set_gid_bit': True, 'sticky_bit': True}) - requested_path = os.path.join(test_root, "directory", "subdirectory") + requested_path = os.path.join(test_root, 'directory', 'subdirectory') paths = ft.create_unused_dirs([requested_path]) self.assertEqual(len(paths), 1) tested_paths = [ - os.path.join(test_root, "directory"), - os.path.join(test_root, "directory", "subdirectory"), + os.path.join(test_root, 'directory'), + os.path.join(test_root, 'directory', 'subdirectory'), ] for path in tested_paths: dir_perms = os.lstat(path)[stat.ST_MODE] self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID) self.assertEqual(dir_perms & stat.S_ISVTX, stat.S_ISVTX) init_config(build_options={'set_gid_bit': None, 'sticky_bit': None}) - shutil.rmtree(test_root) + ft.remove_dir(test_root) # Test that existing directory permissions are not modified - os.mkdir(test_root) + ft.mkdir(test_root) init_config(build_options={'set_gid_bit': True, 'sticky_bit': True}) - requested_path = os.path.join(test_root, "directory", "subdirectory") - existing_parent = os.path.join(test_root, "directory") - os.mkdir(existing_parent) + existing_parent = os.path.join(test_root, 'directory') + requested_path = os.path.join(existing_parent, 'subdirectory') + + ft.mkdir(existing_parent, set_gid=False, sticky=False) paths = ft.create_unused_dirs([requested_path]) self.assertEqual(len(paths), 1) + dir_perms = os.lstat(paths[0])[stat.ST_MODE] self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID) self.assertEqual(dir_perms & stat.S_ISVTX, stat.S_ISVTX) + dir_perms = os.lstat(existing_parent)[stat.ST_MODE] self.assertEqual(dir_perms & stat.S_ISGID, 0) self.assertEqual(dir_perms & stat.S_ISVTX, 0) init_config(build_options={'set_gid_bit': None, 'sticky_bit': None}) - shutil.rmtree(test_root) + ft.remove_dir(test_root) def suite(): From 7baef326921a93ab2aa239a95a27468f34a9e537 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 1 Mar 2025 19:59:29 +0100 Subject: [PATCH 10/25] minor tweaks in is_parent_path function --- easybuild/tools/filetools.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index 9cc76dc197..50b1c7a758 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -603,17 +603,17 @@ def normalize_path(path): return start_slashes + os.path.sep.join(filtered_comps) -def is_parent_path(ancestor, path): +def is_parent_path(path1, path2): """ - Return true if ancestor is prefix of path + Return True if path1 is a prefix of path2 - :param ancestor: absolute or relative path - :param path: absolute or relative path + :param path1: absolute or relative path + :param path2: absolute or relative path """ - ancestor = os.path.realpath(ancestor) - path = os.path.realpath(path) - common_path = os.path.commonprefix([ancestor, path]) - return common_path == ancestor + path1 = os.path.realpath(path1) + path2 = os.path.realpath(path2) + common_path = os.path.commonprefix([path1, path2]) + return common_path == path1 def is_alt_pypi_url(url): From f291d8c13c2dc4422eebe8354f24e7e10b622d3a Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 1 Mar 2025 20:01:42 +0100 Subject: [PATCH 11/25] enhance test for is_parent_path --- test/framework/filetools.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/framework/filetools.py b/test/framework/filetools.py index 8561fcacd4..4e8cfc0393 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -481,6 +481,7 @@ def test_is_parent_path(self): self.assertTrue(ft.is_parent_path('/foo/bar', '/foo/bar/test0')) self.assertTrue(ft.is_parent_path('/foo/bar', '/foo/bar/test0/test1')) self.assertTrue(ft.is_parent_path('/foo/bar', '/foo/bar')) + self.assertFalse(ft.is_parent_path('/foo/bar/test0', '/foo/bar')) self.assertFalse(ft.is_parent_path('/foo/bar', '/foo/test')) # Check that trailing slashes are ignored @@ -492,6 +493,7 @@ def test_is_parent_path(self): self.assertTrue(ft.is_parent_path('foo/bar', 'foo/bar/test0')) self.assertTrue(ft.is_parent_path('foo/bar', 'foo/bar/test0/test1')) self.assertTrue(ft.is_parent_path('foo/bar', 'foo/bar')) + self.assertFalse(ft.is_parent_path('foo/bar/test0', 'foo/bar')) self.assertFalse(ft.is_parent_path('foo/bar', 'foo/test')) # Check that relative paths are accounted From b0a10a12b20b50c8ffd5d1e4c18bc7c81bac7940 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 1 Mar 2025 20:38:51 +0100 Subject: [PATCH 12/25] rename create_unused_dirs to create_non_existing_paths, rename get_first_nonexisting_parent_path to get_first_non_existing_parent_path, minor code cleanup --- easybuild/framework/easyblock.py | 4 +- easybuild/tools/filetools.py | 64 +++++++++++------------- test/framework/filetools.py | 86 ++++++++++++++++---------------- 3 files changed, 75 insertions(+), 79 deletions(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index b7a7de8d21..a9084de0dd 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -91,7 +91,7 @@ from easybuild.tools.filetools import get_cwd, 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 -from easybuild.tools.filetools import copy_dir, is_parent_path, create_unused_dirs +from easybuild.tools.filetools import copy_dir, create_non_existing_paths, is_parent_path from easybuild.tools.hooks import ( BUILD_STEP, CLEANUP_STEP, CONFIGURE_STEP, EXTENSIONS_STEP, EXTRACT_STEP, FETCH_STEP, INSTALL_STEP, MODULE_STEP, MODULE_WRITE, PACKAGE_STEP, PATCH_STEP, PERMISSIONS_STEP, POSTITER_STEP, POSTPROC_STEP, PREPARE_STEP, READY_STEP, @@ -4426,7 +4426,7 @@ def print_dry_run_note(loc, silent=True): def create_persistence_paths(operation_args): persistence_paths = [target_path for (_, _, target_path, _) in operation_args] - persistence_paths = create_unused_dirs(persistence_paths) + persistence_paths = create_non_existing_paths(persistence_paths) for i, (operation, source, _, msg) in enumerate(operation_args): operation_args[i] = (operation, source, persistence_paths[i], msg) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index 50b1c7a758..88859f8b59 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -3111,71 +3111,67 @@ def create_unused_dir(parent_folder, name): return path -def get_first_nonexisting_parent_path(path): +def get_first_non_existing_parent_path(path): """ Get first directory that does not exist, starting at path and going up. """ path = os.path.abspath(path) - nonexisting_parent = None + non_existing_parent = None while not os.path.exists(path): - nonexisting_parent = path + non_existing_parent = path path = os.path.dirname(path) - return nonexisting_parent + return non_existing_parent -def _delete_directories(paths): - for path in paths: - shutil.rmtree(path, ignore_errors=True) - - -def create_unused_dirs(paths, index_upper_bound=10000): +def create_non_existing_paths(paths, max_tries=10000): """ - Create directories with given paths, including the parent directories - When a directory in the same path for any of the path in the path list already exists, then the suffix '_' is - appended for i=0..(index_upper_bound-1) until an index is found so that all required path as unused. All created - directories have the same suffix. + Create directories with given paths (including the parent directories). + When a directory in the same location for any of the specified paths already exists, + then the suffix '_' is appended , with i iteratively picked between 0 and (max_tries-1), + until an index is found so that all required paths are non-existing. + All created directories have the same suffix. :param paths: list of directory paths to be created - :param index_upper_bound: maximum index that will be tried before failing + :param max_tries: maximum number of tries before failing """ paths = [os.path.abspath(p) for p in paths] - for i_path, path in enumerate(paths): - for i_parent, parent in enumerate(paths): - if i_parent != i_path and is_parent_path(parent, path): + for idx_path, path in enumerate(paths): + for idx_parent, parent in enumerate(paths): + if idx_parent != idx_path and is_parent_path(parent, path): raise EasyBuildError(f"Path '{parent}' is a parent path of '{path}'.") - first_nonexisting_parent_paths = [get_first_nonexisting_parent_path(p) for p in paths] + first_non_existing_parent_paths = [get_first_non_existing_parent_path(p) for p in paths] - final_paths = paths + non_existing_paths = paths all_paths_created = False - number = -1 - while number < index_upper_bound and not all_paths_created: + suffix = -1 + while suffix < max_tries and not all_paths_created: tried_paths = [] - if number >= 0: - final_paths = [f"{p}_{number}" for p in paths] + if suffix >= 0: + non_existing_paths = [f'{p}_{suffix}' for p in paths] try: - for p in final_paths: - tried_paths.append(p) - os.makedirs(p) + for path in non_existing_paths: + tried_paths.append(path) + # os.makedirs will raise OSError if directory already exists + os.makedirs(path) all_paths_created = True except OSError as err: # Distinguish between error due to existing folder and anything else if not os.path.exists(tried_paths[-1]): - _delete_directories(tried_paths[0:-1]) raise EasyBuildError("Failed to create directory %s: %s", tried_paths[-1], err) - _delete_directories(tried_paths[0:-1]) + remove(tried_paths[:-1]) except BaseException as err: - _delete_directories(tried_paths) + remove(tried_paths) raise err - number += 1 + suffix += 1 if not all_paths_created: - raise EasyBuildError(f"Exceeded maximum number of attempts ({number}) to generate unique directories.") + raise EasyBuildError(f"Exceeded maximum number of attempts ({max_tries}) to generate non-existing paths") # set group ID and sticky bits, if desired - for path in first_nonexisting_parent_paths: + for path in first_non_existing_parent_paths: set_gid_sticky_bits(path, recursive=True) - return final_paths + return non_existing_paths diff --git a/test/framework/filetools.py b/test/framework/filetools.py index 4e8cfc0393..492d8fce85 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -3744,28 +3744,28 @@ def test_set_gid_sticky_bits(self): self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID) self.assertEqual(dir_perms & stat.S_ISVTX, stat.S_ISVTX) - def test_get_first_nonexisting_parent_path(self): - """Test get_first_nonexisting_parent_path function.""" + def test_get_first_non_existing_parent_path(self): + """Test get_first_non_existing_parent_path function.""" root_path = os.path.join(self.test_prefix, 'a') target_path = os.path.join(self.test_prefix, 'a', 'b', 'c') ft.mkdir(root_path, parents=True) - first_nonexisting_parent = ft.get_first_nonexisting_parent_path(target_path) - self.assertEqual(first_nonexisting_parent, os.path.join(self.test_prefix, 'a', 'b')) + first_non_existing_parent = ft.get_first_non_existing_parent_path(target_path) + self.assertEqual(first_non_existing_parent, os.path.join(self.test_prefix, 'a', 'b')) ft.remove_dir(root_path) # Use a reflexive parent relation root_path = os.path.join(self.test_prefix, 'a', 'b') target_path = os.path.join(self.test_prefix, 'a', 'b', 'c') ft.mkdir(root_path, parents=True) - first_nonexisting_parent = ft.get_first_nonexisting_parent_path(target_path) - self.assertEqual(first_nonexisting_parent, os.path.join(self.test_prefix, 'a', 'b', 'c')) + first_non_existing_parent = ft.get_first_non_existing_parent_path(target_path) + self.assertEqual(first_non_existing_parent, os.path.join(self.test_prefix, 'a', 'b', 'c')) ft.remove_dir(root_path) root_path = os.path.join(self.test_prefix, 'a', 'b', 'c') target_path = os.path.join(self.test_prefix, 'a', 'b', 'c') ft.mkdir(root_path, parents=True) - first_nonexisting_parent = ft.get_first_nonexisting_parent_path(target_path) - self.assertEqual(first_nonexisting_parent, None) + first_non_existing_parent = ft.get_first_non_existing_parent_path(target_path) + self.assertEqual(first_non_existing_parent, None) ft.remove_dir(root_path) def test_create_unused_dir(self): @@ -3806,16 +3806,16 @@ def test_create_unused_dir(self): self.assertEqual(path, os.path.join(self.test_prefix, 'file_0')) self.assertExists(path) - def test_create_unused_dirs(self): - """Test create_unused_dirs function.""" - test_root = os.path.join(self.test_prefix, 'test_create_unused_dirs') + def test_create_non_existing_paths(self): + """Test create_non_existing_paths function.""" + test_root = os.path.join(self.test_prefix, 'test_create_non_existing_paths') ft.mkdir(test_root) requested_paths = [ os.path.join(test_root, 'folder_a'), os.path.join(test_root, 'folder_b'), ] - paths = ft.create_unused_dirs(requested_paths) + paths = ft.create_non_existing_paths(requested_paths) self.assertEqual(paths, requested_paths) self.assertAllExist(paths) ft.remove_dir(test_root) @@ -3829,7 +3829,7 @@ def test_create_unused_dirs(self): for path in requested_paths: ft.mkdir(path) for i in range(10): - paths = ft.create_unused_dirs(requested_paths) + paths = ft.create_non_existing_paths(requested_paths) self.assertEqual(paths, [f'{p}_{i}' for p in requested_paths]) self.assertAllExist(paths) ft.remove_dir(test_root) @@ -3841,7 +3841,7 @@ def test_create_unused_dirs(self): os.path.join(test_root, 'existing_b'), ] ft.mkdir(os.path.join(test_root, 'existing_b')) - paths = ft.create_unused_dirs(requested_paths) + paths = ft.create_non_existing_paths(requested_paths) self.assertEqual(paths, [f'{p}_0' for p in requested_paths]) self.assertNotExists(os.path.join(test_root, 'existing_a')) self.assertAllExist(paths) @@ -3857,16 +3857,16 @@ def test_create_unused_dirs(self): ft.mkdir(os.path.join(test_root, f'existing_suffix_b_{existing_suffix}')) - def expected_suffix(n_calls_to_create_unused_dirs): - if n_calls_to_create_unused_dirs == 0: + def expected_suffix(n_calls_to_create_non_existing_paths): + if n_calls_to_create_non_existing_paths == 0: return "" - new_suffix = n_calls_to_create_unused_dirs - 1 - if n_calls_to_create_unused_dirs > existing_suffix: + new_suffix = n_calls_to_create_non_existing_paths - 1 + if n_calls_to_create_non_existing_paths > existing_suffix: new_suffix += 1 return f"_{new_suffix}" for i in range(3): - paths = ft.create_unused_dirs(requested_paths) + paths = ft.create_non_existing_paths(requested_paths) self.assertEqual(paths, [p + expected_suffix(i) for p in requested_paths]) self.assertAllExist(paths) self.assertNotExists(os.path.join(test_root, f'existing_suffix_a_{existing_suffix}')) @@ -3877,7 +3877,7 @@ def expected_suffix(n_calls_to_create_unused_dirs): # Support creation of parent directories ft.mkdir(test_root) requested_paths = [os.path.join(test_root, 'parent_folder', 'folder')] - paths = ft.create_unused_dirs(requested_paths) + paths = ft.create_non_existing_paths(requested_paths) self.assertEqual(paths, requested_paths) self.assertAllExist(paths) ft.remove_dir(test_root) @@ -3885,11 +3885,11 @@ def expected_suffix(n_calls_to_create_unused_dirs): # Not influenced by similar folder ft.mkdir(test_root) requested_paths = [os.path.join(test_root, 'folder_a2')] - paths = ft.create_unused_dirs(requested_paths) + paths = ft.create_non_existing_paths(requested_paths) self.assertEqual(paths, requested_paths) self.assertAllExist(paths) for i in range(10): - paths = ft.create_unused_dirs(requested_paths) + paths = ft.create_non_existing_paths(requested_paths) self.assertEqual(paths, [f'{p}_{i}' for p in requested_paths]) self.assertAllExist(paths) ft.remove_dir(test_root) @@ -3904,7 +3904,7 @@ def expected_suffix(n_calls_to_create_unused_dirs): try: self.assertErrorRegex( EasyBuildError, "Failed to create directory", - ft.create_unused_dirs, requested_path + ft.create_non_existing_paths, requested_path ) finally: ft.adjust_permissions(readonly_dir, old_perms, relative=False) @@ -3918,12 +3918,12 @@ def expected_suffix(n_calls_to_create_unused_dirs): ft.mkdir(os.path.join(test_root, 'attempt_1')) ft.mkdir(os.path.join(test_root, 'attempt_2')) ft.mkdir(os.path.join(test_root, 'attempt_3')) - idx_ub = 4 + max_tries = 4 self.assertErrorRegex( EasyBuildError, - rf"Exceeded maximum number of attempts \({idx_ub}\) to generate unique directories.", - ft.create_unused_dirs, - requested_paths, index_upper_bound=idx_ub + rf"Exceeded maximum number of attempts \({max_tries}\) to generate non-existing paths", + ft.create_non_existing_paths, + requested_paths, max_tries=max_tries ) ft.remove_dir(test_root) @@ -3931,7 +3931,7 @@ def expected_suffix(n_calls_to_create_unused_dirs): ft.mkdir(test_root) requested_path = os.path.join(test_root, 'file') ft.write_file(requested_path, '') - paths = ft.create_unused_dirs([requested_path]) + paths = ft.create_non_existing_paths([requested_path]) self.assertEqual(paths, [requested_path + '_0']) self.assertAllExist(paths) ft.remove_dir(test_root) @@ -3944,7 +3944,7 @@ def expected_suffix(n_calls_to_create_unused_dirs): self.assertErrorRegex( EasyBuildError, "Path '.*/foo/bar' is a parent path of '.*/foo/bar/baz'", - ft.create_unused_dirs, + ft.create_non_existing_paths, requested_paths ) self.assertNotExists(test_root) # Fail early, do not create intermediate directories @@ -3956,7 +3956,7 @@ def expected_suffix(n_calls_to_create_unused_dirs): self.assertErrorRegex( EasyBuildError, "Path '.*/foo/bar' is a parent path of '.*/foo/bar/baz'", - ft.create_unused_dirs, + ft.create_non_existing_paths, requested_paths ) self.assertNotExists(test_root) # Fail early, do not create intermediate directories @@ -3968,7 +3968,7 @@ def expected_suffix(n_calls_to_create_unused_dirs): self.assertErrorRegex( EasyBuildError, "Path '.*/foo/bar' is a parent path of '.*/foo/bar'", - ft.create_unused_dirs, + ft.create_non_existing_paths, requested_paths ) self.assertNotExists(test_root) # Fail early, do not create intermediate directories @@ -3980,16 +3980,16 @@ def expected_suffix(n_calls_to_create_unused_dirs): os.path.join(test_root, 'nested/foo/baz'), os.path.join(test_root, 'nested/buz'), ] - paths = ft.create_unused_dirs(requested_paths) + paths = ft.create_non_existing_paths(requested_paths) self.assertEqual(paths, requested_paths) self.assertAllExist(paths) ft.remove_dir(test_root) # Test that permissions are set in single directories - ft.mkdir(test_root) + ft.mkdir(test_root, set_gid=False, sticky=False) init_config(build_options={'set_gid_bit': True, 'sticky_bit': True}) requested_path = os.path.join(test_root, 'directory') - paths = ft.create_unused_dirs([requested_path]) + paths = ft.create_non_existing_paths([requested_path]) self.assertEqual(len(paths), 1) dir_perms = os.lstat(paths[0])[stat.ST_MODE] self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID) @@ -3998,10 +3998,10 @@ def expected_suffix(n_calls_to_create_unused_dirs): ft.remove_dir(test_root) # Test that permissions are set correctly across a whole path - ft.mkdir(test_root) + ft.mkdir(test_root, set_gid=False, sticky=False) init_config(build_options={'set_gid_bit': True, 'sticky_bit': True}) requested_path = os.path.join(test_root, 'directory', 'subdirectory') - paths = ft.create_unused_dirs([requested_path]) + paths = ft.create_non_existing_paths([requested_path]) self.assertEqual(len(paths), 1) tested_paths = [ os.path.join(test_root, 'directory'), @@ -4009,8 +4009,8 @@ def expected_suffix(n_calls_to_create_unused_dirs): ] for path in tested_paths: dir_perms = os.lstat(path)[stat.ST_MODE] - self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID) - self.assertEqual(dir_perms & stat.S_ISVTX, stat.S_ISVTX) + self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID, f"set_gid bit should be set for {path}") + self.assertEqual(dir_perms & stat.S_ISVTX, stat.S_ISVTX, f"sticky bit should be set for {path}") init_config(build_options={'set_gid_bit': None, 'sticky_bit': None}) ft.remove_dir(test_root) @@ -4021,16 +4021,16 @@ def expected_suffix(n_calls_to_create_unused_dirs): requested_path = os.path.join(existing_parent, 'subdirectory') ft.mkdir(existing_parent, set_gid=False, sticky=False) - paths = ft.create_unused_dirs([requested_path]) + paths = ft.create_non_existing_paths([requested_path]) self.assertEqual(len(paths), 1) dir_perms = os.lstat(paths[0])[stat.ST_MODE] - self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID) - self.assertEqual(dir_perms & stat.S_ISVTX, stat.S_ISVTX) + self.assertEqual(dir_perms & stat.S_ISGID, stat.S_ISGID, f"set_gid bit should be set for {path}") + self.assertEqual(dir_perms & stat.S_ISVTX, stat.S_ISVTX, f"sticky bit should be set for {path}") dir_perms = os.lstat(existing_parent)[stat.ST_MODE] - self.assertEqual(dir_perms & stat.S_ISGID, 0) - self.assertEqual(dir_perms & stat.S_ISVTX, 0) + self.assertEqual(dir_perms & stat.S_ISGID, 0, f"set_gid bit should not be set for {path}") + self.assertEqual(dir_perms & stat.S_ISVTX, 0, f"sticky bit should not be set for {path}") init_config(build_options={'set_gid_bit': None, 'sticky_bit': None}) ft.remove_dir(test_root) From 7860075d0d4cb0ff38389a145cc36e370874e103 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 1 Mar 2025 21:02:34 +0100 Subject: [PATCH 13/25] rename --artifact-error-path to --failed-installs-build-dirs-path and --log-error-path to --failed-installs-logs-path --- easybuild/framework/easyblock.py | 6 ++--- easybuild/tools/config.py | 35 +++++++++++++-------------- easybuild/tools/options.py | 41 ++++++++++++++++---------------- test/framework/toy_build.py | 12 +++++----- 4 files changed, 46 insertions(+), 48 deletions(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index a9084de0dd..ab04a0e738 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -81,7 +81,7 @@ from easybuild.tools.config import PYTHONPATH, SEARCH_PATH_BIN_DIRS, SEARCH_PATH_LIB_DIRS 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 get_log_error_path, get_artifact_error_path +from easybuild.tools.config import get_failed_installs_build_dirs_path, get_failed_installs_logs_path from easybuild.tools.environment import restore_env, sanitize_env from easybuild.tools.filetools import CHECKSUM_TYPE_SHA256 from easybuild.tools.filetools import adjust_permissions, apply_patch, back_up_file, change_dir, check_lock @@ -4459,7 +4459,7 @@ def persist_failed_compilation_log_and_artifacts(build_successful, application_l datetime_stamp = time.strftime("%Y%m%d") + '-' + time.strftime("%H%M%S") operation_args = [] - log_error_path = get_log_error_path(easyconfig) + log_error_path = get_failed_installs_logs_path(easyconfig) if log_error_path: log_error_path = os.path.join(log_error_path, datetime_stamp) @@ -4480,7 +4480,7 @@ def persist_failed_compilation_log_and_artifacts(build_successful, application_l ) ) - artifact_error_path = get_artifact_error_path(easyconfig) + artifact_error_path = get_failed_installs_build_dirs_path(easyconfig) if artifact_error_path and os.path.isdir(app.builddir): artifact_error_path = os.path.join(artifact_error_path, datetime_stamp) diff --git a/easybuild/tools/config.py b/easybuild/tools/config.py index ebae57ad66..c5b9598c42 100644 --- a/easybuild/tools/config.py +++ b/easybuild/tools/config.py @@ -104,12 +104,13 @@ DEFAULT_MNS = 'EasyBuildMNS' DEFAULT_MODULE_SYNTAX = 'Lua' DEFAULT_MODULES_TOOL = 'Lmod' +FAILED_INSTALLS_SUBDIR = 'failed-installs' DEFAULT_PATH_SUBDIRS = { - 'artifact_error_path': 'error_artifacts', 'buildpath': 'build', 'containerpath': 'containers', + 'failed_installs_build_dirs_path': os.path.join(FAILED_INSTALLS_SUBDIR, 'build-dirs'), + 'failed_installs_logs_path': os.path.join(FAILED_INSTALLS_SUBDIR, 'logs'), 'installpath': '', - 'log_error_path': 'error_logs', 'packagepath': 'packages', 'repositorypath': 'ebfiles_repo', 'sourcepath': 'sources', @@ -492,15 +493,15 @@ class ConfigurationVariables(BaseConfigurationVariables): # list of known/required keys REQUIRED = [ - 'artifact_error_path', 'buildpath', 'config', 'containerpath', + 'failed_installs_build_dirs_path', + 'failed_installs_logs_path', 'installpath', 'installpath_modules', 'installpath_software', 'job_backend', - 'log_error_path', 'logfile_format', 'moduleclasses', 'module_naming_scheme', @@ -869,15 +870,16 @@ def log_path(ec=None): return log_file_format(return_directory=True, ec=ec, date=date, timestamp=timestamp) -def get_log_error_path(ec): +def get_failed_installs_build_dirs_path(ec): """ - Return the 'log_error_path', the location where logs are copied in case of failure + Return the 'failed_installs_build_dirs_path', + the location where build directories are copied if installation failed :param ec: dict-like value with 'name' and 'version' keys defined """ - log_error_path = ConfigurationVariables()['log_error_path'] + failed_installs_build_dirs_path = ConfigurationVariables()['failed_installs_build_dirs_path'] - if not log_error_path: + if not failed_installs_build_dirs_path: return None try: @@ -885,20 +887,19 @@ def get_log_error_path(ec): except KeyError: raise EasyBuildError("The 'name' and 'version' keys are required.") - path = os.path.join(log_error_path, name + '-' + version) + return os.path.join(failed_installs_build_dirs_path, name + '-' + version) - return path - -def get_artifact_error_path(ec): +def get_failed_installs_logs_path(ec): """ - Return the 'artifact_error_path', the location where build directories are copied in case of failure + Return the 'failed_installs_logs_path', + the location where log files are copied if installation failed :param ec: dict-like value with 'name' and 'version' keys defined """ - artifact_error_path = ConfigurationVariables()['artifact_error_path'] + failed_installs_logs_path = ConfigurationVariables()['failed_installs_logs_path'] - if not artifact_error_path: + if not failed_installs_logs_path: return None try: @@ -906,9 +907,7 @@ def get_artifact_error_path(ec): except KeyError: raise EasyBuildError("The 'name' and 'version' keys are required.") - path = os.path.join(artifact_error_path, name + '-' + version) - - return path + return os.path.join(failed_installs_logs_path, name + '-' + version) def get_build_log_path(): diff --git a/easybuild/tools/options.py b/easybuild/tools/options.py index f3f77c0bac..bf252208e0 100644 --- a/easybuild/tools/options.py +++ b/easybuild/tools/options.py @@ -576,9 +576,6 @@ def config_options(self): descr = ("Configuration options", "Configure EasyBuild behavior.") opts = OrderedDict({ - 'artifact-error-path': ("Location where artifacts are copied in case of an error; " - "an empty value disables copying the logs.", - None, 'store', None, {'metavar': "PATH"}), 'avail-module-naming-schemes': ("Show all supported module naming schemes", None, 'store_true', False,), 'avail-modules-tools': ("Show all supported module tools", @@ -593,6 +590,12 @@ def config_options(self): [DEFAULT_ENVVAR_USERS_MODULES]), 'external-modules-metadata': ("List of (glob patterns for) paths to files specifying metadata " "for external modules (INI format)", 'strlist', 'store', None), + 'failed-installs-build-dirs-path': ("Location where build directories are copied if installation fails; " + "an empty value disables copying of build directories", + None, 'store', None, {'metavar': "PATH"}), + 'failed-installs-logs-path': ("Location where log files are copied if installation fails; " + "an empty value disables copying of log files", + None, 'store', None, {'metavar': "PATH"}), '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']), @@ -609,9 +612,6 @@ def config_options(self): None, 'store', None), 'job-backend': ("Backend to use for submitting jobs", 'choice', 'store', DEFAULT_JOB_BACKEND, sorted(avail_job_backends().keys())), - 'log-error-path': ("Location where logs are copied in case of an error; " - "an empty value disables copying the logs.", - None, 'store', None, {'metavar': "PATH"}), # purposely take a copy for the default logfile format 'logfile-format': ("Directory name and format of the log file", 'strtuple', 'store', DEFAULT_LOGFILE_FORMAT[:], {'metavar': 'DIR,FORMAT'}), @@ -1224,9 +1224,9 @@ def _postprocess_config(self): # - the could also specify the location of a *remote* (Git( repository, # which can be done in variety of formats (git@:/), https://, etc.) # (see also https://github.com/easybuilders/easybuild-framework/issues/3892); - path_opt_names = ['artifact_error_path', 'buildpath', 'containerpath', 'git_working_dirs_path', 'installpath', - 'installpath_modules', 'installpath_software', 'log_error_path', 'prefix', 'packagepath', - 'robot_paths', 'sourcepath'] + path_opt_names = ['buildpath', 'containerpath', 'failed_installs_build_dirs_path', 'failed_installs_logs_path', + 'git_working_dirs_path', 'installpath', 'installpath_modules', 'installpath_software', + 'prefix', 'packagepath', 'robot_paths', 'sourcepath'] for opt_name in path_opt_names: self._ensure_abs_path(opt_name) @@ -1234,8 +1234,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 = ['artifact_error_path', 'buildpath', 'containerpath', 'installpath', 'log_error_path', - 'packagepath', 'repository', 'repositorypath', 'sourcepath'] + path_opts = ['buildpath', 'containerpath', 'failed_installs_build_dirs_path', 'failed_installs_logs_path', + 'installpath', 'packagepath', 'repository', 'repositorypath', 'sourcepath'] for dest in path_opts: if not self.options._action_taken.get(dest, False): if dest == 'repository': @@ -1294,19 +1294,18 @@ def _postprocess_config(self): if self.options.inject_checksums or self.options.inject_checksums_to_json: self.options.pre_create_installdir = False - # Prevent permanent storage of logs and artifacts from modifying the build directory - if self.options.buildpath and self.options.log_error_path: - if is_parent_path(self.options.buildpath, self.options.log_error_path): + # Prevent that build directories and logs for failed installations are copied to location for build directories + if self.options.buildpath and self.options.failed_installs_logs_path: + if is_parent_path(self.options.buildpath, self.options.failed_installs_logs_path): raise EasyBuildError( - "The --log-error-path ('%s') cannot reside on a subdirectory of the --buildpath ('%s')" - % (self.options.log_error_path, self.options.buildpath) + f"The --failed-installs-logs-paths --log-error-path ('{self.options.failed_installs_logs_path}') " + f"cannot reside on a subdirectory of the --buildpath ('{self.options.buildpath}')" ) - - if self.options.buildpath and self.options.artifact_error_path: - if is_parent_path(self.options.buildpath, self.options.artifact_error_path): + if self.options.buildpath and self.options.failed_installs_build_dirs_path: + if is_parent_path(self.options.buildpath, self.options.failed_installs_build_dirs_path): raise EasyBuildError( - "The --artifact-error-path ('%s') cannot reside on a subdirectory of the --buildpath ('%s')" - % (self.options.log_error_path, self.options.buildpath) + f"The --failed-installs-build-dirs-paths ('{self.options.failed_installs_build_dirs_path}') " + f"cannot reside on a subdirectory of the --buildpath ('{self.options.buildpath}')" ) def _postprocess_list_avail(self): diff --git a/test/framework/toy_build.py b/test/framework/toy_build.py index 7958876a15..446a3f68b0 100644 --- a/test/framework/toy_build.py +++ b/test/framework/toy_build.py @@ -301,12 +301,12 @@ def test_toy_broken_copy_log_build_dir(self): # set up subdirectories where stuff should go tmpdir = os.path.join(self.test_prefix, 'tmp') tmp_log_dir = os.path.join(self.test_prefix, 'tmp-logs') - tmp_log_error_dir = os.path.join(self.test_prefix, 'logs-failures') - build_dirs_failures_path = os.path.join(self.test_prefix, 'build-dirs-failures') + failed_installs_build_dirs_path = os.path.join(self.test_prefix, 'failed-installs-build-dirs') + failed_installs_logs_path = os.path.join(self.test_prefix, 'failed-installs-logs') extra_args = [ - f'--artifact-error-path={build_dirs_failures_path}', - f'--log-error-path={tmp_log_error_dir}', + f'--failed-installs-build-dirs-path={failed_installs_build_dirs_path}', + f'--failed-installs-logs-path={failed_installs_logs_path}', f'--tmp-logdir={tmp_log_dir}', ] with self.mocked_stdout_stderr(): @@ -319,7 +319,7 @@ def test_toy_broken_copy_log_build_dir(self): log_file = log_files[0] # check that log files were copied - saved_log_files = glob.glob(os.path.join(tmp_log_error_dir, log_file)) + saved_log_files = glob.glob(os.path.join(failed_installs_logs_path, log_file)) self.assertTrue(len(saved_log_files) == 1, f"Unique copy of log file '{log_file}' made") saved_log_file = saved_log_files[0] self.assertTrue(filecmp.cmp(log_file, saved_log_file, shallow=False), @@ -327,7 +327,7 @@ def test_toy_broken_copy_log_build_dir(self): # check that build directories were copied build_dir = self.test_buildpath - topdir = build_dirs_failures_path + topdir = failed_installs_build_dirs_path app_build_dir = os.path.join(build_dir, 'toy', '0.0', 'system-system') subdir_pattern = '????????-??????' From 36aa1184e0bd5697a7b0bdd514f492b7e91be3f9 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 1 Mar 2025 21:41:00 +0100 Subject: [PATCH 14/25] rename persist_failed_compilation_log_and_artifacts to copy_build_dirs_logs_failed_installs + refactoring --- easybuild/framework/easyblock.py | 90 +++++++++++++++----------------- easybuild/tools/options.py | 2 +- test/framework/toy_build.py | 8 +-- 3 files changed, 47 insertions(+), 53 deletions(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index ab04a0e738..7353f07143 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -48,6 +48,7 @@ import inspect import json import os +import random import re import stat import sys @@ -57,6 +58,7 @@ from concurrent.futures import ThreadPoolExecutor from datetime import datetime from enum import Enum +from string import ascii_letters from textwrap import indent import easybuild.tools.environment as env @@ -4424,85 +4426,69 @@ def print_dry_run_note(loc, silent=True): dry_run_msg(msg, silent=silent) -def create_persistence_paths(operation_args): - persistence_paths = [target_path for (_, _, target_path, _) in operation_args] - persistence_paths = create_non_existing_paths(persistence_paths) - - for i, (operation, source, _, msg) in enumerate(operation_args): - operation_args[i] = (operation, source, persistence_paths[i], msg) - - return operation_args - - -def execute_and_log_persistence_operation(operation, source_paths, target_dir, msg, silent): - for path in source_paths: - operation(path, target_dir) - - print_msg(msg, log=_log, silent=silent) - - -def persist_failed_compilation_log_and_artifacts(build_successful, application_log, silent, app, easyconfig): - if not application_log: - return - +def copy_build_dirs_logs_failed_installs(application_log, silent, app, easyconfig): + """ + Copy build directories and log files for failed installation (if desired) + """ # there may be multiple log files, or the file name may be different due to zipping logs = glob.glob(f"{application_log}*") - print_msg( - "Results of the build can be found in the temporary log file(s): " + ", ".join(logs), - log=_log, - silent=silent - ) - if build_successful: - return + datestamp = time.strftime('%Y%m%d') + timestamp = time.strftime('%H%M%S') + salt = ''.join(random.choice(ascii_letters) for i in range(5)) + unique_subdir = f'{datestamp}-{timestamp}-{salt}' - datetime_stamp = time.strftime("%Y%m%d") + '-' + time.strftime("%H%M%S") operation_args = [] - log_error_path = get_failed_installs_logs_path(easyconfig) - if log_error_path: - log_error_path = os.path.join(log_error_path, datetime_stamp) + logs_path = get_failed_installs_logs_path(easyconfig) + if logs_path: + logs_path = os.path.join(logs_path, unique_subdir) - if is_parent_path(app.builddir, log_error_path): + if is_parent_path(app.builddir, logs_path): print_warning( - "Persistent log directory is subdirectory of build directory; not copying logs.", + "Path to copy log files to for failed installs is subdirectory of build directory; not copying", log=_log, silent=silent ) else: - log_file_copies = [os.path.join(log_error_path, os.path.basename(log)) for log in logs] + log_file_copies = [os.path.join(logs_path, os.path.basename(log)) for log in logs] operation_args.append( ( copy_file, logs, - log_error_path, - "Logs of failed build copied to permanent storage: " + ', '.join(log_file_copies) + logs_path, + f"Log file(s) of failed installation copied to {logs_path}" ) ) - artifact_error_path = get_failed_installs_build_dirs_path(easyconfig) - if artifact_error_path and os.path.isdir(app.builddir): - artifact_error_path = os.path.join(artifact_error_path, datetime_stamp) + build_dirs_path = get_failed_installs_build_dirs_path(easyconfig) + if build_dirs_path and os.path.isdir(app.builddir): + build_dirs_path = os.path.join(build_dirs_path, unique_subdir) - if is_parent_path(app.builddir, artifact_error_path): + if is_parent_path(app.builddir, build_dirs_path): print_warning( - "Persistent artifact directory is subdirectory of build directory; not copying artifacts.", + "Path to copy build dirs to for failed installs is subdirectory of build directory; not copying", log=_log, silent=silent ) else: operation_args.append( ( - lambda source, destination: copy_dir(source, destination, dirs_exist_ok=True), + lambda src, dest: copy_dir(src, dest, dirs_exist_ok=True), [app.builddir], - artifact_error_path, - f"Artifacts of failed build copied to permanent storage: {artifact_error_path}" + build_dirs_path, + f"Build directory of failed installation copied to {build_dirs_path}" ) ) - operation_args = create_persistence_paths(operation_args) - for args in operation_args: - execute_and_log_persistence_operation(*args, silent=silent) + persistence_paths = [target_path for (_, _, target_path, _) in operation_args] + persistence_paths = create_non_existing_paths(persistence_paths) + + for idx, (operation, source_paths, _, msg) in enumerate(operation_args): + for source_path in source_paths: + operation(source_path, persistence_paths[idx]) + + print_msg(msg, log=_log, silent=silent) def build_and_install_one(ecdict, init_env): @@ -4758,7 +4744,13 @@ def ensure_writable_log_dir(log_dir): else: dry_run_msg("(no ignored errors during dry run)\n", silent=silent) - persist_failed_compilation_log_and_artifacts(success, application_log, silent, app, ecdict['ec']) + 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) + + if not success: + copy_build_dirs_logs_failed_installs(application_log, silent, app, ecdict['ec']) del app diff --git a/easybuild/tools/options.py b/easybuild/tools/options.py index bf252208e0..1444fd9c03 100644 --- a/easybuild/tools/options.py +++ b/easybuild/tools/options.py @@ -1298,7 +1298,7 @@ def _postprocess_config(self): if self.options.buildpath and self.options.failed_installs_logs_path: if is_parent_path(self.options.buildpath, self.options.failed_installs_logs_path): raise EasyBuildError( - f"The --failed-installs-logs-paths --log-error-path ('{self.options.failed_installs_logs_path}') " + f"The --failed-installs-logs-paths ('{self.options.failed_installs_logs_path}') " f"cannot reside on a subdirectory of the --buildpath ('{self.options.buildpath}')" ) if self.options.buildpath and self.options.failed_installs_build_dirs_path: diff --git a/test/framework/toy_build.py b/test/framework/toy_build.py index 446a3f68b0..a056436873 100644 --- a/test/framework/toy_build.py +++ b/test/framework/toy_build.py @@ -330,15 +330,17 @@ def test_toy_broken_copy_log_build_dir(self): topdir = failed_installs_build_dirs_path app_build_dir = os.path.join(build_dir, 'toy', '0.0', 'system-system') - subdir_pattern = '????????-??????' + # pattern: -- + subdir_pattern = '????????-??????-?????' # find path to toy.c toy_c_files = glob.glob(os.path.join(app_build_dir, '**', 'toy.c')) self.assertTrue(len(toy_c_files) == 1, f"Exactly one toy.c file found: {toy_c_files}") toy_c_file = toy_c_files[0] - res = glob.glob(os.path.join(topdir, 'toy-0.0', subdir_pattern, 'toy-0.0', os.path.basename(toy_c_file))) - self.assertTrue(len(res) == 1, f"Exactly one build dir found in {app_build_dir}: {res}") + path = os.path.join(topdir, 'toy-0.0', subdir_pattern, 'toy-0.0', os.path.basename(toy_c_file)) + res = glob.glob(path) + self.assertTrue(len(res) == 1, f"Exactly one hit found for {path}: {res}") copied_toy_c_file = res[0] self.assertTrue(filecmp.cmp(toy_c_file, copied_toy_c_file, shallow=False), f"Copy of {toy_c_file} should be found under {topdir}") From 6eb27dbbe332fe333dfab2595852e930f15c59cf Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 1 Mar 2025 21:46:59 +0100 Subject: [PATCH 15/25] don't shadow 'source_paths' imported from tools.config in copy_build_dirs_logs_failed_installs --- easybuild/framework/easyblock.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index 7353f07143..c27c42272a 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -4451,7 +4451,6 @@ def copy_build_dirs_logs_failed_installs(application_log, silent, app, easyconfi silent=silent ) else: - log_file_copies = [os.path.join(logs_path, os.path.basename(log)) for log in logs] operation_args.append( ( copy_file, @@ -4484,9 +4483,9 @@ def copy_build_dirs_logs_failed_installs(application_log, silent, app, easyconfi persistence_paths = [target_path for (_, _, target_path, _) in operation_args] persistence_paths = create_non_existing_paths(persistence_paths) - for idx, (operation, source_paths, _, msg) in enumerate(operation_args): - for source_path in source_paths: - operation(source_path, persistence_paths[idx]) + for idx, (operation, paths, _, msg) in enumerate(operation_args): + for path in paths: + operation(path, persistence_paths[idx]) print_msg(msg, log=_log, silent=silent) From 6e4024ebed19cfee5699ddeddccf18e391d1cdbb Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 1 Mar 2025 23:38:20 +0100 Subject: [PATCH 16/25] fix failing tests --- easybuild/tools/options.py | 8 ++++---- test/framework/options.py | 14 +++++++------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/easybuild/tools/options.py b/easybuild/tools/options.py index 1444fd9c03..332ff14eea 100644 --- a/easybuild/tools/options.py +++ b/easybuild/tools/options.py @@ -1298,14 +1298,14 @@ def _postprocess_config(self): if self.options.buildpath and self.options.failed_installs_logs_path: if is_parent_path(self.options.buildpath, self.options.failed_installs_logs_path): raise EasyBuildError( - f"The --failed-installs-logs-paths ('{self.options.failed_installs_logs_path}') " - f"cannot reside on a subdirectory of the --buildpath ('{self.options.buildpath}')" + f"The --failed-installs-logs-path ('{self.options.failed_installs_logs_path}') " + f"cannot reside in a subdirectory of the --buildpath ('{self.options.buildpath}')" ) if self.options.buildpath and self.options.failed_installs_build_dirs_path: if is_parent_path(self.options.buildpath, self.options.failed_installs_build_dirs_path): raise EasyBuildError( - f"The --failed-installs-build-dirs-paths ('{self.options.failed_installs_build_dirs_path}') " - f"cannot reside on a subdirectory of the --buildpath ('{self.options.buildpath}')" + f"The --failed-installs-build-dirs-path ('{self.options.failed_installs_build_dirs_path}') " + f"cannot reside in a subdirectory of the --buildpath ('{self.options.buildpath}')" ) def _postprocess_list_avail(self): diff --git a/test/framework/options.py b/test/framework/options.py index f2ee6e966e..17068dd262 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1656,7 +1656,7 @@ def test_eb_with(option_flag, is_valid): f"{option_flag}={persist_path}", ] - pattern = rf"The {option_flag} \(.*\) cannot reside on a subdirectory of the --buildpath \(.*\)" + pattern = rf"The {option_flag} \(.*\) cannot reside in a subdirectory of the --buildpath \(.*\)" args = base_args args.extend(extra_args) @@ -1671,10 +1671,10 @@ def test_eb_with(option_flag, is_valid): else: self.assertErrorRegex(EasyBuildError, pattern, self.eb_main, args, raise_error=True) - test_eb_with(option_flag='--log-error-path', is_valid=True) - test_eb_with(option_flag='--log-error-path', is_valid=False) - test_eb_with(option_flag='--artifact-error-path', is_valid=True) - test_eb_with(option_flag='--artifact-error-path', is_valid=False) + test_eb_with(option_flag='--failed-installs-logs-path', is_valid=True) + test_eb_with(option_flag='--failed-installs-logs-path', is_valid=False) + test_eb_with(option_flag='--failed-installs-build-dirs-path', is_valid=True) + test_eb_with(option_flag='--failed-installs-build-dirs-path', is_valid=False) def test_missing(self): """Test use of --missing/-M.""" @@ -5460,11 +5460,11 @@ def test_prefix_option(self): regex = re.compile(r"(?P\S*).*%s.*" % self.test_prefix, re.M) expected = [ - 'artifact-error-path', 'buildpath', 'containerpath', + 'failed-installs-build-dirs-path', + 'failed-installs-logs-path', 'installpath', - 'log-error-path', 'packagepath', 'prefix', 'repositorypath', From a4e88562f8cbbf04ba8e2eaba175eec493826ee4 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 2 Mar 2025 10:02:39 +0100 Subject: [PATCH 17/25] fix alphabetical ordering of filetools imports in easyblock.py --- easybuild/framework/easyblock.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index c27c42272a..c4cfdabeb9 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -87,13 +87,13 @@ from easybuild.tools.environment import restore_env, sanitize_env from easybuild.tools.filetools import 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 derive_alt_pypi_url, diff_files, dir_contains_files, download_file -from easybuild.tools.filetools import encode_class_name, extract_file, find_backup_name_candidate -from easybuild.tools.filetools import get_cwd, 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 -from easybuild.tools.filetools import copy_dir, create_non_existing_paths, is_parent_path +from easybuild.tools.filetools import compute_checksum, convert_name, copy_dir, copy_file, create_lock +from easybuild.tools.filetools import create_non_existing_paths, create_patch_info, derive_alt_pypi_url, diff_files +from easybuild.tools.filetools import dir_contains_files, download_file, encode_class_name, extract_file +from easybuild.tools.filetools import find_backup_name_candidate, get_cwd, get_source_tarball_from_git, is_alt_pypi_url +from easybuild.tools.filetools import is_binary, is_parent_path, is_sha256_checksum, mkdir, move_file, move_logs +from easybuild.tools.filetools import read_file, remove_dir, remove_file, remove_lock, symlink, verify_checksum +from easybuild.tools.filetools import weld_paths, write_file from easybuild.tools.hooks import ( BUILD_STEP, CLEANUP_STEP, CONFIGURE_STEP, EXTENSIONS_STEP, EXTRACT_STEP, FETCH_STEP, INSTALL_STEP, MODULE_STEP, MODULE_WRITE, PACKAGE_STEP, PATCH_STEP, PERMISSIONS_STEP, POSTITER_STEP, POSTPROC_STEP, PREPARE_STEP, READY_STEP, From 6a6eef6709676009f21911cb115b06e64f1aed90 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 2 Mar 2025 10:09:41 +0100 Subject: [PATCH 18/25] don't let setting --prefix configuration option imply that --failed-installs-build-dirs-path and --failed-installs-logs-path are also set with a default subdirectory, they should be used opt-in --- easybuild/tools/config.py | 3 --- easybuild/tools/options.py | 11 ++++++----- test/framework/options.py | 2 -- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/easybuild/tools/config.py b/easybuild/tools/config.py index c5b9598c42..6399c86ff3 100644 --- a/easybuild/tools/config.py +++ b/easybuild/tools/config.py @@ -104,12 +104,9 @@ DEFAULT_MNS = 'EasyBuildMNS' DEFAULT_MODULE_SYNTAX = 'Lua' DEFAULT_MODULES_TOOL = 'Lmod' -FAILED_INSTALLS_SUBDIR = 'failed-installs' DEFAULT_PATH_SUBDIRS = { 'buildpath': 'build', 'containerpath': 'containers', - 'failed_installs_build_dirs_path': os.path.join(FAILED_INSTALLS_SUBDIR, 'build-dirs'), - 'failed_installs_logs_path': os.path.join(FAILED_INSTALLS_SUBDIR, 'logs'), 'installpath': '', 'packagepath': 'packages', 'repositorypath': 'ebfiles_repo', diff --git a/easybuild/tools/options.py b/easybuild/tools/options.py index 332ff14eea..778af8b8de 100644 --- a/easybuild/tools/options.py +++ b/easybuild/tools/options.py @@ -1218,7 +1218,7 @@ def _postprocess_config(self): # to avoid incorrect paths being used when EasyBuild changes the current working directory # (see https://github.com/easybuilders/easybuild-framework/issues/3619); # ensuring absolute paths for 'robot' is handled separately below, - # because we need to be careful with the argument pass to --robot; + # because we need to be careful with the argument passed to --robot; # note: repositorypath is purposely not listed here, because it's a special case: # - the value could consist of a 2-tuple (, ); # - the could also specify the location of a *remote* (Git( repository, @@ -1232,10 +1232,11 @@ def _postprocess_config(self): self._ensure_abs_path(opt_name) 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', 'failed_installs_build_dirs_path', 'failed_installs_logs_path', - 'installpath', 'packagepath', 'repository', 'repositorypath', 'sourcepath'] + # prefix applies to selected path configuration options; + # 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'] for dest in path_opts: if not self.options._action_taken.get(dest, False): if dest == 'repository': diff --git a/test/framework/options.py b/test/framework/options.py index 17068dd262..0af0452bc6 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -5462,8 +5462,6 @@ def test_prefix_option(self): expected = [ 'buildpath', 'containerpath', - 'failed-installs-build-dirs-path', - 'failed-installs-logs-path', 'installpath', 'packagepath', 'prefix', From 6f8d903a9e081d10d79cca3dfc75469b3614541a Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 2 Mar 2025 12:33:49 +0100 Subject: [PATCH 19/25] aply suggested changes in copy_build_dirs_logs_failed_installs Co-authored-by: Alexander Grund --- easybuild/framework/easyblock.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index c4cfdabeb9..24ef348b98 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -4441,12 +4441,12 @@ def copy_build_dirs_logs_failed_installs(application_log, silent, app, easyconfi operation_args = [] logs_path = get_failed_installs_logs_path(easyconfig) - if logs_path: + if logs and logs_path: logs_path = os.path.join(logs_path, unique_subdir) if is_parent_path(app.builddir, logs_path): print_warning( - "Path to copy log files to for failed installs is subdirectory of build directory; not copying", + "Path to copy log files of failed installs to is subdirectory of build directory; not copying", log=_log, silent=silent ) @@ -4466,7 +4466,7 @@ def copy_build_dirs_logs_failed_installs(application_log, silent, app, easyconfi if is_parent_path(app.builddir, build_dirs_path): print_warning( - "Path to copy build dirs to for failed installs is subdirectory of build directory; not copying", + "Path to copy build dirs of failed installs to is subdirectory of build directory; not copying", log=_log, silent=silent ) @@ -4486,7 +4486,6 @@ def copy_build_dirs_logs_failed_installs(application_log, silent, app, easyconfi for idx, (operation, paths, _, msg) in enumerate(operation_args): for path in paths: operation(path, persistence_paths[idx]) - print_msg(msg, log=_log, silent=silent) From 25864b853f67623b1f80bf15ad96ff09a27b4b91 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 2 Mar 2025 12:37:28 +0100 Subject: [PATCH 20/25] apply suggested changes for get_failed_installs_build_dirs_path + get_failed_installs_logs_path Co-authored-by: Alexander Grund --- easybuild/tools/config.py | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/easybuild/tools/config.py b/easybuild/tools/config.py index 6399c86ff3..675f3036a1 100644 --- a/easybuild/tools/config.py +++ b/easybuild/tools/config.py @@ -869,14 +869,12 @@ def log_path(ec=None): def get_failed_installs_build_dirs_path(ec): """ - Return the 'failed_installs_build_dirs_path', - the location where build directories are copied if installation failed + Return the location where the build directory is copied to if installation failed :param ec: dict-like value with 'name' and 'version' keys defined """ - failed_installs_build_dirs_path = ConfigurationVariables()['failed_installs_build_dirs_path'] - - if not failed_installs_build_dirs_path: + base_path = ConfigurationVariables()['failed_installs_build_dirs_path'] + if not base_path: return None try: @@ -884,19 +882,17 @@ def get_failed_installs_build_dirs_path(ec): except KeyError: raise EasyBuildError("The 'name' and 'version' keys are required.") - return os.path.join(failed_installs_build_dirs_path, name + '-' + version) + return os.path.join(base_path, f'{name}-{version}') def get_failed_installs_logs_path(ec): """ - Return the 'failed_installs_logs_path', - the location where log files are copied if installation failed + Return the location where log files are copied to if installation failed :param ec: dict-like value with 'name' and 'version' keys defined """ - failed_installs_logs_path = ConfigurationVariables()['failed_installs_logs_path'] - - if not failed_installs_logs_path: + base_path = ConfigurationVariables()['failed_installs_logs_path'] + if not base_path: return None try: @@ -904,7 +900,7 @@ def get_failed_installs_logs_path(ec): except KeyError: raise EasyBuildError("The 'name' and 'version' keys are required.") - return os.path.join(failed_installs_logs_path, name + '-' + version) + return os.path.join(base_path, f'{name}-{version}') def get_build_log_path(): From c50dc4e6ea42ddb55f090b6c95402907eab1da03 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 2 Mar 2025 12:39:10 +0100 Subject: [PATCH 21/25] simplify code to determine name of unique subdirectory in copy_build_dirs_logs_failed_installs --- easybuild/framework/easyblock.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index 24ef348b98..76ed472b4b 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -4433,10 +4433,9 @@ def copy_build_dirs_logs_failed_installs(application_log, silent, app, easyconfi # there may be multiple log files, or the file name may be different due to zipping logs = glob.glob(f"{application_log}*") - datestamp = time.strftime('%Y%m%d') - timestamp = time.strftime('%H%M%S') - salt = ''.join(random.choice(ascii_letters) for i in range(5)) - unique_subdir = f'{datestamp}-{timestamp}-{salt}' + timestamp = time.strftime('%Y%m%d-%H%M%S') + salt = ''.join(random.choices(ascii_letters, k=5)) + unique_subdir = f'{timestamp}-{salt}' operation_args = [] From f63a743799aa81312c4721b2e2794fb79e70d372 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 2 Mar 2025 12:43:43 +0100 Subject: [PATCH 22/25] rename --failed-installs-build-dirs-path to --failed-install-build-dirs-path + --failed-installs-logs-path to --failed-install-logs-path to avoid double plural --- easybuild/framework/easyblock.py | 10 +++++----- easybuild/tools/config.py | 12 ++++++------ easybuild/tools/options.py | 26 +++++++++++++------------- test/framework/options.py | 8 ++++---- test/framework/toy_build.py | 12 ++++++------ 5 files changed, 34 insertions(+), 34 deletions(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index 76ed472b4b..77a91979a1 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -83,7 +83,7 @@ from easybuild.tools.config import PYTHONPATH, SEARCH_PATH_BIN_DIRS, SEARCH_PATH_LIB_DIRS 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 get_failed_installs_build_dirs_path, get_failed_installs_logs_path +from easybuild.tools.config import get_failed_install_build_dirs_path, get_failed_install_logs_path from easybuild.tools.environment import restore_env, sanitize_env from easybuild.tools.filetools import CHECKSUM_TYPE_SHA256 from easybuild.tools.filetools import adjust_permissions, apply_patch, back_up_file, change_dir, check_lock @@ -4426,7 +4426,7 @@ def print_dry_run_note(loc, silent=True): dry_run_msg(msg, silent=silent) -def copy_build_dirs_logs_failed_installs(application_log, silent, app, easyconfig): +def copy_build_dirs_logs_failed_install(application_log, silent, app, easyconfig): """ Copy build directories and log files for failed installation (if desired) """ @@ -4439,7 +4439,7 @@ def copy_build_dirs_logs_failed_installs(application_log, silent, app, easyconfi operation_args = [] - logs_path = get_failed_installs_logs_path(easyconfig) + logs_path = get_failed_install_logs_path(easyconfig) if logs and logs_path: logs_path = os.path.join(logs_path, unique_subdir) @@ -4459,7 +4459,7 @@ def copy_build_dirs_logs_failed_installs(application_log, silent, app, easyconfi ) ) - build_dirs_path = get_failed_installs_build_dirs_path(easyconfig) + build_dirs_path = get_failed_install_build_dirs_path(easyconfig) if build_dirs_path and os.path.isdir(app.builddir): build_dirs_path = os.path.join(build_dirs_path, unique_subdir) @@ -4747,7 +4747,7 @@ def ensure_writable_log_dir(log_dir): print_msg("Results of the build can be found in the log file(s) %s" % ', '.join(logs), log=_log, silent=silent) if not success: - copy_build_dirs_logs_failed_installs(application_log, silent, app, ecdict['ec']) + copy_build_dirs_logs_failed_install(application_log, silent, app, ecdict['ec']) del app diff --git a/easybuild/tools/config.py b/easybuild/tools/config.py index 675f3036a1..15ae21da85 100644 --- a/easybuild/tools/config.py +++ b/easybuild/tools/config.py @@ -493,8 +493,8 @@ class ConfigurationVariables(BaseConfigurationVariables): 'buildpath', 'config', 'containerpath', - 'failed_installs_build_dirs_path', - 'failed_installs_logs_path', + 'failed_install_build_dirs_path', + 'failed_install_logs_path', 'installpath', 'installpath_modules', 'installpath_software', @@ -867,13 +867,13 @@ def log_path(ec=None): return log_file_format(return_directory=True, ec=ec, date=date, timestamp=timestamp) -def get_failed_installs_build_dirs_path(ec): +def get_failed_install_build_dirs_path(ec): """ Return the location where the build directory is copied to if installation failed :param ec: dict-like value with 'name' and 'version' keys defined """ - base_path = ConfigurationVariables()['failed_installs_build_dirs_path'] + base_path = ConfigurationVariables()['failed_install_build_dirs_path'] if not base_path: return None @@ -885,13 +885,13 @@ def get_failed_installs_build_dirs_path(ec): return os.path.join(base_path, f'{name}-{version}') -def get_failed_installs_logs_path(ec): +def get_failed_install_logs_path(ec): """ Return the location where log files are copied to if installation failed :param ec: dict-like value with 'name' and 'version' keys defined """ - base_path = ConfigurationVariables()['failed_installs_logs_path'] + base_path = ConfigurationVariables()['failed_install_logs_path'] if not base_path: return None diff --git a/easybuild/tools/options.py b/easybuild/tools/options.py index 778af8b8de..90b6879c4d 100644 --- a/easybuild/tools/options.py +++ b/easybuild/tools/options.py @@ -590,12 +590,12 @@ def config_options(self): [DEFAULT_ENVVAR_USERS_MODULES]), 'external-modules-metadata': ("List of (glob patterns for) paths to files specifying metadata " "for external modules (INI format)", 'strlist', 'store', None), - 'failed-installs-build-dirs-path': ("Location where build directories are copied if installation fails; " - "an empty value disables copying of build directories", - None, 'store', None, {'metavar': "PATH"}), - 'failed-installs-logs-path': ("Location where log files are copied if installation fails; " - "an empty value disables copying of log files", - None, 'store', None, {'metavar': "PATH"}), + 'failed-install-build-dirs-path': ("Location where build directories are copied if installation fails; " + "an empty value disables copying of build directories", + None, 'store', None, {'metavar': "PATH"}), + 'failed-install-logs-path': ("Location where log files are copied if installation fails; " + "an empty value disables copying of log files", + None, 'store', None, {'metavar': "PATH"}), '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']), @@ -1224,7 +1224,7 @@ def _postprocess_config(self): # - the could also specify the location of a *remote* (Git( repository, # which can be done in variety of formats (git@:/), https://, etc.) # (see also https://github.com/easybuilders/easybuild-framework/issues/3892); - path_opt_names = ['buildpath', 'containerpath', 'failed_installs_build_dirs_path', 'failed_installs_logs_path', + path_opt_names = ['buildpath', 'containerpath', 'failed_install_build_dirs_path', 'failed_install_logs_path', 'git_working_dirs_path', 'installpath', 'installpath_modules', 'installpath_software', 'prefix', 'packagepath', 'robot_paths', 'sourcepath'] @@ -1296,16 +1296,16 @@ def _postprocess_config(self): self.options.pre_create_installdir = False # Prevent that build directories and logs for failed installations are copied to location for build directories - if self.options.buildpath and self.options.failed_installs_logs_path: - if is_parent_path(self.options.buildpath, self.options.failed_installs_logs_path): + if self.options.buildpath and self.options.failed_install_logs_path: + if is_parent_path(self.options.buildpath, self.options.failed_install_logs_path): raise EasyBuildError( - f"The --failed-installs-logs-path ('{self.options.failed_installs_logs_path}') " + f"The --failed-install-logs-path ('{self.options.failed_install_logs_path}') " f"cannot reside in a subdirectory of the --buildpath ('{self.options.buildpath}')" ) - if self.options.buildpath and self.options.failed_installs_build_dirs_path: - if is_parent_path(self.options.buildpath, self.options.failed_installs_build_dirs_path): + if self.options.buildpath and self.options.failed_install_build_dirs_path: + if is_parent_path(self.options.buildpath, self.options.failed_install_build_dirs_path): raise EasyBuildError( - f"The --failed-installs-build-dirs-path ('{self.options.failed_installs_build_dirs_path}') " + f"The --failed-install-build-dirs-path ('{self.options.failed_install_build_dirs_path}') " f"cannot reside in a subdirectory of the --buildpath ('{self.options.buildpath}')" ) diff --git a/test/framework/options.py b/test/framework/options.py index 0af0452bc6..cf962bc7fb 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -1671,10 +1671,10 @@ def test_eb_with(option_flag, is_valid): else: self.assertErrorRegex(EasyBuildError, pattern, self.eb_main, args, raise_error=True) - test_eb_with(option_flag='--failed-installs-logs-path', is_valid=True) - test_eb_with(option_flag='--failed-installs-logs-path', is_valid=False) - test_eb_with(option_flag='--failed-installs-build-dirs-path', is_valid=True) - test_eb_with(option_flag='--failed-installs-build-dirs-path', is_valid=False) + test_eb_with(option_flag='--failed-install-logs-path', is_valid=True) + test_eb_with(option_flag='--failed-install-logs-path', is_valid=False) + test_eb_with(option_flag='--failed-install-build-dirs-path', is_valid=True) + test_eb_with(option_flag='--failed-install-build-dirs-path', is_valid=False) def test_missing(self): """Test use of --missing/-M.""" diff --git a/test/framework/toy_build.py b/test/framework/toy_build.py index a056436873..8235215da9 100644 --- a/test/framework/toy_build.py +++ b/test/framework/toy_build.py @@ -301,12 +301,12 @@ def test_toy_broken_copy_log_build_dir(self): # set up subdirectories where stuff should go tmpdir = os.path.join(self.test_prefix, 'tmp') tmp_log_dir = os.path.join(self.test_prefix, 'tmp-logs') - failed_installs_build_dirs_path = os.path.join(self.test_prefix, 'failed-installs-build-dirs') - failed_installs_logs_path = os.path.join(self.test_prefix, 'failed-installs-logs') + failed_install_build_dirs_path = os.path.join(self.test_prefix, 'failed-install-build-dirs') + failed_install_logs_path = os.path.join(self.test_prefix, 'failed-install-logs') extra_args = [ - f'--failed-installs-build-dirs-path={failed_installs_build_dirs_path}', - f'--failed-installs-logs-path={failed_installs_logs_path}', + f'--failed-install-build-dirs-path={failed_install_build_dirs_path}', + f'--failed-install-logs-path={failed_install_logs_path}', f'--tmp-logdir={tmp_log_dir}', ] with self.mocked_stdout_stderr(): @@ -319,7 +319,7 @@ def test_toy_broken_copy_log_build_dir(self): log_file = log_files[0] # check that log files were copied - saved_log_files = glob.glob(os.path.join(failed_installs_logs_path, log_file)) + saved_log_files = glob.glob(os.path.join(failed_install_logs_path, log_file)) self.assertTrue(len(saved_log_files) == 1, f"Unique copy of log file '{log_file}' made") saved_log_file = saved_log_files[0] self.assertTrue(filecmp.cmp(log_file, saved_log_file, shallow=False), @@ -327,7 +327,7 @@ def test_toy_broken_copy_log_build_dir(self): # check that build directories were copied build_dir = self.test_buildpath - topdir = failed_installs_build_dirs_path + topdir = failed_install_build_dirs_path app_build_dir = os.path.join(build_dir, 'toy', '0.0', 'system-system') # pattern: -- From 7c58cb939ab72cc305e96b28ed07a8daea8326ec Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 2 Mar 2025 14:51:29 +0100 Subject: [PATCH 23/25] fix alphabetical order of imports from tools.config in easyblock.py --- easybuild/framework/easyblock.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index 77a91979a1..23113345ef 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -81,9 +81,9 @@ from easybuild.tools.config import EASYBUILD_SOURCES_URL, EBPYTHONPREFIXES # noqa from easybuild.tools.config import FORCE_DOWNLOAD_ALL, FORCE_DOWNLOAD_PATCHES, FORCE_DOWNLOAD_SOURCES from easybuild.tools.config import PYTHONPATH, SEARCH_PATH_BIN_DIRS, SEARCH_PATH_LIB_DIRS -from easybuild.tools.config import build_option, build_path, get_log_filename, get_repository, get_repositorypath +from easybuild.tools.config import build_option, build_path, get_failed_install_build_dirs_path +from easybuild.tools.config import get_failed_install_logs_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 get_failed_install_build_dirs_path, get_failed_install_logs_path from easybuild.tools.environment import restore_env, sanitize_env from easybuild.tools.filetools import CHECKSUM_TYPE_SHA256 from easybuild.tools.filetools import adjust_permissions, apply_patch, back_up_file, change_dir, check_lock From 905b941bb4a7531646cc259e266a2f2d2cf0ae95 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 2 Mar 2025 14:56:42 +0100 Subject: [PATCH 24/25] minor code cleanup in copy_build_dirs_logs_failed_install --- easybuild/framework/easyblock.py | 45 ++++++++++---------------------- 1 file changed, 14 insertions(+), 31 deletions(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index 23113345ef..e5c36f80f2 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -4430,6 +4430,9 @@ def copy_build_dirs_logs_failed_install(application_log, silent, app, easyconfig """ Copy build directories and log files for failed installation (if desired) """ + logs_path = get_failed_install_logs_path(easyconfig) + build_dirs_path = get_failed_install_build_dirs_path(easyconfig) + # there may be multiple log files, or the file name may be different due to zipping logs = glob.glob(f"{application_log}*") @@ -4439,48 +4442,28 @@ def copy_build_dirs_logs_failed_install(application_log, silent, app, easyconfig operation_args = [] - logs_path = get_failed_install_logs_path(easyconfig) - if logs and logs_path: + if logs_path and logs: logs_path = os.path.join(logs_path, unique_subdir) if is_parent_path(app.builddir, logs_path): - print_warning( - "Path to copy log files of failed installs to is subdirectory of build directory; not copying", - log=_log, - silent=silent - ) + msg = "Path to copy log files of failed installs to is subdirectory of build directory; not copying" + print_warning(msg, log=_log, silent=silent) else: - operation_args.append( - ( - copy_file, - logs, - logs_path, - f"Log file(s) of failed installation copied to {logs_path}" - ) - ) + msg = f"Log file(s) of failed installation copied to {logs_path}" + operation_args.append((copy_file, logs, logs_path, msg)) - build_dirs_path = get_failed_install_build_dirs_path(easyconfig) if build_dirs_path and os.path.isdir(app.builddir): build_dirs_path = os.path.join(build_dirs_path, unique_subdir) if is_parent_path(app.builddir, build_dirs_path): - print_warning( - "Path to copy build dirs of failed installs to is subdirectory of build directory; not copying", - log=_log, - silent=silent - ) + msg = "Path to copy build dirs of failed installs to is subdirectory of build directory; not copying" + print_warning(msg, log=_log, silent=silent) else: - operation_args.append( - ( - lambda src, dest: copy_dir(src, dest, dirs_exist_ok=True), - [app.builddir], - build_dirs_path, - f"Build directory of failed installation copied to {build_dirs_path}" - ) - ) + operation = lambda src, dest: copy_dir(src, dest, dirs_exist_ok=True) + msg = f"Build directory of failed installation copied to {build_dirs_path}" + operation_args.append((operation, [app.builddir], build_dirs_path, msg)) - persistence_paths = [target_path for (_, _, target_path, _) in operation_args] - persistence_paths = create_non_existing_paths(persistence_paths) + persistence_paths = create_non_existing_paths(target_path for (_, _, target_path, _) in operation_args) for idx, (operation, paths, _, msg) in enumerate(operation_args): for path in paths: From e184b6742c0f693e348b2bed15f02e1fe6149446 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sun, 2 Mar 2025 15:02:47 +0100 Subject: [PATCH 25/25] fix code style check failure by creating inner function rather than assigning a lambda definition --- easybuild/framework/easyblock.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py index e5c36f80f2..c25b6a6a09 100644 --- a/easybuild/framework/easyblock.py +++ b/easybuild/framework/easyblock.py @@ -4459,8 +4459,11 @@ def copy_build_dirs_logs_failed_install(application_log, silent, app, easyconfig msg = "Path to copy build dirs of failed installs to is subdirectory of build directory; not copying" print_warning(msg, log=_log, silent=silent) else: - operation = lambda src, dest: copy_dir(src, dest, dirs_exist_ok=True) msg = f"Build directory of failed installation copied to {build_dirs_path}" + + def operation(src, dest): + copy_dir(src, dest, dirs_exist_ok=True) + operation_args.append((operation, [app.builddir], build_dirs_path, msg)) persistence_paths = create_non_existing_paths(target_path for (_, _, target_path, _) in operation_args)