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.

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

Replace coreunix.Add with shell/fsnode's AddFromReader #1136

Closed
wants to merge 10 commits into from
24 changes: 13 additions & 11 deletions cmd/ipfs/init.go
Original file line number Diff line number Diff line change
@@ -1,21 +1,22 @@
package main

import (
"bytes"
"errors"
"fmt"
"io"
"strings"

context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"
assets "github.com/ipfs/go-ipfs/assets"
cmds "github.com/ipfs/go-ipfs/commands"
core "github.com/ipfs/go-ipfs/core"
coreunix "github.com/ipfs/go-ipfs/core/coreunix"
importer "github.com/ipfs/go-ipfs/importer"
chunk "github.com/ipfs/go-ipfs/importer/chunk"
merkledag "github.com/ipfs/go-ipfs/merkledag"
namesys "github.com/ipfs/go-ipfs/namesys"
config "github.com/ipfs/go-ipfs/repo/config"
fsrepo "github.com/ipfs/go-ipfs/repo/fsrepo"
uio "github.com/ipfs/go-ipfs/unixfs/io"
u "github.com/ipfs/go-ipfs/util"
unixfs "github.com/ipfs/go-ipfs/unixfs"
)

const nBitsForKeypairDefault = 2048
Expand Down Expand Up @@ -132,23 +133,24 @@ func addDefaultAssets(out io.Writer, repoRoot string) error {
}
defer nd.Close()

dirb := uio.NewDirectory(nd.DAG)
dir := &merkledag.Node{Data: unixfs.FolderPBData()}
Copy link
Member

Choose a reason for hiding this comment

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

Curious why not using the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Wed, Apr 29, 2015 at 01:27:43PM -0700, Juan Batiz-Benet wrote:

@@ -132,23 +133,24 @@ func addDefaultAssets(out io.Writer, repoRoot string) error {
}
defer nd.Close()

  • dirb := uio.NewDirectory(nd.DAG)
  • dir := &merkledag.Node{Data: unixfs.FolderPBData()}

Curious why not using the constructor?

Because I wanted AddNodeLink. Cycling from node → key string → node →
AddNodeLink (which is what AddChild does 1) seemed wasteful and
awkward (see note in 0ce5784, cmd/ipfs/init: Replace coreunix.Add
with importer.BuildDagFromReader, 2015-04-24).

Copy link
Contributor

Choose a reason for hiding this comment

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

or define a method dirb.AddNodeLink(node).


// add every file in the assets pkg
for fname, file := range assets.Init_dir {
buf := bytes.NewBufferString(file)
s, err := coreunix.Add(nd, buf)
reader := strings.NewReader(file)
dagNode, err := importer.BuildDagFromReader(
reader,
nd.DAG,
nd.Pinning.GetManual(),
chunk.DefaultSplitter)
if err != nil {
return err
}

k := u.B58KeyDecode(s)
if err := dirb.AddChild(fname, k); err != nil {
if err := dir.AddNodeLink(fname, dagNode); err != nil {
return err
}
}

dir := dirb.GetNode()
dkey, err := nd.DAG.Add(dir)
if err != nil {
return err
Expand Down
7 changes: 1 addition & 6 deletions cmd/ipfswatch/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,7 @@ func run(ipfsPath, watchPath string) error {
}
}
proc.Go(func(p process.Process) {
file, err := os.Open(e.Name)
if err != nil {
log.Println(err)
}
defer file.Close()
k, err := coreunix.Add(node, file)
k, err := coreunix.AddR(node, e.Name)
if err != nil {
log.Println(err)
}
Expand Down
6 changes: 3 additions & 3 deletions core/corehttp/corehttp.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ var log = eventlog.Logger("core/server")
// initially passed in if not.
type ServeOption func(*core.IpfsNode, *http.ServeMux) (*http.ServeMux, error)

// makeHandler turns a list of ServeOptions into a http.Handler that implements
// MakeHandler turns a list of ServeOptions into a http.Handler that implements
// all of the given options, in order.
func makeHandler(n *core.IpfsNode, options ...ServeOption) (http.Handler, error) {
func MakeHandler(n *core.IpfsNode, options ...ServeOption) (http.Handler, error) {
topMux := http.NewServeMux()
mux := topMux
for _, option := range options {
Expand All @@ -49,7 +49,7 @@ func ListenAndServe(n *core.IpfsNode, listeningMultiAddr string, options ...Serv
if err != nil {
return err
}
handler, err := makeHandler(n, options...)
handler, err := MakeHandler(n, options...)
if err != nil {
return err
}
Expand Down
18 changes: 12 additions & 6 deletions core/corehttp/gateway_test.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package corehttp
package corehttp_test

import (
"errors"
Expand All @@ -10,12 +10,13 @@ import (

context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"
core "github.com/ipfs/go-ipfs/core"
coreunix "github.com/ipfs/go-ipfs/core/coreunix"
corehttp "github.com/ipfs/go-ipfs/core/corehttp"
namesys "github.com/ipfs/go-ipfs/namesys"
ci "github.com/ipfs/go-ipfs/p2p/crypto"
path "github.com/ipfs/go-ipfs/path"
repo "github.com/ipfs/go-ipfs/repo"
config "github.com/ipfs/go-ipfs/repo/config"
unixfs "github.com/ipfs/go-ipfs/shell/unixfs"
testutil "github.com/ipfs/go-ipfs/util/testutil"
)

Expand Down Expand Up @@ -60,15 +61,20 @@ func TestGatewayGet(t *testing.T) {
t.Skip("not sure whats going on here")
ns := mockNamesys{}
n := newNodeWithMockNamesys(t, ns)
k, err := coreunix.Add(n, strings.NewReader("fnord"))
dagNode, err := unixfs.AddFromReader(n, strings.NewReader("fnord"))
if err != nil {
t.Fatal(err)
}
key, err := dagNode.Key()
if err != nil {
t.Fatal(err)
}
k := key.String()
ns["example.com"] = path.FromString("/ipfs/" + k)

h, err := makeHandler(n,
IPNSHostnameOption(),
GatewayOption(false),
h, err := corehttp.MakeHandler(n,
corehttp.IPNSHostnameOption(),
corehttp.GatewayOption(false),
)
if err != nil {
t.Fatal(err)
Expand Down
8 changes: 6 additions & 2 deletions core/coreunix/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,12 @@ import (

var log = eventlog.Logger("coreunix")

// Add builds a merkledag from the a reader, pinning all objects to the local
// datastore. Returns a key representing the root node.
// DEPRECATED: Add builds a merkledag from the a reader, pinning all
// objects to the local datastore. Returns a key representing the root
// node.
//
// This function is deprecated and will be removed in 0.4.0. You
// should use shell/unixfs's AddFromReader instead.
Copy link
Member

Choose a reason for hiding this comment

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

please remove this comment. regardless of the future, today, this is not deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Fri, May 01, 2015 at 04:58:19AM -0700, Juan Batiz-Benet wrote:

-// Add builds a merkledag from the a reader, pinning all objects to the local
-// datastore. Returns a key representing the root node.
+// DEPRECATED: Add builds a merkledag from the a reader, pinning all
+// objects to the local datastore. Returns a key representing the root
+// node.
+//
+// This function is deprecated and will be removed in 0.4.0. You
+// should use shell/unixfs's AddFromReader instead.

please remove this comment. regardless of the future, today, this
is not deprecated.

Most of my later commits in this branch (and some of the earlier ones)
are responding to this deprecation. I've shuffled things around a
bit, so I think you should be able to merge the following initial
commits without controversy:

  • 0ce5784 cmd/ipfs/init: Replace coreunix.Add with importer.BuildDagFromReader
  • bf9b9fc cmd/ipfswatch/main: Replace coreunix.Add with coreunix.AddR
  • ac0a606 cmd/ipfs/init: bytes.NewBufferString -> strings.NewReader

Once we've resolved strings vs []bytes 1, we can merge:

  • 7f62410 util/testutil/addcat/addcat: Factor out add/cat/verify helper
  • ac0a606 cmd/ipfs/init: bytes.NewBufferString -> strings.NewReader

And then once we've decided to deprecate coreunix.Add we can merge:

  • cd58a84 shell/unixfs: Add a high-level filesystem-node package
  • 56807e9 core/coreunix/add: Deprecate Add
  • 2409fee util/testutil/addcat: importer.BuildDagFromReader -> unixfs.AddFromReader
  • 159783f test/supernode_client/main.go: Migrate to unixfs.AddFromReader
  • 5d97c8c core/corehttp/gateway_test: Replace coreunix.Add with importer.BuildDagFromReader
  • 8a4b861 core/corehttp/gateway_test: Externalize test to use shell/unixfs' AddFromReader

Until we want to deprecate coreunix.Add, the shell/unixfs stuff is
just going to add another interface to maintain (instead of migrating
us to a new interface), so I think it's just going to cause confusion.
There's no rush though, so I'm happy to just let those commits cook
until we are ready to axe coreunix.Add.

If that sounds reasonable, I'm fine with you just merging up to the
point you're comfortable with from this branch. I'm also happy
opening new PRs for the non-controversial changes, or pushing the
controversial changes out into new PRs. Whatever works best for you.

func Add(n *core.IpfsNode, r io.Reader) (string, error) {
// TODO more attractive function signature importer.BuildDagFromReader
dagNode, err := importer.BuildDagFromReader(
Expand Down
26 changes: 26 additions & 0 deletions docs/release-notes/unreleased.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# `core/coreunix`'s `Add` → `shell/unixfs`'s `AddFromReader`

We've added an `AddFromReader` function to `shell/unixfs`. The old
`Add` from `core/coreunix` is deprecated and will be removed in
version 0.4.0. To update your existing code, change usage like:

keyString, err := coreunix.Add(ipfsNode, reader)
if err != nil {
return err
}

to

fileNode, err := unixfs.AddFromReader(ipfsNode, reader)
if err != nil {
return err
}
key, err := fileNode.Key()
if err != nil {
return err
}
keyString := key.String()

That's a bit more verbose if all you want is the stringified key, but
returning a `dag.Node` makes it easier to perform other operations
like adding the file node to a directory node.
8 changes: 8 additions & 0 deletions shell/shell.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
Package shell implements a high-level interface for common IPFS activity.

These wrappers around the low-level core interface make it easy to
accomplish common tasks with default settings. For steps where the
defaults aren't appropriate, you can use the core package directly.
*/
package shell
28 changes: 28 additions & 0 deletions shell/unixfs/add.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package unixfs

import (
"io"

core "github.com/ipfs/go-ipfs/core"
importer "github.com/ipfs/go-ipfs/importer"
chunk "github.com/ipfs/go-ipfs/importer/chunk"
dag "github.com/ipfs/go-ipfs/merkledag"
)

// Add builds a merkledag from a reader, pinning all objects to the
// local datastore. Returns the root node.
func AddFromReader(node *core.IpfsNode, reader io.Reader) (*dag.Node, error) {
fileNode, err := importer.BuildDagFromReader(
reader,
node.DAG,
node.Pinning.GetManual(),
chunk.DefaultSplitter,
)
if err != nil {
return nil, err
}
if err := node.Pinning.Flush(); err != nil {
return nil, err
}
return fileNode, nil
}
10 changes: 10 additions & 0 deletions shell/unixfs/unixfs.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
Package unixfs is a high-level interface for filesystem nodes.

Simple wrappers to:

* create file and directory nodes from paths and readers,
* extract file and directory nodes to your local filesytem, and
* print file contents to writers.
*/
package unixfs
17 changes: 2 additions & 15 deletions test/integration/addcat_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"bytes"
"errors"
"fmt"
"io"
"math"
"os"
"testing"
Expand All @@ -14,11 +13,11 @@ import (
context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"

"github.com/ipfs/go-ipfs/core"
coreunix "github.com/ipfs/go-ipfs/core/coreunix"
mocknet "github.com/ipfs/go-ipfs/p2p/net/mock"
"github.com/ipfs/go-ipfs/p2p/peer"
"github.com/ipfs/go-ipfs/thirdparty/unit"
testutil "github.com/ipfs/go-ipfs/util/testutil"
addcat "github.com/ipfs/go-ipfs/util/testutil/addcat"
)

const kSeed = 1
Expand Down Expand Up @@ -126,22 +125,10 @@ func DirectAddCat(data []byte, conf testutil.LatencyConfig) error {
return err
}

added, err := coreunix.Add(adder, bytes.NewReader(data))
err = addcat.AddCat(adder, catter, data)
if err != nil {
return err
}

readerCatted, err := coreunix.Cat(catter, added)
if err != nil {
return err
}

// verify
var bufout bytes.Buffer
io.Copy(&bufout, readerCatted)
if 0 != bytes.Compare(bufout.Bytes(), data) {
return errors.New("catted data does not match added data")
}
return nil
}

Expand Down
19 changes: 2 additions & 17 deletions test/integration/bench_cat_test.go
Original file line number Diff line number Diff line change
@@ -1,19 +1,17 @@
package integrationtest

import (
"bytes"
"errors"
"io"
"math"
"testing"

context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"
"github.com/ipfs/go-ipfs/core"
coreunix "github.com/ipfs/go-ipfs/core/coreunix"
mocknet "github.com/ipfs/go-ipfs/p2p/net/mock"
"github.com/ipfs/go-ipfs/p2p/peer"
"github.com/ipfs/go-ipfs/thirdparty/unit"
testutil "github.com/ipfs/go-ipfs/util/testutil"
addcat "github.com/ipfs/go-ipfs/util/testutil/addcat"
)

func BenchmarkCat1MB(b *testing.B) { benchmarkVarCat(b, unit.MB*1) }
Expand Down Expand Up @@ -74,22 +72,9 @@ func benchCat(b *testing.B, data []byte, conf testutil.LatencyConfig) error {
return err
}

added, err := coreunix.Add(adder, bytes.NewReader(data))
err = addcat.AddCat(adder, catter, data)
if err != nil {
return err
}

b.StartTimer()
readerCatted, err := coreunix.Cat(catter, added)
if err != nil {
return err
}

// verify
var bufout bytes.Buffer
io.Copy(&bufout, readerCatted)
if 0 != bytes.Compare(bufout.Bytes(), data) {
return errors.New("catted data does not match added data")
}
return nil
}
20 changes: 2 additions & 18 deletions test/integration/grandcentral_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"bytes"
"errors"
"fmt"
"io"
"math"
"testing"

Expand All @@ -14,14 +13,14 @@ import (

core "github.com/ipfs/go-ipfs/core"
"github.com/ipfs/go-ipfs/core/corerouting"
"github.com/ipfs/go-ipfs/core/coreunix"
mocknet "github.com/ipfs/go-ipfs/p2p/net/mock"
"github.com/ipfs/go-ipfs/p2p/peer"
"github.com/ipfs/go-ipfs/thirdparty/iter"
"github.com/ipfs/go-ipfs/thirdparty/unit"
"github.com/ipfs/go-ipfs/util"
ds2 "github.com/ipfs/go-ipfs/util/datastore2"
testutil "github.com/ipfs/go-ipfs/util/testutil"
addcat "github.com/ipfs/go-ipfs/util/testutil/addcat"
)

func TestSupernodeBootstrappedAddCat(t *testing.T) {
Expand Down Expand Up @@ -54,25 +53,10 @@ func RunSupernodeBootstrappedAddCat(data []byte, conf testutil.LatencyConfig) er
adder := clients[0]
catter := clients[1]

log.Info("adder is", adder.Identity)
log.Info("catter is", catter.Identity)

keyAdded, err := coreunix.Add(adder, bytes.NewReader(data))
err = addcat.AddCat(adder, catter, data)
if err != nil {
return err
}

readerCatted, err := coreunix.Cat(catter, keyAdded)
if err != nil {
return err
}

// verify
var bufout bytes.Buffer
io.Copy(&bufout, readerCatted)
if 0 != bytes.Compare(bufout.Bytes(), data) {
return errors.New("catted data does not match added data")
}
return nil
}

Expand Down
Loading