-
Notifications
You must be signed in to change notification settings - Fork 843
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
Workaround #2491 at all call sites #2492
Conversation
Investigating the failure. |
3e69d2a
to
a0c30b7
Compare
The AppVeyor failure appears spurious, so I've rebased and force-pushed to trigger a rebuild. |
Very clean workaround! 👍 I defer merging this to see whether the AppVeyor failure reappears. |
Thanks! |
It seems that these functions sometimes don't create YAML files when they should... and on Windows (64bit) that affects even the basic testsuite and not just the integration tests.
But the obvious basics work even on Windows—the snippet below creates a file with the expected content. While Frankly I'm out of ideas, other than closing this and waiting for the fix in
|
Ah, my decode wrapper reports file not found as the wrong exception type! Apparently those files are fetched when decode fails. Logs show stack trying to read those missing files, before they're written to. Testing this hypothesis. |
The interface we wrap expects only Yaml.ParseException to be thrown, and to be wrapped inside Either. Conform to that.
-- | Wrappers for Yaml functions to workaround | ||
-- https://github.com/commercialhaskell/stack/issues/2491. | ||
-- Import Data.Yaml.Extra in place of Data.Yaml to use this workaround. | ||
-- Beware these functions construct/deconstruct the entire file at once! | ||
module Data.Yaml.Extra (decodeFileEither, encodeFile, module Data.Yaml) where | ||
|
||
import Control.Exception |
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.
Please double-check this — I think it's OK (this is a straightforward synchronous exception, handled in the IO monad) but I'm not yet fluent with Haskell exceptions.
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 have much experience with Haskell exceptions either. Pinging @mgsloan.
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.
Since he seems busy, I'll trust my (more educated) judgement, assume this is fine, and merge this. Details below but feel free to skip.
Based on https://github.com/commercialhaskell/haskelldocumentation/blob/master/content/exceptions-best-practices.md, having read some more docs (including for MonadCatch
, which doesn't apply here), having read the papers on Haskell exceptions (including async ones, also not applying here*), I think this is fine.
*We risk catching any async IOException
, and I understand that's bad. But IOException
shouldn't be used for asynchronous exceptions, and even safe-exceptions
won't help there.
https://www.fpcomplete.com/blog/2016/06/announce-safe-exceptions
In local testing, this commit appears to help a bit... going to sleep now, we'll see tomorrow. |
All tests fixed (also integration: https://jenkins-public.fpcomplete.com/job/stack-integration-tests/179/changes). And the use of exceptions seems reasonable. @sjakobi, time to merge? |
No description provided.