Duplicate build-tools-depends/build-depends chokes new-build

[ezyang]

This happened in practice with pretty-show (https://hackage.haskell.org/package/pretty-show-1.6.15/pretty-show.cabal)

Manifested itself as #4621

I am actually not sure if getAllToolDependencies should merge tool dependencies or not. Maybe not, in which case this patch is sufficient:

diff --git a/Cabal/Distribution/Backpack/ConfiguredComponent.hs b/Cabal/Distribution/Backpack/ConfiguredComponent.hs
index 1d929636f..9e095a39f 100644
--- a/Cabal/Distribution/Backpack/ConfiguredComponent.hs
+++ b/Cabal/Distribution/Backpack/ConfiguredComponent.hs
@@ -40,6 +40,7 @@ import Distribution.Simple.LocalBuildInfo
 import Distribution.Version
 import Distribution.Utils.LogProgress
 import Distribution.Utils.MapAccum
+import Distribution.Utils.Generic
 
 import Control.Monad
 import qualified Data.Set as Set
@@ -191,7 +192,7 @@ toConfiguredComponent pkg_descr this_cid dep_map component = do
                          , pn /= packageName pkg_descr
                          , (cn, e) <- Map.toList comp_map
                          , cn == CLibName ]
-    exe_deps =
+    exe_deps = ordNub $
         [ exe
         | ExeDependency pn cn _ <- getAllToolDependencies pkg_descr bi
         -- The error suppression here is important, because in general
diff --git a/Cabal/Distribution/Types/AnnotatedId.hs b/Cabal/Distribution/Types/AnnotatedId.hs
index cc5991a3c..a424335fd 100644
--- a/Cabal/Distribution/Types/AnnotatedId.hs
+++ b/Cabal/Distribution/Types/AnnotatedId.hs
@@ -16,7 +16,7 @@ data AnnotatedId id = AnnotatedId {
         ann_cname :: ComponentName,
         ann_id    :: id
     }
-    deriving (Show)
+    deriving (Show, Eq, Ord)
 
 instance Package (AnnotatedId id) where
     packageId = ann_pid

This might help unblock someone. I've got a bigger patch coming with (1) lots of new debugging Pretty instances, (2) more asserts and (3) a regression test.

[phadej]

@ezyang Note that Pretty is meant to replace Text's disp. "Debugging Pretty instances" sounds suspicious, it's probably ok if the types won't be ever a type of cabal/project/etc -file field Also please avoid huge patches at all cost. It's PITA to review. Also remember that we have a freeze in two weeks, please tell @23Skidoo & others what you plan to do. Let's be strict with the deadline!

…

Sent from my iPhone

On 1 Jan 2018, at 8.34, Edward Z. Yang ***@***.***> wrote: This happened in practice with pretty-show (https://hackage.haskell.org/package/pretty-show-1.6.15/pretty-show.cabal) Manifested itself as #4621 I am actually not sure if getAllToolDependencies should merge tool dependencies or not. Maybe not, in which case this patch is sufficient: diff --git a/Cabal/Distribution/Backpack/ConfiguredComponent.hs b/Cabal/Distribution/Backpack/ConfiguredComponent.hs index 1d929636f..9e095a39f 100644 --- a/Cabal/Distribution/Backpack/ConfiguredComponent.hs +++ b/Cabal/Distribution/Backpack/ConfiguredComponent.hs @@ -40,6 +40,7 @@ import Distribution.Simple.LocalBuildInfo import Distribution.Version import Distribution.Utils.LogProgress import Distribution.Utils.MapAccum +import Distribution.Utils.Generic import Control.Monad import qualified Data.Set as Set @@ -191,7 +192,7 @@ toConfiguredComponent pkg_descr this_cid dep_map component = do , pn /= packageName pkg_descr , (cn, e) <- Map.toList comp_map , cn == CLibName ] - exe_deps = + exe_deps = ordNub $ [ exe | ExeDependency pn cn _ <- getAllToolDependencies pkg_descr bi -- The error suppression here is important, because in general diff --git a/Cabal/Distribution/Types/AnnotatedId.hs b/Cabal/Distribution/Types/AnnotatedId.hs index cc5991a3c..a424335fd 100644 --- a/Cabal/Distribution/Types/AnnotatedId.hs +++ b/Cabal/Distribution/Types/AnnotatedId.hs @@ -16,7 +16,7 @@ data AnnotatedId id = AnnotatedId { ann_cname :: ComponentName, ann_id :: id } - deriving (Show) + deriving (Show, Eq, Ord) instance Package (AnnotatedId id) where packageId = ann_pid This might help unblock someone. I've got a bigger patch coming with (1) lots of new debugging Pretty instances, (2) more asserts and (3) a regression test. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[ezyang]

Note that Pretty is meant to replace Text's disp. "Debugging Pretty instances" sounds suspicious, it's probably ok if the types won't be ever a type of cabal/project/etc -file field

I was hoping that the invariant was, if you have a "Pretty" instance but not a Parsec instance, then the Pretty instance is not intended to be machine-readable and if you ever did post-facto add a Parsec instance, you would fix the Pretty instance to match (which would be fine to change because no one was relying on it being parseable.)

If this is not the case, then I'll have to add a full parallel Outputable type class hierarchy to Pretty for non-roundtrippable printing. ;)

Also please avoid huge patches at all cost. It's PITA to review.

The plan is:

1. Add a bunch of debug Pretty instances for lots of types, which will be straightforward but fairly boiler-y platey (biggest addition of new code)
2. Add a new pprAssert which takes a Doc and a Bool and prints the Doc when the assert fails
3. Add a few new asserts
4. Add the 4 line patch which fixes the bug, plus a test case

I'll stage them all as separate patches to make it easier to review

[23Skidoo]

Big PRs are okay as long as they consist of a series of reasonably-sized patches that are more or less self-contained and easy to review one by one. #4889 is a good example of a big PR that is fairly easy to review.