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

update to recent bazil.org/fuse #821

Merged
merged 3 commits into from
Mar 1, 2015
Merged

update to recent bazil.org/fuse #821

merged 3 commits into from
Mar 1, 2015

Conversation

cryptix
Copy link
Contributor

@cryptix cryptix commented Feb 25, 2015

The recent version of bazil.org/fuse dropped their custom interrupt (fuse.Intr) in favour of golang.org/x/net/context.

I found two spots in our code where we created a context and cancelled it when Intr was closed. I just passed through the context from fuse now.

@whyrusleeping PTAL, maybe i've missed a case?

also: fuse.Error was dropped as well.

@btc btc added the status/in-progress In progress label Feb 25, 2015
@jbenet
Copy link
Member

jbenet commented Feb 25, 2015

this looks right to me. not sure about other places, yeah let's get @whyrusleeping's thoughts

whyrusleeping
whyrusleeping reviewed Feb 27, 2015
View reviewed changes
Godeps/_workspace/src/bazil.org/fuse/fs/bench/bench_test.go
@@ -40,20 +41,20 @@ type benchDir struct {
var _ = fs.Node(benchDir{})
var _ = fs.NodeStringLookuper(benchDir{})
var _ = fs.Handle(benchDir{})
var _ = fs.HandleReadDirer(benchDir{})
var _ = fs.HandleReadDirAller(benchDir{})
Copy link
Member

Choose a reason for hiding this comment

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

Is this the correct change? Is the API the same?

Copy link
Member

Choose a reason for hiding this comment

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

my bad, this isnt our code...

@cryptix
Copy link
Contributor Author

cryptix commented Feb 27, 2015

Rebased to flatten the tree and see the tests run once more.

@jbenet
Copy link
Member

jbenet commented Feb 27, 2015

LGTM

@whyrusleeping
Copy link
Member

this doesnt look good to me yet. Need to implement the correct interfaces. The tests are green because travis doesnt run fuse tests.

@cryptix
Copy link
Contributor Author

cryptix commented Feb 28, 2015

Added the assignment to an interface that implements a interface collection to ipns and readonly.

Locally this looks a bit absurd.. Only one of them fails.. edit:Scratch that. Fixing now..

[cryptix@higgs ~/go/src/github.com/jbenet/go-ipfs/fuse/ipns:updateFuseToCtxs] go test
# github.com/jbenet/go-ipfs/fuse/ipns
./ipns_unix.go:643: cannot use (*Node)(nil) (type *Node) as type ipnsNode in assignment:
    *Node does not implement ipnsNode (wrong type for Create method)
        have Create(*fuse.CreateRequest, *fuse.CreateResponse, context.Context) ("github.com/jbenet/go-ipfs/Godeps/_workspace/src/bazil.org/fuse/fs".Node, "github.com/jbenet/go-ipfs/Godeps/_workspace/src/bazil.org/fuse/fs".Handle, error)
        want Create(context.Context, *fuse.CreateRequest, *fuse.CreateResponse) ("github.com/jbenet/go-ipfs/Godeps/_workspace/src/bazil.org/fuse/fs".Node, "github.com/jbenet/go-ipfs/Godeps/_workspace/src/bazil.org/fuse/fs".Handle, error)
FAIL    github.com/jbenet/go-ipfs/fuse/ipns [build failed]

I couldnt find an interface for Attr() seems to be GetAttr now. Edit: Wrong, its fs.Node specifies Attr()

ReadDir seems to be ReadDirAll.

@cryptix
Copy link
Contributor Author

cryptix commented Feb 28, 2015

Thanks @tv42 for the assignment to _ idea. I assumed this would take place when the gofuse pkg would start to handle a node but I get it know. Also have to add this kind of test to the Root types as well.

@cryptix cryptix force-pushed the updateFuseToCtxs branch 2 times, most recently from 55e5120 to 6aa49fd Compare February 28, 2015 22:18
@cryptix
Copy link
Contributor Author

cryptix commented Mar 1, 2015

Just FYI: I ran a godoc with -analysis=type on a version of the code at master to get the implements list for the Root and Node types.

@cryptix cryptix force-pushed the updateFuseToCtxs branch 3 times, most recently from 55d9196 to 4fee3ec Compare March 1, 2015 12:14
@cryptix cryptix force-pushed the updateFuseToCtxs branch from 4fee3ec to 94f55f2 Compare March 1, 2015 12:48
@jbenet
Copy link
Member

jbenet commented Mar 1, 2015

❤️ this PR.

@cryptix this is great! and good thing we verify our implementations type-safely.

@tv42 thanks for moving the interface to net/context!

@jbenet
Copy link
Member

jbenet commented Mar 1, 2015

Everything LGTM. merging after tests run.

jbenet added a commit that referenced this pull request Mar 1, 2015
@jbenet
Merge pull request #821 from jbenet/updateFuseToCtxs
dd58878 
update to recent bazil.org/fuse
@jbenet jbenet merged commit dd58878 into master Mar 1, 2015
@jbenet jbenet deleted the updateFuseToCtxs branch March 1, 2015 13:00
@jbenet jbenet removed the status/in-progress In progress label Mar 1, 2015
@jbenet jbenet restored the updateFuseToCtxs branch March 1, 2015 14:13
@jbenet
Copy link
Member

jbenet commented Mar 1, 2015

@whyrusleeping may want to review + comment. we can always fix things as another PR.

@whyrusleeping
Copy link
Member

It looks good to me, I dont see anything that would be bad

@cryptix cryptix deleted the updateFuseToCtxs branch March 9, 2015 14:12
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.
5 participants
@cryptix @jbenet @whyrusleeping @tv42 @btc