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

easybuild/framework/easyconfig/easyconfig.py

@@ -43,6 +43,7 @@
 
 import copy
 import difflib
+import filecmp
 import functools
 import os
 import re
@@ -2623,15 +2624,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)
@@ -2675,6 +2680,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)
 

easybuild/tools/filetools.py

@@ -3095,29 +3095,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
 
@@ -3137,23 +3140,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)
 
         # 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:

test/framework/easyconfig.py

@@ -3572,11 +3572,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')

test/framework/filetools.py

@@ -3637,6 +3637,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."""
 
@@ -3668,7 +3675,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)
@@ -3703,6 +3710,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."""