Install script: Windows-compatible cleanup path. #16928
Conversation
Windows might need some retrying around deleting the target directory.
misc/bazel/internal/install.py
Outdated
| if platform.system() == 'Windows': | ||
| # On Windows we might have virus scanner still looking at the path so | ||
| # attempt removal a couple of times sleeping between each attempt. | ||
| for attempt in [1, 2]: |
There was a problem hiding this comment.
I think there's a bug here:
- we enter the loop with
attempt == 1. - we try
rmtree. Let's suppose we fail withwinerror == 32 - we sleep 1s
- we reenter the loop with
attempt == 2. - we try
rmtreeagain. Let's suppose we fail again withwinerror == 32 - we sleep for 2s, but then...
- ...we exit the loop and we move on, having silently failed to remove the directory, and having slept 2s for no reason
we need to do a final rmtree after the for unless we succeeded. We could do that by adding a
else:
shutil.rmtree(destdir)after the for loop, but I know not all are comfortable with the for: ... else: ... construct (else is visited only if the for was not interrupted by a break)
There was a problem hiding this comment.
Fixed. The code is exceedingly ugly now though :(
misc/bazel/internal/install.py
Outdated
| shutil.rmtree(destdir) | ||
| break | ||
| except OSError as e: | ||
| if e.winerror == 32: |
There was a problem hiding this comment.
can we give a name for this 32 magic number?
There was a problem hiding this comment.
I don't think python exposes a public api for this, they themselves do this: https://github.com/python/cpython/blob/8ad6067bd4556afddc86004f8e350aa672fda217/Lib/shutil.py#L456
There was a problem hiding this comment.
yeah, I found out as much, but we can still give it a name in this script to help the reader 🙂
There was a problem hiding this comment.
actually, as far as I could test an OSError with .winerror == 32 actually gets raised as a more specific PermissionError, but I guess the .winerror == 32 specifies this is the "still in use" error. So I just suggest we add a still_in_use_winerror = 32 and use that for readability
| else: | ||
| shutil.rmtree(destdir) | ||
| else: | ||
| shutil.rmtree(destdir) |
There was a problem hiding this comment.
maybe to lower the ugliness we can extract this code into our own rmtree function, replace break with return, and collapse these two elses into one fall-through
| else: | |
| shutil.rmtree(destdir) | |
| else: | |
| shutil.rmtree(destdir) | |
| shutil.rmtree(destdir) |
?
|
Let's merge this as-is, creating a function is (almost) line-count-neutral. If we had pypi or something available here, I'd rather pull in the retry package with which this could be expressed much neater. |
Windows might need some retrying around deleting
the target directory.