go-path integrated by acruikshank · Pull Request #8028 · ipfs/kubo

acruikshank · 2021-03-26T00:42:56Z

Motivation

We are integrating ipld-prime into IPFS starting with many of its dependencies. go-path has now been converted. This PR updates go-path to the ipld-prime (linksystem) version and makes a few modifications to accommodate that upgrade.

This PR depends on branches that are also in peer review and should not be merged until those upstream changes have been accepted, tag, and the dependencies in this PR are updated

welcome · 2021-03-26T00:42:57Z

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

The context for this PR is clear, with relevant discussion, decisions
and stakeholders linked/mentioned.
Your contribution itself is clear (code comments, self-review for the
rest) and in its best form. Follow the code contribution
guidelines
if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

A maintainer will triage and assign priority to this PR, commenting on
any missing things and potentially assigning a reviewer for high
priority items.
The PR gets reviews, discussed and approvals as needed.
The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

gammazero · 2021-03-26T15:26:33Z

core/coreapi/path.go

-	case "ipld":
-		resolveOnce = resolver.ResolveSingle
-	default:
+	if ipath.Segments()[0] != "ipfs" && ipath.Segments()[0] != "ipld" {


minor: It would be nice if there were some const values for the segment indexes. If those do not already exist, that probably better in a separate future PR.

gammazero

This look OK to me. However, given the test failures it appears that sharded directory handling and a few other things are a problem. I do not know if that is something missing from this PR or needs to be fixed in a different PR.

willscott · 2021-04-04T02:18:08Z

@hannahhoward - confirming this isn't 'in draft' and we should finalize review of use of unixfsnode and then merge this to target merge branch?

License: MIT Signed-off-by: hannahhoward <[email protected]>

aschmahmann · 2021-04-13T19:57:38Z

There's a sharness test failing, is this a known issue and are the deps here up to date?

aschmahmann · 2021-04-13T20:01:48Z

core/coreapi/path.go

+	r := resolver.NewBasicResolver(api.blocks)
+	r.FetchConfig.NodeReifier = unixfsnode.Reify


I guess this is no big deal, but this seems to exactly match the Resolve function declared in core/node/core.go.

Was there a dependency issue or are we just copying the code bc it's only two lines?

aschmahmann · 2021-04-13T20:03:54Z

fuse/readonly/readonly_unix.go

 import (
 	"context"
 	"fmt"
+	"github.com/ipfs/go-cid"


let's group the dependencies here and have separate groupings for builtins, fuse, ipfs+ipld

aschmahmann · 2021-04-14T06:07:30Z

fuse/readonly/readonly_unix.go

+	// convert ipld-prime node to universal node
+	blk, err := s.Ipfs.Blockstore.Get(cidLnk.Cid)
+	if err != nil {
+		log.Debugf("fuse failed to retrieve block: %v: %s", cidLnk, err)
+		return nil, fuse.ENOENT
+	}
+
+	var fnd ipld.Node
+	switch cidLnk.Cid.Prefix().Codec {
+	case cid.DagProtobuf:
+		fnd, err = mdag.ProtoNodeConverter(blk, nd)
+	case cid.Raw:
+		fnd, err = mdag.RawNodeConverter(blk, nd)


This seems confusing/bad in that it'll result in multiple blockstore fetches for the same data. Perhaps if the underlying datastore has caching this doesn't end up being so bad since we likely just fetched the data, but if this is a recurring pattern in the associated set of PRs here then we might need to make some changes.

I added some comments in the go-merkledag PR ipfs/go-merkledag#67

@aschmahmann I don't actually see those :( Can you point me to them?

or maybe you need to submit review or something?

In terms of the more general question -- it's a bit tricky. In IPLD Prime land, we don't always have a block to go with a node, cause nodes don't always match block boundaries. In a future pure IPLD Prime world, this isn't a problem -- we wouldn't load the block here, cause we wouldn't try to produce a legacy merkledag node, cause UnixFS would already work with prime nodes natively. However to convert back to legacy node, we have to do this. Hopefully, it's just a temporary step till we make more progress on go-ipld-prime conversion

Some of my comments seem to have disappeared, but this is the one I was thinking of ipfs/go-merkledag#67 (comment)

aschmahmann · 2021-04-14T06:08:21Z

fuse/readonly/readonly_unix.go

+		return nil, fuse.ENOENT
+	}
+	if err != nil {
+		log.Error("could not convert protobuf node")
+		return nil, fuse.ENOENT


Why did we switch these from fuse.ENOTSUP to fuse.ENOENT?

that's a great question -- I didn't actually write this bit of code here (Alex C did but he's not working on PL stuff any more). Seems odd. I can change it back.

aschmahmann · 2021-04-16T21:53:03Z

core/node/core.go

+	rs := resolver.NewBasicResolver(bs)
+	rs.FetchConfig.NodeReifier = unixfsnode.Reify


This pattern of initialization seems a little weird. I'll make some comments as I get to that part of the stack when reviewing 😄.

I agree. I think we should have a FetchConfig top level dependency in the DI stack, and pass it to NewBasicResolver rather than the block service. I think I've said this elsewhere as well.

aschmahmann · 2021-04-16T21:54:19Z

fuse/readonly/readonly_unix.go

+	cidLnk, ok := ndLnk.(cidlink.Link)
+	if !ok {
+		log.Debugf("non-cidlink returned from ResolvePath: %v", ndLnk)
+		return nil, fuse.ENOENT


fuse.ENOSUP might be more appropriate here. The thing exists we just don't know what to do with it.

License: MIT Signed-off-by: hannahhoward <[email protected]>

Add fetcher config top level dependency

acruikshank force-pushed the feat/update-ipld-go-path branch from 008d8b6 to 3dfe47d Compare March 26, 2021 02:31

gammazero reviewed Mar 26, 2021

View reviewed changes

willscott mentioned this pull request Apr 4, 2021

IPLD Prime In IPFS: Target Merge Branch #7976

Merged

48 tasks

hannahhoward marked this pull request as ready for review April 5, 2021 16:58

acruikshank and others added 5 commits April 5, 2021 11:59

go-path integrated

52094b1

use basic resolver in gateway (provides unixfs adl)

e11af3a

resolve issues with error types from resolver

f5bfb04

feat(path): update to latest go-path

3f7626b

License: MIT Signed-off-by: hannahhoward <[email protected]>

fix(deps): update to latest go-path

a56fbf3

License: MIT Signed-off-by: hannahhoward <[email protected]>

hannahhoward force-pushed the feat/update-ipld-go-path branch from 9f3e725 to a56fbf3 Compare April 5, 2021 19:14

fix(deps): fix unixfsnode imple

e3211c0

License: MIT Signed-off-by: hannahhoward <[email protected]>

hannahhoward force-pushed the feat/update-ipld-go-path branch from 1e58905 to e3211c0 Compare April 5, 2021 20:19

aschmahmann requested changes Apr 16, 2021

View reviewed changes

hannahhoward added 3 commits April 20, 2021 08:18

feat(node): add fetcher config top level dependency

5077fd3

License: MIT Signed-off-by: hannahhoward <[email protected]>

fix(deps): update to latest go-path

63b6593

License: MIT Signed-off-by: hannahhoward <[email protected]>

Merge pull request #8081 from ipfs/feat/fetcher-config-dependency

bae6c8a

Add fetcher config top level dependency

hannahhoward merged commit 8d0ee3b into feat/ipld-in-ipfs Apr 24, 2021

rvagg deleted the feat/update-ipld-go-path branch July 26, 2021 08:25

		r := resolver.NewBasicResolver(api.blocks)
		r.FetchConfig.NodeReifier = unixfsnode.Reify

		rs := resolver.NewBasicResolver(bs)
		rs.FetchConfig.NodeReifier = unixfsnode.Reify

Uh oh!

Conversation

acruikshank commented Mar 26, 2021

Motivation

Uh oh!

welcome bot commented Mar 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gammazero left a comment

Choose a reason for hiding this comment

Uh oh!

willscott commented Apr 4, 2021

Uh oh!

aschmahmann commented Apr 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants