From 426f0a907d45fb2f567c8302e367261432598d46 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 4 Feb 2025 13:12:19 +0100 Subject: [PATCH 1/2] 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 | 44 ++++++------ test/framework/easyconfig.py | 24 +++++-- test/framework/filetools.py | 19 +++++- 5 files changed, 108 insertions(+), 66 deletions(-) diff --git a/easybuild/framework/easyconfig/easyconfig.py b/easybuild/framework/easyconfig/easyconfig.py index 57c910c746..2bc8629ebc 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 @@ -2446,15 +2447,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) @@ -2498,6 +2503,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 c5bf349451..8c874c03fa 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -2890,29 +2890,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 @@ -2932,23 +2935,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 e1751069d5..489660d762 100644 --- a/easybuild/tools/github.py +++ b/easybuild/tools/github.py @@ -1025,28 +1025,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: - 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) - # 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]) @@ -1126,6 +1104,28 @@ def _easyconfigs_pr_common(paths, ecs, start_branch=None, pr_branch=None, start_ raise EasyBuildError("No changed files found when comparing to current develop branch. " "Refused to make empty pull request.") + # figure out commit message to use + if commit_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) + # commit git_repo.index.commit(commit_msg) diff --git a/test/framework/easyconfig.py b/test/framework/easyconfig.py index 09801bde50..ed5d71c804 100644 --- a/test/framework/easyconfig.py +++ b/test/framework/easyconfig.py @@ -3356,11 +3356,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 2e2b030fc8..980543267c 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -3215,6 +3215,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.""" @@ -3246,7 +3253,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) @@ -3281,6 +3288,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 0cb1dd445fe50fde4bfceb69be42e7404c99a156 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 4 Feb 2025 13:19:46 +0100 Subject: [PATCH 2/2] 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 489660d762..1e73758212 100644 --- a/easybuild/tools/github.py +++ b/easybuild/tools/github.py @@ -1061,8 +1061,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])