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

when trying to create a folder gracefully fall back when it already exists #1083

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@dirk-thomas
Copy link

dirk-thomas commented Jul 13, 2017

When installing multiple packages into the same location in parallel each packages installation step tries to create the same directories (e.g. the bin folder). Currently the isdir and makedirs call are performed sequentially which makes this logic vulnerable to a race condition. The isdir call will return False but the following makedirs call will fail since the directory has been created in the meantime by another package being installed in parallel.

This patch changes the logic to use the Pythonic approach to try to create the directory and in case it already exists ignore the exception.

@benoit-pierre

This comment has been minimized.

Copy link
Member

benoit-pierre commented Jul 13, 2017

See #1063 for the CI failures.

dirk-thomas and others added some commits Jul 13, 2017

minimal_egg_info string in bootstrap.py needs to be unicode.
I believe this is likely to be safe in general, but have only tested with 2.7 on on OS X.

Without this change, I am seeing error:

```
$ git clone git@github.com:pypa/setuptools
Cloning into 'setuptools'...
remote: Counting objects: 24321, done.
remote: Compressing objects: 100% (23/23), done.
remote: Total 24321 (delta 11), reused 20 (delta 7), pack-reused 24291
Receiving objects: 100% (24321/24321), 28.77 MiB | 7.38 MiB/s, done.
Resolving deltas: 100% (8669/8669), done.
$ cd setuptools
$ git rev-parse HEAD
995d309
$ python bootstrap.py
adding minimal entry_points
Traceback (most recent call last):
  File "bootstrap.py", line 63, in <module>
    __name__ == '__main__' and main()
  File "bootstrap.py", line 59, in main
    ensure_egg_info()
  File "bootstrap.py", line 37, in ensure_egg_info
    build_egg_info()
  File "bootstrap.py", line 47, in build_egg_info
    ep.write(minimal_egg_info)
TypeError: must be unicode, not str
```
@dirk-thomas

This comment has been minimized.

Copy link
Author

dirk-thomas commented Jul 13, 2017

@benoit-pierre Thank you for the quick pointer. With that change CI passes.

@jaraco
Copy link
Member

jaraco left a comment

Overall, I agree with the sentiment; now just want to get the details right.

@@ -12,7 +12,7 @@
import io


minimal_egg_info = textwrap.dedent("""
minimal_egg_info = textwrap.dedent(u"""

This comment has been minimized.

@jaraco

jaraco Jul 13, 2017

Member

Use from __future__ import unicode_literals if unicode literals are required... which I don't think they are here. Scrap this change.

This comment has been minimized.

@dirk-thomas

dirk-thomas Jul 13, 2017

Author

The change from ##1063 is only being pulled in here to make CI pass.

@@ -2958,8 +2959,11 @@ def _find_adapter(registry, ob):
def ensure_directory(path):
"""Ensure that the parent directory of `path` exists"""
dirname = os.path.dirname(path)
if not os.path.isdir(dirname):
try:

This comment has been minimized.

@jaraco

jaraco Jul 13, 2017

Member

This is good, and I was going to accept it straight-up, but given that this technique is superseded in Python 3.2 with the exists_ok parameter. Therefore, I'd prefer to use the py27compat or py31compat module to house this legacy approach. Can you put it in py31compat (as a makedirs function that accepts exists_ok kwarg)?

This comment has been minimized.

@dirk-thomas

dirk-thomas Jul 13, 2017

Author

That is why the branch is called "makedirs_exist_ok" 😉 But since the code needs to work in Python < 3.2 I didn't use the new parameter.

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jul 13, 2017

Ugh. I now I note that since this is in pkg_resources, and pkg_resources can't import setuptools, we need a new py31compat module in pkg_resources. I'll just make the changes.

@dirk-thomas

This comment has been minimized.

Copy link
Author

dirk-thomas commented Jul 13, 2017

I'll just make the changes.

Great, thank you for getting this fixed.

Can you please comment here with a reference to the applied change once that is done. That will allow future users to trace as of which version the fix from this PR will be available.

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jul 13, 2017

Should be fixed in v36.1.0, going out now (assuming tests pass).

@dirk-thomas

This comment has been minimized.

Copy link
Author

dirk-thomas commented Jul 13, 2017

I took a look at the patch you committed but it seems to be incorrect: see 925dd35#commitcomment-23081646

jaraco added a commit that referenced this pull request Jul 13, 2017

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jul 13, 2017

Should be fixed in 2cd7f22. Release imminent.

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jul 13, 2017

I'm trying to figure out now what's going on with https://travis-ci.org/pypa/setuptools/jobs/253323051.

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jul 13, 2017

Ugh. So exists_ok had some extra complication with it before Python 3.3.6. From the docs:

Changed in version 3.3.6: Before Python 3.3.6, if exist_ok was True and the directory existed, makedirs() would still raise an error if mode did not match the mode of the existing directory. Since this behavior was impossible to implement safely, it was removed in Python 3.3.6. See issue 21082.

Seems it also affects Python 3.2 prior to 3.2.5 and Python 3.4 prior to 3.4.1.

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jul 13, 2017

v36.1.1 going out now.

@dirk-thomas

This comment has been minimized.

Copy link
Author

dirk-thomas commented Jul 13, 2017

Great, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.