Skip to content
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.

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

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

Merged
merged 12 commits into from
Sep 19, 2024

Conversation

jfgrimm
Copy link
Member

@jfgrimm jfgrimm commented Sep 16, 2024

Ran into this issue like this, when configuring EasyBuild with read-only-installdir:

== 2024-09-16 13:26:56,117 build_log.py:171 ERROR EasyBuild crashed with an error (at easybuild/base/exceptions.py:126 in __init__): Failed to copy file /mnt/scratch/users/jfg508/eb/software/EasyBuild/4.9.2/lib/python3.6/site-packages/easybuild/easyblocks/e/easybuildmeta.py to /tmp/eb-l7y97vxy/reprod_20240916132656_823857/easyblocks/easybuildmeta.py: [Errno 13] Permission denied: '/tmp/eb-l7y97vxy/reprod_20240916132656_823857/easyblocks/easybuildmeta.py' (at easybuild/tools/filetools.py:2437 in copy_file)

Having taken a closer look, it seems that on our system shutil.copy2() spits out a PermissionError when the file to be copied is read-only. However, the copy still succeeds (and has the correct contents).

@ocaisa
Copy link
Member

ocaisa commented Sep 16, 2024

I think this is the real problem that I thought I was solving with #4604

@jfgrimm jfgrimm added this to the release after 4.9.3 milestone Sep 16, 2024
boegel
boegel previously requested changes Sep 17, 2024
easybuild/tools/filetools.py Outdated Show resolved Hide resolved
@jfgrimm jfgrimm requested a review from boegel September 17, 2024 18:54
@ocaisa
Copy link
Member

ocaisa commented Sep 18, 2024

To me this now looks good, but can we add a test for it? It may not be very thorough, but you should just be able to create a file, change the permissions to read-only and then attempt a copy. I don't know if that is guaranteed to hit the LoC, but it is worth a shot. Should be a variation on

testdir = os.path.dirname(os.path.abspath(__file__))
toy_ec = os.path.join(testdir, 'easyconfigs', 'test_ecs', 't', 'toy', 'toy-0.0.eb')
target_path = os.path.join(self.test_prefix, 'toy.eb')
ft.copy_file(toy_ec, target_path)
self.assertExists(target_path)
self.assertTrue(ft.read_file(toy_ec) == ft.read_file(target_path))

@jfgrimm
Copy link
Member Author

jfgrimm commented Sep 18, 2024

I have traced the underlying issue to: https://bugs.python.org/issue24538

So this should only appear in python < 3.7

@jfgrimm
Copy link
Member Author

jfgrimm commented Sep 18, 2024

since the _copyxattr issue is filesystem-dependent, and the issue was fixed in python 3.7+, I don't know that it's of much value adding a test?

ocaisa
ocaisa previously approved these changes Sep 18, 2024
@ocaisa ocaisa enabled auto-merge September 18, 2024 15:57
@ocaisa
Copy link
Member

ocaisa commented Sep 18, 2024

Like everything, this was more work than it seemed initially!

@jfgrimm
Copy link
Member Author

jfgrimm commented Sep 18, 2024

@ocaisa I've pushed another commit (or two) that actually use the upstream approach to ensure the extended attributes are truly copied as well. A cleaner fix imo

# 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

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

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

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?

ocaisa
ocaisa previously approved these changes Sep 18, 2024
@boegel
Copy link
Member

boegel commented Sep 18, 2024

There really should be a dedicated test that verifies whether the changes indeed fix the problem at hand.

I puzzled one together, see jfgrimm#1

Unless I'm misunderstanding something, the extra test shows the fix isn't fully working as designed yet, since it fails on Python 3.6.8 in RHEL8...

Comment on lines 2449 to 2451
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

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

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

@boegel boegel changed the title fix issue when copying read-only files using shutil.copy2 implement workaround for permission error when copying read-only files that have extended attributes set and using Python 3.6 Sep 19, 2024
@boegel boegel merged commit bf0af10 into easybuilders:develop Sep 19, 2024
37 checks passed
@jfgrimm jfgrimm deleted the read-only-copy2 branch September 19, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants