Skip to content

implement workaround for permission error when copying read-only files that have extended attributes set and using Python 3.6 #4642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Sep 19, 2024
Merged

implement workaround for permission error when copying read-only files that have extended attributes set and using Python 3.6 #4642

Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4adb577
fix issue when copying read-only files using shutil.copy2
jfgrimm Sep 16, 2024
8cba9e9
log when ignoring permissions error
jfgrimm Sep 16, 2024
55b4c8b
python2 support
jfgrimm Sep 17, 2024
a198ad5
fix import
jfgrimm Sep 17, 2024
5d1f2ce
actually compare file contents in case of permissions error
jfgrimm Sep 17, 2024
df9827d
more explicit python version checking to ignore permissions error
jfgrimm Sep 18, 2024
202ae52
long line
jfgrimm Sep 18, 2024
4a4312e
manually copy xattrs where appropriate
jfgrimm Sep 18, 2024
150d7c3
typo
jfgrimm Sep 18, 2024
a3b26f6
implement test for copy_file to copy read-only file with extended att…
boegel Sep 18, 2024
23b5b78
fix condition in copy_file when hitting PermissionError when copying …
boegel Sep 18, 2024
747eddd
Merge pull request #1 from boegel/read-only-copy2
jfgrimm Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 39 additions & 3 deletions easybuild/tools/filetools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,13 @@
"""
import datetime
import difflib
import filecmp
import glob
import hashlib
import inspect
import itertools
import os
import platform
import re
import shutil
import signal
Expand All @@ -59,7 +61,7 @@
from functools import partial

from easybuild.base import fancylogger
from easybuild.tools import run
from easybuild.tools import LooseVersion, run
# import build_log must stay, to use of EasyBuildLog
from easybuild.tools.build_log import EasyBuildError, dry_run_msg, print_msg, print_warning
from easybuild.tools.config import DEFAULT_WAIT_ON_LOCK_INTERVAL, ERROR, GENERIC_EASYBLOCK_PKG, IGNORE, WARN
Expand Down Expand Up @@ -2435,8 +2437,42 @@ def copy_file(path, target_path, force_in_dry_run=False):
else:
mkdir(os.path.dirname(target_path), parents=True)
if path_exists:
shutil.copy2(path, target_path)
_log.info("%s copied to %s", path, target_path)
try:
# on filesystems that support extended file attributes, copying read-only files with
# shutil.copy2() will give a PermissionError, when using Python < 3.7
# see https://bugs.python.org/issue24538
shutil.copy2(path, target_path)
_log.info("%s copied to %s", path, target_path)
except OSError as err:
# catch the more general OSError instead of PermissionError, since python2 doesn't support
# PermissionError
if not os.stat(path).st_mode & stat.S_IWUSR:
# failure not due to read-only file
raise EasyBuildError("Failed to copy file %s to %s: %s", path, target_path, err)
Copy link
Member

@boegel boegel Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part doesn't make sense to me, I think it should be removed...

If we give up here if the source file is read-only, then why bother with the attempt to fix the issue when using Python 3.6 via shutil._copyxattr below?

Copy link
Member

@boegel boegel Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with @jfgrimm in Slack, the condition is wrong here, should be if os.stat...

Also, it makes more sense to me to check whether the target file is read-only or not?

Fixed in jfgrimm#1


pyver = LooseVersion(platform.python_version())
if pyver >= LooseVersion('3.7'):
raise EasyBuildError("Failed to copy file %s to %s: %s", path, target_path, err)
elif LooseVersion('3.7') > pyver >= LooseVersion('3'):
if not isinstance(err, PermissionError):
raise EasyBuildError("Failed to copy file %s to %s: %s", path, target_path, err)

# double-check whether the copy actually succeeded
if not os.path.exists(target_path) or not filecmp.cmp(path, target_path, shallow=False):
raise EasyBuildError("Failed to copy file %s to %s: %s", path, target_path, err)

try:
# re-enable user write permissions in target, copy xattrs, then remove write perms again
adjust_permissions(target_path, stat.S_IWUSR)
shutil._copyxattr(path, target_path)
adjust_permissions(target_path, stat.S_IWUSR, add=False)
Copy link
Member

@ocaisa ocaisa Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, this is correct but quite confusing (add=False means remove the permissions)

Copy link
Member Author

@jfgrimm jfgrimm Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, hence the comment at the top of that block
it's not the most intuitive that add=False removes the permissions

Copy link
Member

@boegel boegel Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we open an issue to improve the API of adjust_permissions in EasyBuild 5.0?

except OSError as err:
raise EasyBuildError("Failed to copy file %s to %s: %s", path, target_path, err)

msg = ("Failed to copy extended attributes from file %s to %s, due to a bug in shutil (see "
"https://bugs.python.org/issue24538). Copy successful with workaround.")
_log.info(msg, path, target_path)

elif os.path.islink(path):
if os.path.isdir(target_path):
target_path = os.path.join(target_path, os.path.basename(path))
Expand Down