Open mitchellwrosen opened 8 years ago
Wow, I'm shocked at getting an issue opened on this repo already 😗
I'll review tomorrow.
On Wed, Aug 31, 2016, 9:26 PM Mitchell Rosen notifications@github.com wrote:
Might be worth it to redesign unix's exec API to make handling exec errors nicer. Currently it simply does a hacky errno-to-IOError conversion, which unfortunately maps different errnos to the same IOErrorType.
What do you think about this API instead?
import Foreign.C.Error (Errno, getErrno)import qualified System.Posix.Process as Posix -- Only returns if exec failedexecuteFile :: Path -> ... -> IO Errno execureFile path ... = catchIOError (Posix.executeFile path ...) (_ -> getErrno)
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/fpco/replace-process/issues/1, or mute the thread https://github.com/notifications/unsubscribe-auth/AADBB5JT0U7r9J5zmvms7KKD3sB4bscfks5qlcdGgaJpZM4Jx6kR .
Showed up in my news feed, I guess I am following fpco, or something =P
On Aug 31, 2016 3:03 PM, "Michael Snoyman" notifications@github.com wrote:
Wow, I'm shocked at getting an issue opened on this repo already 😗
I'll review tomorrow.
On Wed, Aug 31, 2016, 9:26 PM Mitchell Rosen notifications@github.com wrote:
Might be worth it to redesign unix's exec API to make handling exec errors nicer. Currently it simply does a hacky errno-to-IOError conversion, which unfortunately maps different errnos to the same IOErrorType.
What do you think about this API instead?
import Foreign.C.Error (Errno, getErrno)import qualified System.Posix.Process as Posix -- Only returns if exec failedexecuteFile :: Path -> ... -> IO Errno execureFile path ... = catchIOError (Posix.executeFile path ...) (_ -> getErrno)
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/fpco/replace-process/issues/1, or mute the thread https://github.com/notifications/unsubscribe-auth/ AADBB5JT0U7r9J5zmvms7KKD3sB4bscfks5qlcdGgaJpZM4Jx6kR .
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/fpco/replace-process/issues/1#issuecomment-243866979, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBlpg3HIxfZ8ljvlnZSzc5DLYasUGFpks5qldASgaJpZM4Jx6kR .
I'm not fully on board with such a change. While it certainly makes failure cases more explicit, this seems like a simple rehashing of the typical explicit-vs-implicit failure modes. And given that the jury's out on that, I'd rather not make a change here. Also, for the use case of wanting to just bail when the executable isn't found/isn't chmod +x
d, having the exception thrown with semi-useful information seems better than just returning an Errno
.
That said: if what we're looking at is really just a catchIOError
wrapper like you've provided, what would you say to providing a second function from the module with the Errno
explicit?
Ah, good point about the poor error messages. I didn't mean to conflate explicit/implicit error handling; mostly, I was objecting to the function errnoToIOError
that maps multiple errnos to on IOError and can make it really hard to debug.
If only Errno was itself an Exception, and its pretty-printer included the actual name, e.g. EINTR.
Oh, wow, I had no idea that IOError
doesn't contain information on the specific Errno that was generated, that sucks.
Whoops, I'm half wrong about this, I just had another peek at the source. The errno is stuffed inside the IOError
, but its Show
instance curiously omits it: https://github.com/ghc/ghc/blob/master/libraries/base/GHC/IO/Exception.hs#L362
Might be worth it to redesign
unix
's exec API to make handling exec errors nicer. Currently it simply does a hacky errno-to-IOError conversion, which unfortunately maps different errnos to the sameIOErrorType
.What do you think about this API instead?