bscarlet / llvm-general

Rich LLVM bindings for Haskell (with transfer of LLVM IR to and from C++, detailed compilation pass control, etc.)
http://hackage.haskell.org/package/llvm-general
132 stars 38 forks source link

use transformers-compat > 0.3.3.4 instead of transformers 0.4 #115

Closed cartazio closed 10 years ago

cartazio commented 10 years ago

at least for llvm-general-pure using transformers-compat and mtl >= 2.1.3.1 as the bounds works fine. havent gotten to testing main llvm-general in kind as yet.

the main motivation here is that ghc 7.8 comes with transformers 0.3, so any project that wants to use the GHC API as a frontend and experiment with doing code code

    transformers-compat >= 0.3.3.4,
    mtl >= 2.1.3.1,

is what i've swapped into llvm-general-pure, doing a test build of llvm-general now too

cartazio commented 10 years ago

ok, also needs a bit of work

cartazio commented 10 years ago

(i'm pestering @ekmett upstream to fix some issues for this to work)

ekmett commented 10 years ago

What do I need to fix?

cartazio commented 10 years ago

It looks like the only fix might be one that you may find distasteful, namely having mtl depend on transformers-compat rather than transformers. (Though maybe the premise of this ticket is wrong to begin with, need to check when I'm not on my phone. )

Context being that GHC 7.8 comes with transformers 0.3, and (my understanding here might be antiquated and wrong, also heading to airport right now so I can't test right now) is that its not possible to build a GHC 7.8 application that uses both the GHC API and transformers 0.4.

This single version linking matter goes away with current GHC head I believe.

Idea of this whole thing being that it might be possible to reuse Some of the ghcjs engineering to experiment with other ways of compiling GHC via LLVM. On Sep 17, 2014 1:00 PM, "Edward Kmett" notifications@github.com wrote:

What do I need to fix?

— Reply to this email directly or view it on GitHub https://github.com/bscarlet/llvm-general/issues/115#issuecomment-55925416 .

cartazio commented 10 years ago

I could just be chasing a rabbit hole under false pretenses. I'll do some experimentation once I'm done traveling for the day

It looks like the only fix might be one that you may find distasteful, namely having mtl depend on transformers-compat rather than transformers. (Though maybe the premise of this ticket is wrong to begin with, need to check when I'm not on my phone. )

Context being that GHC 7.8 comes with transformers 0.3, and (my understanding here might be antiquated and wrong, also heading to airport right now so I can't test right now) is that its not possible to build a GHC 7.8 application that uses both the GHC API and transformers 0.4.

This single version linking matter goes away with current GHC head I believe.

Idea of this whole thing being that it might be possible to reuse Some of the ghcjs engineering to experiment with other ways of compiling GHC via LLVM. On Sep 17, 2014 1:00 PM, "Edward Kmett" notifications@github.com wrote:

What do I need to fix?

— Reply to this email directly or view it on GitHub https://github.com/bscarlet/llvm-general/issues/115#issuecomment-55925416 .

ekmett commented 10 years ago

I'm not sure I understand what the issue is there. What instance is it from the mtl you need that would tie to a data type from transformers-compat?

cartazio commented 10 years ago

@ekemtt , I spoke with @hvr to confirm the underlying problem

[14/09/17-14:43:35]  <carter>    hvr: rwbarton  am i correct in thinking that for 7.8,. I can't write a program that links to both the ghc API and transformers 0.4 ?
[14/09/17-14:46:25]  <@hvr>  carter: ...unless you build your own GHC 7.8 w/ trafo-0.4

(trafo-0.4 being transformers-0.4 )

I (believe, but cant test till this evening) that MTL 2.2 should be build-able with tranformers 0.3 if MTL instead depended on transformers-compat, but perhaps I'm overlooking something.

TL;DR : any program the depends on MTL 2.2 thus depends on transformers 0.4, and i believe thus cant link to the GHC API. A simple minimal test I can try this evening is try to build ghc-mod in a sandbox after installing current MTL 2.2 in the sandbox.

Does that make more sense?

bscarlet commented 10 years ago

The title of this bug is a cure, and after all this discussion it's still not clear what the actual symptom is. Please open a new bug in terms of the problem rather than the proposed solution, and attach a proposed solution as a proper pull request if you like. As it is I can't judge the merits of the proposed change for lack of a clear understanding either of exactly what change you propose or precisely what it's an attempt to fix.

ekmett commented 10 years ago

@cartazio: I have to confess I find the notion of mtl depending on transformers-compat unpalatable, in fact I can pretty much indicate it'd take a lot of convincing for me to make that change, as it would create a great deal of drama. It may be a technically viable fix, but its pretty much dead on social grounds.

Depending on Control.Monad.Except when there isn't a version of ghc that ships with that version of transformers seems to be the problem.

bscarlet commented 10 years ago

There's some package I'm not supposed to upgrade? Rather big pitfall, that. I rather wish there'd been a way for me to know that. Should transformers be dependent on something in ghc to make that dependency explicit?

Would it be possible to get an mtl-compat in the spirit of transformers-compat?

hvr commented 10 years ago

@bscarlet there are only a few real non-upgradable packages currently (ghc,base, ghc-prim, integer-{simple,gmp}). And unless you depend on the ghc-package, you can upgrade transformers just fine. Things just get more difficult as soons as you start depending on ghc.

Here's all the direct deps of ghc you need to take into account if you link against ghc:

$ ghc-pkg dot  | grep '^"ghc-7'
"ghc-7.8.3" -> "Cabal-1.18.1.3"
"ghc-7.8.3" -> "array-0.5.0.0"
"ghc-7.8.3" -> "base-4.7.0.1"
"ghc-7.8.3" -> "bin-package-db-0.0.0.0"
"ghc-7.8.3" -> "bytestring-0.10.4.0"
"ghc-7.8.3" -> "containers-0.5.5.1"
"ghc-7.8.3" -> "directory-1.2.1.0"
"ghc-7.8.3" -> "filepath-1.3.0.2"
"ghc-7.8.3" -> "hoopl-3.10.0.1"
"ghc-7.8.3" -> "hpc-0.6.0.1"
"ghc-7.8.3" -> "process-1.2.0.0"
"ghc-7.8.3" -> "template-haskell-2.9.0.0"
"ghc-7.8.3" -> "time-1.4.2"
"ghc-7.8.3" -> "transformers-0.3.0.0"
"ghc-7.8.3" -> "unix-2.7.0.1"
ekmett commented 10 years ago

The problem is when the latest Haskell platform was cut it ignored the latest releases of mtl and transformers.

This means the entire Haskell ecosystem is kinda frozen out on the latest changes there for another year, at least if they want to be able to use transformers and the ghc package at the same time.

Unfortunately things like doctest need to be able to use the ghc package, so this effectively includes almost everyone who does testing.

ekmett commented 10 years ago

The main source of frustration there is that Ross deprecated a bunch of stuff in the latest transformers, added Control.Monad.Except, and did a bunch of fiddly additions, that now will sit around for a year unused.

mtl-compat may be an option, as galling as it would be to write a package that consists one one orphan instance.

ekmett commented 10 years ago

Do you use the actual MonadError instance for Except?

Or do you just use Except somewhere in a context where you could use Control.Monad.Trans.Except from transformers-compat instead of Control.Monad.Except?

bscarlet commented 10 years ago

The monad stacks I use for the actual work use MonadError - e.g. EncodeAST.

ekmett commented 10 years ago

If you wanted to avoid the need for creating an mtl-compat with orphan instances (ick) you could hand write 2 instances (the MonadState and MonadError) for your composite EncodeAST type. This would let you work with transformers-compat and older transformers + mtl.

Alternately reverting to ErrorT from ExceptT would avoid the need for transformers-compat entirely, but leave you with deprecation warnings when built with transformers 0.4.

-Edward

On Thu, Sep 18, 2014 at 11:02 PM, Benjamin S. Scarlet < notifications@github.com> wrote:

The monad stacks I use for the actual work use MonadError - e.g. EncodeAST https://github.com/bscarlet/llvm-general/blob/master/llvm-general/src/LLVM/General/Internal/EncodeAST.hs#L52 .

— Reply to this email directly or view it on GitHub https://github.com/bscarlet/llvm-general/issues/115#issuecomment-56130184 .

cartazio commented 10 years ago

I tried doing some of the more complicated ideas yesterday (ended in tears and madness), and I'd be willing to help write a PR for doing the EncodeAST etc direct/by hand instances if that was deemed a viable move

bscarlet commented 10 years ago

Thanks for the offer, but I can't judge which solution makes sense without a better idea of what the problem is. I've so far been able to engage in limited discussion because I can agree that unnecessarily narrow dependencies are bad, but you still haven't told me what you're actually trying to do in any way I can replicate.

bscarlet commented 10 years ago

Edward,

I'm not sure what hypothetical orphan instance you're talking about. I was imagining mtl-compat would contain essentially the same ExceptT code as mtl, but be dependent on transformers-compat.

Alternately, would the objections to mtl depending on transformers-compat admit a dependency on: transformers == 0.4.* or (transformers == 0.3., transformers-compat == 0.3.) or such? In other words, is even allowing the use of the compat package unpalatable?

ekmett commented 10 years ago

ExceptT has an instance for the existing MonadError class from Control.Monad.Error, which you are currently using in EncodeAST.

transformers >= 0.4 provides ExceptT as does transformers-compat, for transformers >= 0.2 && < 0.4

mtl defines MonadError.

instance Monad m => MonadError e (ExceptT e m) doesn't have a place to live for the ExceptT in transformers-compat, which doesn't depend on mtl without an orphan or without transformers-compat picking up a dependency on mtl. For folks who were using transformers-compat explicitly to avoid the mtl, that becomes an annoying proposition and a sign of further troubles.

They key here is the mtl code for ExceptT would be an orphan if supplied by another package.

Currently you aren't actually needing the instance for that class for that data type per se, you are needing a way to make your EncodeAST an instance of MonadErrorand ofMonadStateThis is a much easier fix to supply, as you can write it in ~7 lines of code locally, basically just needs to have those two instances lifted overExceptT` by hand.

That would avoid needing the creation of an mtl-compat with orphan instances or mtl having to incur a dependency on transformers-compat, and could be done as purely a local change.

Why might folks care?

I think the reason @cartazio cares is he'd like to be able to use this code in code that also links against the ghc package, which incurs a transformers dependency fixed at 0.3 on current GHC.

Scenario A is writing GHC plugins or something but scenario B, is just writing any code that uses llvm-general that uses doctest, which incurs a dependency on ghc.

ekmett commented 10 years ago

Alternately, would the objections to mtl depending on transformers-compat admit a dependency on: transformers == 0.4.* or (transformers == 0.3., transformers-compat == 0.3.) or such? In other words, is even allowing the use of the compat package unpalatable?

The problem here is that if we put a flag in a package like mtl that hits the global backtracking limit for everyone on all of hackage. This means anyone who isn't using the most very recent version of cabal just stop being able to build high end packages.

The design of transformers-compat was chosen to avoid incurring a hit to the backtracking limit by various bits of trickery that exploit the way the cabal solver resolves packages, so the only sensible way to incur a dependency on transformers-compat is to bake it into mtl as a dependency for this, or to give up and punt the instances to something like mtl-compat.

Given my strong dislike for orphans, and my desire to avoid forcing folks into mtl who just want transformers, something would have to give, hence why I'm offering the idea that there is a local quick patch that wouldn't require us to have to hoist me upon the horns of that dilemma. ;)

cartazio commented 10 years ago

I'm writing several branches to try out several different versions of these approaches to see whats sane. Edward has offered to help sanity check which ones are sane for human consumption before sharing any of them here :)

bscarlet commented 10 years ago

If you've got cycles for it, certainly start with Edward's local hack suggestion. It makes me sad, but everything else makes me sadder. (The mess with the backtracking limit is particularly galling).

cartazio commented 10 years ago

a. So I'm hitting a challenge with the local change that just uses the transformers-compat ExceptT as follows in LLVM/General/Internal/FFI/Module.hs with the following class

class LLVMAssemblyInput s where
  llvmAssemblyMemoryBuffer ::
              (Inject String e, MonadError e m, MonadIO m, MonadAnyCont IO m)
                              => s -> m (FFI.OwnerTransfered (Ptr FFI.MemoryBuffer))

this is used in 1-2 spots in a way that requires m=ExceptT e I could probably just add a somewhat specialized version that look like

  llvmAssemblyMemoryBufferExcept :: (Inject String e, MonadIO m, MonadAnyCont IO m)
                              => s -> ExceptionT e m (FFI.OwnerTransfered (Ptr FFI.MemoryBuffer))        

though that might be a bit icky

b. I've gotten a branch working that uses newtype ExceptabeT e m a = ExceptableT {getExceptT :: ExceptT e m a} and that (aside from writing a few basic things and the MTL related instances) is just a teeny bit of renaming and newtype deriving, and then I can keep all the current apis, including the ones that currently return ExceptT something something . (Granted, this is tantamount to having a private copy of ExceptT)

I like the choice (b) approach, and if you're receptive to considering it, i'm happy to prepare it for a PR. (a) is getting pretty gnarly as I push it along

cartazio commented 10 years ago

the way i've been figuring out (a) is just using empty instances for the Monad* stuff for EncodeAST