fix: CLI should accept content from stdin (#474) #785

gpestana · 2017-03-11T22:39:06Z

Adds support for unix-like piping in js-ipfs.

e.g. echo "hello" | IPFS_PATH=~/.ipfs-go ipfs add is handled as expected

Adds support for unix-like piping in js-ipfs, like in go-ipfs cli. e.g. `echo "hello" | IPFS_PATH=~/.ipfs-go ipfs add` is handled as expected

gpestana · 2017-03-12T06:31:14Z

Failing in the module used to parse the piped arguments. Looking into it.

What do you guys think about the testing approach?

daviddias

Thanks for bringing this fix, it will still require some improvement, but you are in the right track :)

daviddias · 2017-03-12T17:23:39Z

src/cli/bin.js

@@ -2,6 +2,7 @@

 'use strict'

+require('pipe-args').load()


Cool module!

It is doing a bit more than what we want, though. If enabled, every command will take stdin->args, which opens space for other errors.

Yeah, but it's not only add that supports stdin. You can find a list of the commands that supports stdin and which argument it applies to here: https://github.com/ipfs/go-ipfs/search?utf8=%E2%9C%93&q=EnableStdin

We should probably have something similar for js-ipfs.

@diasdavid being /dev/stdin a symbolic link to /proc/self/fd/0 (and /proc/self a symlink to the running process), we want to grab whatever it is there and pass it as argument, right? The problem I see is echo "file.sx file.json" | ipfs add will not work since the input 'filesx file.json' will be passed as one string. Or I am missing something else?

there are commands like ipfs swarm peers that should not take input from stdin

@victorbjelkholm yep, the committed tests are just as proof of concept. Once the feature is polished, I will add for all the tests for commands that must support stdin redirection. The link is super helpful, thanks!!

gpestana · 2017-03-15T06:45:06Z

@diasdavid added the necessary changes for the pipe-args to copy the stdin to process.argv in selected commands. It could also be done explicitly in the cli.js without using the pipe-args opts object.

There was a fix in pipe-args module related to the Error: ENXIO, no such device or address '/dev/stdin' error that pop up in the previous tests (gpestana/pipe-args#2)

Let me know what you think so I can go ahead and add more tests or change what's needed :)

daviddias · 2017-03-29T05:42:17Z

src/cli/bin.js

+const enableStdin = [
+  'files', 'path', 'object data', 'ref', 'domain-name', 'key', 'ipfs-path',
+  'name', 'address', 'data', 'peer', 'recursive', 'default-config', 'peer ID'
+]


what is domain name?

https://github.com/ipfs/go-ipfs/blob/2cc5ce45b875e45fdbfc779e1a7c4bef99bb9877/core/commands/dns.go#L46

It should be renamed to 'dns'

add, get and cat should also be here temporary as we have them as aliases.

js-ipfs/src/cli/bin.js

Lines 26 to 35 in ebaf9a0

// NOTE: This creates an alias of

// `jsipfs files {add, get, cat}` to `jsipfs {add, get, cat}`.

// This will stay until https://github.com/ipfs/specs/issues/98 is resolved.

const addCmd = require('./commands/files/add')

const catCmd = require('./commands/files/cat')

const getCmd = require('./commands/files/get')

const aliases = [addCmd, catCmd, getCmd]

aliases.forEach((alias) => {

cli.command(alias.command, alias.describe, alias.builder, alias.handler)

})

daviddias

needs rebase from master
needs a test for jsipfs block put

gpestana · 2017-04-06T10:54:09Z

Next coming up on this PR:

victorb · 2017-04-06T12:59:14Z

@gpestana no need for "domain-name -> dns" since js-ipfs does not support dnslink resolution yet.

daviddias · 2017-07-07T15:36:36Z

@gpestana mind rebasing master into your branch?

@victorbjelkholm could you provide the final review and recommendation to get this merged?

Thank you both :)

gpestana · 2017-07-07T20:58:57Z

@diasdavid Will do, thanks for reminding me :)

@victorbjelkholm, let's get this done! In meanwhile, if you come up with anything besides rebase, jsipfs block put test and improve test coverage let me know.

victorb · 2017-07-10T11:21:54Z

src/cli/bin.js

+
+pipe.load({ commands: enableStdin })
+
+const yargs = require('yargs')


Just put this at the top so all the dependencies can be seen easily

victorb · 2017-07-10T11:25:22Z

test/cli/files.js

@@ -127,6 +127,23 @@ describe('files', () => runOnAndOff((thing) => {
      })
  })

+  it('add with piped argument', () => {
+    // echo 'src/init-files/init-docs/readme' | jsipfs files add
+    return ipfs('files add', { piped: 'src/init-files/init-docs/readme' })


I'm unable to actually test this due to conflicts, if those could be resolved, would be 👍

Also, judging by this test, the output of echo 'src/init-files/init-docs/readme' | jsipfs files add should place src/init-files/init-docs/readme as the content, without any filename, but the assertion seems to say that it just added a file called readme which is incorrect.

Conflicts are resolved now! And good point, gonna take a look at that

Yeah, just got a chance to try it again and this should work:

head -c 10 /dev/urandom | base64 | node src/cli/bin.js files add

but instead is giving me an error:

/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/node_modules/yargs/yargs.js:1079 else throw err ^ Error: ENOENT: no such file or directory, stat 'Ksw1ReYL8xR2VA==' at Error (native) at Object.fs.statSync (fs.js:987:18) at checkPath (/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/src/cli/commands/files/add.js:32:10) at Object.handler (/Users/user/projects/ipfs/js-monorepo/packages/js-ipfs/src/cli/commands/files/add.js:72:20)

Because it's trying to find a file with that name, rather than actually using the data from stdin.

If I understood it correctly, it seems that it might be an issue with the CLI itself. The piped arguments work exactly the same way as if the arguments were passed directly:

node src/cli/bin.js files add $(head -c 10 /dev/urandom | base64)

/Users/home/dev/js-ipfs/node_modules/yargs/yargs.js:1079 else throw err ^ Error: ENOENT: no such file or directory, stat 're+g0ZVkYUUsKg==' at Object.fs.statSync (fs.js:967:11) at checkPath (/Users/home/dev/js-ipfs/src/cli/commands/files/add.js:32:10) at Object.handler (/Users/home/dev/js-ipfs/src/cli/commands/files/add.js:72:20) at Object.self.runCommand (/Users/home/dev/js-ipfs/node_modules/yargs/lib/command.js:233:22) at Object.Yargs.self._parseArgs (/Users/home/dev/js-ipfs/node_modules/yargs/yargs.js:990:30) at Object.self.runCommand (/Users/home/dev/js-ipfs/node_modules/yargs/lib/command.js:204:45) at Object.Yargs.self._parseArgs (/Users/home/dev/js-ipfs/node_modules/yargs/yargs.js:990:30) at Object.Yargs.self.parse (/Users/home/dev/js-ipfs/node_modules/yargs/yargs.js:532:23) at utils.getIPFS (/Users/home/dev/js-ipfs/src/cli/bin.js:67:6) at IPFS.node.once (/Users/home/dev/js-ipfs/src/cli/utils.js:64:5)

Or am I missing something?

No, using stdin and passing a argument should not work the same way. Passing a argument should use that argument as a path to a file. Using stdin should use whatever comes from stdin as the content to add, without any filename.

Compare it to how go-ipfs works, and maybe it'll be a bit easier to understand.

daviddias · 2017-07-10T12:28:37Z

@gpestana do you think we can get this done before #910 (say this week)

gpestana · 2017-07-10T20:33:59Z

@diasdavid yep, I think it's possible. Let's aim at getting this merged by end of the week.

victorb · 2017-07-10T21:36:15Z

src/cli/bin.js

+const enableStdin = [
+  'files', 'path', 'object data', 'ref', 'domain-name', 'key', 'ipfs-path',
+  'name', 'address', 'data', 'peer', 'recursive', 'default-config', 'peer ID'
+]


add, get and cat should also be here temporary as we have them as aliases.

js-ipfs/src/cli/bin.js

Lines 26 to 35 in ebaf9a0

// NOTE: This creates an alias of

// `jsipfs files {add, get, cat}` to `jsipfs {add, get, cat}`.

// This will stay until https://github.com/ipfs/specs/issues/98 is resolved.

const addCmd = require('./commands/files/add')

const catCmd = require('./commands/files/cat')

const getCmd = require('./commands/files/get')

const aliases = [addCmd, catCmd, getCmd]

aliases.forEach((alias) => {

cli.command(alias.command, alias.describe, alias.builder, alias.handler)

})

… it as a file path

gpestana · 2017-07-11T22:25:10Z

@victorbjelkholm, the new commit should fix what you pointed out in the last comment: in the files add command with parameters coming from the stdin, the input is added directly (not pointing at a file). My suggestion was to create a separate pull stream for when the arguments are passed from the stdin.

Not sure if this would be possible to fork the flow in the pull stream itself, but I'm not that experienced with it so I didn't figure out other way. Any other suggestion?

There is still a problem though. When running the tests, I get the following error:

  1) cli files daemon on (through http-api) add with piped argument:
     Command failed: /Users/home/dev/js-ipfs/src/cli/bin.js files add
events.js:163
      throw er; // Unhandled 'error' event
      ^

TypeError: Invalid non-string/buffer chunk
    at validChunk (_stream_writable.js:209:10)
    at PassThrough.Writable.write (_stream_writable.js:239:21)
    at PassThrough.Writable.end (_stream_writable.js:475:10)
    at Multipart.addPart (/Users/home/dev/js-ipfs/node_modules/multipart-stream/index.js:59:14)
    at flatmap.forEach (/Users/home/dev/js-ipfs/node_modules/ipfs-api/src/get-files-stream.js:131:8)
    at Array.forEach (native)
    at getFilesStream (/Users/home/dev/js-ipfs/node_modules/ipfs-api/src/get-files-stream.js:130:6)
    at requestAPI (/Users/home/dev/js-ipfs/node_modules/ipfs-api/src/request-api.js:94:14)
    at send (/Users/home/dev/js-ipfs/node_modules/ipfs-api/src/request-api.js:188:12)
    at Function.send.andTransform (/Users/home/dev/js-ipfs/node_modules/ipfs-api/src/request-api.js:196:12)

This happens only with the cli files daemon on (through http-api) but the test set when the daemon is off works as expected. Does this case (using stdin to pass parameters) make sense with the daemon on? Either ways, I will keep investigating this issue.

daviddias

Hi @gpestana, just went through your code and a couple of things are not making sense to me.

Essentially, this feature should be in place by doing something:

if (hasStdin) {
  ipfs.files.add({path:'', content: process.stdin}, (err, files)
} else {
 // do the normal stuff
}

daviddias · 2017-07-12T20:21:08Z

src/cli/commands/files/add.js

-      }
+
+        addDataPipeline([dataStream], addStream)
+      })


Something doesn't look right here:

createAddStream is going to glob a dir and return the stream which has all the files resulting from that glob.

If you want to add one stream, you should not need to call that.

I see. So the right solution would be to do something like this?

if (hasStdin) { ipfs.files.add({path:'', content: process.stdin}, (err, files) } else { // as it is now }

Yeah, see: #785 (review)

daviddias · 2017-07-12T20:21:29Z

src/cli/commands/files/add.js

+    if(hasPipedArgs) {
+      const data = argv.file
+      const dataStream = new stream.Readable()
+      dataStream.push(Buffer.from(data, 'utf8'))


It might not be utf8, that option out, it doesn't really matter.

Yep, it can use the default. I will remove that option parameter.

daviddias · 2017-07-12T20:22:00Z

src/cli/commands/files/add.js

+    // if there are piped arguments, input is data to publish instead of a file
+    // path
+    if(hasPipedArgs) {
+      const data = argv.file


If I do cat someFile.txt | ipfs add does pipe-args buffer the whole file first? Well, then it is missing the point of pipes, right?

Yes, the whole file is going to be buffered before the command is triggered. I assumed that since this is for a CLI interface, it would make sense to do so. So it's better to buffer the file using streams and make it async?

daviddias · 2017-09-01T08:32:11Z

PR now needs rebase from master

daviddias · 2018-01-24T00:58:30Z

@hacdias would you like to get this PR merged?

victorb · 2018-02-19T16:40:34Z

src/cli/bin.js

 const yargs = require('yargs')
 const updateNotifier = require('update-notifier')
 const readPkgUp = require('read-pkg-up')
 const utils = require('./utils')

+const enableStdin = [
+  'data', 'path', 'object data', 'ref', 'key', 'ipfs-path', 'add', 'get', 'cat',
+  'name', 'address', 'files', 'peer', 'recursive', 'default-config', 'peer ID'


Seems to be missing a few commands. config and pubsub are two I remember from the top of my head, but I guess there is more.

Here is a Github search that shows which go-ipfs commands accepts stdin: https://github.com/ipfs/go-ipfs/search?utf8=%E2%9C%93&q=EnableStdin&type=

daviddias · 2018-03-16T22:05:59Z

@hacdias can you land this one?

gpestana · 2018-08-13T09:39:37Z

@diasdavid old and most likely not the best approach. closing this PR

fix: CLI should accept content from stdin ipfs#474)

242b165

Adds support for unix-like piping in js-ipfs, like in go-ipfs cli. e.g. `echo "hello" | IPFS_PATH=~/.ipfs-go ipfs add` is handled as expected

gpestana changed the title ~~fix: CLI should accept content from stdin #474)~~ fix: CLI should accept content from stdin (#474) Mar 11, 2017

daviddias suggested changes Mar 12, 2017

View reviewed changes

Enables stdin->process.argv only on selected commands

ed3f719

daviddias added the status/deferred Conscious decision to pause or backlog label Mar 15, 2017

daviddias reviewed Mar 29, 2017

View reviewed changes

daviddias suggested changes Mar 29, 2017

View reviewed changes

daviddias requested a review from dignifiedquire March 29, 2017 05:44

daviddias added status/in-progress In progress and removed status/deferred Conscious decision to pause or backlog labels Mar 29, 2017

daviddias added status/ready Ready to be worked and removed status/in-progress In progress labels Apr 5, 2017

daviddias mentioned this pull request Jul 7, 2017

CLI should accept content from stdin #474

Closed

daviddias requested a review from victorb July 7, 2017 15:36

victorb reviewed Jul 10, 2017

View reviewed changes

victorb suggested changes Jul 10, 2017

View reviewed changes

gpestana force-pushed the cli_pipe_474 branch 2 times, most recently from a0220ce to ed3f719 Compare July 10, 2017 20:10

Merge branch 'master' into cli_pipe_474

bd8a691

victorb suggested changes Jul 10, 2017

View reviewed changes

fix(stdin): Uses input from stdin as content to add, instead of using…

784046c

… it as a file path

daviddias suggested changes Jul 12, 2017

View reviewed changes

daviddias added P3 Low: Not priority right now exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue labels Oct 18, 2017

daviddias mentioned this pull request Feb 17, 2018

feat: add keysize through an option to spawn ipfs/js-ipfsd-ctl#203

Merged

victorb reviewed Feb 19, 2018

View reviewed changes

hugomrdias self-assigned this Apr 12, 2018

hugomrdias removed their assignment May 2, 2018

gpestana closed this Aug 13, 2018

ghost removed the status/ready Ready to be worked label Aug 13, 2018

	// NOTE: This creates an alias of
	// `jsipfs files {add, get, cat}` to `jsipfs {add, get, cat}`.
	// This will stay until https://github.com/ipfs/specs/issues/98 is resolved.
	const addCmd = require('./commands/files/add')
	const catCmd = require('./commands/files/cat')
	const getCmd = require('./commands/files/get')
	const aliases = [addCmd, catCmd, getCmd]
	aliases.forEach((alias) => {
	cli.command(alias.command, alias.describe, alias.builder, alias.handler)
	})


		pipe.load({ commands: enableStdin })

		const yargs = require('yargs')

fix: CLI should accept content from stdin (#474) #785

fix: CLI should accept content from stdin (#474) #785

Conversation

gpestana commented Mar 11, 2017

gpestana commented Mar 12, 2017

daviddias left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpestana commented Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias left a comment • edited Loading

Choose a reason for hiding this comment

gpestana commented Apr 6, 2017 • edited by fsdiogo Loading

victorb commented Apr 6, 2017

daviddias commented Jul 7, 2017

gpestana commented Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpestana Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpestana Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

victorb Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Jul 10, 2017

gpestana commented Jul 10, 2017

Choose a reason for hiding this comment

gpestana commented Jul 11, 2017 • edited Loading

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpestana Jul 14, 2017 • 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

Choose a reason for hiding this comment

daviddias commented Sep 1, 2017

daviddias commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Mar 16, 2018

gpestana commented Aug 13, 2018

gpestana commented Mar 15, 2017 •

edited

Loading

daviddias left a comment •

edited

Loading

gpestana commented Apr 6, 2017 •

edited by fsdiogo

Loading

gpestana commented Jul 7, 2017 •

edited

Loading

gpestana Jul 10, 2017 •

edited

Loading

gpestana Jul 11, 2017 •

edited

Loading

victorb Jul 11, 2017 •

edited

Loading

gpestana commented Jul 11, 2017 •

edited

Loading

gpestana Jul 14, 2017 •

edited

Loading