gh-113188: Fix shutil.copymode() on Windows #113189
Conversation
Previously it worked differenly if dst is a symbolic link: it modified the permission bits of dst itself rather than the file it points to if follow_symlinks is true or src is not a symbolic link, and did nothing if follow_symlinks is false and src is a symbolic link. Also document similar changes in shutil.copystat() etc.
Doc/library/shutil.rst
Outdated
| In older Python versions :func:`!copymode` worked differenly on Windows | ||
| if *dst* is a symbolic link: it modified the permission bits of *dst* | ||
| itself rather than the file it points to if *follow_symlinks* is true | ||
| or *src* is not a symbolic link, and did not modify the permission bits | ||
| if *follow_symlinks* is false and *src* is a symbolic link. |
There was a problem hiding this comment.
I would prefer this to be phrased as what the function does now, probably just "copymode now behaves as intended on Windows when follow_symlinks is True", perhaps followed by "Previously, it would ..."
Unfortunately, the logic here is so complicated I can't figure out exactly what it does. I just spent 15 minutes trying to propose better wording. But that's probably a sign that it is going to confuse people.
There was a problem hiding this comment.
In short, it did os.lchmod() when POSIX does os.chmod(), and did nothing when POSIX does os.lchmod().
If we are going to consider it a bug fix (even if it never worked correctly on Windows), we do not need these complicated versionchanged and What's New entries, only a NEWS entry. I'm leaning towards this solution, but want to hear an outside opinion.
There was a problem hiding this comment.
But the backports will use more complicated code, because os.chmod(follow_symlinks=True) did not work.
There was a problem hiding this comment.
If it requires implementing chmod properly as well, then I'd say don't backport.
There was a problem hiding this comment.
The workaround is to resolve a symlink and calling chmod for a resolved path.
It can also be a half-solution -- only fix it for follow_symlinks=False, it is much easier.
There was a problem hiding this comment.
I guess I don't have a strong opinion then.
My main worry would be breaking someone who understands the current behaviour and has chosen not to work around it because it already does what they want.
It looks like follow_symlinks is already present on maintenance versions, so maybe it's just best to backport the chmod fix and switch the default value on Windows to reflect the current behaviour? That gives us a bigger matrix for people to specify an explicit value and get the right behaviour, and would keep the code more consistent between versions. I assume we can do different defaults on different platforms, yes?
Previously it worked differently if dst is a symbolic link: it modified the permission bits of dst itself rather than the file it points to if follow_symlinks is true or src is not a symbolic link, and did nothing if follow_symlinks is false and src is a symbolic link. Also document similar changes in shutil.copystat().
Previously it worked differently if dst is a symbolic link: it modified the permission bits of dst itself rather than the file it points to if follow_symlinks is true or src is not a symbolic link, and did nothing if follow_symlinks is false and src is a symbolic link. Also document similar changes in shutil.copystat().
Previously it worked differently if dst is a symbolic link: it modified the permission bits of dst itself rather than the file it points to if follow_symlinks is true or src is not a symbolic link, and did nothing if follow_symlinks is false and src is a symbolic link. Also document similar changes in shutil.copystat().
Previously it worked differently if dst is a symbolic link: it modified the permission bits of dst itself rather than the file it points to if follow_symlinks is true or src is not a symbolic link, and did nothing if follow_symlinks is false and src is a symbolic link. Also document similar changes in shutil.copystat().
Previously it worked differenly if dst is a symbolic link: it modified the permission bits of dst itself rather than the file it points to if follow_symlinks is true or src is not a symbolic link, and did nothing if follow_symlinks is false and src is a symbolic link.
Also document similar changes in shutil.copystat() etc.
📚 Documentation preview 📚: https://cpython-previews--113189.org.readthedocs.build/