initial support for commands to use external binaries #1961

whyrusleeping · 2015-11-11T21:39:36Z

Allows us to have ipfs-update as a separate binary be called as ipfs update

License: MIT
Signed-off-by: Jeromy jeromyj@gmail.com

ghost · 2015-11-11T21:52:44Z

Also ipfs screencap and ipfs paste eh?

whyrusleeping · 2015-11-11T22:04:12Z

@lgierth maybe in the future. Right now its done via whitelisting commands manually (the easiest route).

The git style subcommand extension is really nice, but we do want to be careful with what 'our tool' does, if the user has an out of date ipfs-XYZ binary, and it breaks, that looks bad for us.

chriscool · 2015-11-12T01:09:42Z

There is a related discussion currently on the Git mailing list:

http://thread.gmane.org/gmane.comp.version-control.git/281124

rht · 2015-11-12T01:37:56Z

The git style subcommand extension is really nice, but we do want to be careful with what 'our tool' does, if the user has an out of date ipfs-XYZ binary, and it breaks, that looks bad for us.

External commands are wired in git alias through the '!' prefix. This is explicit, and I don't think any user would mistakenly mix between the built-in tools and the external ones.
Also, git allows redefining shorthand verbs from the built-in ones + flags, if this is going to be added.

whyrusleeping · 2015-11-12T03:04:42Z

@rht if i make a script named git-amazing that does rm -rf / --no-preserve-root and put it in my path, then run git amazing, my harddrive is now kill, and it 'looks' like git did it.

I want to make sure that running the ipfs tool doesn't accidentally cause bad things.

rht · 2015-11-12T04:05:33Z

In that case, it does 'look' like git did it, but git still allows such script. Maybe because people have done less bad things through git than through the web. And I take it that ipfs, in the context as a git+web, is restricted (of what it allows people can do) by the potential bad things people would do through the web?
(while the web already has plenty of symlinks)

Would be nice to have the shorthand aliases, though it is not required to deliver ipfs-update.
Alternatively, as from the git built-ins override discussion, some of the default built-ins can already be modified through config options (e.g. branch.autosetuprebase), that it is not necessary.

whyrusleeping · 2015-11-12T08:34:46Z

@rht yeah, i'm not totally against having the mechanism be more generic, but i definitely think it needs some more thought. For now, this works so we can ship ipfs-update, in the future we can look at making it more extensible.

jbenet · 2015-11-16T16:53:59Z

commands/command.go

+				if req.Arguments()[0] == "--help" {
+					buf := new(bytes.Buffer)
+					fmt.Fprintf(buf, "%s is an 'external' command.\n", binname)
+					fmt.Fprintf(buf, "it does not currently appear to be installated.\n")


"installed"

installificated!

jbenet · 2015-11-16T16:56:28Z

@whyrusleeping this LGTM. but should the ipfs-update bin be referenced (at least with gx so it's pinned?) not sure.

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

whyrusleeping · 2015-11-16T17:18:17Z

@jbenet for now, i'm thinking that we use gobuilder for ipfs-update at least until we have our own build system set up.

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

rht · 2015-11-16T19:44:58Z

README.md

@@ -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


"independent"

good catch.

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

whyrusleeping · 2015-12-04T08:00:01Z

@jbenet final thoughts here? Lets merge this so we can ship 0.3.10

jbenet · 2015-12-04T13:19:46Z

commands/command.go

@@ -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 {


this whole function is very ipfs-specific. it does not belong in the commands lib

(or make it general, but may be harder atm)

jbenet · 2015-12-04T13:27:35Z

responded

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

whyrusleeping · 2015-12-07T01:27:02Z

@jbenet LGTY?

jbenet · 2015-12-07T05:34:07Z

@whyrusleeping LGTM

initial support for commands to use external binaries

jbenet added the status/in-progress In progress label Nov 11, 2015

whyrusleeping force-pushed the feat/external-bins branch from 448e775 to ae0a45a Compare November 11, 2015 22:29

jbenet reviewed Nov 16, 2015
View reviewed changes

initial support for commands to use external binaries

7681b66

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

fixes from PR

2d2aa66

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

whyrusleeping force-pushed the feat/external-bins branch from ae0a45a to 2d2aa66 Compare November 16, 2015 17:53

rht reviewed Nov 16, 2015
View reviewed changes

spelling

b99b778

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

jbenet reviewed Dec 4, 2015
View reviewed changes

address CR feedback

96a0726

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

whyrusleeping added this to the IPFS 0.3.10 milestone Dec 6, 2015

jbenet added a commit that referenced this pull request Dec 7, 2015

Merge pull request #1961 from ipfs/feat/external-bins

d58053b

initial support for commands to use external binaries

jbenet merged commit d58053b into master Dec 7, 2015

jbenet removed the status/in-progress In progress label Dec 7, 2015

jbenet deleted the feat/external-bins branch December 7, 2015 05:34

jbenet mentioned this pull request Dec 14, 2015

Sprint Dec 7 ipfs/team-mgmt#67

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial support for commands to use external binaries #1961

initial support for commands to use external binaries #1961

whyrusleeping commented Nov 11, 2015

ghost commented Nov 11, 2015

whyrusleeping commented Nov 11, 2015

chriscool commented Nov 12, 2015

rht commented Nov 12, 2015

whyrusleeping commented Nov 12, 2015

rht commented Nov 12, 2015

whyrusleeping commented Nov 12, 2015

jbenet Nov 16, 2015

whyrusleeping Nov 16, 2015

jbenet commented Nov 16, 2015

whyrusleeping commented Nov 16, 2015

rht Nov 16, 2015

whyrusleeping Nov 17, 2015

whyrusleeping commented Dec 4, 2015

jbenet Dec 4, 2015

jbenet Dec 4, 2015

jbenet Dec 7, 2015

jbenet commented Dec 4, 2015

whyrusleeping commented Dec 7, 2015

jbenet commented Dec 7, 2015

initial support for commands to use external binaries #1961

initial support for commands to use external binaries #1961

Conversation

whyrusleeping commented Nov 11, 2015

ghost commented Nov 11, 2015

whyrusleeping commented Nov 11, 2015

chriscool commented Nov 12, 2015

rht commented Nov 12, 2015

whyrusleeping commented Nov 12, 2015

rht commented Nov 12, 2015

whyrusleeping commented Nov 12, 2015

jbenet Nov 16, 2015

Choose a reason for hiding this comment

whyrusleeping Nov 16, 2015

Choose a reason for hiding this comment

jbenet commented Nov 16, 2015

whyrusleeping commented Nov 16, 2015

rht Nov 16, 2015

Choose a reason for hiding this comment

whyrusleeping Nov 17, 2015

Choose a reason for hiding this comment

whyrusleeping commented Dec 4, 2015

jbenet Dec 4, 2015

Choose a reason for hiding this comment

jbenet Dec 4, 2015

Choose a reason for hiding this comment

jbenet Dec 7, 2015

Choose a reason for hiding this comment

jbenet commented Dec 4, 2015

whyrusleeping commented Dec 7, 2015

jbenet commented Dec 7, 2015