Extract and rework commands package #3856

keks · 2017-04-06T10:00:46Z

This PR rips out commands/, changes a lot of stuff and puts it in go-ipfs-cmds. This PR is WIP and primarily here for reviewing.

see #3524 and go-ipfs-cmds

Some remarks:

Backwards compatibility

We don't want to rewrite all of core/commands at once, so I built a shimming layer that allows us to use the current go-ipfs/commands.Command structs (with some limitations). All the shimming code is in go-ipfs-cmds/legacy.go. That is a lot of code and in the medium term it would be great to dump it. For that we need to use the new commands library in all of core/commands. That is not a short-term project though.

Shared code

There is quite a lot of shared code between go-ipfs/commands and go-ipfs-cmds. To reduce the number of type conversions, I moved most of it to go-ipfs-cmds/cmdsutil. I don't think this is a very good name though and it might change in the future. $pkg/${pkg}util usually contains code that operates on types in $pkg and here it's the other way around: cmdsutil contains a lot of basic tools (e.g. an error type) that is used by both go-ipfs-cmds and go-ipfs/commands. Maybe go-ipfs-cmds/core is better?

Basic model

Most changes I made affected the type Response, which I split up into Response and ResponseEmitter. Basically a producer is given a ResponseEmitter on which it can call Emit(v) a few times (basically it's a channel write) - or just once if v is an io.Reader. These values can be received by a consumer which has the corresponding Response by calling v, err := res.Next(). These can be chained at will - which actually happens when there is a PostRun.

Marshalers, Encoders, PostRun

When I started this project I complained about PostRun. I still think it's a bit weird, but it fulfills an important use case.
In go-ipfs/commands, the Marshal function takes the value passed to SetOutput and do it's magic to build a bytestream from that. In go-ipfs-cmds there is no singular value, instead we operate on streams. Encode() is very similar, but instead it operates on a single emitted value. It is called once per call to Emit. This means that no state is kept between emitted values. If you want to e.g. not abort on errors and print an error digest after everything completed, you need to use PostRun.
PostRun takes a request and the actual ResponseEmitter and returns a new ResponseEmitter. Usually the first thing that happens in a PostRun is to create a ResponseEmitter/Response pair backed by a channel. In a goroutine we read from that Response, do our thing, and send it to the underlying ResponseEmitter. After calling the goroutine we return the channel-backed ResponseEmitter. Usually the call to PostRun looks like this:

re = cmd.PostRun[enc](req, re)
// ...
cmd.Run(req, re)

That way the returned ResponseEmitter will be used by the Run function and all emitted values will pass through PostRun before ending up at the final destination (usually a cmds/cli.ResponseEmitter). This allows building flexible pipelines.

Changes to the sharness tests

I changed some small stuff in the sharness tests. I don't intend to keep these changes, but they are currently handy. The changes basically include a lot of debug output and a few disabled tests, because my computer doesn't like tests that use nc. I don't know why, but they also fail for master. Maybe you can try to run them? That is t0060, t0061 and t0235. Remove the test_done from the test's preamble.

I also wasn't able to test fuse and docker.

That was a lot of explanation. Let me know what you think!

whyrusleeping · 2017-04-06T20:12:39Z

core/commands/active.go

+		ctx := req.InvocContext()
+		log.Debug("active reqs: ctx=", ctx)
+		reqLog := ctx.ReqLog
+		log.Debug("active reqs: reqlog=", reqLog)


I assume you can remove all the debug logging here once youre done?

whyrusleeping · 2017-04-06T20:13:16Z

core/commands/add.go

@@ -17,7 +20,7 @@ import (
 	mfs "github.com/ipfs/go-ipfs/mfs"
 	ft "github.com/ipfs/go-ipfs/unixfs"

-	u "gx/ipfs/QmZuY8aV7zbNXVy6DyN9SmnuH3o9nG852F4aTiSBpts8d1/go-ipfs-util"
+	//u "gx/ipfs/QmZuY8aV7zbNXVy6DyN9SmnuH3o9nG852F4aTiSBpts8d1/go-ipfs-util"


no longer importing this package? Thats wonderful

I can make an effort to get rid of it in other places as well if you want.

whyrusleeping · 2017-04-06T20:16:28Z

core/commands/add.go

 		}
-		res.SetOutput(nil)
+	},
+	PostRun: map[cmds.EncodingType]func(cmds.Request, cmds.ResponseEmitter) cmds.ResponseEmitter{


This should be type-defed to something like cmds.EmitterMap

whyrusleeping · 2017-04-06T20:19:23Z

core/commands/add.go

+						}
+
+						// TODO why does err != io.EOF return true?
+						if err.Error() != "EOF" {


odd. Is this still the case? Maybe we recreated an error somewhere like errors.New(err.Error())

whyrusleeping · 2017-04-06T20:22:25Z

core/commands/bootstrap.go

+			)
+
+			if ch, ok = res.Output().(<-chan interface{}); !ok {
+				if ch, ok = res.Output().(chan interface{}); !ok {


we should probably ensure that its only ever a <-chan interface{}

Hmm I think I tried that due to the weird shimming layer, but I'll take another look

whyrusleeping · 2017-04-06T20:54:18Z

core/commands/dht.go

-				return nil, u.ErrCast()
-			}
-
+		cmds.Text: func() func(cmds.Response) (io.Reader, error) {


should have a type defined for this

I think I couldn't come up with a good name :)

uhm.... hrm. Maybe ResponseGenFunc or something?

whyrusleeping · 2017-04-06T20:57:32Z

core/commands/filestore.go

 			return
 		}
 		args := req.Arguments()
 		if len(args) > 0 {
 			out := perKeyActionToChan(args, func(c *cid.Cid) *filestore.ListRes {
 				return filestore.List(fs, c)
 			}, req.Context())
-			res.SetOutput(out)
+
+			for v := range out {


for this sort of thing, we should do a for-select with the request context to allow for cancellation

Good catch!

I think we rather need to pass ctx to Emit

whyrusleeping · 2017-04-06T20:59:29Z

core/commands/filestore.go

 			out := listResToChan(next, req.Context())
-			res.SetOutput(out)
+			for v := range out {


maybe since we do this often, we should have some sort of re.PipeOutput(ch) method

Actually some of the ResponseEmitters already copy on Emit. Will make sure they all behave that way

whyrusleeping · 2017-04-06T21:32:44Z

core/commands/filestore.go

+					errors bool
+				)
+
+				for err == nil {


i'd use a switch here

I'm not sure how a switch would help here but I made a change that removes the condition from the loop head

whyrusleeping · 2017-04-06T22:10:21Z

core/commands/get.go

+
+				v, err := res.Next()
+				if err != nil {
+					//TODO XXX


whyrusleeping · 2017-04-06T22:24:50Z

core/commands/id.go

+			ch, ok := res.Output().(chan interface{})
+			if !ok {
+				log.Debugf("received type %T", res.Output())
+				return nil, u.ErrCast()


maybe we should have a new error thing that prints out the expected and actual types. Instead of the ugly ErrCast nonsense (no need to do this now though)

whyrusleeping · 2017-04-06T22:30:26Z

core/commands/object/object.go

+			if !ok {
+				ch, ok = res.Output().(<-chan interface{})
+				if !ok {
+					panic("object.get marshaler: res.Output() is not chan interface{}")


probably don't want to panic. returning an error is preferable here

whyrusleeping · 2017-04-06T22:33:13Z

core/commands/object/object.go

+				}
+			}
+
+			node := (<-ch).(*Node)


Because I don't check the type? I guess I need to find a general answer for this...ErrCast is also stupid

whyrusleeping · 2017-04-06T22:33:49Z

core/commands/object/object.go

+			if ch_, ok := res.Output().(chan interface{}); ok {
+				ch = ch_
+			} else {
+				ch = res.Output().(chan interface{})


"else, fuck it, do it anyways"

whyrusleeping · 2017-04-06T22:35:26Z

core/commands/pin.go

+				if out.Pins != nil {
+					added = out.Pins
+				} else {
+					fmt.Fprintf(res.Stderr(), "Fetched/Processed %d nodes\r", out.Progress)


this should only be done when the progress option is enabled

That is implicit because out.Pins is non-nil if the progress option is disabled

whyrusleeping · 2017-04-06T22:45:33Z

core/commands/repo.go

-				out <- &VerifyProgress{Message: "verify complete, some blocks were corrupt."}
+				//out <- &VerifyProgress{Message: "verify complete, some blocks were corrupt."}
+				log.Debugf("Run.go.else: %T.SetError", res)
+				res.SetError(fmt.Errorf("verify complete, some blocks were corrupt"), cmdsutil.ErrNormal)


I'd love to make SetError accept just a string optionally

Two thoughts. First, this is the old Response and I didn't change it too much. Here we still have to pass error, ErrorType. With the new ones it is interface{}, ErrorType. Second, making it accept only a string (or interface{} for that matter) seems kind of sketchy because the only way to do it is to make it variadic and thus accept more than one ErrorType which is also weird:

func (re *ResponsEmitter) SetError(msg interface{}, codes ...ErrorType)

I'm fine with not changing the old stuff, but theres no need to make it variadic. Just change the signature to:

func (res Response) SetError(msg interface{}, code ErrorType)

Then in the function you would type switch msg to either an error or a string, and just fmt.Sprint anything else that wasnt one of those.

But, like it said, don't bother spending time changing the legacy stuff :)

whyrusleeping · 2017-04-06T22:55:25Z

test/sharness/t0010-basic-commands.sh

@@ -13,6 +13,7 @@ test_expect_success "current dir is writable" '
 '

 test_expect_success "ipfs version succeeds" '
+	ipfs version


is this needed?

whyrusleeping · 2017-04-06T22:55:50Z

test/sharness/t0020-init.sh

@@ -128,7 +128,7 @@ test_expect_success "clean up ipfs dir" '

 test_init_ipfs

-test_launch_ipfs_daemon
+test_launch_ipfs_daemon -D


what is the -D for?

whyrusleeping · 2017-04-06T22:56:29Z

test/sharness/t0030-mount.sh

@@ -58,6 +58,7 @@ test_expect_success "mount directories cannot be removed while active" '
 '

 test_expect_success "unmount directories" '
+	sleep 1


why is this needed?

whyrusleeping · 2017-04-06T22:57:35Z

test/sharness/t0051-object.sh

@@ -89,6 +89,10 @@ test_object_cmd() {
 	'

 	test_expect_success "'ipfs object put broken.hxml' output looks good" '
+		# TODO remove this


whyrusleeping

Looking really good. I left a few comments, mostly will want all the excess debug statements and commented out things removed before merge. Also i'm very skeptical of any changes to the sharness tests.

Kubuxu · 2017-04-07T15:41:09Z

Also go-ipfs-cmds has to be gxify'ed, Codeclimate and CI should pass.
I think you should be able to dynamically add signoffs to those comments. This might be requirement.

keks · 2017-04-07T16:23:39Z

Looking really good. I left a few comments, mostly will want all the excess debug statements and commented out things removed before merge. Also i'm very skeptical of any changes to the sharness tests.

Thanks! Yes, sharness changes are temporary so I see why the test fails and to skip those that don't work on my machine

Also go-ipfs-cmds has to be gxify'ed, Codeclimate and CI should pass.
I think you should be able to dynamically add signoffs to those comments. This might be requirement.

Of course! Once it is gxified CI should be at least able to test it. And I'll stir those commits again anyway, in the end there will be a clean signed off and licensed set of commits. Maybe I'll even squash it down to one commit so there are no commits that don't pass the tests. What is your preference on this?

Kubuxu · 2017-04-07T16:28:51Z

If there are commits that do not pass, better squash them.

whyrusleeping · 2017-04-16T18:12:44Z

@keks could you either sign those commits off or squash them? The gitcop notifications are murdering me

keks · 2017-04-16T18:16:06Z

Yeah sorry. I wanted to squash later and going through all of them is also a hassle. I'll just squash early.

Btw it seems I surfaced the race condition problem. Not sure yet how to tackle that one when it only fires on CI but not locally.

daviddias · 2017-10-19T09:42:46Z

Me and @whyrusleeping were inspecting and exploring options on how to proceed. tl;dr; Here is the type of responses + respective headers we need

x-catdog + application/json -> ndjson
x-catdog + plain-text or acted-stream -> stream of data
application/json (without x-catdog) -> single JSON object
# x-catdog is just a name to signal that it is a stream of things, we can continue using x-stream or x-chunked, bot not both (pick one, stick to it

This is a breaking http-api change but it might not be breaking for every API client as JS and Go were the primary users of x-chunked vs x-stream that we no longer need.

whyrusleeping · 2017-10-19T09:45:40Z

(note: 'catdog' is just a placeholder so we didnt confuse ourselves with 'Stream' and 'Chunk' because they have meanings other than the meaning that is implied by the header)

Kubuxu · 2017-10-19T14:43:04Z

We could also be explicit about it: ContentType: application/json vs aplication/ndjson.

keks · 2017-10-23T17:32:01Z

Whoops. Wrong button.

This now passes js-ipfs-api, go test and sharness. I made a tiny change to one sharness test, I hope that's fine. I'm happy to change the HTTP header semantics, but please let's not do it in this PR as it is not actually within scope.

whyrusleeping · 2017-10-25T18:43:51Z

@keks i think you need to update your go-ipfs-cmds dep. I'm getting some weird preamble thing printed out in my daemon

License: MIT Signed-off-by: keks <keks@cryptoscope.co>

whyrusleeping · 2017-11-18T18:10:56Z

off we go!

Extract and rework commands package

jbenet added the status/in-progress In progress label Apr 6, 2017

whyrusleeping reviewed Apr 6, 2017

View reviewed changes

core/commands/get.go Outdated

v, err := res.Next()

if err != nil {

//TODO XXX

Copy link

Member

whyrusleeping Apr 6, 2017

Choose a reason for hiding this comment

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

TODO

whyrusleeping reviewed Apr 6, 2017

View reviewed changes

keks force-pushed the feat/commands2.0 branch 3 times, most recently from 34245d5 to 2f8df94 Compare April 16, 2017 17:11

keks force-pushed the feat/commands2.0 branch 3 times, most recently from d05110d to 2625c2c Compare October 23, 2017 16:43

keks closed this Oct 23, 2017

keks reopened this Oct 23, 2017

ghost added status/in-progress In progress and removed status/in-progress In progress labels Oct 23, 2017

keks force-pushed the feat/commands2.0 branch 2 times, most recently from 6ac1c1e to 7d2039a Compare November 2, 2017 15:04

keks force-pushed the feat/commands2.0 branch from 7d2039a to 434ee5c Compare November 13, 2017 14:12

keks and others added 3 commits November 17, 2017 15:22

cmd: use go-ipfs-cmds

f287524

License: MIT Signed-off-by: keks <keks@cryptoscope.co>

compatible to js-ipfs-api

0d9d218

License: MIT Signed-off-by: keks <keks@cryptoscope.co>

update to go-ipfs-cmds 0.4.9

d95a87c

License: MIT Signed-off-by: keks <keks@cryptoscope.co>

keks force-pushed the feat/commands2.0 branch from 434ee5c to d95a87c Compare November 17, 2017 14:23

whyrusleeping merged commit 83df676 into master Nov 18, 2017

ghost removed the status/in-progress In progress label Nov 18, 2017

djdv mentioned this pull request Nov 19, 2017

Fix orphaned gx dependency #4400

Merged

ghost mentioned this pull request Nov 20, 2017

PubSub tests hang with go-ipfs master ipfs/go-ipfs-api#62

Closed

Stebalien mentioned this pull request Nov 20, 2017

gx: Update go-datastore to 1.4.0 #4399

Merged

keks mentioned this pull request Dec 13, 2017

commands: Rework Response interface (also affects Command) #3524

Closed

kevina mentioned this pull request Jan 29, 2018

ipfs stats bw contains extra row with unused headers #4620

Closed

gammazero deleted the feat/commands2.0 branch April 14, 2021 18:14

laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022

Merge pull request ipfs#3856 from ipfs/feat/commands2.0

425d21d

Extract and rework commands package

laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022

go-ipfs-config: Merge pull request ipfs#3856 from ipfs/feat/commands2.0

3b9b989

Extract and rework commands package

Extract and rework commands package #3856

Extract and rework commands package #3856

Conversation

keks commented Apr 6, 2017 • edited Loading

Backwards compatibility

Shared code

Basic model

Marshalers, Encoders, PostRun

Changes to the sharness tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping left a comment

Choose a reason for hiding this comment

Kubuxu commented Apr 7, 2017

keks commented Apr 7, 2017

Kubuxu commented Apr 7, 2017

whyrusleeping commented Apr 16, 2017

keks commented Apr 16, 2017

daviddias commented Oct 19, 2017

whyrusleeping commented Oct 19, 2017

Kubuxu commented Oct 19, 2017

keks commented Oct 23, 2017

whyrusleeping commented Oct 25, 2017

whyrusleeping commented Nov 18, 2017

keks commented Apr 6, 2017 •

edited

Loading