[WIP] New node constructor

[magik6k]

Putting this here for visibility, there is still some fixing and cleanups needed.

Feel free to have a look at the code, but don't bother to review fully yet (there are some questions in TODOs that may need decisions).

Notes:

• There was some restructuring of cmd/ipfs package done.
• New constructor code lives in core/coreapi/node.go
  • For now in the coreapi package because of some circular import problems, there are few potential solutions I didn't try yet.
• Old code wasn't removed yet

Questions:

• Should I try to rewrite old constructor on top of this for compatibility? (It should be quite easy)
• Do we want to start deprecating the IpfsNode struct in favour of something else, like just the DI container?

[Stebalien]

Should I try to rewrite old constructor on top of this for compatibility? (It should be quite easy)

I wouldn't bother. If it's easy, it's easy for users to migrate. We can also migrate ipget as an example.

Do we want to start deprecating the IpfsNode struct in favour of something else, like just the DI container?

👍

[magik6k]

Status update: This is ready for a review, and should be possible to merge in less than a week.

• I'm debugging a case where the daemon won't close when a pubsub sub is running
• There are still a few minor todos that need to happen before this is merged.
• I'd prefer this to be merged soon, rebasing this will be very, very painful

@Stebalien would be great if you could have a look at at least core/coreapi/node.go

[Kubuxu]

@magik6k can you either squash it or (more work) make commits correspond to logical units of work. This is a big change so reviewing it will be difficult.

[magik6k]

Many commits touch things in multiple places, and bulk of changes has happened in a few big commits. I'd go about reviewing file by file (and leave commits unsquashed as it might be helpful when debugging regressions if one happens)

Summary of changes:

• cmd/ipfs was split into more files and we construct node in one place
  • cmd/ipfs/api.go resolves and opens api endpoint if one is running
  • cmd/ipfs/debug.go contains few debug related helpers
  • cmd/ipfs/daemon.go
    • The special deprecation notice for the supernode routing option was dropped entirely, there probably aren't any nodes left which keep this configured
  • cmd/ipfs/plugin.go contains plugin-related logic
  • I did that in an effort to make understanding the code flow in those files easier, plus some of this logic will be ported to the constructor itself (openning repo, loading plugins, handling api endpoints, etc. (commands will likely be a plugin, daemon too))
• commands/context.go - ConfigRoot renamed to RepoPath - makes it more obvious what it points to
• core/builder.go - entrypoint to the new constructor, removed
• core/coreapi/node.go - The new constructor
  • Living in the CoreAPI package because of some very annoying circular import problems
    • We should probably just create a separate package for it (or just put it in repo root, so the entry point to ipfs in Go code would be where most library users would expect a library entry point to be)
  • It's basically applying a bit extended pattern of functional options to setup fx options in the desired way. The API probaly isn't ideal at this point, which is why I asked whether we should try to keep the old constructor (the one from core/builder.go) for a little longer (It should be rather simple to do)
• core/node/builder.go / core/node/groups.go removed
  • Part of the old constructor, code in core/coreapi/node.go should do the same thing, but in an arguably cleaner way (and definitely more extensible)
• core/node/libp2p/host.go
  • There is now no Routing/HostOption, instead we provide few alternative constructors user can swap with the Override option (or if we create one, a nicer helper function)
• repo/repo.go - Dropped repo.Datastore type in favour of just using ds.Batching directly
  • This makes some DI stuff nicer
• Rest of the changes are mostly swapping the old constructor for the new one

I'd probably start the review from core/coreapi/node.go, then scroll back to the top and go file by file. Most changes are quite file scoped, so there is little reason to go commit-by-commit (even if squashed), plus the fact that we have nicely organized packages makes related changes be next to each other.

[Kubuxu]

@magik6k from what I can see, most of those commits don't build or fail tests so they are useless for finding regressions (bisect won't work and walking them one by one probably won't work either).

[magik6k]

In most of these commits most tests pass, and given that when using bisect you usually don't run the whole test suite - you just test the affected thing, this shouldn't affect bisecting much. If you can't run bisect on some commit because it's between some breaking change, and relevant update, you just call git bisect skip.

Having this squashed to 3-5 commits that pass tests, doing git bisect would get you one huge commit which wouldn't narrow the scope of what could be broken much.

[Stebalien]

I wish there were a better way to override/provide defaults but I can't think of anything better. My best "pure fx" solution is to use named provides but I can't think of a way to do this without a bunch of boilerplate.

package main

import (
	"context"
	"fmt"

	"go.uber.org/fx"
)

type foo string
type bar string

// Override overrides the named option.
func Override(thing interface{}) fx.Option {
	return fx.Provide(fx.Annotated{
		Name:   "override",
		Target: thing,
	})
}

// DefaultFoobar provides a default foobar unless an override has been
// specified.
func DefaultFoobar(s struct {
	fx.In
	Foo foo `name:"override" optional:"true"`
	Bar bar `name:"override" optional:"true"`
}) (foo, bar) {
	if s.Foo == "" {
		s.Foo = "default foo"
	}
	if s.Bar == "" {
		s.Bar = "default bar"
	}
	return s.Foo, s.Bar
}

func main() {
	app := fx.New(
		Override(func() foo { return "explicit foo" }),
		//Override(func() bar { return "explicit bar" }),
		fx.Provide(DefaultFoobar),
		fx.Invoke(func(foo foo, bar bar) {
			fmt.Println(foo)
			fmt.Println(bar)
		}),
	)

	if err := app.Start(context.Background()); err != nil {
		panic(err)
	}
}

---

I wonder how well this will integrate with libp2p once we switch that over to something like fx? I'd like to share the same fx instance.

[Stebalien]

Possibly relevant: uber-go/fx#653

[Stebalien]

We should probably start experimenting with libp2p a bit before we continue.