Ignore unmodified files when using --new-pr or --update_pr

easybuild/framework/easyconfig/easyconfig.py

@@ -43,12 +43,13 @@
 
 import copy
 import difflib
+import filecmp
 import functools
 import os
 import re
 from collections import OrderedDict
 from contextlib import contextmanager
-from typing import Optional
+from typing import List, Optional
 
 import easybuild.tools.filetools as filetools
 from easybuild.base import fancylogger
@@ -2651,7 +2652,7 @@ def clean_up_easyconfigs(paths):
         write_file(path, ectxt, forced=True)
 
 
-def det_file_info(paths, target_dir):
+def det_file_info(paths: List[str], target_dir: str, ignore_unchanged_files: bool = False):
     """
     Determine useful information on easyconfig files relative to a target directory,
     before any actual operation (e.g. copying) is performed
@@ -2673,15 +2674,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 ignore_unchanged_files and 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)
@@ -2694,15 +2699,15 @@ def det_file_info(paths, target_dir):
     return file_info
 
 
-def copy_easyconfigs(paths, target_dir):
+def copy_easyconfigs(paths: List[str], target_dir: str, ignore_unchanged_files: bool = False):
     """
     Copy easyconfig files to specified directory, in the 'right' location and using the filename expected by robot.
 
     :param paths: list of paths to copy to git working dir
     :param target_dir: target directory
     :return: dict with useful information on copied easyconfig files (corresponding EasyConfig instances, paths, status)
     """
-    file_info = det_file_info(paths, target_dir)
+    file_info = det_file_info(paths, target_dir, ignore_unchanged_files)
 
     for path, target_path in zip(file_info['paths'], file_info['paths_in_repo']):
         copy_file(path, target_path, force_in_dry_run=True)
@@ -2725,6 +2730,9 @@ 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):
+            _log.debug(f"Skipping copy for file {patch_path}, identical file already exists at {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)
 

easybuild/framework/easyconfig/tools.py

@@ -513,7 +513,7 @@ def review_pr(paths=None, pr=None, colored=True, branch='develop', testing=False
             lines.extend(['', "(no related easyconfigs found for %s)\n" % os.path.basename(ec['spec'])])
 
     if pr:
-        file_info = det_file_info(pr_files, download_repo_path)
+        file_info = det_file_info(pr_files, download_repo_path, ignore_unchanged_files=True)
 
         pr_target_account = build_option('pr_target_account')
         github_user = build_option('github_user')

easybuild/tools/filetools.py

@@ -3093,29 +3093,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
 
@@ -3135,23 +3138,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
 

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
@@ -1137,38 +1137,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])
@@ -1201,21 +1169,20 @@ 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)
-        all_dep_info = copy_easyconfigs(dep_paths, target_dir)
+        all_dep_info = copy_easyconfigs(dep_paths, target_dir, ignore_unchanged_files=True)
 
         # 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])
 
     # 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]
@@ -1254,6 +1221,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)
 
@@ -1427,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:
@@ -1891,7 +1889,7 @@ def add_pr_labels(pr, branch=GITHUB_DEVELOP_BRANCH):
 
     pr_files = [p for p in fetch_easyconfigs_from_pr(pr) if p.endswith('.eb')]
 
-    file_info = det_file_info(pr_files, download_repo_path)
+    file_info = det_file_info(pr_files, download_repo_path, ignore_unchanged_files=True)
 
     pr_target_account = build_option('pr_target_account')
     github_user = build_option('github_user')
@@ -2085,7 +2083,7 @@ def new_pr_from_branch(branch_name, title=None, descr=None, pr_target_repo=None,
         # path to easyconfig files is expected to be absolute in det_file_info
         ec_paths = [os.path.join(git_working_dir, pr_target_repo, x) for x in ec_paths]
 
-        file_info = det_file_info(ec_paths, target_dir)
+        file_info = det_file_info(ec_paths, target_dir, ignore_unchanged_files=True)
 
     labels = det_pr_labels(file_info, pr_target_repo)
 
@@ -2954,7 +2952,7 @@ def sync_branch_with_develop(branch_name):
 
 # copy functions for --new-pr
 COPY_FUNCTIONS = {
-    GITHUB_EASYCONFIGS_REPO: copy_easyconfigs,
+    GITHUB_EASYCONFIGS_REPO: functools.partial(copy_easyconfigs, ignore_unchanged_files=True),
     GITHUB_EASYBLOCKS_REPO: copy_easyblocks,
     GITHUB_FRAMEWORK_REPO: copy_framework_files,
 }

test/framework/easyconfig.py

@@ -3561,11 +3561,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, ignore_unchanged_files=True)
+        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')