bpo-38688: Consume iterator and create list of entries to prevent infinite loop

[kinow]

Hi,

Reverted part of 33695 due to the regression reported in 38688. Text below is a copy-paste from my comment in the bugs.python.org issue.

It looks to me that the iterator returned by with os.scandir(...) is including the newly created dst directory. See the call for os.makedirs(dst, exist_ok=dirs_exist_ok), just before the iterator is open:

cpython/Lib/shutil.py

Lines 449 to 453 in e27449d

	os.makedirs(dst, exist_ok=dirs_exist_ok)
	errors = []
	use_srcentry = copy_function is copy2 or copy_function is copy
	for srcentry in entries:

This results in the function finding an extra directory, and repeating the steps for this folder and its subfolder recursively. This only happens because in the example reported in the issue, dst is a subdirectory of src.

The bpo-33695 commit had more changes, so I've reverted just this block of the copytree as a tentative fix, and added a unit test.

--

Here's a simplified version of what's going on:

import os
import shutil

# create dirs and files
shutil.rmtree('/tmp/test/', True)
os.makedirs('/tmp/test')
with open('/tmp/test/foo', 'w+') as f:
  f.write('foo')

# now we have /tmp/test/foo, let's simulate what happens in copytree on master

with os.scandir('/tmp/test') as entries:
  # up to this point, /tmp/test/foo is the only entry
  os.makedirs('/tmp/test/1/2/3/', exist_ok=True) # <---- when the iterator starts below in `f in entries`, it will find 1 too
  # now 1 will have been added too
  for f in entries:
    print(f)

My first PR here. Let me know if I forgot anything. Happy to update the PR as necessary 👍

Bruno

https://bugs.python.org/issue38688

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@kinow

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

[kinow]

CLA signed, though not sure how long it may take for the bot to kick in again 👍

[giampaolo]

Please also do:

-        ignored_names = ignore(src, set(os.listdir(src)))
+        ignored_names = ignore(src, set([x.name for x in entries]))

[kinow]

ignored_names = ignore(src, set([x.name for x in entries]))

Ah, good spot! Done 👍

[giampaolo]

Add a Misc/NEWS entry and we're good to go (check out "python devguide" on how to do it).

[kinow]

@giampaolo I've pushed a new commit with the news entry (used the blurb tool). First time contributing, so not sure if I've chosen the right category and if the text looks good.

I've rebased again, and push-force with a change to the previous commit, but only to the test. There was a failure on Travis/Azure but on another test that I think shouldn't be related to this change. But there is also a warning that the test I've added alters the environment.

I had copied another test to use as reference, so I changed a bit the test clean up to see if this message will go away 🤞

And huge thanks for the help and patience so far!
Bruno

[kinow]

Well, test warning for altering execution environment is still there, but not sure if introduced in this change. test_pty failed, but I guess it's not related to this change? At least couldn't see shutil being used.

[bedevere-bot]

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[kinow]

I have made the requested changes; please review again.

[bedevere-bot]

Thanks for making the requested changes!

@giampaolo: please review the changes made to this pull request.

[giampaolo]

        src_dir = os.path.join(base_dir, "t", "pg")
        dst_dir = os.path.join(src_dir, "somevendor", "1.0")

[kinow]

Ah! So close! Fixed. Thanks!

[miss-islington]

Thanks @kinow for the PR, and @giampaolo for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

[miss-islington]

Sorry, @kinow and @giampaolo, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 9bbcbc9f6dfe1368fe7330b117707f828e6a2c18 3.8

[giampaolo]

@kinow please also create a backport for 3.8.

[kinow]

On it @giampaolo, if nothing goes wrong hoping to have a PR in a couple hours or more (just need to learn how to use the cherry-picker tool first).

[giampaolo]

No rush. Thanks.

[bedevere-bot]

GH-17397 is a backport of this pull request to the 3.8 branch.