From 6d967939445b53768e1e52b589645e33d7577255 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 4 Feb 2025 13:12:19 +0100 Subject: [PATCH 1/6] Ignore unmodified files when using `--new-pr` or `--update_pr` When including an unmodified EasyConfig with `--new-pr` an error is shown that a commit message is required because this EC is modified which is not the case. Adjust the `copy_*` functions to ignore any unmodified file. This especially ommits them in the `file_info` struct returned that is then used to determine commit message/title etc. Fixes #4751 --- easybuild/framework/easyconfig/easyconfig.py | 17 +++-- easybuild/tools/filetools.py | 70 +++++++++++--------- easybuild/tools/github.py | 65 +++++++++--------- test/framework/easyconfig.py | 24 +++++-- test/framework/filetools.py | 19 +++++- 5 files changed, 118 insertions(+), 77 deletions(-) diff --git a/easybuild/framework/easyconfig/easyconfig.py b/easybuild/framework/easyconfig/easyconfig.py index ac4345cc85..194be40988 100644 --- a/easybuild/framework/easyconfig/easyconfig.py +++ b/easybuild/framework/easyconfig/easyconfig.py @@ -43,6 +43,7 @@ import copy import difflib +import filecmp import functools import os import re @@ -2621,15 +2622,19 @@ def det_file_info(paths, target_dir): for path in paths: ecs = process_easyconfig(path, validate=False) if len(ecs) == 1: - file_info['paths'].append(path) - file_info['ecs'].append(ecs[0]['ec']) - - soft_name = file_info['ecs'][-1].name - ec_filename = file_info['ecs'][-1].filename() + ec = ecs[0]['ec'] + soft_name = ec.name + ec_filename = ec.filename() target_path = det_location_for(path, target_dir, soft_name, ec_filename) new_file = not os.path.exists(target_path) + if not new_file and filecmp.cmp(path, target_path): + continue # Ignore unchanged files + + file_info['paths'].append(path) + file_info['ecs'].append(ec) + new_folder = not os.path.exists(os.path.dirname(target_path)) file_info['new'].append(new_file) file_info['new_folder'].append(new_folder) @@ -2673,6 +2678,8 @@ def copy_patch_files(patch_specs, target_dir): } for patch_path, soft_name in patch_specs: target_path = det_location_for(patch_path, target_dir, soft_name, os.path.basename(patch_path)) + if os.path.exists(target_path) and filecmp.cmp(patch_path, target_path): + continue # Skip copy and entry if not modified copy_file(patch_path, target_path, force_in_dry_run=True) patched_files['paths_in_repo'].append(target_path) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index 3126c1c020..c631745463 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -3041,29 +3041,32 @@ def copy_easyblocks(paths, target_dir): } subdir = os.path.join('easybuild', 'easyblocks') - if os.path.exists(os.path.join(target_dir, subdir)): - for path in paths: - cn = get_easyblock_class_name(path) - if not cn: - raise EasyBuildError("Could not determine easyblock class from file %s" % path) + if not os.path.exists(os.path.join(target_dir, subdir)): + raise EasyBuildError("Could not find %s subdir in %s", subdir, target_dir) - eb_name = remove_unwanted_chars(decode_class_name(cn).replace('-', '_')).lower() + for path in paths: + cn = get_easyblock_class_name(path) + if not cn: + raise EasyBuildError("Could not determine easyblock class from file %s" % path) - if is_generic_easyblock(cn): - pkgdir = GENERIC_EASYBLOCK_PKG - else: - pkgdir = eb_name[0] + eb_name = remove_unwanted_chars(decode_class_name(cn).replace('-', '_')).lower() - target_path = os.path.join(subdir, pkgdir, eb_name + '.py') + if is_generic_easyblock(cn): + pkgdir = GENERIC_EASYBLOCK_PKG + else: + pkgdir = eb_name[0] - full_target_path = os.path.join(target_dir, target_path) - file_info['eb_names'].append(eb_name) - file_info['paths_in_repo'].append(full_target_path) - file_info['new'].append(not os.path.exists(full_target_path)) - copy_file(path, full_target_path, force_in_dry_run=True) + target_path = os.path.join(subdir, pkgdir, eb_name + '.py') + full_target_path = os.path.join(target_dir, target_path) - else: - raise EasyBuildError("Could not find %s subdir in %s", subdir, target_dir) + new_file = not os.path.exists(full_target_path) + if not new_file and filecmp.cmp(path, full_target_path): + continue # Skip unmodified file + + file_info['eb_names'].append(eb_name) + file_info['paths_in_repo'].append(full_target_path) + file_info['new'].append(new_file) + copy_file(path, full_target_path, force_in_dry_run=True) return file_info @@ -3083,23 +3086,24 @@ def copy_framework_files(paths, target_dir): target_path = None dirnames = os.path.dirname(path).split(os.path.sep) - if framework_topdir in dirnames: - # construct subdirectory by grabbing last entry in dirnames until we hit 'easybuild-framework' dir - subdirs = [] - while dirnames[-1] != framework_topdir: - subdirs.insert(0, dirnames.pop()) - - parent_dir = os.path.join(*subdirs) if subdirs else '' - target_path = os.path.join(target_dir, parent_dir, os.path.basename(path)) - else: + if framework_topdir not in dirnames: raise EasyBuildError("Specified path '%s' does not include a '%s' directory!", path, framework_topdir) - if target_path: - file_info['paths_in_repo'].append(target_path) - file_info['new'].append(not os.path.exists(target_path)) - copy_file(path, target_path) - else: - raise EasyBuildError("Couldn't find parent folder of updated file: %s", path) + # construct subdirectory by grabbing last entry in dirnames until we hit 'easybuild-framework' dir + subdirs = [] + while dirnames[-1] != framework_topdir: + subdirs.insert(0, dirnames.pop()) + + parent_dir = os.path.join(*subdirs) if subdirs else '' + target_path = os.path.join(target_dir, parent_dir, os.path.basename(path)) + + new_file = not os.path.exists(target_path) + if not new_file and filecmp.cmp(path, target_path): + continue # Ignore unchanged files + + file_info['paths_in_repo'].append(target_path) + file_info['new'].append(new_file) + copy_file(path, target_path) return file_info diff --git a/easybuild/tools/github.py b/easybuild/tools/github.py index fa6e5519eb..f9a824a935 100644 --- a/easybuild/tools/github.py +++ b/easybuild/tools/github.py @@ -1124,38 +1124,6 @@ def _easyconfigs_pr_common(paths, ecs, start_branch=None, pr_branch=None, start_ print_msg("copying files to %s..." % target_dir) file_info = COPY_FUNCTIONS[pr_target_repo](ec_paths, target_dir) - # figure out commit message to use - if commit_msg: - if (pr_target_repo == GITHUB_EASYCONFIGS_REPO and all(file_info['new']) and not paths['files_to_delete'] - and is_new_pr): # Only if opening a new PR - msg = "When only adding new easyconfigs a PR commit msg (--pr-commit-msg) should not be used, as " - msg += "the PR title will be automatically generated." - if build_option('force'): - print_msg(msg) - print_msg("Using the specified --pr-commit-msg as the force build option was specified.") - else: - raise EasyBuildError(msg) - cnt = len(file_info['paths_in_repo']) - _log.debug("Using specified commit message for all %d new/modified files at once: %s", cnt, commit_msg) - elif pr_target_repo == GITHUB_EASYCONFIGS_REPO and all(file_info['new']) and not paths['files_to_delete']: - # automagically derive meaningful commit message if all easyconfig files are new - commit_msg = "adding easyconfigs: %s" % ', '.join(os.path.basename(p) for p in file_info['paths_in_repo']) - if paths['patch_files']: - commit_msg += " and patches: %s" % ', '.join(os.path.basename(p) for p in paths['patch_files']) - elif pr_target_repo == GITHUB_EASYBLOCKS_REPO and all(file_info['new']): - commit_msg = "adding easyblocks: %s" % ', '.join(os.path.basename(p) for p in file_info['paths_in_repo']) - else: - msg = '' - modified_files = [os.path.basename(p) for new, p in zip(file_info['new'], file_info['paths_in_repo']) - if not new] - if modified_files: - msg += '\nModified: ' + ', '.join(modified_files) - if paths['files_to_delete']: - msg += '\nDeleted: ' + ', '.join(paths['files_to_delete']) - raise EasyBuildError("A meaningful commit message must be specified via --pr-commit-msg when " - "modifying/deleting files or targeting the framework repo." + msg, - exit_code=EasyBuildExit.OPTION_ERROR) - # figure out to which software name patches relate, and copy them to the right place if paths['patch_files']: patch_specs = det_patch_specs(paths['patch_files'], file_info, [target_dir]) @@ -1188,7 +1156,6 @@ def _easyconfigs_pr_common(paths, ecs, start_branch=None, pr_branch=None, start_ # include missing easyconfigs for dependencies, if robot is enabled if ecs is not None: - abs_paths = [os.path.realpath(os.path.abspath(path)) for path in ec_paths] dep_paths = [ec['spec'] for ec in ecs if os.path.realpath(ec['spec']) not in abs_paths] _log.info("Paths to easyconfigs for missing dependencies: %s", dep_paths) @@ -1241,6 +1208,38 @@ def _easyconfigs_pr_common(paths, ecs, start_branch=None, pr_branch=None, start_ exit_code=EasyBuildExit.FAIL_GITHUB ) + # figure out commit message to use + if commit_msg: + if (pr_target_repo == GITHUB_EASYCONFIGS_REPO and all(file_info['new']) and not paths['files_to_delete'] + and is_new_pr): # Only if opening a new PR + msg = "When only adding new easyconfigs a PR commit msg (--pr-commit-msg) should not be used, as " + msg += "the PR title will be automatically generated." + if build_option('force'): + print_msg(msg) + print_msg("Using the specified --pr-commit-msg as the force build option was specified.") + else: + raise EasyBuildError(msg) + cnt = len(file_info['paths_in_repo']) + _log.debug("Using specified commit message for all %d new/modified files at once: %s", cnt, commit_msg) + elif pr_target_repo == GITHUB_EASYCONFIGS_REPO and all(file_info['new']) and not paths['files_to_delete']: + # automagically derive meaningful commit message if all easyconfig files are new + commit_msg = "adding easyconfigs: %s" % ', '.join(os.path.basename(p) for p in file_info['paths_in_repo']) + if paths['patch_files']: + commit_msg += " and patches: %s" % ', '.join(os.path.basename(p) for p in paths['patch_files']) + elif pr_target_repo == GITHUB_EASYBLOCKS_REPO and all(file_info['new']): + commit_msg = "adding easyblocks: %s" % ', '.join(os.path.basename(p) for p in file_info['paths_in_repo']) + else: + msg = '' + modified_files = [os.path.basename(p) for new, p in zip(file_info['new'], file_info['paths_in_repo']) + if not new] + if modified_files: + msg += '\nModified: ' + ', '.join(modified_files) + if paths['files_to_delete']: + msg += '\nDeleted: ' + ', '.join(paths['files_to_delete']) + raise EasyBuildError("A meaningful commit message must be specified via --pr-commit-msg when " + "modifying/deleting files or targeting the framework repo." + msg, + exit_code=EasyBuildExit.OPTION_ERROR) + # commit git_repo.index.commit(commit_msg) diff --git a/test/framework/easyconfig.py b/test/framework/easyconfig.py index 7a07eab449..e80f8a16a5 100644 --- a/test/framework/easyconfig.py +++ b/test/framework/easyconfig.py @@ -3525,11 +3525,25 @@ def test_copy_easyconfigs(self): self.assertTrue(os.path.samefile(res['paths_in_repo'][0], expected)) # check whether easyconfigs were copied (unmodified) to correct location - for orig_ec, src_ec in test_ecs: - orig_ec = os.path.basename(orig_ec) - copied_ec = os.path.join(ecs_target_dir, orig_ec[0].lower(), orig_ec.split('-')[0], orig_ec) - self.assertExists(copied_ec) - self.assertEqual(read_file(copied_ec), read_file(os.path.join(self.test_prefix, src_ec))) + def verify_copied_ecs(): + for orig_ec, src_ec in test_ecs: + orig_ec = os.path.basename(orig_ec) + copied_ec = os.path.join(ecs_target_dir, orig_ec[0].lower(), orig_ec.split('-')[0], orig_ec) + self.assertExists(copied_ec) + self.assertEqual(read_file(copied_ec), read_file(os.path.join(self.test_prefix, src_ec))) + verify_copied_ecs() + + # Unmodified files get excluded + modified_file = expected + write_file(modified_file, "") + res = copy_easyconfigs(ecs_to_copy, target_dir) + self.assertEqual(len(res['ecs']), 1) + self.assertEqual(res['new'], [False]) + self.assertEqual(len(res['paths_in_repo']), 1) + self.assertTrue(os.path.samefile(res['paths_in_repo'][0], expected)) + + # modified file should be replaced and others still be the same, so run the same check again + verify_copied_ecs() # create test easyconfig that includes comments & build stats, just like an archived easyconfig toy_ec = os.path.join(self.test_prefix, 'toy.eb') diff --git a/test/framework/filetools.py b/test/framework/filetools.py index ea0b877b4d..9488c2819c 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -3572,6 +3572,13 @@ def test_copy_easyblocks(self): self.assertEqual(ft.read_file(copied_toy_eb), ft.read_file(toy_eb)) self.assertTrue(os.path.samefile(res['paths_in_repo'][2], copied_toy_eb)) + # Copy only modified files + ft.write_file(copied_toy_eb, "") + res = ft.copy_easyblocks(test_ebs, self.test_prefix) + self.assertEqual(res['eb_names'], ['toy']) + self.assertEqual(res['new'], [False]) + self.assertEqual(ft.read_file(copied_toy_eb), ft.read_file(toy_eb)) + def test_copy_framework_files(self): """Test for copy_framework_files function.""" @@ -3603,7 +3610,7 @@ def test_copy_framework_files(self): expected_new = [True, False, True] # we include setup.py conditionally because it may not be there, - # for example when running the tests on an actual easybuild-framework instalation, + # for example when running the tests on an actual easybuild-framework installation, # as opposed to when running from a repository checkout... # setup.py is an important test case, since it has no parent directory # (it's straight in the easybuild-framework directory) @@ -3638,6 +3645,16 @@ def test_copy_framework_files(self): self.assertEqual(res['new'], expected_new) + # Copy unmodified files (i.e. same again) does nothing + res = ft.copy_framework_files(test_paths, target_dir) + self.assertEqual(res, {'paths_in_repo': [], 'new': []}) + + # Copy single modified file + modified_file = os.path.join(target_dir, test_files[0]) + ft.write_file(modified_file, "") + res = ft.copy_framework_files(test_paths, target_dir) + self.assertEqual(res, {'paths_in_repo': [modified_file], 'new': [False]}) + def test_locks(self): """Tests for lock-related functions.""" From 8d8e71115c8f13c843d94815bae92118e94d2218 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 4 Feb 2025 13:19:46 +0100 Subject: [PATCH 2/6] Minor simplification for adding dependenc ECs to PRs --- easybuild/tools/github.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/easybuild/tools/github.py b/easybuild/tools/github.py index f9a824a935..2c2f0ea4a0 100644 --- a/easybuild/tools/github.py +++ b/easybuild/tools/github.py @@ -1162,8 +1162,8 @@ def _easyconfigs_pr_common(paths, ecs, start_branch=None, pr_branch=None, start_ all_dep_info = copy_easyconfigs(dep_paths, target_dir) # only consider new easyconfig files for dependencies (not updated ones) - for idx in range(len(all_dep_info['ecs'])): - if all_dep_info['new'][idx]: + for idx, new in enumerate(all_dep_info['new']): + if new: for key, info in dep_info.items(): info.append(all_dep_info[key][idx]) From 6dbb6fa5cbb80c6b590a5a41f32ec1eec8894531 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 10 Apr 2026 08:40:06 +0200 Subject: [PATCH 3/6] Only check '*.eb' files for EasyConfigs with patches --- easybuild/tools/github.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/easybuild/tools/github.py b/easybuild/tools/github.py index 9eb1382f5c..f42e5382e0 100644 --- a/easybuild/tools/github.py +++ b/easybuild/tools/github.py @@ -1426,8 +1426,7 @@ def find_software_name_for_patch(patch_name, ec_dirs): if ignore_dirs: dirnames[:] = [i for i in dirnames if i not in ignore_dirs] for fn in filenames: - # TODO: In EasyBuild 5.x only check for '*.eb' files - if fn != 'TEMPLATE.eb' and os.path.splitext(fn)[1] not in ('.py', '.patch'): + if fn != 'TEMPLATE.eb' and os.path.splitext(fn)[1] == '.eb': path = os.path.join(dirpath, fn) rawtxt = read_file(path) if 'patches' in rawtxt: From 26e7a16b4a22b51fa6bf166a9f10f0fda32d0311 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 10 Apr 2026 09:30:30 +0200 Subject: [PATCH 4/6] Add test for --new-pr with only changed patch --- test/framework/options.py | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/test/framework/options.py b/test/framework/options.py index 6f12703d40..5c36037215 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -37,13 +37,12 @@ import tempfile import textwrap from importlib import reload -from unittest import TextTestRunner +from unittest import TextTestRunner, mock from urllib.request import URLError import easybuild.main import easybuild.tools.build_log import easybuild.tools.options -import easybuild.tools.toolchain from easybuild.base import fancylogger from easybuild.framework.easyblock import EasyBlock from easybuild.framework.easyconfig import BUILD, CUSTOM, DEPENDENCIES, EXTENSIONS, FILEMANAGEMENT, LICENSE @@ -4659,13 +4658,13 @@ def test_github_new_update_pr(self): ]) write_file(toy_ec, toy_ec_txt) - args = [ + # Shared arguments for dry-run opening a new PR + common_new_pr_args = [ '--new-pr', '--github-user=%s' % GITHUB_TEST_ACCOUNT, - toy_ec, '-D', - '--disable-cleanup-tmpdir', ] + args = common_new_pr_args + ['--disable-cleanup-tmpdir', toy_ec] txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) # determine location of repo clone, can be used to test --git-working-dirs-path (and save time) @@ -4674,7 +4673,7 @@ def test_github_new_update_pr(self): git_working_dir = dirs[0] else: self.fail("Failed to find temporary git working dir: %s" % dirs) - args.append(f'--git-working-dirs-path={git_working_dir}') + common_new_pr_args.append(f'--git-working-dirs-path={git_working_dir}') remote = 'git@github.com:%s/easybuild-easyconfigs.git' % GITHUB_TEST_ACCOUNT regexs = [ @@ -4691,7 +4690,32 @@ def test_github_new_update_pr(self): ] self.assert_multi_regex(regexs, txt) + # New patch only + with mock.patch('easybuild.tools.github.find_software_name_for_patch') as mock_find_sw_name: + mock_find_sw_name.return_value = 'toy' + args = common_new_pr_args + [toy_patch] + with self.mocked_stdout_stderr(): + self.assertErrorRegex(EasyBuildError, 'use --pr-title', self.eb_main, args, raise_error=True) + args += ['--pr-title=New patch'] + txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) + self.assert_multi_regex(( + r'^\* target: easybuilders/easybuild-easyconfigs:develop', + r'^\* title: "New patch"', + r"^\* overview of changes:", + rf".*/{os.path.basename(toy_patch)}\s*\|", + ), txt) + # With corresponding EC no pr-title needs to be specified + args = common_new_pr_args + [toy_patch, toy_ec] + txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) + self.assert_multi_regex(( + r'^\* target: easybuilders/easybuild-easyconfigs:develop', + r'^\* title: ".*toy v0.0.*"', + r"^\* overview of changes:", + rf".*/{os.path.basename(toy_patch)}\s*\|", + ), txt) + # Commit message must not be specified for only new ECs + args = common_new_pr_args + [toy_ec] args_new_pr = args + ['--pr-commit-msg=just a test'] error_msg = r"PR commit msg \(--pr-commit-msg\) should not be used" with self.mocked_stdout_stderr(): From 5e6897b951040ad1d8044a3e0299fd14eb81757d Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 10 Apr 2026 09:45:46 +0200 Subject: [PATCH 5/6] Fix error message when passing only non-modified easyconfigs to --new-pr When all passed easyconfigs got filtered out creating the branch label fails with an IndexError. Catch this case early. --- easybuild/tools/github.py | 2 +- test/framework/options.py | 42 +++++++++++++++++++++++++++------------ 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/easybuild/tools/github.py b/easybuild/tools/github.py index f42e5382e0..ba9ef9dfce 100644 --- a/easybuild/tools/github.py +++ b/easybuild/tools/github.py @@ -1182,7 +1182,7 @@ def _easyconfigs_pr_common(paths, ecs, start_branch=None, pr_branch=None, start_ # checkout target branch if pr_branch is None: - if ec_paths and pr_target_repo == GITHUB_EASYCONFIGS_REPO: + if pr_target_repo == GITHUB_EASYCONFIGS_REPO and file_info.get('ecs'): label = file_info['ecs'][0].name + re.sub('[.-]', '', file_info['ecs'][0].version) elif pr_target_repo == GITHUB_EASYBLOCKS_REPO and paths.get('py_files'): label = os.path.splitext(os.path.basename(paths['py_files'][0]))[0] diff --git a/test/framework/options.py b/test/framework/options.py index 5c36037215..6128364cd5 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -4690,21 +4690,37 @@ def test_github_new_update_pr(self): ] self.assert_multi_regex(regexs, txt) + # Should error when no files were modified + # Pick an existing easyconfig, use SYSTEM toolchain to allow easy parsing + gcc_ecs = glob.glob(os.path.join(git_working_dir, '**', 'GCC', 'GCC-*.eb'), recursive=True) + if not gcc_ecs: + self.fail("Found no GCC easyconfigs in checkout to use for this test") + unmodified_ec = os.path.join(self.test_prefix, os.path.basename(gcc_ecs[0])) + copy_file(gcc_ecs[0], unmodified_ec) + args = common_new_pr_args + [unmodified_ec] + with self.mocked_stdout_stderr(): + self.assertErrorRegex(EasyBuildError, 'No changed files.*empty pull request', + self.eb_main, args, raise_error=True) + # New patch only with mock.patch('easybuild.tools.github.find_software_name_for_patch') as mock_find_sw_name: - mock_find_sw_name.return_value = 'toy' - args = common_new_pr_args + [toy_patch] - with self.mocked_stdout_stderr(): - self.assertErrorRegex(EasyBuildError, 'use --pr-title', self.eb_main, args, raise_error=True) - args += ['--pr-title=New patch'] - txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) - self.assert_multi_regex(( - r'^\* target: easybuilders/easybuild-easyconfigs:develop', - r'^\* title: "New patch"', - r"^\* overview of changes:", - rf".*/{os.path.basename(toy_patch)}\s*\|", - ), txt) - # With corresponding EC no pr-title needs to be specified + for with_unmodified_ec in (False, True): + with self.subTest(f'With unmodified easyconfig: {with_unmodified_ec}'): + mock_find_sw_name.return_value = 'toy' + args = common_new_pr_args + [toy_patch] + if with_unmodified_ec: + args.append(unmodified_ec) + with self.mocked_stdout_stderr(): + self.assertErrorRegex(EasyBuildError, 'use --pr-title', self.eb_main, args, raise_error=True) + args += ['--pr-title=New patch'] + txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) + self.assert_multi_regex(( + r'^\* target: easybuilders/easybuild-easyconfigs:develop', + r'^\* title: "New patch"', + r"^\* overview of changes:", + rf".*/{os.path.basename(toy_patch)}\s*\|", + ), txt) + # With corresponding, new EC no pr-title needs to be specified args = common_new_pr_args + [toy_patch, toy_ec] txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) self.assert_multi_regex(( From 4f52eac08314ef385d22646411e917d73877a707 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 10 Apr 2026 15:35:42 +0200 Subject: [PATCH 6/6] Speed up test_github_new_update_pr Avoid calling `git fetch` muliple times by mocking that call and copy the existing remote directory. --- easybuild/tools/github.py | 6 +- test/framework/options.py | 396 +++++++++++++++++++------------------- 2 files changed, 205 insertions(+), 197 deletions(-) diff --git a/easybuild/tools/github.py b/easybuild/tools/github.py index ba9ef9dfce..f527e18245 100644 --- a/easybuild/tools/github.py +++ b/easybuild/tools/github.py @@ -94,13 +94,13 @@ GITHUB_API_URL = 'https://api.github.com' GITHUB_BRANCH_MAIN = 'main' GITHUB_BRANCH_MASTER = 'master' -GITHUB_DIR_TYPE = u'dir' +GITHUB_DIR_TYPE = 'dir' GITHUB_EB_MAIN = 'easybuilders' GITHUB_EASYBLOCKS_REPO = 'easybuild-easyblocks' GITHUB_EASYCONFIGS_REPO = 'easybuild-easyconfigs' GITHUB_FRAMEWORK_REPO = 'easybuild-framework' GITHUB_DEVELOP_BRANCH = 'develop' -GITHUB_FILE_TYPE = u'file' +GITHUB_FILE_TYPE = 'file' GITHUB_PR_STATE_OPEN = 'open' GITHUB_PR_STATES = [GITHUB_PR_STATE_OPEN, 'closed', 'all'] GITHUB_PR_ORDER_CREATED = 'created' @@ -979,7 +979,7 @@ def setup_repo_from(git_repo, github_url, target_account, branch_name, silent=Fa ) if res: - if res[0].flags & res[0].ERROR: + if res[0].flags & git.remote.FetchInfo.ERROR: raise EasyBuildError( "Fetching branch '%s' from remote %s failed: %s", branch_name, origin, res[0].note, exit_code=EasyBuildExit.FAIL_GITHUB diff --git a/test/framework/options.py b/test/framework/options.py index 6128364cd5..4f20784d36 100644 --- a/test/framework/options.py +++ b/test/framework/options.py @@ -4690,213 +4690,221 @@ def test_github_new_update_pr(self): ] self.assert_multi_regex(regexs, txt) - # Should error when no files were modified - # Pick an existing easyconfig, use SYSTEM toolchain to allow easy parsing - gcc_ecs = glob.glob(os.path.join(git_working_dir, '**', 'GCC', 'GCC-*.eb'), recursive=True) - if not gcc_ecs: - self.fail("Found no GCC easyconfigs in checkout to use for this test") - unmodified_ec = os.path.join(self.test_prefix, os.path.basename(gcc_ecs[0])) - copy_file(gcc_ecs[0], unmodified_ec) - args = common_new_pr_args + [unmodified_ec] - with self.mocked_stdout_stderr(): - self.assertErrorRegex(EasyBuildError, 'No changed files.*empty pull request', - self.eb_main, args, raise_error=True) - - # New patch only - with mock.patch('easybuild.tools.github.find_software_name_for_patch') as mock_find_sw_name: - for with_unmodified_ec in (False, True): - with self.subTest(f'With unmodified easyconfig: {with_unmodified_ec}'): - mock_find_sw_name.return_value = 'toy' - args = common_new_pr_args + [toy_patch] - if with_unmodified_ec: - args.append(unmodified_ec) - with self.mocked_stdout_stderr(): - self.assertErrorRegex(EasyBuildError, 'use --pr-title', self.eb_main, args, raise_error=True) - args += ['--pr-title=New patch'] - txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) - self.assert_multi_regex(( - r'^\* target: easybuilders/easybuild-easyconfigs:develop', - r'^\* title: "New patch"', - r"^\* overview of changes:", - rf".*/{os.path.basename(toy_patch)}\s*\|", - ), txt) - # With corresponding, new EC no pr-title needs to be specified - args = common_new_pr_args + [toy_patch, toy_ec] - txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) - self.assert_multi_regex(( - r'^\* target: easybuilders/easybuild-easyconfigs:develop', - r'^\* title: ".*toy v0.0.*"', - r"^\* overview of changes:", - rf".*/{os.path.basename(toy_patch)}\s*\|", - ), txt) - - # Commit message must not be specified for only new ECs - args = common_new_pr_args + [toy_ec] - args_new_pr = args + ['--pr-commit-msg=just a test'] - error_msg = r"PR commit msg \(--pr-commit-msg\) should not be used" - with self.mocked_stdout_stderr(): - self.assertErrorRegex(EasyBuildError, error_msg, self.eb_main, args_new_pr, raise_error=True, testing=False) - - # But commit message can still be specified when using --force - args_new_pr.append('--force') - txt, _ = self._run_mock_eb(args_new_pr, do_build=True, raise_error=True, testing=False) - regexs_with_msg = [ - error_msg, # Still shown as a warning - r'== Using the specified --pr-commit-msg', - r'\* title: "just a test"', - ] - self.assert_multi_regex(regexs_with_msg, txt) - - # add unstaged file to git working dir, to check on later - unstaged_file = os.path.join('easybuild-easyconfigs', 'easybuild', 'easyconfigs', 'test.eb') - write_file(os.path.join(git_working_dir, unstaged_file), 'test123') - # Remove other temporary git working dirs - res = glob.glob(os.path.join(self.test_prefix, 'eb-*', 'eb-*', 'git-working-dir*')) - res = [d for d in res if d != git_working_dir] - for path in res: - remove_dir(path) - - ec_name = 'bzip2-1.0.8.eb' - # a custom commit message is required when doing more than just adding new easyconfigs (e.g., deleting a file) - args.append(f':{ec_name}') - error_msg = f"A meaningful commit message must be specified via --pr-commit-msg.*\nDeleted: {ec_name}" - - with self.mocked_stdout_stderr(): - self.assertErrorRegex(EasyBuildError, error_msg, self.eb_main, args, raise_error=True, testing=False) - - # check whether unstaged file in git working dir was copied (it shouldn't) - res = glob.glob(os.path.join(self.test_prefix, 'eb-*', 'eb-*', 'git-working-dir*')) - res = [d for d in res if d != git_working_dir] - if len(res) == 1: - unstaged_file_full = os.path.join(res[0], unstaged_file) - self.assertNotExists(unstaged_file_full) - else: - self.fail("Found copy of easybuild-easyconfigs working copy") - - # add required commit message, try again - args.append('--pr-commit-msg=just a test') - txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) - - regexs[-1] = r"^\s*2 files changed" - regexs.remove(r"^\* title: \"\{tools\}\[gompi/2018a\] toy v0.0 w/ test\"") - regexs.append(r"^\* title: \"just a test\"") - regexs.append(rf".*/{ec_name}\s*\|") - regexs.append(r".*[0-9]+ deletions\(-\)") - self.assert_multi_regex(regexs, txt) - - GITHUB_TEST_ORG = 'test-organization' - args.extend([ - '--pr-branch-name=branch_name_for_new_pr_test', - '--pr-commit-msg="this is a commit message. really!"', - '--pr-descr="moar letters foar teh lettre box"', - '--pr-target-branch=main', - '--github-org=%s' % GITHUB_TEST_ORG, - '--pr-target-account=boegel', # we need to be able to 'clone' from here (via https) - '--pr-title=test-1-2-3', - ]) - txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) - - regexs = [ - r"^== fetching branch 'main' from https://github.com/boegel/easybuild-easyconfigs.git...", - r"^Opening pull request \[DRY RUN\]", - r"^\* target: boegel/easybuild-easyconfigs:main", - r"^\* from: %s/easybuild-easyconfigs:branch_name_for_new_pr_test" % GITHUB_TEST_ORG, - r"\(created using `eb --new-pr`\)", # description - r"moar letters foar teh lettre box", # also description (see --pr-descr) - r"^\* title: \"test-1-2-3\"", - r"^\* overview of changes:", - r".*/toy-0.0-gompi-2018a-test.eb\s*\|", - rf".*/{ec_name}\s*\|", - r"^\s*2 files changed", - r".*[0-9]+ deletions\(-\)", - ] - self.assert_multi_regex(regexs, txt) - - # should also work with a patch - args.append(toy_patch) - with self.mocked_stdout_stderr(): - self.eb_main(args, do_build=True, raise_error=True, testing=False) - txt = self.get_stdout() + # Do not really fetch remote again which won't include changes and speeds up the test a lot + def get_orig_remote_dir(repo): + return glob.glob(os.path.join(git_working_dir, repo, '.git', 'refs', 'remotes', '*'))[0] + + def fake_fetch(self, *args, **kwargs): + repo_dir = self.repo.working_dir + copy_dir(get_orig_remote_dir(os.path.basename(repo_dir)), + os.path.join(repo_dir, '.git', 'refs', 'remotes', self.name)) + return [mock.Mock(flags=0)] + + with mock.patch('git.remote.Remote.fetch', new=fake_fetch): + # Should error when no files were modified + # Pick an existing easyconfig, use SYSTEM toolchain to allow easy parsing + gcc_ecs = glob.glob(os.path.join(git_working_dir, '**', 'GCC', 'GCC-*.eb'), recursive=True) + if not gcc_ecs: + self.fail("Found no GCC easyconfigs in checkout to use for this test") + unmodified_ec = os.path.join(self.test_prefix, os.path.basename(gcc_ecs[0])) + copy_file(gcc_ecs[0], unmodified_ec) + args = common_new_pr_args + [unmodified_ec] + with self.mocked_stdout_stderr(): + self.assertErrorRegex(EasyBuildError, 'No changed files.*empty pull request', + self.eb_main, args, raise_error=True) + + # New patch only + with mock.patch('easybuild.tools.github.find_software_name_for_patch') as mock_find_sw_name: + for with_unmodified_ec in (False, True): + with self.subTest(f'With unmodified easyconfig: {with_unmodified_ec}'): + mock_find_sw_name.return_value = 'toy' + args = common_new_pr_args + [toy_patch] + if with_unmodified_ec: + args.append(unmodified_ec) + with self.mocked_stdout_stderr(): + self.assertErrorRegex(EasyBuildError, 'use --pr-title', self.eb_main, args, + raise_error=True, testing=False) + args += ['--pr-title=New patch'] + txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) + self.assert_multi_regex(( + r'^\* target: easybuilders/easybuild-easyconfigs:develop', + r'^\* title: "New patch"', + r"^\* overview of changes:", + rf".*/{os.path.basename(toy_patch)}\s*\|", + ), txt) + # With corresponding, new EC no pr-title needs to be specified + args = common_new_pr_args + [toy_patch, toy_ec] + txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) + self.assert_multi_regex(( + r'^\* target: easybuilders/easybuild-easyconfigs:develop', + r'^\* title: ".*toy v0.0.*"', + r"^\* overview of changes:", + rf".*/{os.path.basename(toy_patch)}\s*\|", + ), txt) + + # Commit message must not be specified for only new ECs + args = common_new_pr_args + [toy_ec] + args_new_pr = args + ['--pr-commit-msg=just a test'] + error_msg = r"PR commit msg \(--pr-commit-msg\) should not be used" + with self.mocked_stdout_stderr(): + self.assertErrorRegex(EasyBuildError, error_msg, self.eb_main, args_new_pr, + raise_error=True, testing=False) + + # But commit message can still be specified when using --force + args_new_pr.append('--force') + txt, _ = self._run_mock_eb(args_new_pr, do_build=True, raise_error=True, testing=False) + regexs_with_msg = [ + error_msg, # Still shown as a warning + r'== Using the specified --pr-commit-msg', + r'\* title: "just a test"', + ] + self.assert_multi_regex(regexs_with_msg, txt) + + # add unstaged file to git working dir, to check on later + unstaged_file = os.path.join('easybuild-easyconfigs', 'easybuild', 'easyconfigs', 'test.eb') + write_file(os.path.join(git_working_dir, unstaged_file), 'test123') + # Remove other temporary git working dirs + res = glob.glob(os.path.join(self.test_prefix, 'eb-*', 'eb-*', 'git-working-dir*')) + res = [d for d in res if d != git_working_dir] + for path in res: + remove_dir(path) + + ec_name = 'bzip2-1.0.8.eb' + # a custom commit message is required when not just adding new easyconfigs (e.g., deleting a file) + args.append(f':{ec_name}') + error_msg = f"A meaningful commit message must be specified via --pr-commit-msg.*\nDeleted: {ec_name}" - regexs[-2] = r"^\s*3 files changed" - regexs.append(r".*_fix-silly-typo-in-printf-statement.patch\s*\|") - self.assert_multi_regex(regexs, txt) + with self.mocked_stdout_stderr(): + self.assertErrorRegex(EasyBuildError, error_msg, self.eb_main, args, raise_error=True, testing=False) + + # check whether unstaged file in git working dir was copied (it shouldn't) + res = glob.glob(os.path.join(self.test_prefix, 'eb-*', 'eb-*', 'git-working-dir*')) + res = [d for d in res if d != git_working_dir] + if len(res) == 1: + unstaged_file_full = os.path.join(res[0], unstaged_file) + self.assertNotExists(unstaged_file_full) + else: + self.fail("Found copy of easybuild-easyconfigs working copy") + + # add required commit message, try again + args.append('--pr-commit-msg=just a test') + txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) + + regexs[-1] = r"^\s*2 files changed" + regexs.remove(r"^\* title: \"\{tools\}\[gompi/2018a\] toy v0.0 w/ test\"") + regexs.append(r"^\* title: \"just a test\"") + regexs.append(rf".*/{ec_name}\s*\|") + regexs.append(r".*[0-9]+ deletions\(-\)") + self.assert_multi_regex(regexs, txt) + + GITHUB_TEST_ORG = 'test-organization' + args.extend([ + '--pr-branch-name=branch_name_for_new_pr_test', + '--pr-commit-msg="this is a commit message. really!"', + '--pr-descr="moar letters foar teh lettre box"', + '--pr-target-branch=main', + '--github-org=%s' % GITHUB_TEST_ORG, + '--pr-target-account=boegel', # we need to be able to 'clone' from here (via https) + '--pr-title=test-1-2-3', + ]) + txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) + + regexs = [ + r"^== fetching branch 'main' from https://github.com/boegel/easybuild-easyconfigs.git...", + r"^Opening pull request \[DRY RUN\]", + r"^\* target: boegel/easybuild-easyconfigs:main", + r"^\* from: %s/easybuild-easyconfigs:branch_name_for_new_pr_test" % GITHUB_TEST_ORG, + r"\(created using `eb --new-pr`\)", # description + r"moar letters foar teh lettre box", # also description (see --pr-descr) + r"^\* title: \"test-1-2-3\"", + r"^\* overview of changes:", + r".*/toy-0.0-gompi-2018a-test.eb\s*\|", + rf".*/{ec_name}\s*\|", + r"^\s*2 files changed", + r".*[0-9]+ deletions\(-\)", + ] + self.assert_multi_regex(regexs, txt) - # modifying an existing easyconfig requires a custom PR title; - # we need to use a sufficiently recent GCC version, since easyconfigs for old versions have been archived - gcc_ec = os.path.join(test_ecs, 'g', 'GCC', 'GCC-10.2.0.eb') - gcc_new_ec = os.path.join(self.test_prefix, 'GCC-14.3.0.eb') - gcc_new_txt = read_file(gcc_ec).replace('10.2.0', '14.3.0') - write_file(gcc_new_ec, gcc_new_txt) + # should also work with a patch + args.append(toy_patch) + with self.mocked_stdout_stderr(): + self.eb_main(args, do_build=True, raise_error=True, testing=False) + txt = self.get_stdout() - args = [ - '--new-pr', - '--github-user=%s' % GITHUB_TEST_ACCOUNT, - toy_ec, - gcc_new_ec, - '-D', - ] - error_msg = "A meaningful commit message must be specified via --pr-commit-msg.*\n" - error_msg += "Modified: " + os.path.basename(gcc_new_ec) - with self.mocked_stdout_stderr(): - self.assertErrorRegex(EasyBuildError, error_msg, self.eb_main, args, raise_error=True) + regexs[-2] = r"^\s*3 files changed" + regexs.append(r".*_fix-silly-typo-in-printf-statement.patch\s*\|") + self.assert_multi_regex(regexs, txt) - # also specifying commit message is sufficient; PR title is inherited from commit message - args.append('--pr-commit-msg=this is just a test') - txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) + # modifying an existing easyconfig requires a custom PR title; + # we need to use a sufficiently recent GCC version, since easyconfigs for old versions have been archived + gcc_ec = os.path.join(test_ecs, 'g', 'GCC', 'GCC-10.2.0.eb') + gcc_new_ec = os.path.join(self.test_prefix, 'GCC-14.3.0.eb') + gcc_new_txt = read_file(gcc_ec).replace('10.2.0', '14.3.0') + write_file(gcc_new_ec, gcc_new_txt) - regex = re.compile(r'^\* title: "this is just a test"', re.M) - self.assertRegex(txt, regex) + args = common_new_pr_args + [toy_ec, gcc_new_ec] + error_msg = "A meaningful commit message must be specified via --pr-commit-msg.*\n" + error_msg += "Modified: " + os.path.basename(gcc_new_ec) + with self.mocked_stdout_stderr(): + self.assertErrorRegex(EasyBuildError, error_msg, self.eb_main, args, raise_error=True) - args = [ - # PR for EasyBuild v2.5.0 release - # we need a PR where the base branch is still available ('develop', in this case) - '--update-pr=2237', - '--github-user=%s' % GITHUB_TEST_ACCOUNT, - toy_ec, - '-D', - # only to speed things up - '--git-working-dirs-path=%s' % git_working_dir, - ] + # also specifying commit message is sufficient; PR title is inherited from commit message + args.append('--pr-commit-msg=this is just a test') + txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) - error_msg = "A meaningful commit message must be specified via --pr-commit-msg when using --update-pr" - with self.mocked_stdout_stderr(): - self.assertErrorRegex(EasyBuildError, error_msg, self.eb_main, args, raise_error=True) + regex = re.compile(r'^\* title: "this is just a test"', re.M) + self.assertRegex(txt, regex) - args.append('--pr-commit-msg=just a test') - txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) + args = [ + # PR for EasyBuild v2.5.0 release + # we need a PR where the base branch is still available ('develop', in this case) + '--update-pr=2237', + '--github-user=%s' % GITHUB_TEST_ACCOUNT, + toy_ec, + '-D', + # only to speed things up + '--git-working-dirs-path=%s' % git_working_dir, + ] - regexs = [ - r"^== Determined branch name corresponding to easybuilders/easybuild-easyconfigs PR #2237: develop", - r"^== fetching branch 'develop' from https://github.com/easybuilders/easybuild-easyconfigs.git...", - r".*/toy-0.0-gompi-2018a-test.eb\s*\|", - r"^\s*1 file(s?) changed", - r"^== pushing branch 'develop' to remote '.*' \(git@github.com:easybuilders/easybuild-easyconfigs.git\)", - r"^== pushed updated branch 'develop' to easybuilders/easybuild-easyconfigs \[DRY RUN\]", - r"^== updated https://github.com/easybuilders/easybuild-easyconfigs/pull/2237 \[DRY RUN\]", - ] - self.assert_multi_regex(regexs, txt) + error_msg = "A meaningful commit message must be specified via --pr-commit-msg when using --update-pr" + with self.mocked_stdout_stderr(): + self.assertErrorRegex(EasyBuildError, error_msg, self.eb_main, args, raise_error=True) + + args.append('--pr-commit-msg=just a test') + txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) + + repo_slug = "easybuilders/easybuild-easyconfigs" + regexs = [ + rf"^== Determined branch name corresponding to {repo_slug} PR #2237: develop", + rf"^== fetching branch 'develop' from https://github.com/{repo_slug}.git...", + r".*/toy-0.0-gompi-2018a-test.eb\s*\|", + r"^\s*1 file(s?) changed", + rf"^== pushing branch 'develop' to remote '.*' \(git@github.com:{repo_slug}.git\)", + rf"^== pushed updated branch 'develop' to {repo_slug} \[DRY RUN\]", + rf"^== updated https://github.com/{repo_slug}/pull/2237 \[DRY RUN\]", + ] + self.assert_multi_regex(regexs, txt) - # also check behaviour under --extended-dry-run/-x - args.remove('-D') - args.append('-x') + # also check behaviour under --extended-dry-run/-x + args.remove('-D') + args.append('-x') - txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) + txt, _ = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False) - regexs.extend([ - r"Full patch:", - r"^\+\+\+\s*.*toy-0.0-gompi-2018a-test.eb", - r"^\+name = 'toy'", - ]) - self.assert_multi_regex(regexs, txt) + regexs.extend([ + r"Full patch:", + r"^\+\+\+\s*.*toy-0.0-gompi-2018a-test.eb", + r"^\+name = 'toy'", + ]) + self.assert_multi_regex(regexs, txt) - # check whether comments/buildstats get filtered out - regexs = [ - r"# Built with EasyBuild", - r"# Build statistics", - r"buildstats\s*=", - ] - self.assert_multi_regex(regexs, txt, assert_true=False) + # check whether comments/buildstats get filtered out + regexs = [ + r"# Built with EasyBuild", + r"# Build statistics", + r"buildstats\s*=", + ] + self.assert_multi_regex(regexs, txt, assert_true=False) def test_github_new_pr_warning_missing_patch(self): """Test warning printed by --new-pr (dry run only) when a specified patch file could not be found."""