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

cmds/update: use new cmds lib #5730

Merged
merged 1 commit into from
Nov 9, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 26 additions & 27 deletions core/commands/external.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ import (
"os/exec"
"strings"

cmds "github.com/ipfs/go-ipfs/commands"
commands "github.com/ipfs/go-ipfs/commands"

"gx/ipfs/Qmde5VP1qUkyQXKCfmEUA7bP64V2HAptbJ7phuPp7jXWwg/go-ipfs-cmdkit"
cmds "gx/ipfs/Qma6uuSyjkecGhMFFLfzyJDPyoDtNJSHJNweDccZhaWkgU/go-ipfs-cmds"
cmdkit "gx/ipfs/Qmde5VP1qUkyQXKCfmEUA7bP64V2HAptbJ7phuPp7jXWwg/go-ipfs-cmdkit"
)

func ExternalBinary() *cmds.Command {
Expand All @@ -19,29 +20,27 @@ func ExternalBinary() *cmds.Command {
cmdkit.StringArg("args", false, true, "Arguments for subcommand."),
},
External: true,
Run: func(req cmds.Request, res cmds.Response) {
binname := strings.Join(append([]string{"ipfs"}, req.Path()...), "-")
Run: func(req *cmds.Request, res cmds.ResponseEmitter, env cmds.Environment) error {
binname := strings.Join(append([]string{"ipfs"}, req.Path...), "-")
_, err := exec.LookPath(binname)
if err != nil {
// special case for '--help' on uninstalled binaries.
for _, arg := range req.Arguments() {
for _, arg := range req.Arguments {
if arg == "--help" || arg == "-h" {
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
return res.Emit(buf)
}
}

res.SetError(fmt.Errorf("%s not installed", binname), cmdkit.ErrNormal)
return
return fmt.Errorf("%s not installed", binname)
}

r, w := io.Pipe()

cmd := exec.Command(binname, req.Arguments()...)
cmd := exec.Command(binname, req.Arguments...)

// TODO: make commands lib be able to pass stdin through daemon
//cmd.Stdin = req.Stdin()
Expand All @@ -50,39 +49,39 @@ func ExternalBinary() *cmds.Command {
cmd.Stderr = w

// setup env of child program
env := os.Environ()
osenv := os.Environ()

// Get the node iff already defined.
if req.InvocContext().Online {
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is important. We should onlu get the node if-and-only-if (iff) it is already defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevina Should we add a helper function cmdenv.IsOnline()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it used in more than one or two places? Is do I would define one. Otherwise I wouldn't bother.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevina Could u help me review it again?

nd, err := req.InvocContext().GetNode()
if cctx, ok := env.(*commands.Context); ok && cctx.Online {
nd, err := cctx.GetNode()
if err != nil {
res.SetError(fmt.Errorf(
"failed to start ipfs node: %s",
err,
), cmdkit.ErrFatal)
return
return fmt.Errorf("failed to start ipfs node: %s", err)
}
env = append(env, fmt.Sprintf("IPFS_ONLINE=%t", nd.OnlineMode()))
osenv = append(osenv, fmt.Sprintf("IPFS_ONLINE=%t", nd.OnlineMode()))
}

cmd.Env = env
cmd.Env = osenv

err = cmd.Start()
if err != nil {
res.SetError(fmt.Errorf("failed to start subcommand: %s", err), cmdkit.ErrNormal)
return
return fmt.Errorf("failed to start subcommand: %s", err)
}

res.SetOutput(r)
errC := make(chan error)

go func() {
var err error
defer func() { errC <- err }()
err = cmd.Wait()
if err != nil {
res.SetError(err, cmdkit.ErrNormal)
}

w.Close()
}()

err = res.Emit(r)
if err != nil {
return err
}

return <-errC
},
}
}
2 changes: 1 addition & 1 deletion core/commands/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ var rootSubcommands = map[string]*cmds.Command{
"swarm": SwarmCmd,
"tar": TarCmd,
"file": unixfs.UnixFSCmd,
"update": lgc.NewCommand(ExternalBinary()),
"update": ExternalBinary(),
"urlstore": urlStoreCmd,
"version": VersionCmd,
"shutdown": daemonShutdownCmd,
Expand Down