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
TST/BF: enable overwriting downloads under Windows #5201
Conversation
mental note: remove the known_failure_githubciwin annotation of |
yay, the two tests in question ( |
Codecov Report
@@ Coverage Diff @@
## maint #5201 +/- ##
==========================================
- Coverage 89.90% 89.90% -0.01%
==========================================
Files 294 294
Lines 40953 40951 -2
==========================================
- Hits 36820 36818 -2
Misses 4133 4133
Continue to review full report at Codecov.
|
Thanks, @adswa. I wasn't aware of
Hmm, do you mean by looking at the builds for this PR? I think we haven't had a windows build for the github ci since gh-4303. |
That's ubuntu with
Given it fixed these test failures on your end, I'd say it's fine to keep. If at some point a github ci windows build is re-enabled, we'll need assess where things stand anyway, so the worst case is that these two tests need to be marked up again. |
Any objections to moving this to maint? |
Not at all. I have to admit, I too rarely think of this in advance. I can rebase shortly, will just finish a commit on another branch |
@adswa I'm guessing the build failures are from a push followed by changing the base on github rather than the other way around. Triggering a new build (with a noop rewrite or a rebase to the latest maint) would hopefully sort it out. Sorry for the hassle. |
On Windows systems, an os.rename will always raise a FileExistsError if the file exits, while on Unix systems the existing file is replaced silently if the user has permissions. os.replace() appears to be a cross plattform compatible solution.
I have rebased to the latest maint |
This PR aims to fix a test failure that surfaced when a re-download attempts to overwrite an existing file (#5199 (comment)). On Windows systems, an
os.rename()
will always raise a FileExistsErrorif the file exits, while on Unix systems the existing file is replaced silently if the user has permissions, see https://docs.python.org/3/library/os.html#os.rename.
os.replace()
appears to be a cross-platform compatible solution, but let's hope that this doesn't break something else - then I could just do itif on_windows
.