Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong command output #8498

Closed
4 tasks
RiccardoM opened this issue Feb 3, 2021 · 25 comments · Fixed by #8628
Closed
4 tasks

Wrong command output #8498

RiccardoM opened this issue Feb 3, 2021 · 25 comments · Fixed by #8628
Labels
T:Bug T:tech debt Tech debt that should be cleaned up

Comments

@RiccardoM
Copy link
Contributor

RiccardoM commented Feb 3, 2021

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
@ethanfrey
Copy link
Contributor

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
Copy link
Contributor

alessio commented Feb 18, 2021

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
Copy link
Contributor

alessio commented Feb 18, 2021

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

@alexanderbez
Copy link
Contributor

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

@ethanfrey
Copy link
Contributor

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
Copy link
Contributor

alessio commented Feb 18, 2021

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

@ethanfrey
Copy link
Contributor

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
Copy link
Contributor

alessio commented Feb 18, 2021

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
Copy link

mdyring commented Feb 19, 2021

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)

@mergify mergify bot closed this as completed in #8628 Apr 17, 2021
@wilwade
Copy link

wilwade commented Oct 11, 2021

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
Copy link
Contributor

nooomski commented Jan 19, 2022

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 nooomski reopened this Jan 19, 2022
@ValarDragon
Copy link
Contributor

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

@alessio
Copy link
Contributor

alessio commented Jan 20, 2022

@amaury1093
Copy link
Contributor

Anyone here is already taking a look at this?

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

@likhita-809
Copy link
Contributor

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
Copy link
Contributor

likhita-809 commented Mar 16, 2022

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

@ValarDragon
Copy link
Contributor

ValarDragon commented Mar 16, 2022

@czarcas7ic would have a better idea than me.

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

@czarcas7ic
Copy link
Contributor

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
Copy link
Contributor

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

@likhita-809
Copy link
Contributor

likhita-809 commented Mar 17, 2022

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 amaury1093 moved this from Ready to In Progress in Cosmos SDK Maintenance Mar 17, 2022
@amaury1093
Copy link
Contributor

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.

Repository owner moved this from In Progress to Done in Cosmos SDK Maintenance Mar 25, 2022
@okwme
Copy link
Contributor

okwme commented Jun 20, 2022

re-opening here this issue isn't resolved.

cosmos/gaia#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
Copy link
Contributor

okwme commented Jun 20, 2022

@glnro was going to add one more

@okwme okwme reopened this Sep 16, 2022
@okwme
Copy link
Contributor

okwme commented Sep 16, 2022

cc @Pantani

@kocubinski
Copy link
Member

Not able to reproduce with the version command at dbacaa6:

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

@github-project-automation github-project-automation bot moved this from 📝 Todo to 👏 Done in Cosmos-SDK Jan 24, 2023
mergify bot pushed a commit that referenced this issue Mar 6, 2023
## Description

This PR introduces the `cmdtest` package, offering a lightweight wrapper around cobra commands to simplify testing CLI utilities.

I backfilled tests for the `version` command, which was an example of a very simple test setup; and for the `export` command, which was more involved due to the server and client context requirements.

I did notice that there are some existing tests for some utilities, but the `cmdtest` package follows a simple pattern that has been easy to use successfully in [the relayer](https://github.com/cosmos/relayer/blob/main/internal/relayertest/system.go) and in other projects outside the Cosmos ecosystem.

While filling in these tests, I started removing uses of `cmd.Print`, as that is the root cause of issues like #8498, #7964, #15167, and possibly others. Internal to cobra, the print family of methods write to `cmd.OutOrStderr()` -- meaning that if the authors call `cmd.SetOutput()` before executing the command, the output will be written to stdout as expected; otherwise it will go to stderr. I don't understand why that would be the default behavior, but it is probably too late to change from cobra's side.

Instead of `cmd.Print`, we prefer to `fmt.Fprint(cmd.OutOrStdout())` or `fmt.Fprint(cmd.ErrOrStderr())` as appropriate, giving an unambiguous destination for output. And the new tests collect those outputs in plain `bytes.Buffer` values so that we can assert their content appropriately.

In the longer term, I would like to deprecate and eventually remove the `testutil` package's `ApplyMockIO` method and its `BufferWriter` and `BufferReader` types, as they are unnecessary indirection when a simpler solution exists. But that can wait until `cmdtest` has propagated through the codebase more.

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] ~~provided a link to the relevant issue or specification~~
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed all author checklist items have been addressed
- [ ] confirmed that this PR does not change production code
@tac0turtle tac0turtle removed this from Cosmos-SDK May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:Bug T:tech debt Tech debt that should be cleaned up
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.