Opened 6 years ago
Closed 6 years ago
#21599 closed defect (fixed)
Work around non-deterministic failure of uncompress on Windows
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-7.4 |
Component: | porting: Cygwin | Keywords: | |
Cc: | Merged in: | ||
Authors: | Erik Bray | Reviewers: | Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | 69830cd (Commits, GitHub, GitLab) | Commit: | 69830cd977cd6ca47d623044e3cdf9fea6f164ae |
Dependencies: | Stopgaps: |
Description
I've noticed sometimes while building on Windows / Cygwin (though this isn't cygwin-specific) some package builds can fail randomly, usually during the uncompress step. Re-rerunning the build a second time always succeeds.
This happened one time recently and I realized there was a Python traceback leading back to the os.rename
call wrapped by the attached patch.
This can happen because if any background process happens to have any file in the tree open, even briefly, the renaming the entire directory can fail. So this is especially likely to happen when unpacking source tarballs containing a large number of files.
There may be other points where this can happen, and I'll apply the same workaround if/when I encounter them.
Change History (12)
comment:1 Changed 6 years ago by
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Status changed from needs_review to needs_work
comment:3 Changed 6 years ago by
D'oh! Agreed with all of the above.
comment:4 Changed 6 years ago by
And yes it's an ugly hack but not an uncommon one. I feel like I've done this a dozen other times for Windows support in various places.
comment:5 Changed 6 years ago by
- Commit changed from 21d46ab3f71017d5e982ee6c844b44c6ca2f2528 to 822f0d11c83d0f9812d435ef65a47f750820e142
Branch pushed to git repo; I updated commit sha1. New commits:
822f0d1 | Re-raise the exception if it is raised on the final try.
|
comment:6 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:7 Changed 6 years ago by
- Status changed from needs_review to needs_work
Two minor things again:
- Replace
func()
byreturn func()
. This way, you support functions which return something. It also means that you can remove theelse: break
.
- The condition
while tries > 0:
is now always true. So for simplicity, I would prefer to seewhile True
there. That makes it clear that the loop can never be exited normally. By the way, this is a good example of code which would be cleaner with agoto
statement.
comment:8 Changed 6 years ago by
Agreed again on all of the above.
The only people who think gotos are evil have never written non-trivial C code and haven't read/don't understand the context of Dijkstra's essay :)
comment:9 Changed 6 years ago by
- Commit changed from 822f0d11c83d0f9812d435ef65a47f750820e142 to 69830cd977cd6ca47d623044e3cdf9fea6f164ae
Branch pushed to git repo; I updated commit sha1. New commits:
69830cd | Additional minor cleanup:
|
comment:10 Changed 6 years ago by
- Status changed from needs_work to needs_review
New commits:
69830cd | Additional minor cleanup:
|
comment:11 Changed 6 years ago by
- Reviewers set to Jeroen Demeyer
- Status changed from needs_review to positive_review
comment:12 Changed 6 years ago by
- Branch changed from u/embray/bug/uncompress-windows to 69830cd977cd6ca47d623044e3cdf9fea6f164ae
- Resolution set to fixed
- Status changed from positive_review to closed
And then some bikeshedding:
delay *= 2
in the while loop.tries
instead ofretries
. I find it easier to understand "3 tries" instead of "1 try and 2 retries". And you know that, because you found it necessary to explain in the docstring.And of course this is an ugly hack, but that's not your fault.