Use stderr for Cabal's diagnostic messages instead of stdout #4789

Rufflewind · 2017-09-23T01:03:58Z

Please include the following checklist in your PR:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary.
If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

Manually tested on a minimal Cabal project. cabal build 2>/dev/null shows nothing.

mention-bot · 2017-09-23T01:04:02Z

@Rufflewind, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts, @ezyang and @phadej to be potential reviewers.

Cabal/Cabal.cabal

hvr · 2017-09-23T07:11:14Z

Does this only affect new-run or does this mean that effectively all output of say cabal new-build will suddenly go to stderr? Or put differently, which output will go to stdout at all after this patch? To be honest, I'm not convinced everything should go to stderr, as this doesn't match my intuition from other buildtools (most notably make) which use stdout for info/progress messages, while using stderr to denote critical errors and exceptions.

Rufflewind · 2017-09-23T09:16:30Z

Does this only affect new-run or does this mean that effectively all output of say cabal new-build will suddenly go to stderr?

It should affect everything, since I rewired all the debug/notice/info functions per @23Skidoo's suggestion.

I personally only care about #4652 in relation to run, test, and bench, but this happened to be the most straightforward implementation.

Not really sure what could be done about the need for process-1.2.1.0, short of falling back to using stdout if process-1.2.1.0 is unavailable.

mgsloan · 2017-09-24T01:22:20Z

Stack switched to sending most output to stderr pretty early on, some discussion here - commercialhaskell/stack#604 commercialhaskell/stack#446 . It's worked out well so far, the primary motivation is to distinguish output you might want to handle programmatically from output that's purely intended to be diagnostic, read by humans. I think is true to the intent of stdout / stderr, or at least what it means these days - the "err" in stderr is a bit of a misnomer. See https://www.jstorimer.com/blogs/workingwithcode/7766119-when-to-use-stderr-instead-of-stdout

hvr · 2017-09-24T07:12:55Z

@mgsloan coincidentally, since I wrote that comment I investigated the topic a bit, and I come to a similar conclusion. And for things like new-run this rather clear-cut; but the question for me is what you consider something to handle programmatically in e.g. cabal new-build --dry where the diagnostic output is the normal output (and I often call cabal new-build --dry for the sake of handling its output programmatically!) :-)

23Skidoo · 2017-09-24T09:37:41Z

Having to special-case stuff like --dry-run is annoying and we likely won't get everything right the first time. Also the same diagnostic emitted by --dry-run is emitted when you run e.g. new-build -v3, so going that route will require us to switch between stdout/stderr dynamically depending on mode.

For such corner cases, my preference in is to decide either on stderr or stdout and stick with that.

23Skidoo · 2017-09-24T09:42:11Z

@Rufflewind Have you also tried grepping for putStr / putStrLn? They may be used in a couple of places in the codebase.

BardurArantsson · 2017-09-24T12:09:44Z

@mgsloan I think it's more that the distinction between stdout/stderr never made sense in the first place. It obviously only works in extremely simplistic scenarios and is pointless for anything beyond that. (Hence, exit codes and the like. Why have a secondary signaling mechanism if stdout/stderr was a thing you could rely on? It's absurd and pointless. Slitting the output into two streams only leads to race conditions, etc.)

Rufflewind · 2017-09-25T03:03:52Z

@hvr I removed the dependency on process-1.2.1.0. Turns out createProcess has a guard to make sure that standard handles are never closed, so createProcess_ is (luckily) not needed here.

@23Skidoo: Went through each of them again; see updated commit. The following files were not changed because:

Cabal/Distribution/Simple/Register.hs: intended output
Cabal/Distribution/Make.hs: help output
Cabal/Distribution/Parsec/Parser.hs: for testing
Cabal/Distribution/Simple.hs: help output
Cabal/Distribution/PackageDescription/Quirks.hs: unused function
cabal-install/Distribution/Client/Sandbox.hs: intended output
cabal-install/Distribution/Client/Init.hs: only useful for interactive use
cabal-install/Distribution/Client/List.hs: intended output
cabal-install/Distribution/Client/Upload.hs: only useful for interactive use
cabal-install/Distribution/Client/Check.hs: intended output
cabal-install/main/Main.hs: intended output

23Skidoo

Looks OK overall modulo some comments, my only concern is handling of corner cases like --dry-run that @hvr mentioned. We will probably have to identify them one by one and manually switch to use stdout (maybe conditionally).

23Skidoo · 2017-09-26T09:46:56Z

cabal-install/Distribution/Client/GenBounds.hs

@@ -131,7 +133,8 @@ genBounds verbosity packageDBs repoCtxt comp platform progdb mSandboxPkgInfo
                  verbosity packageDBs repoCtxt comp platform progdb
                  mSandboxPkgInfo globalFlags freezeFlags

-       putStrLn boundsNeededMsg
+       hPutStrLn stderr boundsNeededMsg
+       hFlush stderr


Hmm, why do you need to flush stderr? Isn't one of the points of stderr is that it's unbuffered?

I added it as a precaution because I saw stdout being used a few lines later. I've removed it.

I think I saw stderr being set to line-buffered in the code somewhere, but I don't know for sure. But if that is the case then the flush shouldn't be needed.

cabal-install/Distribution/Client/GenBounds.hs

23Skidoo · 2017-09-26T09:48:48Z

Cabal/Distribution/Utils/LogProgress.hs

@@ -54,7 +55,7 @@ runLogProgress verbosity (LogProgress m) =
      }
    step_fn :: LogMsg -> NoCallStackIO a -> NoCallStackIO a
    step_fn doc go = do
-        putStrLn (render doc)
+        hPutStrLn stderr (render doc)


Could be replaced with notice as well, I think...

Reverted this particular change. Changing runLogProgress.step_fn to use notice instead of hPutStrLn breaks too many tests as notice will wrap the messages with -----BEGIN CABAL OUTPUT----- markers, causing them to get caught in the test output.

23Skidoo · 2017-09-26T10:13:04Z

Cabal/Distribution/Simple/Program/GHC.hs

@@ -281,7 +281,8 @@ ghcInvocation :: ConfiguredProgram -> Compiler -> Platform -> GhcOptions
              -> ProgramInvocation
 ghcInvocation prog comp platform opts =
    (programInvocation prog (renderGhcOptions comp platform opts)) {
-        progInvokePathEnv = fromNubListR (ghcOptExtraPath opts)
+        progInvokePathEnv = fromNubListR (ghcOptExtraPath opts),
+        progInvokeOutAsErr = True


Why only do this for GHC? Perhaps it should be the default?

I'm not entirely sure. Do these other programs write non-essential messages to stdout? I think the use of stdout for progress messages as GHC does is not normal, but I suppose Cabal could be proactive and force everything to stderr.

23Skidoo · 2017-09-27T09:04:49Z

CI failure is due to -Werror and a redundant import.

Rufflewind · 2017-09-27T19:12:56Z

Removed redundant imports.

23Skidoo · 2017-09-27T20:46:22Z

Looks like this PR breaks a whole load of tests, which I guess is expected -- the test suite expects the output to be on stdout.

tfausak · 2018-07-10T19:55:08Z

I recently ran into an annoyance where new-exec printed the following output to STDOUT:

Resolving dependencies...

I was able to work around it, but it would be nice if Cabal sent output like that to STDERR instead.

Is this PR the best chance for that happening? Should I follow any other issues or PRs?

Send all diagnostic messages (including debug, info, notice) to stderr instead of stdout. Likewise, redirect GHC's stdout to stderr as GHC sends progress output to stdout due to GHC Trac haskell#3636.

Rufflewind · 2020-01-02T07:30:19Z

All tests pass now.

phadej

If the problem is that v2-run may output something to stdout, then this is no solution? What if I need to capture stderr of program I v2-run?

The only solution is to make -vsilent truly silent (i.e. changing manual putStrLns to primitives in Verbosity), use verboseUnmarkOutput where appropriate to not break tests.

Rufflewind · 2020-01-03T08:11:34Z

What if I need to capture stderr of program I v2-run?

In that case, the stderr may contain messages from Cabal as well. This is reasonable because there is no general expectation that the contents of stderr has any structure. stderr is normally meant for human consumption; this contrasts with stdout, which may contain structured data or other outputs that the user intend to capture or forward to another program. It is uncommon to capture stderr and also to expect the same degree of structure as captured stdout.

The only solution is to make -vsilent truly silent

That is a separate and orthogonal issue. This PR is only intended to address the issue in which Cabal writes unstructured diagnostic messages to stdout.

Making Cabal truly silent is not without downsides, such as the inability for Cabal to return useful diagnostics to the user when something goes wrong. Some CLI tools (e.g. curl --silent --show-error) can be configured to be silent on success, but display error messages in stderr on failure.

fumieval · 2020-09-07T02:21:49Z

I've been annoyed by "Up to date" or build messages from cabal. I agree that making it completely silent is a separate issue; I do want to see errors from cabal if something went wrong.

Kleidukos · 2023-05-17T06:33:23Z

Marking this PR as draft 🙂

Rufflewind force-pushed the stderr branch from 20d5790 to a2ac4dc Compare September 23, 2017 01:04

hvr reviewed Sep 23, 2017

View reviewed changes

Cabal/Cabal.cabal Outdated Show resolved Hide resolved

Rufflewind force-pushed the stderr branch from a2ac4dc to 9e8431d Compare September 25, 2017 03:00

23Skidoo approved these changes Sep 26, 2017

View reviewed changes

Rufflewind force-pushed the stderr branch from 9e8431d to 4eb5938 Compare September 27, 2017 05:20

Rufflewind force-pushed the stderr branch from 4eb5938 to 7dadaf8 Compare September 27, 2017 19:12

23Skidoo added the blocked: action-required label Sep 28, 2017

23Skidoo mentioned this pull request Jan 3, 2018

cabal new-run: Do not print Up to date #4994

Closed

23Skidoo mentioned this pull request Oct 15, 2018

new-run doesn't suppress its own stdout when being invoked as part of a pipe #5595

Closed

Use stderr for Cabal's diagnostic messages instead of stdout

e752ad0

Send all diagnostic messages (including debug, info, notice) to stderr instead of stdout. Likewise, redirect GHC's stdout to stderr as GHC sends progress output to stdout due to GHC Trac haskell#3636.

Rufflewind force-pushed the stderr branch from 7dadaf8 to e752ad0 Compare January 2, 2020 05:17

Rufflewind requested review from 23Skidoo and hvr January 2, 2020 07:28

phadej suggested changes Jan 2, 2020

View reviewed changes

fgaz mentioned this pull request Nov 2, 2021

cabal run should send info logging to stderr not stdout #7790

Open

fredefox mentioned this pull request Nov 27, 2021

Write debug output to stderr #7838

Draft

3 tasks

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 1, 2022

ulysses4ever removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 3, 2022

slotThe mentioned this pull request Sep 5, 2022

recompilation errors go to stderr, not xmonad.errors xmonad/xmonad#403

Open

2 tasks

andreasabel mentioned this pull request Dec 30, 2022

Warning: "debug build with assertions" interferes with --numeric-version #8645

Closed

Kleidukos marked this pull request as draft May 17, 2023 06:33

ulysses4ever mentioned this pull request Jul 17, 2024

cabal path writes purely informational output to stdout instead of stderr #10200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use stderr for Cabal's diagnostic messages instead of stdout #4789

Use stderr for Cabal's diagnostic messages instead of stdout #4789

Rufflewind commented Sep 23, 2017 •

edited

Loading

mention-bot commented Sep 23, 2017

hvr commented Sep 23, 2017 •

edited

Loading

Rufflewind commented Sep 23, 2017

mgsloan commented Sep 24, 2017

hvr commented Sep 24, 2017

23Skidoo commented Sep 24, 2017 •

edited

Loading

23Skidoo commented Sep 24, 2017

BardurArantsson commented Sep 24, 2017 •

edited

Loading

Rufflewind commented Sep 25, 2017

23Skidoo left a comment

23Skidoo Sep 26, 2017

Rufflewind Sep 27, 2017

23Skidoo Sep 26, 2017

Rufflewind Sep 27, 2017

Rufflewind Jan 2, 2020

23Skidoo Sep 26, 2017

Rufflewind Sep 27, 2017

23Skidoo commented Sep 27, 2017

Rufflewind commented Sep 27, 2017

23Skidoo commented Sep 27, 2017 •

edited

Loading

tfausak commented Jul 10, 2018

Rufflewind commented Jan 2, 2020

phadej left a comment

Rufflewind commented Jan 3, 2020

fumieval commented Sep 7, 2020

Kleidukos commented May 17, 2023

Use stderr for Cabal's diagnostic messages instead of stdout #4789

Are you sure you want to change the base?

Use stderr for Cabal's diagnostic messages instead of stdout #4789

Conversation

Rufflewind commented Sep 23, 2017 • edited Loading

mention-bot commented Sep 23, 2017

hvr commented Sep 23, 2017 • edited Loading

Rufflewind commented Sep 23, 2017

mgsloan commented Sep 24, 2017

hvr commented Sep 24, 2017

23Skidoo commented Sep 24, 2017 • edited Loading

23Skidoo commented Sep 24, 2017

BardurArantsson commented Sep 24, 2017 • edited Loading

Rufflewind commented Sep 25, 2017

23Skidoo left a comment

Choose a reason for hiding this comment

23Skidoo Sep 26, 2017

Choose a reason for hiding this comment

Rufflewind Sep 27, 2017

Choose a reason for hiding this comment

23Skidoo Sep 26, 2017

Choose a reason for hiding this comment

Rufflewind Sep 27, 2017

Choose a reason for hiding this comment

Rufflewind Jan 2, 2020

Choose a reason for hiding this comment

23Skidoo Sep 26, 2017

Choose a reason for hiding this comment

Rufflewind Sep 27, 2017

Choose a reason for hiding this comment

23Skidoo commented Sep 27, 2017

Rufflewind commented Sep 27, 2017

23Skidoo commented Sep 27, 2017 • edited Loading

tfausak commented Jul 10, 2018

Rufflewind commented Jan 2, 2020

phadej left a comment

Choose a reason for hiding this comment

Rufflewind commented Jan 3, 2020

fumieval commented Sep 7, 2020

Kleidukos commented May 17, 2023

Rufflewind commented Sep 23, 2017 •

edited

Loading

hvr commented Sep 23, 2017 •

edited

Loading

23Skidoo commented Sep 24, 2017 •

edited

Loading

BardurArantsson commented Sep 24, 2017 •

edited

Loading

23Skidoo commented Sep 27, 2017 •

edited

Loading