bpo-36656: Add race-free os.symlink wrapper / helper #14464
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
|
This is so long overdue. I would suggest to use the implementation here as well. https://github.com/python/cpython/blob/master/Lib/distutils/dir_util.py#L152 |
|
Until this is merged, and for those who need support for this kind of thing in older pythons, here's an example of how to workaround it with monkeypatching: You can replace the provided implementation with the race-free logic @HaleTom provided if necessary. |
|
@solvingj thanks for drawing my attention back to this, and yet another reason to implement it. This is my first contribution to cpython, and I'll be asking on the tutor mail list on how to proceed. Please feel free to poke me if you don't see any activity within 2 weeks. |
There was a problem hiding this comment.
Blurb needed. This did not make 3.8 or 3.9, so 3.10 is soonest for what's new.
Move todo to issue.
Reread https://www.python.org/dev/peps/pep-0008/
In particular https://www.python.org/dev/peps/pep-0008/#blank-lines
"Method definitions inside a class are surrounded by a single blank line."
Doc/whatsnew/3.8.rst
Outdated
| @@ -1138,6 +1138,10 @@ The new :func:`shlex.join` function acts as the inverse of :func:`shlex.split`. | |||
| shutil | |||
| ------ | |||
|
|
|||
| Added :func:`shutil.link` and :func:`shutil.symlink` which allow for multiple | |||
There was a problem hiding this comment.
Move this entry to to a blurb. One is required. Since we are well before the 3.10 cutoff next May/June, maybe to 3.10.rst, but not required.
There was a problem hiding this comment.
@terryjreedy w.r.t the blank lines, I think that my additions to shutil.py are covered by:
Surround top-level function and class definitions with two blank lines.
So, given this, (and flake8's complaints), I intend to revert the commit I made to reduce them to only one.
Please tell me if I've misunderstood or if you were referring to test_shutil.py instead, where I've used the following to group tests:
Extra blank lines may be used (sparingly) to separate groups of related functions.
I used 8 extra blank lines for functional area grouping.
|
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 |
|
Did you originally create shutil-links branch from master? Did you analyze all of the failures (no longer accessible from here) to see if related to patch? Is there any way patch could affect builds? If not, there could be intermittent unrelated problem. I just got a clean 'all pass' with a trivial PR. |
| import tempfile | ||
| try: | ||
| # Loop until successful creation of previously non-existent temp pathname | ||
| while True: |
There was a problem hiding this comment.
Can this use mkdtemp and create the temp file inside of that dir, instead of the loop?
bpo-36656: Add race-free os.symlink wrapper / helper
Based on the positive support of Inada Naoki and Serhiy Storchaka here:
https://code.activestate.com/lists/python-ideas/56421/
I offer this PR as a proposed implementation for
shutil.symlink.Notes
shutil._create_or_replaceis called by bothshutil.linkandshutil.symlink, the latter two being simple wrappers._create_or_replacedoes the work of assuring that if a destination already exists, it is atomically replaced.Related BPO
This PR is an enabler to resolve the symlink overwrite issue:
bpo-41134:
distutils.dir_util.copy_treeFileExistsErrorwhen updating symlinksPrior art
python-ideasdiscussion: https://code.activestate.com/lists/python-ideas/55992/https://bugs.python.org/issue36656