Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: improved error handling on the CLI #1335

Closed
wants to merge 10 commits into from
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
test/repo-tests*
**/bundle.js
docs
.vscode
.eslintrc
# Logs
logs
*.log
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@
"ipfs-block": "~0.7.1",
"ipfs-block-service": "~0.14.0",
"ipfs-multipart": "~0.1.0",
"ipfs-repo": "0.20.0",
"ipfs-repo": "~0.21.0",
"ipfs-unixfs": "~0.1.14",
"ipfs-unixfs-engine": "~0.29.0",
"ipld": "~0.17.0",
Expand Down
181 changes: 119 additions & 62 deletions src/cli/bin.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,104 +2,161 @@

'use strict'

const yargs = require('yargs')
const updateNotifier = require('update-notifier')
const readPkgUp = require('read-pkg-up')
const fs = require('fs')
const path = require('path')
const utils = require('./utils')
const print = utils.print
const yargs = require('yargs/yargs')
const updateNotifier = require('update-notifier')
const readPkgUp = require('read-pkg-up')
const { disablePrinting, print, getNodeOrAPI } = require('./utils')
const addCmd = require('./commands/files/add')
const catCmd = require('./commands/files/cat')
const getCmd = require('./commands/files/get')
Copy link
Member

Choose a reason for hiding this comment

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

It's faster if these 3 are only required when needed

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed with last commit


const pkg = readPkgUp.sync({cwd: __dirname}).pkg

updateNotifier({
pkg,
updateCheckInterval: 1000 * 60 * 60 * 24 * 7 // 1 week
}).notify()

const args = process.argv.slice(2)
const MSG_USAGE = `Usage:
ipfs - Global p2p merkle-dag filesystem.

ipfs [options] <command> ...`
const MSG_EPILOGUE = `Use 'ipfs <command> --help' to learn more about each command.
Copy link
Member

Choose a reason for hiding this comment

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

Minor - too much indentation!

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry where? 26 has no identation. It's exactly the same as the go impl.

Copy link
Member Author

Choose a reason for hiding this comment

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

i see now, its const eslint auto fixes that wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed with last commit


ipfs uses a repository in the local file system. By default, the repo is
located at ~/.ipfs. To change the repo location, set the $IPFS_PATH
environment variable:

export IPFS_PATH=/path/to/ipfsrepo

// Determine if the first argument is a sub-system command
EXIT STATUS

The CLI will exit with one of the following values:

0 Successful execution.
1 Failed executions.
`
const MSG_NO_CMD = 'You need at least one command before moving on'

const argv = process.argv.slice(2)
const commandNames = fs.readdirSync(path.join(__dirname, 'commands'))
const isCommand = commandNames.includes(`${args[0]}.js`)
const isCommand = commandNames.includes(`${argv[0]}.js`)

const cli = yargs
let args = {}
let cli = yargs(argv)
.usage(MSG_USAGE)
.option('silent', {
desc: 'Write no output',
type: 'boolean',
default: false,
coerce: ('silent', silent => silent ? utils.disablePrinting() : silent)
coerce: disablePrinting
})
.option('debug', {
desc: 'Show debug output',
type: 'boolean',
default: false,
alias: 'D'
})
.option('pass', {
desc: 'Pass phrase for the keys',
type: 'string',
default: ''
})
.option('api', {
desc: 'Use a specific API instance.',
type: 'string'
})
.commandDir('commands', {
// Only include the commands for the sub-system we're using, or include all
// if no sub-system command has been passed.
include (path, filename) {
if (!isCommand) return true
return `${args[0]}.js` === filename
return `${argv[0]}.js` === filename
}
})
.epilog(utils.ipfsPathHelp)
.demandCommand(1)
.fail((msg, err, yargs) => {
if (err) {
throw err // preserve stack

if(!isCommand){
cli
// 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.
.command(addCmd)
.command(catCmd)
.command(getCmd)
}
cli
.demandCommand(1, MSG_NO_CMD)
.alias('help', 'h')
.epilogue(MSG_EPILOGUE)
.strict()
// .recommendCommands()
.completion()

if (['daemon', 'init', 'id', 'version'].includes(argv[0])) {
args = cli.fail((msg, err, yargs) => {
if (err instanceof Error && err.message && !msg) {
msg = err.message
}

if (args.length > 0) {
print(msg)
// Cli specific error messages
if (err && err.code === 'ERR_REPO_NOT_INITIALIZED') {
msg = `No IPFS repo found in ${err.path}.
please run: 'ipfs init'`
}

yargs.showHelp()
})
// Show help and error message
if (!args.silent) {
yargs.showHelp()
console.error('Error: ' + msg)
}

// If not a sub-system command then load the top level aliases
if (!isCommand) {
// 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)
})
}
// Write to stderr when debug is on
if (err && args.debug) {
console.error(err)
}

// Need to skip to avoid locking as these commands
// don't require a daemon
if (args[0] === 'daemon' || args[0] === 'init') {
cli
.help()
.strict()
.completion()
.parse(args)
process.exit(1)
}).argv
} else {
// here we have to make a separate yargs instance with
// only the `api` option because we need this before doing
// the final yargs parse where the command handler is invoked..
yargs().option('api').parse(process.argv, (err, argv, output) => {
if (err) {
throw err
}
utils.getIPFS(argv, (err, ipfs, cleanup) => {
if (err) { throw err }

cli
.help()
.strict()
.completion()
.parse(args, { ipfs: ipfs }, (err, argv, output) => {
if (output) { print(output) }

cleanup(() => {
if (err) { throw err }
yargs()
.option('pass', {
desc: 'Pass phrase for the keys',
type: 'string',
default: ''
})
.option('api', {
desc: 'Use a specific API instance.',
type: 'string'
})
.parse(argv, (err, parsedArgv, output) => {
if (err) {
console.error(err)
} else {
getNodeOrAPI(parsedArgv)
.then(node => {
args = cli
.parse(argv, { ipfs: node }, (err, parsedArgv, output) => {
if (output) {
print(output)
}
if (node && node._repo && !node._repo.closed) {
node._repo.close(err => {
if (err) {
console.error(err)
}
})
}
if (err && parsedArgv.debug) {
console.error(err)
}
})
})
.catch(err => {
console.error(err)
process.exit(1)
})
})
}
})
})
}
22 changes: 5 additions & 17 deletions src/cli/commands/daemon.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@

const HttpAPI = require('../../http')
const utils = require('../utils')
const promisify = require('promisify-es6')
const print = utils.print

let httpAPI

module.exports = {
command: 'daemon',

Expand All @@ -31,21 +30,8 @@ module.exports = {
handler (argv) {
print('Initializing daemon...')

const repoPath = utils.getRepoPath()
httpAPI = new HttpAPI(process.env.IPFS_PATH, null, argv)

httpAPI.start((err) => {
if (err && err.code === 'ENOENT' && err.message.match(/Uninitalized repo/i)) {
print('Error: no initialized ipfs repo found in ' + repoPath)
print('please run: jsipfs init')
process.exit(1)
}
if (err) {
throw err
}
print('Daemon is ready')
})

const httpAPI = new HttpAPI(process.env.IPFS_PATH, null, argv)
const start = promisify(httpAPI.start)
const cleanup = () => {
print(`Received interrupt signal, shutting down..`)
httpAPI.stop((err) => {
Expand All @@ -60,5 +46,7 @@ module.exports = {
process.on('SIGTERM', cleanup)
process.on('SIGINT', cleanup)
process.on('SIGHUP', cleanup)

return start().then(() => print('Daemon is ready'))
}
}
16 changes: 8 additions & 8 deletions src/cli/commands/id.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict'
const print = require('../utils').print

const {print, getNodeOrAPI} = require('../utils')

module.exports = {
command: 'id',
Expand All @@ -15,12 +16,11 @@ module.exports = {

handler (argv) {
// TODO: handle argv.format
argv.ipfs.id((err, id) => {
if (err) {
throw err
}

print(JSON.stringify(id, '', 2))
})
return getNodeOrAPI(argv)
.then(node => Promise.all([Promise.resolve(node), node.id()]))
.then(([node, id]) => {
print(JSON.stringify(id, '', 2))
node.stop()
Copy link
Member

Choose a reason for hiding this comment

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

How come stop() is called here but not in version.js?

Do all our commands have to remember to call stop() after they're done, and also catch any error and call stop() to clean up correctly? Would it be better to have IPFS created and destroyed once in bin.js like it is at the moment? Is there another benefit to moving the IPFS node getter into each command?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this command is requiring a online node? Shouldn't this just work and return the ID without having to start a node? Feels wildly inefficient.

Looking at what go-ipfs does: when you call ID with a online daemon, it returns the ID object but with swarm address. When offline, it returns the same but with addresses empty.

In contrast, what this command seems to be doing is to either grab the API if daemon is running but if none is running, start a node.

Copy link
Member

Choose a reason for hiding this comment

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

That too, but I think that should be fixed outside of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

The more i look, more code paths i see, events, cb(err), fsm, etc. This is tricky. I'm gonna push couple commits to improve further on this.
But still to answer your questions:

How come stop() is called here but not in version.js?

you r right every cmd needs to do cleanup (easier cleanup coming in the next commits)

Do all our commands have to remember to call stop() after they're done, and also catch any error and call stop() to clean up correctly? Would it be better to have IPFS created and destroyed once in bin.js like it is at the moment? Is there another benefit to moving the IPFS node getter into each command?

I think being explicit and each cmd control their own lifecycle is better. Also we could remove all this code https://github.com/ipfs/js-ipfs/blob/master/src/cli/bin.js#L73-L104, which got even bigger in this PR after adding more safe guards and error handling. In the end we would only have one code path ending in fail() and proper stack traces, as we do now for daemon, init, id and version.

I'm not sure why this command is requiring a online node? Shouldn't this just work and return the ID without having to start a node? Feels wildly inefficient.
Looking at what go-ipfs does: when you call ID with a online daemon, it returns the ID object but with swarm address. When offline, it returns the same but with addresses empty.
In contrast, what this command seems to be doing is to either grab the API if daemon is running but if none is running, start a node.

It works exactly like that now

{
  "id": "QmYHif2UK4aFaMYBiNZYrA7xLvYJajYSQt14H5tmHyagvV",
  "publicKey": "CAASpgIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDBJRc5+YxkfPX1LfwUXGY18ezEUpsUmWLAQxReOdeedbEHly8O7WYfYW1iDV1obVttDAbsYLMLh2gmoPqyMjvcAtA/JFnkCjTJ1GCAJFPvrHly5ZFO67knMTHXfVTaKJRBs98JnwrVgFRAbZAwoeVBRbPeP53FOmiw3pXoraOXU7YvGV3vc29PUTn4+1g4seoznyHAx5Ezp52MspiBvb0QmkEGvqZEKFRBUSz8pqMPCDYtItXMzOoBJG29TxxUztGTHvfogdxY9EOpPxVW6Nm0YRWbs9Vc8m/Vjtb88VQGrXXiplgag8OzyT8qM7Axx44nqOkAnQM3L50lOJ/ezGr9AgMBAAE=",
  "addresses": [],
  "agentVersion": "js-ipfs/0.28.2",
  "protocolVersion": "9000"
}

})
}
}
32 changes: 12 additions & 20 deletions src/cli/commands/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@

const Repo = require('ipfs-repo')
const IPFS = require('../../core')
const utils = require('../utils')
const print = utils.print
const { ipfsPathHelp, getRepoPath, print } = require('../utils')

module.exports = {
command: 'init',
Expand All @@ -12,7 +11,7 @@ module.exports = {

builder (yargs) {
return yargs
.epilog(utils.ipfsPathHelp)
.epilog(ipfsPathHelp)
.option('bits', {
type: 'number',
alias: 'b',
Expand All @@ -22,33 +21,26 @@ module.exports = {
.option('emptyRepo', {
alias: 'e',
type: 'boolean',
describe: "Don't add and pin help files to the local storage"
describe: 'Don\'t add and pin help files to the local storage'
})
},

handler (argv) {
const path = utils.getRepoPath()
const path = getRepoPath()

print(`initializing ipfs node at ${path}`)

const node = new IPFS({
return IPFS.createNodePromise({
repo: new Repo(path),
init: false,
start: false
})
Copy link
Member

Choose a reason for hiding this comment

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

Does createNodePromise resolve when start: false?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, ready is always emitted if an error doesn't happen
https://github.com/ipfs/js-ipfs/blob/feat/better-errors-cli/src/core/boot.js#L53


node.init({
bits: argv.bits,
emptyRepo: argv.emptyRepo,
pass: argv.pass,
log: print
}, (err) => {
if (err) {
if (err.code === 'EACCES') {
err.message = `EACCES: permission denied, stat $IPFS_PATH/version`
}
throw err
}
}).then(node => {
return node.init({
bits: argv.bits,
emptyRepo: argv.emptyRepo,
pass: argv.pass,
log: print
})
})
}
}
Loading