ekmett / distributive

Dual Traversable
http://hackage.haskell.org/package/distributive
Other
41 stars 25 forks source link

Fix distributive check for transformers-compat #47

Closed matthewbauer closed 6 years ago

matthewbauer commented 6 years ago

This check didn't work when using ghcjs. The ghc version check will be false when using ghcjs, causing transformer-compat to be pulled in.

ekmett commented 6 years ago

I've merged this. But this might be generally problematic, though. We rather carefully use this style to avoid issues when working on other compilers. So this pattern is pretty common. It might be better to do an explicit ghcjs check.

matthewbauer commented 6 years ago

/cc @ericson2314

Ericson2314 commented 6 years ago

@ekmett Which style is problematic? I'd think the newer one is better for precisely the reason of not actually triggering on other compilers---I wish Cabal had more of an abstract interpretation mindset and would e.g. falsify the entire condition expression on the undefined ghc. OTOH, with GHC skipping 7.8.0 I think the <= wrong captures the intent---this me and not @matthewbauer unthinkingly doing the usual interval complement transformation and I apologize. This makes makes me think best would be one of:

impl(ghc < 7.8)
impl(ghc) && !impl(ghc > 7.8)

In any event, we will no doubt be making many more similar changes as we try to improve GHCJS support, so I'm curious what your advice on the best style is.

phadej commented 6 years ago

I'd recommend to phrase those as

you need transformers-except, except on new GHC (and GHCJS and ...)

Then

you don't need transformers-compat, except on old GHC


it would be clearer to say:

build-depends: transformers-compat >= 0.3 && <0.6
if impl(ghc >= 7.8)
  build-depends: transformers-compat unneeded
if impl(ghcjs >= ??)
  build-depends: transformers-compat unneeded

but we cannot do it this way with current .cabal syntax, so today we encode that as

if !impl(ghc >= 7.8) && !impl(ghcjs >= ??)
  build-depends: transformers-compat >= 0.3 && <0.6

I.e. start from the safe "works everywhere" assumption, and relax on evidence; not vice versa.

matthewbauer commented 6 years ago

The same issue seems to be happening in tagged:

https://github.com/ekmett/tagged/blob/5ebc938cea8013a8656be63ef7ee93e9870e8212/tagged.cabal#L78-L81

ekmett commented 6 years ago

@phadej summarized things nicely.

I think the right thing to do for tagged, this, etc. is to add an explicit && !impl(ghcjs >= ??) to the existing bound rather than convert to the < bound proposed.

There are more compilers that are compatible with older ghc versions than with current ones. e.g. hugs, nhc, yhc, etc. ghcjs is sort of the odd man out here needing special treatment because it is almost but not quite ghc.

Ericson2314 commented 6 years ago

Ah OK, I understand now.

I.e. start from the safe "works everywhere" assumption, and relax on evidence; not vice versa.

I totally agree with that principle. And in practice transformers-compat may be the common case, but in principle I feel like it shouldn't be. If I understand correctly, transformers-compat is only needed where transformers is a wired-in library, and the very notion of "wired-in libraries" is just implementation-specific matter. Assuming such implementation-specific mechanisms seems the morally wrong thing to do. [I'll admit, I'm probably a bit biased as I detest notions of wired-in libraries too :).]

ekmett commented 6 years ago

If I understand correctly, transformers-compat is only needed where transformers is a wired-in library,

That isn't quite true. transformers-compat is required wherever an older version of transformers may be selected. Without care, if we just turn it off and leave the transformers bounds intact then we risk build configurations where we try to build against older transformers and don't supply the compatibility shims.

Newer versions of transformers basically dropped support for older haskell compilers completely, so narrowing the window ourselves, and relaxing it just on ghc is somewhat hazardous, as it basically precludes any plausible build plan. OTOH, the transformers-compat-by-default approach at least ensures that we never screw up and keeps the ridiculously painful version selection process isolated in one place.

Ericson2314 commented 6 years ago

Hmm. It would be nice if the "base shim" hacks in Cabal were generalized so we could "borrow" some unused patch versions of transformers to publish transformers-compat with a dependency on old transformers. I.e. transormers-0.4.x.9000 would depend on transormers-0.2. Then downstream dependencies can just depend on transformers >= 0.4 or whatever and not care.

RyanGlScott commented 6 years ago

It looks like the consensus here is to change the bounds to:

if !impl(ghc >= 7.8) && !impl(ghcjs)
  build-depends: transformers-compat >= 0.3 && < 1

Right? I've optimistically applied this change in 2b4c3ad92f6342430492c38e0632488cf439eeed—please speak up if I've goofed it up.

phadej commented 6 years ago

@RyanGlScott :+1: