Only read stdout when running with --more-verbose
#5
Conversation
src/QuickBench.hs
Outdated
| t2 <- getCurrentTime | ||
| when (not $ null o) $ outvv opts $ (if verbose opts then "\n" else "") ++ o | ||
| unless (c == ExitSuccess) $ out opts $ " (error: " ++ clean e ++ ") " |
There was a problem hiding this comment.
Instead of capturing stderr, I now let the child process inherit stderr from the parent process (the default). So any error messages written to stderr by say hledger will be verbatim written to the stderr of quickbench.
There was a problem hiding this comment.
The other change is that now, a failure of the childprocess will immediately result in failure of the parent process. I consider this an improvement, because before errors from the child process could get obscured by the normal output table. But perhaps you want to keep the original behavior @simonmichael ?
There was a problem hiding this comment.
Hmm. Should quickbench require that all tested commands succeed ? I think it's better to continue if some fail, because sometimes you're benchmarking many things and partial information is better than none.
I think it would be an improvement if it could show the failed status (or some of the error output, if it's not too disruptive of layout) in the results table though ?
thomie
force-pushed
the
do-not-read-stdout
branch
from
583be8f to
05cbdd2
Compare
|
Let's rebase this and I'll test. |
When not running with `-V` or `--more-verbose`, we now pipe the stdout of the executable under test to /dev/null. This prevents quickbench from running out of memory in case the output is huge (GBs). MINOR REMARK With my version of GHC (9.6.6), all exceptions unfortunately get annoted `withBinaryFile`, see https://gitlab.haskell.org/ghc/ghc/-/issues/20886. For example, when running `quickbench -w doesnotexist`, the error message is: ``` /dev/null: withBinaryFile: does not exist (No such file or directory) ``` When running `quickbench -w doesnotexist --more-verbose`, avoiding the call to `withBinaryFile`, the error message is the much clearer: ``` doesnotexist: readCreateProcess: posix_spawnp: does not exist (No such file or directory) ``` This is not ideal, but I believe using the latest version of GHC will fix it.
thomie
force-pushed
the
do-not-read-stdout
branch
from
05cbdd2 to
2823eb9
Compare
Thanks for noting this. I think we'll ignore it. I see this change in output (old and new are compiled with newer GHC): old: new: The new behaviour seems better (reports the command line error once and exits early). |
| `catch` \(e :: IOException) -> return (ExitFailure 1, "", show e) | ||
| callProcessIgnoreOutput :: FilePath -> [String] -> IO () | ||
| callProcessIgnoreOutput cmd args = | ||
| withBinaryFile "/dev/null" WriteMode $ \devNull -> |
There was a problem hiding this comment.
I think this is a unixism, unfortunately ? quickbench should be robust on Windows too.
There was a problem hiding this comment.
Just to be sure: supporting only WSL is not sufficient?
There was a problem hiding this comment.
Yes, supporting only WSL is not sufficient.
When not running with
-Vor--more-verbose, we now pipe the stdout of the executable under test to/dev/null. This prevents quickbench from running out of memory in case the output is huge (GBs).MINOR REMARK
With my version of GHC (9.6.6), all exceptions unfortunately get annoted
withBinaryFile, see https://gitlab.haskell.org/ghc/ghc/-/issues/20886.For example, when running
quickbench -w doesnotexist, the error message is:When running
quickbench -w doesnotexist --more-verbose, avoiding the call towithBinaryFile, the error message is the much clearer:This is not ideal, but I believe using the latest version of GHC will fix it.
Fixes #4 .