gh-91205: fix bug in shutil.copytree with relative links and ignore_dangling_symlinks=True #132984
Conversation
…nore_dangling_symlinks=True.
Misc/NEWS.d/next/Library/2025-04-26-13-44-46.gh-issue-91205.kYPo51.rst
Outdated
Show resolved
Hide resolved
| def test_copytree_relative_symlink(self): | ||
| # Ensure valid relative symlinks are copied regardless of the value of | ||
| # the ignore_dangling_symlinks flag (see gh-91205) | ||
| src_dir = self.mkdtemp() |
There was a problem hiding this comment.
| src_dir = self.mkdtemp() | |
| src_dir = self.mkdtemp() | |
| self.addCleanup(shutil.rmtree, src_dir, ignore_errors=True) |
There was a problem hiding this comment.
I don't think this is necessary (here and elsewhere): the self.mkdtemp() method called here calls self.addCleanup(os_helper.rmtree, d) on the result, so unless I'm missing something it shouldn't be required. I note most of the calls to self.mkdtemp() in this module do not add extra cleanup, although some do. The test doesn't warn about files being left behind, so they are being cleaned up properly without this.
|
|
||
| for ignore_dangling_symlinks in (True, False): | ||
| with self.subTest(ignore_dangling_symlinks=ignore_dangling_symlinks): | ||
| dst_dir = os.path.join(self.mkdtemp(), 'x') |
There was a problem hiding this comment.
| dst_dir = os.path.join(self.mkdtemp(), 'x') | |
| dst_dir = os.path.join(self.mkdtemp(), 'x') | |
| self.addCleanup(shutil.rmtree, dst_dir, ignore_errors=True) |
| # if the link is not to an absolute path it is relative to | ||
| # the source (see gh-91205) | ||
| if not os.path.isabs(linkto): | ||
| linkto = os.path.join(os.path.dirname(srcname), linkto) |
There was a problem hiding this comment.
The original patch used os.path.normpath. Do we need it? Also, shouldn't we retest whether linkto is a symlink or not? if not, please also add a test
There was a problem hiding this comment.
I don't think we should use normpath: it does a string replacement which can change the meaning if symbolic links are involved, which I think would be a bug if it can happen. If it doesn't change the meaning then it shouldn't matter, since we just use the result in one place immediately after, to check whether it exists or not.
We don't need to check whether the result is a link: it may or may not be, but is correctly handled either way by the following code. os.path.exists will return false if it is a dangling link (and we will either skip it or carry on and raise an error depending on ignore_dangling_symlinks). Note that absolute symbolic links are handled the same way, so if this was necessary they would already be broken.
Having said that, good point about testing: AFAICT this case is not currently under test. I'll add a few levels of valid links-to-links to the new test case and expand test_copytree_dangling_symlinks to test dangling links-to-links.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
… nested symbolic link behaviour.
Fix bug where
shutil.copytreewas skipping copying the contents of symbolic links to relative paths when theignore_dangling_symlinksflag was set.