Plugin-style asset backends #362
Conversation
|
Thanks for tackling this. TBH I haven't worked with plugins, only read a bit about them, so this is very informative to me. However, it's going to take a bit of extra review on my part. |
|
Thanks for your patience on this. Since this is not an MVP feature and we're trying to get some critical stuff addressed before inviting alpha testers, it's not the highest priority now. But we want to support loading assets with plugins at some point and this is an important step. |
|
Could you write a build script in server/cmd/dcrdex that will |
I think should be a script on server/cmd/pluginbuilding Is that okay? @buck54321 |
|
A script in server/cmd/dcrdex is better for me. The script should build the server as well. |
There was a problem hiding this comment.
Looks like a good start, although I'm starting to have concerns about plugins in general (cgo, require exact build environment, etc.) so we need to have a legacy build mode. Let's do this by creating a server/cmd/dcrdex/assets_plugins.go that does the plugin loading with // +build plugins, and assets.go that does the imports instead with // +build !plugins. Note that the default if just building dcrdex without a build script would then to not use the plugins.
server/cmd/dcrdex/main.go
Outdated
| if err == nil && !file.IsDir() && filepath.Ext(file.Name()) == ".so" { | ||
| plugin.Open(filepath.Join(cfg.AssetsDataDir, file.Name())) |
There was a problem hiding this comment.
Need to be less platform specific. e.g. Darwin uses the .dylib extension, I believe.
There was a problem hiding this comment.
@chappjc Did you tested it. I do not use Darwin. As I researched it mean macOS. A my friend tested for me and the result is .so too
server/cmd/dcrdex/main.go
Outdated
| if err == nil && !file.IsDir() && filepath.Ext(file.Name()) == ".so" { | ||
| plugin.Open(filepath.Join(cfg.AssetsDataDir, file.Name())) |
There was a problem hiding this comment.
Need to check error returned from Open. There are a lot of ways opening a dynamic lib can fail.
README.md
Outdated
| ### Build and run assets backend plugin | ||
|
|
||
| From a command prompt, navigate to **server/asset/build** . | ||
| Build the plugin assets and copy to resource application folder by running: | ||
| ``` | ||
| go build -buildmode=plugin ./btc/ | ||
| cp ./btc.so ~/.dcrdex/assets/btc.so | ||
| go build -buildmode=plugin ./dcr/ | ||
| cp ./dcr.so ~/.dcrdex/assets/dcr.so | ||
| go build -buildmode=plugin ./ltc/ | ||
| cp ./ltc.so ~/.dcrdex/assets/ltc.so |
There was a problem hiding this comment.
In playing with this plugin PR, I discovered that plugins practically require the user to build both main and all the other packages with an IDENTICAL build setup, so I agree with buck that a script for building the plugins only makes sense if that script is also building dcrdex.
| go build -buildmode=plugin ./ltc/ | ||
| cp ./ltc.so ~/.dcrdex/assets/ltc.so | ||
| ``` | ||
|
|
There was a problem hiding this comment.
It might prove to be a deal breaker that plugins require CGO.
We'll need to update the README stating that because of plugins a C complier (gcc) is also required, but otherwise dcrdex does not require cgo.
| _ "decred.org/dcrdex/server/asset/btc" // register btc asset | ||
| _ "decred.org/dcrdex/server/asset/dcr" // register dcr asset | ||
| _ "decred.org/dcrdex/server/asset/ltc" // register ltc asset |
There was a problem hiding this comment.
Eliminating three lines and the need to edit this file to add an additional asset seems to be the one and only thing we get from plugins given the requirement of identical build environments, we'll have to see how we like this when we start using it.
There was a problem hiding this comment.
As we move to decentralize the server, it will become more important to remove ourselves from decisions about what assets and markets are available.
There was a problem hiding this comment.
I agree with that. An op should be able to add and remove asset plugins without code modification, although the op would realistically need to build the plugins and the rest of the server code at the same time on the same machine due to limitations of the Go plugin system.
Although, I'd very much like the ability to have a statically linked option as described in the main review comment #362 (review). And personally I'd prefer the default build to be non-plugins, with a build script for the plugins version. EDIT: Though in the interest of dog fooding, I'd accept plugins as the default mode.
There was a problem hiding this comment.
I'm okay with doing both too. Have some built in (nobody has to use the asset), and then also allow plugins.
|
Note the proposed spec update at https://github.com/buck54321/dcrdex/blob/spec-roundup/spec/fundamentals.mediawiki/#Adding_New_Assets |
server/asset/build/btc/btc.go
Outdated
| package main | ||
|
|
||
| import ( | ||
| "decred.org/dcrdex/server/asset" | ||
| "decred.org/dcrdex/server/asset/btc" | ||
| ) | ||
|
|
||
| func init() { | ||
| asset.Register(btc.AssetName, &btc.Driver{}) | ||
| } |
There was a problem hiding this comment.
Plugin backends will need to Register themselves, so I think that's the pattern we should use here too. Let's move these back to the server/asset/{btc,ltc.dcr} package inits.
There was a problem hiding this comment.
Plugin backends will need to
Registerthemselves, so I think that's the pattern we should use here too. Let's move these back to the server/asset/{btc,ltc.dcr} packageinits.
fixed
|
Closing since plugins turned out to be a no-go on account of cgo as well as poor platform support (no openbsd and others). We can revisit this in the future. |
#127
Build plugin asset and move it to
~/.dcrdex/assets/.dcrdexwill look up all file.soin this folder and run it as Plugin