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 easybuilders#4751

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)
 

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
 

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)
 

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')

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."""