From 80dc7a673fd2a287996323945d5dbdc4f691f2da Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Mon, 12 Aug 2024 13:39:24 +0200 Subject: [PATCH 1/2] fix `copy_file` with a folder as the target This use case comes up with e.g. `--copy-ec /tmp`. The existing code will see that `/tmp` exists and has a different UID and hence will use `shutil.copy_file` which expects the target to not be a folder. It hence fails with "[Errno 21] Is a directory" Use `shutil.copy` as was likely intended anyway. --- easybuild/tools/filetools.py | 10 +++++++--- test/framework/filetools.py | 13 +++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index 50118c2e28..6d0f89f8b0 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -2419,14 +2419,18 @@ def copy_file(path, target_path, force_in_dry_run=False): try: # check whether path to copy exists (we could be copying a broken symlink, which is supported) path_exists = os.path.exists(path) + # If target is a folder, the target_path will be a file with the same name inside the folder + if os.path.isdir(target_path): + target_path = os.path.join(target_path, os.path.basename(path)) target_exists = os.path.exists(target_path) + if target_exists and path_exists and os.path.samefile(path, target_path): _log.debug("Not copying %s to %s since files are identical", path, target_path) # if target file exists and is owned by someone else than the current user, - # try using shutil.copyfile to just copy the file contents - # since shutil.copy2 will fail when trying to copy over file metadata (since chown requires file ownership) + # copy just the file contents (shutil.copy instead of shutil.copy2) + # since copying the file metadata will fail since chown requires file ownership elif target_exists and os.stat(target_path).st_uid != os.getuid(): - shutil.copyfile(path, target_path) + shutil.copy(path, target_path) _log.info("Copied contents of file %s to %s", path, target_path) else: mkdir(os.path.dirname(target_path), parents=True) diff --git a/test/framework/filetools.py b/test/framework/filetools.py index ae32993d9e..1a0294a02f 100644 --- a/test/framework/filetools.py +++ b/test/framework/filetools.py @@ -1856,6 +1856,19 @@ def test_copy_file(self): # printing this message will make test suite fail in Travis/GitHub CI, # since we check for unexpected output produced by the tests print("Skipping overwrite-file-owned-by-other-user copy_file test (%s is missing)" % test_file_to_overwrite) + # Copy a file to a directory owned by some other user, e.g. /tmp (owned by root) + # This might be a common choice for e.g. --copy-ec + target_file_path = tempfile.mktemp("easybuild", dir="/tmp") + test_file_to_copy = os.path.join(self.test_prefix, os.path.basename(target_file_path)) + ft.write_file(test_file_to_copy, test_file_contents) + try: + ft.copy_file(test_file_to_copy, '/tmp') + self.assertEqual(ft.read_file(target_file_path), test_file_contents) + finally: + try: + os.remove(target_file_path) + except FileNotFoundError: + pass # also test behaviour of copy_file under --dry-run build_options = { From f178aca139794c74088bae0a1900a6a9c8b7e231 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Mon, 12 Aug 2024 14:38:49 +0200 Subject: [PATCH 2/2] Switch back to `shutil.copyfile` `shutil.copy` tries to change permissions which isn't allowed in the case where it is used. As we already made sure the target is a file we can use `shutil.copyfile` --- easybuild/tools/filetools.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/easybuild/tools/filetools.py b/easybuild/tools/filetools.py index 6d0f89f8b0..06ca1dddb5 100644 --- a/easybuild/tools/filetools.py +++ b/easybuild/tools/filetools.py @@ -2427,10 +2427,10 @@ def copy_file(path, target_path, force_in_dry_run=False): if target_exists and path_exists and os.path.samefile(path, target_path): _log.debug("Not copying %s to %s since files are identical", path, target_path) # if target file exists and is owned by someone else than the current user, - # copy just the file contents (shutil.copy instead of shutil.copy2) - # since copying the file metadata will fail since chown requires file ownership + # copy just the file contents (shutil.copyfile instead of shutil.copy2) + # since copying the file metadata/permissions will fail since chown requires file ownership elif target_exists and os.stat(target_path).st_uid != os.getuid(): - shutil.copy(path, target_path) + shutil.copyfile(path, target_path) _log.info("Copied contents of file %s to %s", path, target_path) else: mkdir(os.path.dirname(target_path), parents=True)