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

plugin: create plugin API and loader, add ipld-git plugin #4033

Merged
merged 23 commits into from
Jul 16, 2017

Conversation

whyrusleeping
Copy link
Member

This is very much a work in progress, but it works and is pretty fun to play around with.

If you want to try it, pull this branch down and:

make install
mkdir $IPFS_PATH/plugins
go build -buildmode=plugin -o="$IPFS_PATH/plugins/git.so" ./plugins/git

Then start ipfs, it will now be loaded with git support. You can install https://github.com/magik6k/git-remote-ipld and then try out git push ipld:: to push a git repo into ipfs and then git clone ipld::GITSHA to clone it from ipfs.

@whyrusleeping whyrusleeping added the status/in-progress In progress label Jul 4, 2017
@whyrusleeping
Copy link
Member Author

cc @kumavis and @hermanjunge, this will be how we load in blockchain support too

@victorb
Copy link
Member

victorb commented Jul 4, 2017

Really cool to have plugin support finally :) However, the following disclaimer exists in the documentation: Currently plugins only work on Linux. but I'm unsure if this is for building with plugins, building the plugins or including the plugins...

@whyrusleeping
Copy link
Member Author

@victorbjelkholm its for all of the above. The plugin stuff only works on linux. If people want this on platforms other than linux, pester the go team.

LoadPlugins = LoadPluginsLinux
}

func LoadPluginsLinux(cfgroot string) error {
Copy link
Member

Choose a reason for hiding this comment

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

this can be private.

"github.com/ipfs/go-cid"
"github.com/ipfs/go-ipfs/core/coredag"
git "github.com/ipfs/go-ipld-git"
"gx/ipfs/QmUBtPvHKFAX43XMsyxsYpMi3U5VwZ4jYFTo4kFhvAR33G/go-ipld-format"
Copy link
Member

Choose a reason for hiding this comment

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

Spacing and go-ipld-git is not imported.

var DefaultInputEncParsers = InputEncParsers{
"json": DefaultJsonParsers,
"raw": DefaultRawParsers,
}
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if making this global is the right decision.
This will make testing it harder.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we dont use a global, registration becomes a much more complicated thing to do. Any ideas on a good way to do that?

Copy link
Member

Choose a reason for hiding this comment

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

Can't we do the same thing we're doing with BlockDecoders (pass it to register)?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I am planning that.

Copy link
Member

Choose a reason for hiding this comment

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

BlockDecoder is doing the same (it is using DefaultBlockDecoder from go-ipld-format).

Copy link
Member

Choose a reason for hiding this comment

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

Good point. The problem isn't really registration, it's using the the decoders (e.g. in ParseInputs).

Copy link
Member

Choose a reason for hiding this comment

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

In the Plugin API for IPLD I've sketched out I've made the registration map an argument so at least the plugins can be tested.

We have to figure out how to use the decoders well with non-global states.

regfunci, err := plugin.Lookup("Register")
if err != nil {
return fmt.Errorf("ipld plugin had no Register function: %s", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Regarding API for plugins themselves, I think it might be better to have a plugin expose a structure of callback functions, implemented interface or overridden structure. This way the API is cleanly defined in Go code instead of having to be written somewhere separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrm... so we could maybe do something like:

plug, _ := plugin.Lookup("Plugin")
switch plug := plug.(type) {
case IpldPlugin:
case Libp2pPlugin:
case DatastorePlugin:
default:
   // unsupported plugin type!
}

Copy link
Member

Choose a reason for hiding this comment

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

That would work.

@Kubuxu
Copy link
Member

Kubuxu commented Jul 5, 2017

Once we decide on structure more I will add Makefile support for it.

@Stebalien
Copy link
Member

This may be beyond the scope of this PR but it would be really nice to make encoders/decoders composable and separate. That is, when we re-encode, always decode to a common (in-memory) format/interface and then re-encode to the target format. Unfortunately, the only way I think of doing this in go is to decode to a dynamically typed interface{} of maps, arrays, integers, strings, etc... So, how about something like ipfs/go-ipld-format#18.

@Kubuxu Kubuxu force-pushed the feat/git-plugin branch 2 times, most recently from 510f5c1 to 8c84c76 Compare July 9, 2017 18:36
@Kubuxu
Copy link
Member

Kubuxu commented Jul 9, 2017

This should be ready for first CR, I will try adding some tests and more UX regarding plugins on other systems (display warnings if there is a plugin in directory but system is not supported).

@Kubuxu
Copy link
Member

Kubuxu commented Jul 9, 2017

Hah, I can't request review from @whyrusleeping's own PR.

@whyrusleeping
Copy link
Member Author

would be pretty cool if github had a 'take over PR' button

typePls, ok := pls.([]iplugin.Plugin)
if !ok {
return nil, errors.New("filed 'Plugins' didn't contain correct type")
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

no need for the else

@whyrusleeping
Copy link
Member Author

would be pretty cool if github had a 'take over PR' button.

You also probably don't want to commit the plugin git.so binary.

@Kubuxu
Copy link
Member

Kubuxu commented Jul 10, 2017

Oops.

@Kubuxu Kubuxu changed the title Feat/git plugin plugin: create plugin API and loader, add ipld-git plugin Jul 10, 2017
@Kubuxu Kubuxu force-pushed the feat/git-plugin branch 6 times, most recently from a91be3c to 9b2cfae Compare July 12, 2017 18:50
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu
Copy link
Member

Kubuxu commented Jul 13, 2017

Whoa, tests are green. Git IPLD plugin is being loaded dynamically and works (passes sharness tests written by @magik6k).

Should be ready for CR: @whyrusleeping @Stebalien

This PR contains quite a bit of Makefile magic to glue plugins and tests together, provide option for preloading plugins (plugin/loader/preload_list) and allow for dynamic loading of plugins and preloading them (to load plugin dynamically it has to be defined in a main package, to preload it can't).

@Kubuxu
Copy link
Member

Kubuxu commented Jul 13, 2017

I just saw that coverage collection failed, so please do not merge I have to fix it first.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

What I've looked at so far looks good. There are a few things I'd like to see improved eventually but they're not reasons to hold this up.


I'm a bit concerned about loading plugins from a directory owned by the user IPFS is running under but this is very convenient (and probably not a problem). However, in the future, we'll probably want to allow loading plugins out of /usr/lib/plugins/... (for distros and package managers).

Now, it would be awesome if we could load plugins directly from IPFS but we can discuss that later.


I really don't like having one stream parser per input-output pair. However, doing this the correct way would require quit a bit of work and thought so I'm going to say that this is good enough for now.

)

func init() {
loadPluginsFunc = linxuLoadFunc
Copy link
Member

Choose a reason for hiding this comment

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

s/linxu/linux/g

format "gx/ipfs/QmYNyRZJBUYPNrLszFmrBrPJbsBh2vMsefz5gnDpB5M1P6/go-ipld-format"
)

func initalize(plugins []plugin.Plugin) error {
Copy link
Member

Choose a reason for hiding this comment

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

s/initalize/initialize/g

License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu
Copy link
Member

Kubuxu commented Jul 14, 2017

I don't think it is possible for me to build plugins with coverage collection. This means I will either have to preload plugins for coverage collection or disable plugins during coverage collection.

I think for now I will disable plugins and plugin tests during coverage collection.

License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu
Copy link
Member

Kubuxu commented Jul 14, 2017

Coverage of the patch is low as the sharness won't work well with the plugins.
In future I might work on running go-ipfs with plugins preloaded for testing.

I can't test the dynamic plugin loading in the sharness and this is the part that misses coverage the most.

@Kubuxu Kubuxu mentioned this pull request Jul 14, 2017
return nil
}

func runIPLDPlugin(pl plugin.Plugin) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

do a type switch up in the 'run' function, and have this function accept the proper type. Then we can error out if the types don't match correctly instead of silently ignoring the problem (which could easily happen with a mis-compiled plugin)

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about it, but a Plugin could in theory implement multiple interfaces, then typeswich isn't really helping us as it will choose just one interface (the first in the switch statement).

Miss-compiled plugin will error out or log if: the plugin lib can't be loaded, there is no Plugins variable, it has wrong type.

}

var err error
err = ipldpl.RegisterBlockDecoders(format.DefaultBlockDecoder)
Copy link
Member Author

Choose a reason for hiding this comment

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

why not just err := .... ?

return err
}

err = ipldpl.RegisterInputEncParsers(coredag.DefaultInputEncParsers)
Copy link
Member Author

Choose a reason for hiding this comment

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

return ipldpl.Register....

@@ -0,0 +1,2 @@
*.so
*/main
Copy link
Member Author

Choose a reason for hiding this comment

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

what is this main ignore for?

Copy link
Member

Choose a reason for hiding this comment

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

This is autogenerated package in directory of every plugin. This is what I have to do so the plugin can be either pre-loaded (statically compiled with go-ipfs, for non-linux builds) or be compiled for dynamic linking as plugin.

Go requires plugins to be defined in package named main, and at the same time it doesn't allow for static linking of main package. So I have main plugin inside its own package and I generate main package that rebinds Plugins variable.

@@ -0,0 +1,57 @@
#!/bin/sh
#
# Copyright (c) 2016 Jakub Sztandera
Copy link
Member Author

Choose a reason for hiding this comment

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

its 2017 btw

@whyrusleeping
Copy link
Member Author

This LGTM, great work @Kubuxu :)

I'll wait for your confirmation before merging, but I think this is ready to go.

License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@whyrusleeping whyrusleeping merged commit 71bb417 into master Jul 16, 2017
@whyrusleeping whyrusleeping deleted the feat/git-plugin branch July 16, 2017 09:02
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Jul 16, 2017
@magik6k magik6k added this to the Ipfs 0.4.11 milestone Aug 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants