-
Notifications
You must be signed in to change notification settings - Fork 691
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
Improve tool finding failure #8066
base: master
Are you sure you want to change the base?
Conversation
@@ -339,7 +339,7 @@ configureProgram verbosity prog progdb = do | |||
if absolute | |||
then return (Just (UserSpecified path, [])) | |||
else findProgramOnSearchPath verbosity (progSearchPath progdb) path | |||
>>= maybe (die' verbosity notFound) | |||
>>= maybe (dieAll verbosity notFound (Just path) 77) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path
goes to stdout and we set exit code to 77... this can serve as a hint for calling tools, which may parse stdout and do stuff with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If one doesn't like stdout (because bad scripts don't check exit code before eating stdout): another approach might be to have a better format for the stderr message, that can be parsed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be reasonable to guard this behavior through a cli switch. It might break existing scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, this is a breaking change, otherwise. Additionally, I don't think we can in general rely on that nothing else has written to stdout, since e.g. haddock and hpc just pollute everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can in general rely on that nothing else has written to stdout, since e.g. haddock and hpc just pollute everything
I'm not sure I follow. What do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a general concern that stdout is often polluted by other external tools, such as haddock and hpc. In this specific situation, it should be safe for external tools, such as hls, to try to read the last line of stdout to get the path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there will be any stdout when cabal exec ghc
fails to find ghc
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we maybe have a test-case how Cabal's behaviour has changed now? 👼
=<< pure . wrapTextVerbosity verbosity | ||
=<< pure . addErrorPrefix | ||
=<< prefixWithProgName stderrMsg | ||
forM_ stdoutMsg $ hPutStrLn stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forM_ stdoutMsg $ hPutStrLn stdout | |
for_ stdoutMsg $ hPutStrLn stdout |
Cabal re-exports for_
with its prelude
@@ -214,6 +215,7 @@ import Data.Typeable | |||
( cast ) | |||
import qualified Data.ByteString.Lazy as BS | |||
|
|||
import Control.Monad (forM_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import Control.Monad (forM_) |
Not needed with the suggestion for_
@hasufell I'll mark this PR as draft, feel free to rebase & fix the CI when you have some free time (I know it's hard to come by these days :) |
Related: