Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Backport from go-ipfs, fix tests, hopefully fix Windows #18

Merged
merged 7 commits into from Apr 26, 2015
Merged

Backport from go-ipfs, fix tests, hopefully fix Windows #18

merged 7 commits into from Apr 26, 2015

Conversation

tv42
Copy link
Contributor

@tv42 tv42 commented Apr 24, 2015

Untested on Windows, hoping some kind soul on IRC will do that.

To test, run whatever is the Windows equivalent of:

go get github.com/jbenet/go-datastore && cd $GOPATH/src/github.com/jbenet/go-datastore && git remote add tv42 https://github.com/tv42/go-datastore && git fetch tv42 && git checkout flatfs-rides-again && go test ./flatfs

jbenet
jbenet reviewed Apr 24, 2015
View reviewed changes
flatfs/sync_windows.go
package flatfs

func syncDir(dir string) error {
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so i guess windows doesnt like sync?

@jbenet
Copy link
Member

jbenet commented Apr 24, 2015

@tv42 this all LGTM. Ready For Merge?

@tv42
Copy link
Contributor Author

tv42 commented Apr 24, 2015

Now this should be good to merge. Not tested yet on Windows, as far as I know.

@tv42
Copy link
Contributor Author

tv42 commented Apr 24, 2015

@gatesvp your help would be greatly appreciated; go test ./flatfs again, on Windows.

@tv42 tv42 mentioned this pull request Apr 24, 2015
Closed
@gatesvp
Copy link

gatesvp commented Apr 25, 2015

Will test this shortly, I am in transit right now. Will double check in a
few hours.
On Apr 24, 2015 4:58 PM, "Tv" notifications@github.com wrote:

@gatesvp https://github.com/gatesvp your help would be greatly
appreciated; go test ./flatfs again, on Windows.

—
Reply to this email directly or view it on GitHub
#18 (comment).

@gatesvp
Copy link

gatesvp commented Apr 25, 2015

we are improving :)

--- FAIL: TestStorage (0.01s) flatfs_test.go:186: file should not be world accessible: 0666 FAIL FAIL github.com/jbenet/go-datastore/flatfs 0.121s

@tv42
Copy link
Contributor Author

tv42 commented Apr 25, 2015

I guess I should just disable file mode checking on Windows.

@tv42
Copy link
Contributor Author

tv42 commented Apr 25, 2015

@gatesvp Like a game of table tennis.

@gatesvp
Copy link

gatesvp commented Apr 25, 2015

ok github.com/jbenet/go-datastore/flatfs 0.096s

pong :)

On Fri, Apr 24, 2015 at 8:19 PM, Tv notifications@github.com wrote:

@gatesvp https://github.com/gatesvp Like a game of table tennis.

—
Reply to this email directly or view it on GitHub
#18 (comment).

whyrusleeping and others added 7 commits April 25, 2015 15:25
@whyrusleeping @tv42
remove / prefix and close directories properly in query
a04a9f5 
Backported from commit e965c53780003a01fcbd94af96333ace4b3e2748 in the
vendored copy in https://github.com/ipfs/go-ipfs
@tv42
flatfs: Fix tests after previous commit
5f42b00 
Previous commit made flatfs strip first byte of keys (under the
assumption that datastore always makes it be "/").
@tv42
flatfs: Avoid syncing directories in Windows
9d9c61b 
Used to error out with

    flatfs_test.go:66: Put fail: failed to open dir: fsync: The handle is invalid

It seems Windows does not want syscall.Fsync to be called on directory
handles.
@tv42
flatfs: Adjust tests to Windows paths with backslashes
b9b3e98
@tv42
Vendor github.com/jbenet/go-os-rename
a3d1eea
@tv42
flatfs: Windows os.Rename refuses to overwrite a file; call MoveFiles…
58d30fe 
…Ex directly

For background, see golang/go#3366

Here's the failure seen:

    --- FAIL: TestPutOverwrite (0.03s)
    	flatfs_test.go:118: Put fail: rename C:\Users\gates\AppData\Local\Temp\test-datastore-flatfs-679014441\7175\put- 717454915 C:\Users\gates\AppData\Local\Temp\test-datastore-flatfs-679014441/7175/71757578.data: Cannot create a file when that file already exists.
@tv42
flatfs: Don't test file modes on Windows
d3aa062
@tv42
Copy link
Contributor Author

tv42 commented Apr 25, 2015

Whee! Rebased to make it sit on top of master.

@jbenet, this is ready for merge; once that's good, I'll vendor it into ipfs.

@jbenet
Copy link
Member

jbenet commented Apr 26, 2015

thanks @tv42 and @gatesvp -- merging.

jbenet added a commit that referenced this pull request Apr 26, 2015
@jbenet
Merge pull request #18 from tv42/flatfs-rides-again
2525cae 
Backport from go-ipfs, fix tests, hopefully fix Windows
@jbenet jbenet merged commit 2525cae into ipfs:master Apr 26, 2015
@tv42 tv42 deleted the flatfs-rides-again branch April 26, 2015 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tv42 @jbenet @gatesvp @whyrusleeping