New API + use bin-wrapper (bundle in all arch without any concern) + global install #89

JGAntunes · 2016-07-13T08:48:13Z

[WIP]
This should close #78, it's still a work in progress but let me know what you think so far.

There are still some issues though and further testing is needed. Also somehow the ipfs init command hangs for version 0.4.2, still have to look at what's going on with it.

JGAntunes · 2016-07-13T10:06:36Z

Ok, looks like the 0.4.2 issue is a known one ipfs/kubo#2748

daviddias · 2016-07-13T10:10:31Z

package.json

    "ipfs-api": "^4.1.0",
    "multiaddr": "^2.0.0",
    "rimraf": "^2.4.5",
    "run-series": "^1.1.4",
    "shutdown": "^0.2.4",
-    "subcomandante": "^1.0.5"
+    "subcomandante": "^1.0.5",


do we still need this module with bin-wrapper?

Yeah, because despite having a run method (which is poorly named in my opinion) this will only act like a search for the binary and run a test command to check everything is ok (usually --version or version in our case), not considering any outputs and such. So we still need something to run the command itself and, as we already use subcomandante, I assume it's probably the best choice.

I just filed an issue with node-core about not cleaning up spawned processes. So maybe in a near future we don't need it anymore :)
nodejs/node#7951

daviddias · 2016-07-13T10:18:45Z

This is awesome @JGAntunes :D

Yeah, the issue with 0.4.2 exists, it has been fixed for 0.4.3, we just have to wait for that release. This module hasn't have any documentation so far, would you like to give a stab at it too? It will help us make sure we are fulfilling the right interface needs, @haadcode and @dignifiedquire will probably want to review the interface too.

daviddias · 2016-07-13T10:20:09Z

test/index.spec.js

@@ -322,7 +322,7 @@ describe('external ipfs binaray', () => {
    ipfsd.local((err, node) => {
      if (err) throw err

-      assert.equal(node.exec, '/some/path')
+      assert.equal(node.exec, '/some/path/ipfs')


Could you change from assert to expect?

Sure thing 👍

JGAntunes · 2016-07-13T16:38:15Z

@diasdavid as for the documentation, sure! I'll give it a look once all this stuff is sorted out.

daviddias · 2016-07-13T18:56:05Z

Thank you :)

dignifiedquire · 2016-07-18T17:10:33Z

config.js

@@ -0,0 +1,16 @@
+'use strict'


All files should be under src

JGAntunes · 2016-07-18T17:56:15Z

Ok, let me know what you all think. Still missing documentation and further testing @diasdavid @dignifiedquire

JGAntunes · 2016-07-21T11:54:20Z

While building some more tests I've noticed there's an issue with the way subcomandante handles the close event.
https://github.com/jbenet/node-subcomandante/blob/master/fork.js#L19
It may emit a new error when no further events are expected. This makes it practically impossible to handle it correctly as it is emitted after the close and end events and one doesn't know what to expect next.

daviddias · 2016-07-27T11:50:47Z

@JGAntunes Do we still need to use subcomandante with bin-wrapper?

Let's make this PR work with 0.4.3-rc1, so that we can use it to test js-ipfs-api as well

Do you need help with the documentation part?

JGAntunes · 2016-07-27T12:08:45Z

@diasdavid we still need something to run and exec the commands, the bin-wrapper run method only serves for the single purpose of testing the binary and not much more. Although, if we still want to use subcomandante, we need to fix the way the exit code is handled.

Alright, I'll update the binary for 0.4.3-rc1

I was waiting for some feedback on the matters above so I can start documenting stuff. If the API will remain unchanged I can start working on that right away.

dignifiedquire · 2016-07-27T12:43:48Z

src/config.js

+config.baseUrl = 'https://dist.ipfs.io/go-ipfs/' +
+  config.version +
+  '/go-ipfs_' +
+  config.version + '_'


Could use path.join

const distPage = 'https://dist.ipfs.io' const version = config.version config.baseUrl = path.join(distPage, 'go-ipfs', version, `go-ipfs_${version}`)

Unfortunately path.join when used on URLs strips down the // to / and invalidates it. The result would be https:/dist.ipfs.io/go-ipfs/v0.4.1/go-ipfs_v0.4.1. I can leave the protocol aside, join it later and join everything else with path.join if you think that's worth it.

oh right, I forgot about that :( No it's not worth it then, but you could instead do [..].join('/')

Yeah, true! I was also thinking on using template strings, something like:

const version = config.version config.baseUrl = `https://dist.ipfs.io/go-ipfs/${version}/go-ipfs_${version}_`

Do you prefer this or the Array.join?

I think the just template strings is actually much more readable in this case :)

daviddias · 2016-07-27T13:51:50Z

@JGAntunes you can run any command with bin-wrapper -- https://www.npmjs.com/package/bin-wrapper#runcmd-callback -- too, running run('daemon', () =>) will keep the daemon process till it is closed. Check how Akasha did it https://github.com/AkashaProject/ipfs-connector/blob/master/src/IpfsConnector.js

dignifiedquire · 2016-07-27T14:07:37Z

@diasdavid we just need to ensure that it closes the process when the parent process dies (which is why node-subcommandante exists in the first place)

JGAntunes · 2016-07-27T14:12:20Z

@diasdavid if you check the bin-wrapper source code the run method will only run as a binary check, it is not appropriate to run complex commands such as those needed here https://github.com/kevva/bin-wrapper/blob/master/index.js#L123. It will basically check the exit code of the command and that's it, nothing more.

I've already looked at how Akasha did it and if you look at it they also don't use bin-wrapper run to run anything at all besides the binary check - https://github.com/AkashaProject/ipfs-connector/blob/edd901e5b4bff9ed2783f44cb82797f49d68d54b/src/IpfsBin.js#L24. For the commands they still rely on a regular childProcess.spawn - https://github.com/AkashaProject/ipfs-connector/blob/master/src/IpfsConnector.js#L112

daviddias · 2016-07-27T16:17:27Z

@JGAntunes got it, thank you :)

daviddias · 2016-07-27T16:21:05Z

I was waiting for some feedback on the matters above so I can start documenting stuff. If the API will remain unchanged I can start working on that right away.

We need to at least:

spawn daemon - spawn a deamon with a repo from a 'path'
spawn daemon and get a js-ipfs-api object that is connected to it
spawn disposable daemon (optional config) - a daemon with a fresh repo
spawn disposable daemon and get a js-ipfs-api object that is connected to it

haadcode · 2016-07-27T16:25:07Z

We also need:

make sure the daemon process gets killed in all cases
daemon respects IPFS_PATH env variable (ie. spawn a daemon using repo from IPGS_PATH)

JGAntunes · 2016-08-01T19:42:26Z

This PR should fix the issues around node-subcomandate jbenet/node-subcomandante#5. FYI @dignifiedquire @diasdavid

dignifiedquire · 2016-08-04T18:57:17Z

@JGAntunes can you please add the aegir@6 update to this PR

dignifiedquire · 2016-08-04T18:58:13Z

@JGAntunes as soon as sindresorhus/execa#53 is merged and released I suggest using that instead of subcomandante to spawn and control the daemon. It's a much nicer and well maintained module.

JGAntunes · 2016-08-05T10:38:10Z

Hey folks, sorry for being kind of away towards the end of this week. @dignifiedquire will update the aegir module during this weekend and make the change to execa once your PR is merged 👍 I'll also try to give it a go on the documentation.

… version

…g test working

dignifiedquire · 2016-12-20T20:56:32Z

Thank you @JGAntunes for the great work here. We have updated go-ipfs-dep to include a lot of the things we learned from here and more, so we don't need to implement all the binary handling here :)
The internal improvements of using execa and your exec.js I have pulled in manually in 7b09529

Again thank you for all the work and keeping with us.

In terms of the actual interface changes I think we will want to go with the ideas here: ipfs/js-ipfs#556
as we are attempting to unify the interfaces between spawning a js and a go daemon.

daviddias · 2016-12-23T07:07:51Z

Merged #140, closing this one. @JGAntunes again thank you for helping out with these issues and my apologies for the huge lag from PR to actually getting something merged in, this module is highly sensible as it touches running processes at the OS level and so improvements have to happen slowly (execa vs subcomandante), as for the remaining features added, as @dignifiedquire noted, they were added to npm-go-ipfs-dep, which is now capable of picking the right dist through a package.json value.

Again, thank you <3

JGAntunes · 2016-12-27T14:01:32Z

Np folks, glad I could help out anyway 👍

daviddias reviewed Jul 13, 2016
View reviewed changes

dignifiedquire reviewed Jul 18, 2016
View reviewed changes

config.js

@@ -0,0 +1,16 @@

'use strict'

Copy link

Member

dignifiedquire Jul 18, 2016

Choose a reason for hiding this comment

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

All files should be under src

daviddias reacted with thumbs up emoji

dignifiedquire reviewed Jul 27, 2016
View reviewed changes

JGAntunes mentioned this pull request Aug 1, 2016

Fix issue around exit events jbenet/node-subcomandante#5

Open

daviddias mentioned this pull request Aug 4, 2016

release 0.4.x ipfs/npm-kubo#10

Closed

JGAntunes added 22 commits September 15, 2016 11:21

parse config async, bump version to 0.4.1

258fbcb

use class

cf54866

parse config sync

0de6a35

use a separate config file

c7cadba

updated npmignore and gitignore files with vendor folder

e6b4f77

use chai for test assertion

331deab

move config to src

67bee6a

tests to check bin state

a837058

fix shutdown logic

e738f0d

fix examples

e92eaf4

add more config related tests

97a305d

bump ipfs dist version & some minor changes

ce0ef0e

replaced subcomandante by execa, bumped aegir version & bumped bin rc…

88054c8

… version

Update README.md

3253762

improved exec source & get config parses response & new replace confi…

a4a3d2b

…g test working

solving linting issues on tests

0b1b625

remove default params

289d65b

bump aegir version

e95741c

updated classes to match new API & solved a couple of bugs

05fb3ef

bump aegir to 8.1.0

c03c3ed

refactor binary wrapper & added postinstall hook

fe852d4

typo on README

1214340

JGAntunes force-pushed the master branch from 6ecd09c to 1214340 Compare September 15, 2016 10:29

dignifiedquire mentioned this pull request Dec 20, 2016

feat: better stream handling #140

Merged

daviddias added the status/deferred Conscious decision to pause or backlog label Dec 22, 2016

daviddias closed this Dec 23, 2016

daviddias removed the status/deferred Conscious decision to pause or backlog label Dec 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New API + use bin-wrapper (bundle in all arch without any concern) + global install #89

New API + use bin-wrapper (bundle in all arch without any concern) + global install #89

JGAntunes commented Jul 13, 2016

JGAntunes commented Jul 13, 2016

daviddias Jul 13, 2016

JGAntunes Jul 13, 2016

dignifiedquire Aug 2, 2016

daviddias commented Jul 13, 2016

daviddias Jul 13, 2016

JGAntunes Jul 13, 2016

JGAntunes commented Jul 13, 2016

daviddias commented Jul 13, 2016

dignifiedquire Jul 18, 2016

JGAntunes commented Jul 18, 2016

JGAntunes commented Jul 21, 2016

daviddias commented Jul 27, 2016

JGAntunes commented Jul 27, 2016

dignifiedquire Jul 27, 2016 •

edited

Loading

JGAntunes Jul 28, 2016

dignifiedquire Jul 28, 2016

JGAntunes Jul 28, 2016

dignifiedquire Jul 28, 2016

daviddias commented Jul 27, 2016

dignifiedquire commented Jul 27, 2016

JGAntunes commented Jul 27, 2016

daviddias commented Jul 27, 2016

daviddias commented Jul 27, 2016

haadcode commented Jul 27, 2016

JGAntunes commented Aug 1, 2016

dignifiedquire commented Aug 4, 2016

dignifiedquire commented Aug 4, 2016

JGAntunes commented Aug 5, 2016

dignifiedquire commented Dec 20, 2016

daviddias commented Dec 23, 2016

JGAntunes commented Dec 27, 2016

New API + use bin-wrapper (bundle in all arch without any concern) + global install #89

New API + use bin-wrapper (bundle in all arch without any concern) + global install #89

Conversation

JGAntunes commented Jul 13, 2016

JGAntunes commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGAntunes commented Jul 13, 2016

daviddias commented Jul 13, 2016

Choose a reason for hiding this comment

JGAntunes commented Jul 18, 2016

JGAntunes commented Jul 21, 2016

daviddias commented Jul 27, 2016

JGAntunes commented Jul 27, 2016

dignifiedquire Jul 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Jul 27, 2016

dignifiedquire commented Jul 27, 2016

JGAntunes commented Jul 27, 2016

daviddias commented Jul 27, 2016

daviddias commented Jul 27, 2016

haadcode commented Jul 27, 2016

JGAntunes commented Aug 1, 2016

dignifiedquire commented Aug 4, 2016

dignifiedquire commented Aug 4, 2016

JGAntunes commented Aug 5, 2016

dignifiedquire commented Dec 20, 2016

daviddias commented Dec 23, 2016

JGAntunes commented Dec 27, 2016

dignifiedquire Jul 27, 2016 •

edited

Loading