Skip to content
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

Merged
merged 2 commits into from
Aug 16, 2016
Merged

Conversation

Blaisorblade
Copy link
Collaborator

No description provided.

@Blaisorblade
Copy link
Collaborator Author

Investigating the failure.
Also, if merged, snoyberg/yaml#91 would make this unnecessary.

@Blaisorblade
Copy link
Collaborator Author

The AppVeyor failure appears spurious, so I've rebased and force-pushed to trigger a rebuild.

@sjakobi
Copy link
Member

sjakobi commented Aug 14, 2016

Very clean workaround! 👍

I defer merging this to see whether the AppVeyor failure reappears.

@Blaisorblade
Copy link
Collaborator Author

Thanks!
But https://jenkins-public.fpcomplete.com/job/stack-integration-tests/177/console and https://jenkins-public.fpcomplete.com/job/stack-integration-tests/178/changes suggests something is wrong with these changes.
My 32bit Windows build (on 64bit Windows) keeps passing normal tests. I'm retesting with 64-bit Stack on the same Windows.

@Blaisorblade
Copy link
Collaborator Author

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.

Failures:

  src/test\Stack\BuildPlanSpec.hs:55:
  1) Stack.BuildPlan finds missing transitive dependencies #159
       uncaught exception: IOException of type NoSuchThing (C:\sr\build-plan\lts-2.9.yaml: openFile: does not exist (No such file or directory))

  src/test\Stack\ConfigSpec.hs:127:
  2) Stack.Config.loadConfig finds the config file in a parent directory
       uncaught exception: IOException of type NoSuchThing (C:\sr\build-plan\lts-2.10.yaml: openFile: does not exist (No such file or directory))

  src/test\Stack\ConfigSpec.hs:138:
  3) Stack.Config.loadConfig respects the STACK_YAML env variable
       uncaught exception: IOException of type NoSuchThing (C:\sr\build-plan\lts-2.10.yaml: openFile: does not exist (No such file or directory))

Randomized with seed 690703526

Finished in 7.4692 seconds
684 examples, 3 failures

But the obvious basics work even on Windows—the snippet below creates a file with the expected content. While C:\sr\build-plan\lts-2.10.yaml indeed doesn't exist, though the directory does.

Frankly I'm out of ideas, other than closing this and waiting for the fix in yaml.

>stack exec ghci
GHCi, version 7.10.3: http://www.haskell.org/ghc/  :? for help
Prelude> import qualified Data.ByteString as B
Prelude B> import System.IO
Prelude B System.IO> withFile  "a.b" WriteMode $ \hnd -> B.hPut hnd (B.pack [1,2])

Prelude B System.IO>
Leaving GHCi.

@Blaisorblade
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator Author

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

@Blaisorblade
Copy link
Collaborator Author

In local testing, this commit appears to help a bit... going to sleep now, we'll see tomorrow.

@Blaisorblade
Copy link
Collaborator Author

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?

@Blaisorblade Blaisorblade merged commit 5d712bc into master Aug 16, 2016
@Blaisorblade Blaisorblade deleted the 2491-workaround-yaml-intl-bug branch August 16, 2016 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants