cosmos-sdk icon indicating copy to clipboard operation
cosmos-sdk copied to clipboard

Wrong command output

Open RiccardoM opened this issue 4 years ago • 24 comments

Summary of Bug

Most of the commands print their outputs to stderr instead of stdout, making it harder to pipe with other utilities (such as jq).

Version

v0.40.1

Steps to Reproduce

  1. Run almost any command (eg. simd status)
  2. Try piping that command with jq
simd status | jq '.'

This will return the default output of the command, instead of the output of jq. A workaround is to redirect stderr to stdout for while running the command:

simd status 2>&1 | jq '.'

Possible solution

Taking a look at the commands code I noticed that, although many commands use the cmd.Print methods, only few correctly set their output using cmd.SetOut. This causes the cmd.Print to print the output using stderr, as per documentation:

// Println is a convenience method to Println to the defined output, fallback to Stderr if not set.

I think that a possible solution could be to add the following lines to the app.go's initRootCmd function:

rootCmd.PersistentPreRun = func(cmd *cobra.Command, args []string) {
  cmd.SetOut(cmd.OutOrStdout())
}

This would make sure that by default all commands are set to have their output to stdout, with the ability to override it later.


For Admin Use

  • [ ] Not duplicate issue
  • [ ] Appropriate labels applied
  • [ ] Appropriate contributors tagged
  • [ ] Contributor assigned/self-assigned

RiccardoM avatar Feb 03 '21 08:02 RiccardoM

I agree totally with this one. I keep doing some-command > foo.json 2>&1 or some-command 2>&1 | grep "Committed" as well.

Nice UNIX tricks, but not ergonomic at all. I feel I brought this up in another issue, but maybe it was just some rant on discord.

ethanfrey avatar Feb 17 '21 22:02 ethanfrey

I know @ethanfrey, yet POSIX tells us to print the least number of messages. Diagnostics and errors should always go to stderr. Only when a command's main purpose is to produce an output, then such output should go to stdout.

I'd propose a different approach here: why don't we audit all commands and find which have streams wrongly redirected to stderr by default?

alessio avatar Feb 18 '21 13:02 alessio

@RiccardoM simd status should print to stdout BTW, I agree.

alessio avatar Feb 18 '21 13:02 alessio

Looks like we need to do some grooming here. Ensure all query-based commands output to STDOUT.

alexanderbez avatar Feb 18 '21 13:02 alexanderbez

Is there any way to redirect logs to a file?

We have waaaayyyyy too verbose log output and it dumps on stderr rather than a file. Most processes that log like crazy are meant to dump to a file rather than showing users

ethanfrey avatar Feb 18 '21 14:02 ethanfrey

I think a flag could be introduced, e.g. --errlog=myfile, but how is that better than 2>myfile?

alessio avatar Feb 18 '21 14:02 alessio

I think a flag could be introduced, e.g. --errlog=myfile, but how is that better than 2>myfile?

I wonder about the name. If it was only error log and not tendermint debug spam, I would not argue about pumping to stderr.

If config files were supported (why was that removed?) we could just set the logfile in the config file and not worry about redirects on cli. Which is more or less how most long-living daemons I know work. Ideally, there could be 2 log files - one for real errors, one for the logs.

No one complains about real errors going to stdout. Just mixing these logs everywhere hurts.

ethanfrey avatar Feb 18 '21 15:02 ethanfrey

If config files were supported (why was that removed?)

I hear you, loud and clear. We should prioritize config file-driven CLI customization. IMHO it's more urgent than many other things.

alessio avatar Feb 18 '21 15:02 alessio

Just chiming in regarding logging to file. Quick workaround for systemd users, add the below to the .service file:

StandardOutput=file:${INSTALL_PREFIX}/log/output.log
StandardError=file:${INSTALL_PREFIX}/log/error.log

(and yes, everything is currently sent to stderr per this issue)

mdyring avatar Feb 19 '21 08:02 mdyring

Did this actually get fixed?

$ starport chain build
Cosmos SDK's version is: stargate - v0.44.1

🛠️  Building proto...
📦 Installing dependencies...
🛠️  Building the blockchain...
🗃  Installed. Use with: batchd
$ batchd status 2> err.test > out.test
$ wc *.test
       1       1    1042 err.test // Contains Expected Output
       0       0       0 out.test
       1       1    1042 total

wilwade avatar Oct 11 '21 17:10 wilwade

This issue still isn't resolved as far as I'm aware? We've been getting word on the Hub that people are still getting command output to stderr.

nooomski avatar Jan 19 '22 11:01 nooomski

A bunch of commands outputs got unexpectedly switched to stderr in the SDK v0.44 upgrade

ValarDragon avatar Jan 20 '22 02:01 ValarDragon

https://github.com/cosmos/gaia/issues/1073#issuecomment-1016484018

alessio avatar Jan 20 '22 10:01 alessio

Anyone here is already taking a look at this?

If not, we can assign someone on Regen on this.

amaury1093 avatar Jan 20 '22 10:01 amaury1093

A bunch of commands outputs got unexpectedly switched to stderr in the SDK v0.44 upgrade

@ValarDragon do you have any idea on which commands outputs got switched to stderr ?

likhita-809 avatar Mar 16 '22 07:03 likhita-809

I tested a few CLI subcommands with JSON outputs and piping them to jq, they all work fine on master.

likhita-809 avatar Mar 16 '22 09:03 likhita-809

@czarcas7ic would have a better idea than me.

The big one that we had to work around was state exports

ValarDragon avatar Mar 16 '22 19:03 ValarDragon

Currently the only one I know off the top of my head is when we do state exports like so:

osmosisd export 2> testnet_genesis.json

In the osmosis installer, I muted both stdout and stderr during all CLI subcommands since I wasn't sure which were mixed up. Soon I'm going to go back through and enable all stderrs and will report back here which ones output stdout instead.

czarcas7ic avatar Mar 16 '22 21:03 czarcas7ic

@amaurym do you think we should wait until @czarcas7ic reports back here ?

likhita-809 avatar Mar 17 '22 09:03 likhita-809

I've tried testing this with export, status, query account, tx sign, query tx, authz tx cmds and these all are working fine on master and v0.45

likhita-809 avatar Mar 17 '22 10:03 likhita-809

Proposing to close this, since it seems it works in v0.45 and master.

If there's a strong will to backport this to v0.44, let us know here.

amaury1093 avatar Mar 25 '22 09:03 amaury1093

re-opening here this issue isn't resolved.

https://github.com/cosmos/gaia/issues/1542

I think we should do @alessio 's suggestion of:

audit all commands and find which have streams wrongly redirected to stderr by default?

The ones collected in the gaia issue are as follows:

  • gaiad version
  • gaiad export
  • gaiad keys parse
  • gaiad q tendermint-validator-set
  • gaiad keys multisign

okwme avatar Jun 20 '22 15:06 okwme

@glnro was going to add one more

okwme avatar Jun 20 '22 15:06 okwme

cc @pantani

okwme avatar Sep 16 '22 13:09 okwme

Not able to reproduce with the version command at dbacaa67032875235413e28902e6aae9e82370f1:

$ ./build/simd version > out 2> error && echo -n "out: " && cat out && echo -n "error: " && cat error
out: 0.46.0-beta2-1363-gdbacaa670
error: % 

kocubinski avatar Jan 23 '23 22:01 kocubinski