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

change copy_file to ignore failing copy of file metadata by using shutil.copyfile + shutil.copystat rather than shutil.copy2 #3912

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

boegel
Copy link
Member

@boegel boegel commented Dec 9, 2021

No description provided.

…til.copyfile + shutil.copystat rather than shutil.copy2 (fixes easybuilders#3910)
@akesandgren
Copy link
Contributor

akesandgren commented Dec 10, 2021

When reading the docco on shutil.copy2 I don't see the point in this change:

copy2() will preserve all the metadata it can; copy2() never raises an exception because it cannot preserve file metadata.

copy2() uses copystat() to copy the file metadata. Please see copystat() for more information about platform support for modifying symbolic link metadata.

Note the part where copy2 won't raise an exception reg metadata...

@boegel
Copy link
Member Author

boegel commented Dec 10, 2021

Hmm, that confuses me a bit, since the problem report in #3910 does show that an error was raised.
It's a No space left on device error though, due to an inode issue, I guess that would also be raised by copyfile?

@ofisette Any chance you can test the changes proposed here to see if it actually helps for the problem you're seeing? Seems like it won't...

cc @bartoldeman

@ofisette
Copy link

@boegel The copy2 function definitely raises exceptions when trying to copy metadata, contradicting the doc:

>>> from shutil import copy2
>>> copy2("/cvmfs/soft.computecanada.ca/easybuild/site-packages/easybuild-easyblocks/easybuild/easyblocks/generic/cmakemake.py", "/mnt/testfs/tmp/")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib/python3.7/shutil.py", line 267, in copy2
    copystat(src, dst, follow_symlinks=follow_symlinks)
  File "/cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib/python3.7/shutil.py", line 209, in copystat
    _copyxattr(src, dst, follow_symlinks=follow)
  File "/cvmfs/soft.computecanada.ca/gentoo/2020/usr/lib/python3.7/shutil.py", line 165, in _copyxattr
    os.setxattr(dst, name, value, follow_symlinks=follow_symlinks)
OSError: [Errno 28] No space left on device: '/mnt/testfs/tmp/cmakemake.py'

Looking at the shutil module, exceptions are only suppressed in copystat when metadata type is unsupported, EOPNOTSUPP. (And copy2 is just copyfile followed by copystat.) I hesitate to report this as a Python bug because it seems like a good choice, but the doc could be clearer.

@bartoldeman
Copy link
Contributor

copystat does

  1. utime
  2. _copyxattr
  3. chmod
    so if _copyxattr fails chmod will not be called.
    one can try copymode however if copystat fails, and then just the permission bits are copied.

There's another issue with this patch is that the if/else logic could possibly be simplified since there's another copyfile already a little further up. I'll need to look carefully to determine that.

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.

None yet

4 participants