Add Windows support to cacheddownloader #6
Conversation
|
Thanks, @lvarvel! I've created this Diego story to evaluate the PR. |
|
Could you please clarify which tests were failing on Windows? Was it the cacheddownloader unit tests, or other tests from diego-release? Thanks, |
|
The tests were the cacheddownloader unit tests. I don't think we had any other failing diego tests. |
|
There is also one final test that fails intermittently on Windows. We've dropped a chore into our backlog, and we'll get back to you on that one later. This PR should be good to go. |
|
@lvarvel @mavenraven: The first commit was fine, but the second one (with the Thanks, |
|
Sorry about that. We've made some changes to use build tags. |
|
Related: golang/go#8914 TLDR: os.Rename doesn't work as expected on Windows, and the Golang core team won't fix it. They might add an os.Replace function in go 1.5 that does the right thing (essentially what we've done here). |
|
Excellent, thanks for the reference to the outstanding issue. If we can remove this with Go 1.5 or later, that would be great. We tried the entire units/inigo/DATs run on OS X/bosh-lite and it seems fine, so we can merge and push this once ketchup is green. In the meantime, I have a few minor comments to make inline. Thanks, |
| @@ -22,8 +22,8 @@ type CachedDownloader interface { | |||
| Fetch(urlToFetch *url.URL, cacheKey string, transformer CacheTransformer, cancelChan <-chan struct{}) (io.ReadCloser, int64, error) | |||
| } | |||
|
|
|||
| func NoopTransform(source, destination string) (int64, error) { | |||
| err := os.Rename(source, destination) | |||
| func NoopTransform(source string, destination string) (int64, error) { | |||
There was a problem hiding this comment.
It's idiomatic Go to collapse type declarations for adjacent arguments of the same type, as was done before the change.
Fix file moving to be safe on Windows. A shim was added that calls into the Win32 API / MoveFileEx that prevents an error occurring on windows if the destination already exists. This shim might not be needed once os.Replace lands, possibly in Go 1.5. See: golang/go#8914 [#88907658]
|
We've completed the requested changes, waiting for Varvel to give us push access. |
|
Access granted, mavenraven. |
For windows compatibility. We added a test for windows that exercises this.
Windows timer increments in 15.6ms ticks, which caused the eviction test to fail since all files in the cache (sometimes) have the same timestamp. The patch adds a sleep before fetching `A' to make sure it has a more recent timestamp.
lvarvel
changed the title
Signed-off-by: Eric Malm <emalm@pivotal.io>
Cacheddownloader has a number of issues on Windows, mostly due to irritating differences between Windows and Linux file open/closing and removal. This fixes one of them.
There's no additional test code necessary, because this change actually fixes three tests that currently fail when run on Windows.
After this change, there will still be 29 failing tests (previously 32), and we'll be submitting more pull requests as we stomp those.