-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
initial support for commands to use external binaries #1961
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,11 @@ dependencies as well. | |
* Shell command completion is available in `misc/completion/ipfs-completion.bash`. Read [docs/command-completion.md](docs/command-completion.md) to learn how to install it. | ||
* See the [init examples](https://github.com/ipfs/examples/tree/master/examples/init) for how to connect IPFS to systemd or whatever init system your distro uses. | ||
|
||
### Updating | ||
ipfs has an updating tool that can be accessed through `ipfs update`. The tool is | ||
not installed alongside ipfs in order to keep that logic indepedent of the main | ||
codebase. To install ipfs update, either [download it here](https://gobuilder.me/github.com/ipfs/ipfs-update) | ||
or install it from source with `go get -u github.com/ipfs/ipfs-update`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we likely should install it along with ipfs. we can use ipfs itself to install it. or we should explore the idea of distributing a toolchain with multiple binaries. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, i think once we get dist.ipfs.io working, we should have an installer that pulls all these things down. But for now, this is true. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||
|
||
## Usage | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,9 +9,12 @@ output to the user, including text, JSON, and XML marshallers. | |
package commands | ||
|
||
import ( | ||
"bytes" | ||
"errors" | ||
"fmt" | ||
"io" | ||
"os" | ||
"os/exec" | ||
"reflect" | ||
"strings" | ||
|
||
|
@@ -59,6 +62,10 @@ type Command struct { | |
Marshalers map[EncodingType]Marshaler | ||
Helptext HelpText | ||
|
||
// External denotes that a command is actually an external binary. | ||
// fewer checks and validations will be performed on such commands. | ||
External bool | ||
|
||
// Type describes the type of the output of the Command's Run Function. | ||
// In precise terms, the value of Type is an instance of the return type of | ||
// the Run Function. | ||
|
@@ -262,3 +269,67 @@ func checkArgValue(v string, found bool, def Argument) error { | |
func ClientError(msg string) error { | ||
return &Error{Code: ErrClient, Message: msg} | ||
} | ||
|
||
func ExternalBinary() *Command { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this whole function is very ipfs-specific. it does not belong in the commands lib There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (or make it general, but may be harder atm) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed. |
||
return &Command{ | ||
Arguments: []Argument{ | ||
StringArg("args", false, true, "arguments for subcommand"), | ||
}, | ||
External: true, | ||
Run: func(req Request, res Response) { | ||
binname := strings.Join(append([]string{"ipfs"}, req.Path()...), "-") | ||
_, err := exec.LookPath(binname) | ||
if err != nil { | ||
// special case for '--help' on uninstalled binaries. | ||
if req.Arguments()[0] == "--help" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it may not be the first argument. should check any of the args. maybe also for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed. |
||
buf := new(bytes.Buffer) | ||
fmt.Fprintf(buf, "%s is an 'external' command.\n", binname) | ||
fmt.Fprintf(buf, "it does not currently appear to be installed.\n") | ||
fmt.Fprintf(buf, "please refer to the ipfs documentation for instructions\n") | ||
res.SetOutput(buf) | ||
return | ||
} | ||
|
||
res.SetError(fmt.Errorf("%s not installed."), ErrNormal) | ||
return | ||
} | ||
|
||
r, w := io.Pipe() | ||
|
||
cmd := exec.Command(binname, req.Arguments()...) | ||
|
||
// TODO: make commands lib be able to pass stdin through daemon | ||
//cmd.Stdin = req.Stdin() | ||
cmd.Stdin = io.LimitReader(nil, 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so i guess cannot use stdin yet? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope. The daemon doesnt pass it through in an easy to use way, the best we get is reading it all in beforehand, and passing it as an argument. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. other commands have a proper stdin argument thing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (but ok) |
||
cmd.Stdout = w | ||
cmd.Stderr = w | ||
|
||
// setup env of child program | ||
env := os.Environ() | ||
|
||
nd, err := req.InvocContext().GetNode() | ||
if err == nil { | ||
env = append(env, fmt.Sprintf("IPFS_ONLINE=%t", nd.OnlineMode())) | ||
} | ||
|
||
cmd.Env = env | ||
|
||
err = cmd.Start() | ||
if err != nil { | ||
res.SetError(fmt.Errorf("failed to start subcommand: %s", err), ErrNormal) | ||
return | ||
} | ||
|
||
res.SetOutput(r) | ||
|
||
go func() { | ||
err = cmd.Wait() | ||
if err != nil { | ||
res.SetError(err, ErrNormal) | ||
} | ||
|
||
w.Close() | ||
}() | ||
}, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"independent"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch.