Fixing build with MonadFailDesugaring #80

recursion-ninja · 2018-08-21T04:22:23Z

This resolves #79.

I had to use CPP for conditional imports and constraints, which is ugly, but not too intrusive.

ivan-m · 2018-08-21T05:04:25Z

.travis.yml

@@ -38,6 +38,15 @@ matrix:
    - env: CABALVER=1.24 GHCVER=8.0.1
      compiler: ": #GHC 8.0.1"
      addons: {apt: {packages: [cabal-install-1.24,ghc-8.0.1], sources: [hvr-ghc]}}
+    - env: CABALVER=1.24 GHCVER=8.2.2


Can you add these into the .cabal file as well please?

I added these to to the tested-with field of fgl.cabal.

ivan-m · 2018-08-21T05:05:02Z

Data/Graph/Inductive/Monad.hs

@@ -1,3 +1,4 @@
+{-# LANGUAGE CPP                   #-}


Please merge this into the next line:

{-# LANGUAGE CPP, MultiParamTypeClasses #-}

ivan-m · 2018-08-21T05:06:49Z

Data/Graph/Inductive/Monad.hs

@@ -39,7 +44,11 @@ import Data.Graph.Inductive.Graph

 -- Monadic Graph
 --
+#if MIN_VERSION_base(4,12,0)


One way to make this a little less intrusive is to have something like:

class #if MIN_VERSION_base(4,12,0) (MonadFail m) #else (Monad m) #endif => GraphM m gr where

The advantage of this approach is that you can ensure that GraphM is defined regardless in the same way, just that the constraint differs.

ivan-m · 2018-08-21T05:10:43Z

I don't mind CPP; I've had to use it enough times myself :/

If nothing else, this PR has also pointed out to me that I may need to re-implement the benchmarks away from microbench as it hasn't been touched in 10 years and looks like it won't work with 8.8 :(

recursion-ninja · 2018-08-21T11:57:09Z

I try to avoid CPP if I can and like to let people know when I've added it.

I applied your code review suggestions and squashed the commits.

recursion-ninja · 2018-08-23T14:17:39Z

Should be ready to merge.

peti · 2018-09-27T14:56:59Z

Ping?

ivan-m · 2018-09-27T14:58:56Z

Sorry, work has been rather busy so I haven't had a chance to look at it. Hopefully next week. On mobile; please excuse any tpyos.

…

On Thu, 27 Sep. 2018, 10:57 pm Peter Simons, ***@***.***> wrote: Ping? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAwXc_F1KmibGRiqf2MiYYknipypmkicks5ufOdngaJpZM4WFFNo> .

phadej · 2018-09-30T00:10:17Z

.travis.yml

+      addons: {apt: {packages: [cabal-install-1.24,ghc-8.4.3], sources: [hvr-ghc]}}
+    - env: CABALVER=1.24 GHCVER=8.6.1
+      compiler: ": #GHC 8.6.1"
+      addons: {apt: {packages: [cabal-install-1.24,ghc-8.6.1], sources: [hvr-ghc]}}


please use cabal-install-2.4 for GHC-8.6.1, 2.2 for 8.4, 2.0 for 8.2. Otherwise things may fail.

I have updated the cabal versions to match the newer ghc versions.

phadej · 2018-09-30T00:16:05Z

Data/Graph/Inductive/Monad.hs

@@ -19,6 +19,10 @@ module Data.Graph.Inductive.Monad(


 import Data.Graph.Inductive.Graph
+#if MIN_VERSION_base(4,12,0)
+import Control.Monad.Fail


recommended way is to depend on fail package, then you can provide uniform interface across different GHC versions

http://hackage.haskell.org/package/fail

https://ghc.haskell.org/trac/ghc/wiki/Migration/8.0#base-4.9.0.0

In general https://ghc.haskell.org/trac/ghc/wiki/Migration/8.6 and other migration guides are good reads.

Note that changing context of a class is breaking change, requiring a major bump to fgl-5.7

You are absolutely correct that this is a breaking change and I've bumped the version to fgl-5.7 to reflect that along with updating the changelog.

I was also unaware of the fail package. I'm happy to also update fgl to depend on the fail package and remove the CPP I had to add in lieu of that solution. However, I want to get feedback from @ivan-m to make sure that's their preferred way for fgl to support the MonadFail constraint on the GraphM typeclass.

…[skip ci]

phadej · 2018-09-30T20:55:56Z

@recursion-ninja if you think breaking change is better way, could you depend on fail to provide uniform API.

It would be very surprising for downstream if when upgrading to GHC-8.6 they need to write MonadFail instance for their monad (if someone is so silly that doesn't use IO or ST directly).

ivan-m · 2018-09-30T23:00:56Z

I prefer to keep FGL depending solely on boot packages.

recursion-ninja · 2018-09-30T23:22:51Z

@ivan-m, so we'll stick with the CPP and not depend on the fail package.

Since that's the case, this should be ready to merge.

recursion-ninja · 2018-10-09T18:57:44Z

Is there a time line for merging this? I'd like it to be able to use the fgl and libraries that transitively depend of fgl with GHC-8.6.1. Is there anything else required for this to be merged?

martijnbastiaan · 2018-10-15T10:18:44Z

@recursion-ninja as a temporary workaround you could use:

source-repository-package
  type: git
  location: https://github.com/recursion-ninja/fgl.git
  tag: d49cd7b0cd766d9f4647e140e263de1b94ace0c5

in your cabal.project. I believe this only works if you're already using new-*, though. I've tested this for https://github.com/clash-lang/clash-compiler/ which seems to work fine. It would be great if this patch could be merged soon though!

recursion-ninja · 2018-10-22T20:14:10Z

@martijnbastiaan that is a nice, temporary solution. However, this pull request should be merged and a new version of fglupload sooner rather than later.

peti · 2018-11-08T14:24:38Z

Does someone have the time to initiate a non-maintainer upload request as per https://wiki.haskell.org/Hackage_trustees?

ivan-m · 2018-11-08T14:26:19Z

Sorry, this slipped my mind; I'll do it this weekend.

ivan-m · 2018-11-13T15:11:47Z

I'm not sure why Travis-CI didn't catch this, but this actually fails with cabal-install-2.4 due to the deprecation of cabal install.

grumbles

phadej · 2018-11-13T20:28:13Z

@ivan-m, cabal-install-2.4 print the warning, but it doesn't fail.

…

Sent from my iPhone

On 13 Nov 2018, at 17.11, Ivan Lazar Miljenovic ***@***.***> wrote: I'm not sure why Travis-CI didn't catch this, but this actually fails with cabal-install-2.4 due to the deprecation of cabal install. grumbles — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

ivan-m · 2018-11-14T01:07:47Z

Sorry, you're right; I fail at reading error messages.

ivan-m reviewed Aug 21, 2018

View reviewed changes

Fixing issue with MonadFailDesugaring.

344a7e4

recursion-ninja force-pushed the master branch from d6735ce to 344a7e4 Compare August 21, 2018 10:55

peti mentioned this pull request Sep 27, 2018

fgl-5.6.0.0 fails with ghc-8.6.1 #81

Closed

phadej reviewed Sep 30, 2018

View reviewed changes

recursion-ninja added 2 commits September 30, 2018 15:10

Correcting the cabal versions to match newer ghc versions.

0c795fe

Updating version and changelog in the presence of a breaking change. …

d49cd7b

…[skip ci]

ivan-m merged commit 4381a44 into haskell:master Nov 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing build with MonadFailDesugaring #80

Fixing build with MonadFailDesugaring #80

recursion-ninja commented Aug 21, 2018

ivan-m Aug 21, 2018

recursion-ninja Aug 21, 2018

ivan-m Aug 21, 2018 •

edited

Loading

ivan-m Aug 21, 2018

ivan-m commented Aug 21, 2018

recursion-ninja commented Aug 21, 2018

recursion-ninja commented Aug 23, 2018

peti commented Sep 27, 2018

ivan-m commented Sep 27, 2018 via email

phadej Sep 30, 2018

recursion-ninja Sep 30, 2018

phadej Sep 30, 2018

recursion-ninja Sep 30, 2018 •

edited

Loading

phadej commented Sep 30, 2018

ivan-m commented Sep 30, 2018

recursion-ninja commented Sep 30, 2018

recursion-ninja commented Oct 9, 2018

martijnbastiaan commented Oct 15, 2018

recursion-ninja commented Oct 22, 2018

peti commented Nov 8, 2018

ivan-m commented Nov 8, 2018

ivan-m commented Nov 13, 2018

phadej commented Nov 13, 2018 via email

ivan-m commented Nov 14, 2018

Fixing build with MonadFailDesugaring #80

Fixing build with MonadFailDesugaring #80

Conversation

recursion-ninja commented Aug 21, 2018

ivan-m Aug 21, 2018

Choose a reason for hiding this comment

recursion-ninja Aug 21, 2018

Choose a reason for hiding this comment

ivan-m Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

ivan-m Aug 21, 2018

Choose a reason for hiding this comment

ivan-m commented Aug 21, 2018

recursion-ninja commented Aug 21, 2018

recursion-ninja commented Aug 23, 2018

peti commented Sep 27, 2018

ivan-m commented Sep 27, 2018 via email

phadej Sep 30, 2018

Choose a reason for hiding this comment

recursion-ninja Sep 30, 2018

Choose a reason for hiding this comment

phadej Sep 30, 2018

Choose a reason for hiding this comment

recursion-ninja Sep 30, 2018 • edited Loading

Choose a reason for hiding this comment

phadej commented Sep 30, 2018

ivan-m commented Sep 30, 2018

recursion-ninja commented Sep 30, 2018

recursion-ninja commented Oct 9, 2018

martijnbastiaan commented Oct 15, 2018

recursion-ninja commented Oct 22, 2018

peti commented Nov 8, 2018

ivan-m commented Nov 8, 2018

ivan-m commented Nov 13, 2018

phadej commented Nov 13, 2018 via email

ivan-m commented Nov 14, 2018

ivan-m Aug 21, 2018 •

edited

Loading

recursion-ninja Sep 30, 2018 •

edited

Loading