Version selection flags are marked "manual"

glguy commented 7 years ago

Why are the version flags two, three, four, and five marked as manual? Aren't they intended to be automatic flags? Merijn Verstraaten reports on #haskell on Freenode that a bug fix in Cabal is making this an issue now. Seems clear that we ought to change it, but since it was intentionally specified as manual I'm opening this ticket to ask the question.

cc @RyanGlScott @ekmett

glguy commented 7 years ago

Also cc @merijn

RyanGlScott commented 7 years ago

I can really only offer speculation here, but I strongly suspect the current design was motivated by bugs in (old versions of?) cabal-install. (See of the discussion here, for instance.)

I'd be curious to know what this new cabal-install fix is that is motivating this change.

phadej commented 6 years ago

I'm in favour of this change. The bug was in cabal-install solver. IIRC it did automatic flag assignment locally, and didn't backtrack bad guesses (or something similarly dumb).

TL;DR the problem is with cabal-install-1.16 and older. Hopefully nobody uses them for real anymore.

WIth patch below there is install plan for each GHC using installed transformers:

for HCVER in 7.0.4 7.2.2 7.4.2 7.6.3 7.8.4 7.10.3 8.0.2 8.2.2 8.4.1; do echo $HCVER; cabal new-build -w ghc-$HCVER --dry >/dev/null; cabal-plan topo| grep transformers; done

outputs (note not-latest transformers in GHC-7.8.4 – 8.2.2)

7.0.4
transformers-compat-tests-0.1 test:spec
transformers-compat-0.6.1.6
transformers-0.5.5.0
7.2.2
transformers-compat-tests-0.1 test:spec
transformers-compat-0.6.1.6
transformers-0.5.5.0
7.4.2
transformers-compat-tests-0.1 test:spec
transformers-compat-0.6.1.6
transformers-0.5.5.0
7.6.3
transformers-compat-tests-0.1 test:spec
transformers-compat-0.6.1.6
transformers-0.5.5.0
7.8.4
transformers-compat-tests-0.1 test:spec
transformers-compat-0.6.1.6
transformers-0.3.0.0
7.10.3
transformers-compat-tests-0.1 test:spec
transformers-compat-0.6.1.6
transformers-0.4.2.0
8.0.2
transformers-compat-tests-0.1 test:spec
transformers-compat-0.6.1.6
transformers-0.5.2.0
8.2.2
transformers-compat-tests-0.1 test:spec
transformers-compat-0.6.1.6
transformers-0.5.2.0
8.4.1
transformers-compat-tests-0.1 test:spec
transformers-compat-0.6.1.6
transformers-0.5.5.0

I fail to reproduce bad behavior of old cabal-install, cabal-install-1.18 successfully finds install plan (I suspect the problem is with bigger plans)

Edward (on 2016-01-18) mentions

Without it [this flag business] you hit a bug in the backtracker on older cabal versions.

https://github.com/ekmett/parsers/pull/55/files I use 1.18 for older GHC (not 1.16) (Nov 2015)
https://github.com/bos/aeson/commit/b2346d1c036ca5c9a5e8811f8024cc84310476f1#diff-354f30a63fb0907d4ad57269548329e3 By you @RyanGlScott! Seems one need cabal-install-1.16 to exhibit the problem. 1.18 works fine

% ghc --version
The Glorious Glasgow Haskell Compilation System, version 7.8.4

% cabal-1.18 --version
cabal-install version 1.18.0.4
using version 1.18.1.3 of the Cabal library

% cabal-1.18 install --dry
Resolving dependencies...
In order, the following would be installed (use -v for more details):
generic-deriving-1.12.1
mtl-2.1.3.1 (latest: 2.2.2)
transformers-compat-0.6.1.6

1.16 fails hard today:

% cabal-1.16 --version
cabal-install version 1.16.0.2
using version 1.16.0 of the Cabal library 

% cabal-1.16 configure
cabal-1.16: Command.optionToFieldDescr: feature not implemented

but that doesn't seem to be the case whether there are automatic flags or not.

patch

From 6e5f8231e14403154c03f80b9243a91e3719751c Mon Sep 17 00:00:00 2001
From: Oleg Grenrus <oleg.grenrus@iki.fi>
Date: Fri, 20 Apr 2018 14:54:08 +0300
Subject: [PATCH] Make automatic flags behave well

---
 transformers-compat.cabal | 67 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/transformers-compat.cabal b/transformers-compat.cabal
index 3d4cf64..a0b8ded 100644
--- a/transformers-compat.cabal
+++ b/transformers-compat.cabal
@@ -52,23 +52,28 @@ source-repository head
 flag two
   default: False
   description: Use transformers 0.2. This will be selected by cabal picking the appropriate version.
-  manual: True
+  manual: False

 flag three
   default: False
-  manual: True
+  manual: False
   description: Use transformers 0.3. This will be selected by cabal picking the appropriate version.

 flag four
   default: False
-  manual: True
+  manual: False
   description: Use transformers 0.4. This will be selected by cabal picking the appropriate version.

 flag five
   default: False
-  manual: True
+  manual: False
   description: Use transformers 0.5 up until (but not including) 0.5.3. This will be selected by cabal picking the appropriate version.

+flag five-three
+  default: False
+  manual: False
+  description: Use transformers 0.5.3. This will be selected by cabal picking the appropriate version.
+
 flag mtl
   default: True
   manual: True
@@ -96,31 +101,45 @@ library
   other-modules:
     Paths_transformers_compat

+  -- automatic flags
+  if flag(five-three)
+    hs-source-dirs: 0.5
+    build-depends: transformers >= 0.5.3 && < 0.6
+  else
+    build-depends: transformers < 0.5.3
+
   if flag(five)
     hs-source-dirs: 0.5
     build-depends: transformers >= 0.5 && < 0.5.3
   else
-    if flag(four)
-      cpp-options: -DTRANSFORMERS_FOUR
-      hs-source-dirs: 0.5
-      -- Don't allow transformers-0.4.0.0
-      -- See https://github.com/ekmett/transformers-compat/issues/35
-      build-depends: transformers >= 0.4.1 && < 0.5
-    else
-      if flag(three)
-        hs-source-dirs: 0.3 0.5
-        build-depends: transformers >= 0.3 && < 0.4
-        if flag(mtl)
-          build-depends: mtl >= 2.1 && < 2.2
-      else
-        if flag(two)
-          hs-source-dirs: 0.2 0.3 0.5
-          build-depends: transformers >= 0.2 && < 0.3
-          if flag(mtl)
-            build-depends: mtl >= 2.0 && < 2.1
-        else
-          build-depends: transformers >= 0.5.3 && < 0.6
+    build-depends: transformers < 0.5 || >= 0.5.3
+
+  if flag(four)
+    cpp-options: -DTRANSFORMERS_FOUR
+    hs-source-dirs: 0.5
+    -- Don't allow transformers-0.4.0.0
+    -- See https://github.com/ekmett/transformers-compat/issues/35
+    build-depends: transformers >= 0.4.1 && < 0.5
+  else
+    build-depends: transformers < 0.4 || >= 0.5
+
+  if flag(three)
+    hs-source-dirs: 0.3 0.5
+    build-depends: transformers >= 0.3 && < 0.4
+    if flag(mtl)
+      build-depends: mtl >= 2.1 && < 2.2
+  else
+    build-depends: transformers < 0.3 || >= 0.4
+
+  if flag(two)
+    hs-source-dirs: 0.2 0.3 0.5
+    build-depends: transformers >= 0.2 && < 0.3
+    if flag(mtl)
+      build-depends: mtl >= 2.0 && < 2.1
+  else
+    build-depends: transformers >= 0.3

+  -- other flags
   if impl(ghc >= 7.2) || flag(generic-deriving)
     hs-source-dirs: generics
     build-depends: ghc-prim
-- 
2.7.4

RyanGlScott commented 6 years ago

Silly question: if the goal is to abandon the use of manual Cabal flags, is there any point in making several concurrent releases for each flag? That is, the constraint solver would be smart enough for pick the right versions in all cases, correct?

phadej commented 6 years ago

@RyanGlScott yes, solver is smart enough to pick correct flag assignments, so single transformers-compat version would be enough.

That's the (my) whole motivation. From "multiples version + manual flags" to "single version + automatic flags". Latter is better.

RyanGlScott commented 6 years ago

Alright. In principle, I don't have any objection to this idea. @ekmett, what do you think?

ekmett commented 6 years ago

We used manual flags because the old cabal solver wasn't smart enough to backtrack chains of flag assignments to resolve the flag. The hack was put in place, and it worked for 3 flags, but now that we have 4-5, it seems like it has its own problems; I've been bitten by "no plan" complaints when using transformers-compat a couple of times since we added 0.6 support, but haven't been able to narrow it down nicely.

I'm all for a better solution. I'd be psyched if we could just require a modern enough version of cabal and just use flag backtracking the way the package was originally designed.

@phadej You said it finds a plan, but have you tried it with each of the old transformers version ranges individually pre-installed and had it automatically select the right branch? The old behavior only found problems in that sort of scenario.

phadej commented 6 years ago

@ekmett I'm not sure what you mean by pre-installed?

the #38 PR witnesses that GHC-bundled transformers are ok.

I run a new-build sweep with --constraint=tranformers==version

screenshot from 2018-04-24 15-47-17

new-build finds the plan for each of those.

I also tried cabal sandbox with ghc-7.8.4:

clean sanbox works
after installing transformers-0.4.1.0 also works
after installing transformers-0.5.5.0 also works

Note: stack users (to first approximation) never use non-bundled transformers versions, and anyway aren't affected by solver targeting changes.

ekmett commented 6 years ago

Since it seems to be working then by all means, let's simplify the package!

RyanGlScott commented 6 years ago

This was addressed in #38.

ekmett / transformers-compat