cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.22k stars 3.59k forks source link

Wrong command output #8498

Closed RiccardoM closed 1 year ago

RiccardoM commented 3 years ago

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

ethanfrey commented 3 years ago

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.

alessio commented 3 years ago

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 commented 3 years ago

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

alexanderbez commented 3 years ago

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

ethanfrey commented 3 years ago

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

alessio commented 3 years ago

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

ethanfrey commented 3 years ago

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.

alessio commented 3 years ago

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.

mdyring commented 3 years ago

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)

wilwade commented 2 years ago

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
nooomski commented 2 years ago

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.

ValarDragon commented 2 years ago

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

alessio commented 2 years ago

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

amaury1093 commented 2 years ago

Anyone here is already taking a look at this?

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

likhita-809 commented 2 years ago

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 commented 2 years ago

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

ValarDragon commented 2 years ago

@czarcas7ic would have a better idea than me.

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

czarcas7ic commented 2 years ago

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.

likhita-809 commented 2 years ago

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

likhita-809 commented 2 years ago

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

amaury1093 commented 2 years ago

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.

okwme commented 2 years ago

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:

okwme commented 2 years ago

@glnro was going to add one more

okwme commented 2 years ago

cc @pantani

kocubinski commented 1 year ago

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: %