Fix #4155 with pkg:libcomp in build-depends

Cabal/Cabal.cabal

@@ -307,8 +307,10 @@ library
     Distribution.Types.Dependency
     Distribution.Types.ExeDependency
     Distribution.Types.LegacyExeDependency
+    Distribution.Types.LibDependency
     Distribution.Types.PkgconfigDependency
     Distribution.Types.DependencyMap
+    Distribution.Types.LibDependencyMap
     Distribution.Types.ComponentId
     Distribution.Types.MungedPackageId
     Distribution.Types.PackageId

Cabal/Distribution/Backpack/ComponentsGraph.hs

@@ -16,9 +16,10 @@ import Distribution.PackageDescription as PD hiding (Flag)
 import Distribution.Simple.BuildToolDepends
 import Distribution.Simple.LocalBuildInfo
 import Distribution.Types.ComponentRequestedSpec
-import Distribution.Types.UnqualComponentName
+import Distribution.Types.LibDependency
 import Distribution.Compat.Graph (Graph, Node(..))
 import qualified Distribution.Compat.Graph as Graph
+import Distribution.Types.Mixin
 
 import Distribution.Text
     ( Text(disp) )
@@ -64,18 +65,16 @@ mkComponentsGraph enabled pkg_descr =
     -- The dependencies for the given component
     componentDeps component =
       (CExeName <$> getAllInternalToolDependencies pkg_descr bi)
-
-      ++ [ if pkgname == packageName pkg_descr
-           then CLibName
-           else CSubLibName toolname
-         | Dependency pkgname _ <- targetBuildDepends bi
-         , let toolname = packageNameToUnqualComponentName pkgname
-         , toolname `elem` internalPkgDeps ]
+      ++ mixin_deps
+      ++ [ maybe CLibName CSubLibName (libDepLibraryName ld)
+         | ld  <- targetBuildDepends bi
+         , libDepPackageName ld == packageName pkg_descr ]
       where
         bi = componentBuildInfo component
-        internalPkgDeps = map (conv . libName) (allLibraries pkg_descr)
-        conv Nothing = packageNameToUnqualComponentName $ packageName pkg_descr
-        conv (Just s) = s
+        mixin_deps =
+         [ maybe CLibName CSubLibName (mixinLibraryName mix)
+         | mix <- mixins bi
+         , mixinPackageName mix == packageName pkg_descr ]
 
 -- | Given the package description and a 'PackageDescription' (used
 -- to determine if a package name is internal or not), sort the

Cabal/Distribution/Backpack/ConfiguredComponent.hs

@@ -22,15 +22,14 @@ import Distribution.Compat.Prelude hiding ((<>))
 import Distribution.Backpack.Id
 
 import Distribution.Types.AnnotatedId
-import Distribution.Types.Dependency
+import Distribution.Types.LibDependency
 import Distribution.Types.ExeDependency
 import Distribution.Types.IncludeRenaming
 import Distribution.Types.ComponentId
 import Distribution.Types.PackageId
 import Distribution.Types.PackageName
 import Distribution.Types.Mixin
 import Distribution.Types.ComponentName
-import Distribution.Types.UnqualComponentName
 import Distribution.Types.ComponentInclude
 import Distribution.Package
 import Distribution.PackageDescription as PD hiding (Flag)
@@ -43,7 +42,6 @@ import Distribution.Utils.MapAccum
 import Distribution.Utils.Generic
 
 import Control.Monad
-import qualified Data.Set as Set
 import qualified Data.Map as Map
 import Distribution.Text
 import Text.PrettyPrint
@@ -96,102 +94,85 @@ dispConfiguredComponent cc =
                | incl <- cc_includes cc
                ])
 
--- | Construct a 'ConfiguredComponent', given that the 'ComponentId'
--- and library/executable dependencies are known.  The primary
--- work this does is handling implicit @backpack-include@ fields.
-mkConfiguredComponent
+-- | This is a mapping that keeps track of package-internal libraries
+-- and executables.  Although a component of the key is a general
+-- 'ComponentName', actually only 'CLib', 'CSubLib' and 'CExe' will ever
+-- be here.
+type ConfiguredComponentMap =
+        Map PackageName (Map ComponentName (AnnotatedId ComponentId))
+
+-- Executable map must be different because an executable can
+-- have the same name as a library. Ew.
+
+-- | Given some ambient environment of package names that
+-- are "in scope", looks at the 'BuildInfo' to decide
+-- what the packages actually resolve to, and then builds
+-- a 'ConfiguredComponent'.
+toConfiguredComponent
     :: PackageDescription
     -> ComponentId
-    -> [AnnotatedId ComponentId] -- lib deps
-    -> [AnnotatedId ComponentId] -- exe deps
+    -> ConfiguredComponentMap
     -> Component
     -> LogProgress ConfiguredComponent
-mkConfiguredComponent pkg_descr this_cid lib_deps exe_deps component = do
-    -- Resolve each @mixins@ into the actual dependency
-    -- from @lib_deps@.
-    explicit_includes <- forM (mixins bi) $ \(Mixin name rns) -> do
-        let keys = fixFakePkgName pkg_descr name
-        aid <- case Map.lookup keys deps_map of
-                Nothing ->
-                    dieProgress $
-                        text "Mix-in refers to non-existent package" <+>
-                        quotes (disp name) $$
-                        text "(did you forget to add the package to build-depends?)"
-                Just r  -> return r
+toConfiguredComponent pkg_descr this_cid dep_map component = do
+    let reg_lib_deps =
+            if newPackageDepsBehaviour pkg_descr
+            then
+                [ (pn, cn)
+                | LibDependency pn mb_ln _ <- targetBuildDepends bi
+                , let cn = libraryComponentName mb_ln ]
+            else
+                -- dep_map contains a mix of internal and external deps.
+                -- We want all the public libraries (dep_cn == CLibName)
+                -- of all external deps (dep /= pn).  Note that this
+                -- excludes the public library of the current package:
+                -- this is not supported by old-style deps behavior
+                -- because it would imply a cyclic dependency for the
+                -- library itself.
+                [ (pn, cn)
+                | (pn, comp_map) <- Map.toList dep_map
+                , pn /= packageName pkg_descr
+                , (cn, _) <- Map.toList comp_map
+                , cn == CLibName ]
+
+        reg_lib_map, mixin_map :: Map (PackageName, ComponentName) (IncludeRenaming, Bool)
+
+        reg_lib_map = Map.fromList $
+            reg_lib_deps `zip` repeat (defaultIncludeRenaming, True)
+
+        mixin_map = Map.fromList
+            [ ((pn, cn), (rns, False))
+            | Mixin pn mb_ln rns <- mixins bi
+            , let cn = libraryComponentName mb_ln ]
+
+        lib_deps = Map.toList $ reg_lib_map `Map.union` mixin_map
+
+    mixin_includes <- forM lib_deps $ \((pname, cname), (rns, implicit)) -> do
+        aid <- case Map.lookup cname =<< Map.lookup pname dep_map of
+            Nothing -> dieProgress $
+                text "Dependency on unbuildable" <+>
+                text (showComponentName cname) <+>
+                text "from" <+> disp pname
+            Just r  -> return r
         return ComponentInclude {
                 ci_ann_id   = aid,
                 ci_renaming = rns,
-                ci_implicit = False
+                ci_implicit = implicit
             }
 
-        -- Any @build-depends@ which is not explicitly mentioned in
-        -- @backpack-include@ is converted into an "implicit" include.
-    let used_explicitly = Set.fromList (map ci_id explicit_includes)
-        implicit_includes
-            = map (\aid -> ComponentInclude {
-                                ci_ann_id = aid,
-                                ci_renaming = defaultIncludeRenaming,
-                                ci_implicit = True
-                            })
-            $ filter (flip Set.notMember used_explicitly . ann_id) lib_deps
-
     return ConfiguredComponent {
             cc_ann_id = AnnotatedId {
                     ann_id = this_cid,
                     ann_pid = package pkg_descr,
                     ann_cname = componentName component
                 },
             cc_component = component,
-            cc_public = is_public,
+            cc_public = componentName component == CLibName,
             cc_exe_deps = exe_deps,
-            cc_includes = explicit_includes ++ implicit_includes
+            cc_includes = mixin_includes
         }
   where
     bi = componentBuildInfo component
-    deps_map = Map.fromList [ ((packageName dep, ann_cname dep), dep)
-                            | dep <- lib_deps ]
-    is_public = componentName component == CLibName
-
-type ConfiguredComponentMap =
-        Map PackageName (Map ComponentName (AnnotatedId ComponentId))
-
-toConfiguredComponent
-    :: PackageDescription
-    -> ComponentId
-    -> ConfiguredComponentMap
-    -> Component
-    -> LogProgress ConfiguredComponent
-toConfiguredComponent pkg_descr this_cid dep_map component = do
-    lib_deps <-
-        if newPackageDepsBehaviour pkg_descr
-            then forM (targetBuildDepends bi) $ \(Dependency name _) -> do
-                    let (pn, cn) = fixFakePkgName pkg_descr name
-                    value <- case Map.lookup cn =<< Map.lookup pn dep_map of
-                        Nothing ->
-                            dieProgress $
-                                text "Dependency on unbuildable" <+>
-                                text (showComponentName cn) <+>
-                                text "from" <+> disp pn
-                        Just v -> return v
-                    return value
-            else return old_style_lib_deps
-    mkConfiguredComponent
-       pkg_descr this_cid
-       lib_deps exe_deps component
-  where
-    bi = componentBuildInfo component
-    -- dep_map contains a mix of internal and external deps.
-    -- We want all the public libraries (dep_cn == CLibName)
-    -- of all external deps (dep /= pn).  Note that this
-    -- excludes the public library of the current package:
-    -- this is not supported by old-style deps behavior
-    -- because it would imply a cyclic dependency for the
-    -- library itself.
-    old_style_lib_deps = [ e
-                         | (pn, comp_map) <- Map.toList dep_map
-                         , pn /= packageName pkg_descr
-                         , (cn, e) <- Map.toList comp_map
-                         , cn == CLibName ]
     -- We have to nub here, because 'getAllToolDependencies' may return
     -- duplicates (see #4986).  (NB: This is not needed for lib_deps,
     -- since those elaborate into includes, for which there explicitly
@@ -282,16 +263,3 @@ newPackageDepsBehaviourMinVersion = mkVersion [1,7,1]
 newPackageDepsBehaviour :: PackageDescription -> Bool
 newPackageDepsBehaviour pkg =
    specVersion pkg >= newPackageDepsBehaviourMinVersion
-
--- | 'build-depends:' stanzas are currently ambiguous as the external packages
--- and internal libraries are specified the same. For now, we assume internal
--- libraries shadow, and this function disambiguates accordingly, but soon the
--- underlying ambiguity will be addressed.
-fixFakePkgName :: PackageDescription -> PackageName -> (PackageName, ComponentName)
-fixFakePkgName pkg_descr pn =
-  if subLibName `elem` internalLibraries
-  then (packageName pkg_descr, CSubLibName subLibName)
-  else (pn,                    CLibName)
-  where
-    subLibName = packageNameToUnqualComponentName pn
-    internalLibraries = mapMaybe libName (allLibraries pkg_descr)

Cabal/Distribution/PackageDescription/Check.hs

@@ -53,6 +53,7 @@ import Distribution.System
 import Distribution.Text
 import Distribution.Types.ComponentRequestedSpec
 import Distribution.Types.CondTree
+import Distribution.Types.LibDependency
 import Distribution.Types.ExeDependency
 import Distribution.Types.UnqualComponentName
 import Distribution.Utils.Generic                    (isAscii)
@@ -544,6 +545,11 @@ checkFields pkg =
         ++ ". This version range does not include the current package, and must "
         ++ "be removed as the current package's library will always be used."
 
+  , check (not (null depMissingInternalLibrary)) $
+      PackageBuildImpossible $
+           "The package depends on a missing internal library: "
+        ++ commaSep (map display depInternalExecutableWithImpossibleVersion)
+
   , check (not (null depInternalExecutableWithExtraVersion)) $
       PackageBuildWarning $
            "The package has an extraneous version range for a dependency on an "
@@ -586,17 +592,14 @@ checkFields pkg =
       | (compiler, vr) <- testedWith pkg
       , isNoVersion vr ]
 
-    internalLibraries =
-        map (maybe (packageName pkg) (unqualComponentNameToPackageName) . libName)
-            (allLibraries pkg)
+    internalLibraries = mapMaybe libName $ allLibraries pkg
 
     internalExecutables = map exeName $ executables pkg
 
     internalLibDeps =
       [ dep
-      | bi <- allBuildInfo pkg
-      , dep@(Dependency name _) <- targetBuildDepends bi
-      , name `elem` internalLibraries
+      | dep@(LibDependency name _ _) <- allBuildDepends pkg
+      , name == packageName pkg
       ]
 
     internalExeDeps =
@@ -608,17 +611,23 @@ checkFields pkg =
 
     depInternalLibraryWithExtraVersion =
       [ dep
-      | dep@(Dependency _ versionRange) <- internalLibDeps
+      | dep@(LibDependency _ _ versionRange) <- internalLibDeps
       , not $ isAnyVersion versionRange
       , packageVersion pkg `withinRange` versionRange
       ]
 
     depInternalLibraryWithImpossibleVersion =
       [ dep
-      | dep@(Dependency _ versionRange) <- internalLibDeps
+      | dep@(LibDependency _ _ versionRange) <- internalLibDeps
       , not $ packageVersion pkg `withinRange` versionRange
       ]
 
+    depMissingInternalLibrary =
+      [ dep
+      | dep@(LibDependency _ (Just lName) _) <- internalLibDeps
+      , not $ lName `elem` internalLibraries
+      ]
+
     depInternalExecutableWithExtraVersion =
       [ dep
       | dep@(ExeDependency _ _ versionRange) <- internalExeDeps
@@ -1201,7 +1210,7 @@ checkCabalVersion pkg =
       PackageDistInexcusable $
            "The package uses full version-range expressions "
         ++ "in a 'build-depends' field: "
-        ++ commaSep (map displayRawDependency versionRangeExpressions)
+        ++ commaSep (map displayRawLibDependency versionRangeExpressions)
         ++ ". To use this new syntax the package needs to specify at least "
         ++ "'cabal-version: >= 1.8'. Alternatively, if broader compatibility "
         ++ "is important, then convert to conjunctive normal form, and use "
@@ -1216,7 +1225,7 @@ checkCabalVersion pkg =
         ++ "'cabal-version: >= 1.6'. Alternatively, if broader compatibility "
         ++ "is important then use: " ++ commaSep
            [ display (Dependency name (eliminateWildcardSyntax versionRange))
-           | Dependency name versionRange <- depsUsingWildcardSyntax ]
+           | LibDependency name Nothing versionRange <- depsUsingWildcardSyntax ]
 
     -- check use of "build-depends: foo ^>= 1.2.3" syntax
   , checkVersion [2,0] (not (null depsUsingMajorBoundSyntax)) $
@@ -1227,8 +1236,8 @@ checkCabalVersion pkg =
         ++ ". To use this new syntax the package need to specify at least "
         ++ "'cabal-version: >= 2.0'. Alternatively, if broader compatibility "
         ++ "is important then use: " ++ commaSep
-           [ display (Dependency name (eliminateMajorBoundSyntax versionRange))
-           | Dependency name versionRange <- depsUsingMajorBoundSyntax ]
+           [ display (LibDependency name lname (eliminateMajorBoundSyntax versionRange))
+           | LibDependency name lname versionRange <- depsUsingMajorBoundSyntax ]
 
   , checkVersion [2,1] (any (not . null)
                         (concatMap buildInfoField
@@ -1363,7 +1372,7 @@ checkCabalVersion pkg =
       _                   -> False
 
     versionRangeExpressions =
-        [ dep | dep@(Dependency _ vr) <- allBuildDepends pkg
+        [ dep | dep@(LibDependency _ _ vr) <- allBuildDepends pkg
               , usesNewVersionRangeSyntax vr ]
 
     testedWithVersionRangeExpressions =
@@ -1391,10 +1400,11 @@ checkCabalVersion pkg =
         alg (VersionRangeParensF _) = 3
         alg _ = 1 :: Int
 
-    depsUsingWildcardSyntax = [ dep | dep@(Dependency _ vr) <- allBuildDepends pkg
-                                    , usesWildcardSyntax vr ]
+    depsUsingWildcardSyntax = [ dep
+                              | dep@(LibDependency _ _ vr) <- allBuildDepends pkg
+                              , usesWildcardSyntax vr ]
 
-    depsUsingMajorBoundSyntax = [ dep | dep@(Dependency _ vr) <- allBuildDepends pkg
+    depsUsingMajorBoundSyntax = [ dep | dep@(LibDependency _ _ vr) <- allBuildDepends pkg
                                       , usesMajorBoundSyntax vr ]
 
     usesBackpackIncludes = any (not . null . mixins) (allBuildInfo pkg)
@@ -1492,6 +1502,12 @@ displayRawDependency :: Dependency -> String
 displayRawDependency (Dependency pkg vr) =
   display pkg ++ " " ++ display vr
 
+displayRawLibDependency :: LibDependency -> String
+displayRawLibDependency (LibDependency pkg ml vr) =
+  display pkg
+  ++ ":lib:" ++ maybe (display pkg) display ml
+  ++ " " ++ display vr
+
 
 -- ------------------------------------------------------------
 -- * Checks on the GenericPackageDescription
@@ -1541,7 +1557,7 @@ checkPackageVersions pkg =
           foldr intersectVersionRanges anyVersion baseDeps
         where
           baseDeps =
-            [ vr | Dependency pname vr <- allBuildDepends pkg'
+            [ vr | LibDependency pname _ vr <- allBuildDepends pkg'
                  , pname == mkPackageName "base" ]
 
       -- Just in case finalizePD fails for any reason,