From 115fcd2c55e340c2e0ae82c6af4552436229975d Mon Sep 17 00:00:00 2001 From: Phil de Joux Date: Tue, 13 Feb 2024 14:10:39 -0500 Subject: [PATCH 1/7] Warn on duplicate non-cyclical project imports - Add Y-forking import test - A test for detecting when the same config is imported via many different paths - Error on duplicate imports - Do the filtering in duplicateImportMsg - Use duplicateImportMsg for cycles too - Add haddocks to IORef parameter - Add changelog entry - Use ordNub instead of nub - Use NubList - Share implement of duplicate and cyclical messages - Update expectation for non-cyclical duplicate import - Only show a warning - Add woops project with a time cost - Use noticeDoc instead of warn - Render duplicate imports - Add Ord instance for Dupes, sort on dupesNormLocPath - Fixups after rebase - Satisfy hlint - Remove -XMultiWayIf - Remove mention of yops from the changelog - Satisfy fix-whitespace - Test with a time cost of duplicate imports --- .../Solver/Types/ProjectConfigPath.hs | 18 +- .../Client/ProjectConfig/Legacy.hs | 68 ++- .../ConditionalAndImport/cabal.out | 446 ++++++++++++++++-- .../ConditionalAndImport/cabal.test.hs | 10 +- .../ConditionalAndImport/woops-0.project | 7 + .../ConditionalAndImport/woops-2.config | 2 + .../ConditionalAndImport/woops-4.config | 2 + .../ConditionalAndImport/woops-6.config | 2 + .../ConditionalAndImport/woops-8.config | 2 + .../ConditionalAndImport/woops/woops-1.config | 2 + .../ConditionalAndImport/woops/woops-3.config | 2 + .../ConditionalAndImport/woops/woops-5.config | 2 + .../ConditionalAndImport/woops/woops-7.config | 2 + .../ConditionalAndImport/woops/woops-9.config | 2 + changelog.d/pr-9933 | 23 + 15 files changed, 537 insertions(+), 53 deletions(-) create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/woops-0.project create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/woops-2.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/woops-4.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/woops-6.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/woops-8.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-1.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-3.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-5.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-7.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-9.config create mode 100644 changelog.d/pr-9933 diff --git a/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs b/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs index 17543b5f2de..21e3a0e8ef6 100644 --- a/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs +++ b/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs @@ -14,6 +14,7 @@ module Distribution.Solver.Types.ProjectConfigPath , docProjectConfigPath , docProjectConfigFiles , cyclicalImportMsg + , duplicateImportMsg , untrimmedUriImportMsg , docProjectConfigPathFailReason @@ -174,9 +175,24 @@ docProjectConfigFiles ps = vcat -- | A message for a cyclical import, a "cyclical import of". cyclicalImportMsg :: ProjectConfigPath -> Doc cyclicalImportMsg path@(ProjectConfigPath (duplicate :| _)) = + seenImportMsg (text "cyclical import of" <+> text duplicate <> semi) duplicate path [] + +-- | A message for a duplicate import, a "duplicate import of". If a check for +-- cyclical imports has already been made then this would report a duplicate +-- import by two different paths. +duplicateImportMsg :: Doc -> FilePath -> ProjectConfigPath -> [(FilePath, ProjectConfigPath)] -> Doc +duplicateImportMsg intro = seenImportMsg intro + +seenImportMsg :: Doc -> FilePath -> ProjectConfigPath -> [(FilePath, ProjectConfigPath)] -> Doc +seenImportMsg intro duplicate path seenImportsBy = vcat - [ text "cyclical import of" <+> text duplicate <> semi + [ intro , nest 2 (docProjectConfigPath path) + , nest 2 $ + vcat + [ docProjectConfigPath dib + | (_, dib) <- filter ((duplicate ==) . fst) seenImportsBy + ] ] -- | A message for an import that has leading or trailing spaces. diff --git a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs index 10858d5601d..ee175d72ce3 100644 --- a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs +++ b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs @@ -1,7 +1,6 @@ {-# LANGUAGE ConstraintKinds #-} {-# LANGUAGE DataKinds #-} {-# LANGUAGE DeriveGeneric #-} -{-# LANGUAGE LambdaCase #-} {-# LANGUAGE NamedFieldPuns #-} {-# LANGUAGE RecordWildCards #-} {-# LANGUAGE ScopedTypeVariables #-} @@ -35,6 +34,7 @@ module Distribution.Client.ProjectConfig.Legacy ) where import Data.Coerce (coerce) +import Data.IORef import Distribution.Client.Compat.Prelude import Distribution.Types.Flag (FlagName, parsecFlagAssignment) @@ -142,7 +142,8 @@ import Distribution.Types.CondTree ) import Distribution.Types.SourceRepo (RepoType) import Distribution.Utils.NubList - ( fromNubList + ( NubList + , fromNubList , overNubList , toNubList ) @@ -194,7 +195,7 @@ import Distribution.Utils.Path hiding ) import qualified Data.ByteString.Char8 as BS -import Data.Functor ((<&>)) +import Data.List (sortOn) import qualified Data.Map as Map import qualified Data.Set as Set import Network.URI (URI (..), nullURIAuth, parseURI) @@ -203,9 +204,12 @@ import System.FilePath (isAbsolute, isPathSeparator, makeValid, splitFileName, ( import Text.PrettyPrint ( Doc , render + , semi + , text + , vcat , ($+$) ) -import qualified Text.PrettyPrint as Disp +import qualified Text.PrettyPrint as Disp (empty, int, render, text) ------------------------------------------------------------------ -- Handle extended project config files with conditionals and imports. @@ -256,19 +260,43 @@ parseProject -> ProjectConfigToParse -- ^ The contents of the file to parse -> IO (ProjectParseResult ProjectConfigSkeleton) -parseProject rootPath cacheDir httpTransport verbosity configToParse = - do - let (dir, projectFileName) = splitFileName rootPath - projectDir <- makeAbsolute dir - projectPath <- canonicalizeConfigPath projectDir (ProjectConfigPath $ projectFileName :| []) - parseProjectSkeleton cacheDir httpTransport verbosity projectDir projectPath configToParse - -- NOTE: Reverse the warnings so they are in line number order. - <&> \case ProjectParseOk ws x -> ProjectParseOk (reverse ws) x; x -> x +parseProject rootPath cacheDir httpTransport verbosity configToParse = do + let (dir, projectFileName) = splitFileName rootPath + projectDir <- makeAbsolute dir + projectPath@(ProjectConfigPath (canonicalRoot :| _)) <- canonicalizeConfigPath projectDir (ProjectConfigPath $ projectFileName :| []) + importsBy <- newIORef $ toNubList [(canonicalRoot, projectPath)] + dupesMap <- newIORef mempty + result <- parseProjectSkeleton cacheDir httpTransport verbosity importsBy dupesMap projectDir projectPath configToParse + dupes <- Map.filter ((> 1) . length) <$> readIORef dupesMap + unless (Map.null dupes) (noticeDoc verbosity $ vcat (dupesMsg <$> Map.toList dupes)) + return result + +data Dupes = Dupes + { dupesUniqueImport :: FilePath + , dupesNormLocPath :: ProjectConfigPath + , dupesSeenImportsBy :: [(FilePath, ProjectConfigPath)] + } + deriving (Eq) + +instance Ord Dupes where + compare = compare `on` length . dupesSeenImportsBy + +type DupesMap = Map FilePath [Dupes] + +dupesMsg :: (FilePath, [Dupes]) -> Doc +dupesMsg (duplicate, ds@(take 1 . sortOn dupesNormLocPath -> dupes)) = + vcat $ + ((text "Warning:" <+> Disp.int (length ds) <+> text "imports of" <+> text duplicate) <> semi) + : ((\Dupes{..} -> duplicateImportMsg Disp.empty dupesUniqueImport dupesNormLocPath dupesSeenImportsBy) <$> dupes) parseProjectSkeleton :: FilePath -> HttpTransport -> Verbosity + -> IORef (NubList (FilePath, ProjectConfigPath)) + -- ^ The imports seen so far, used to report on cycles and duplicates and to detect duplicates that are not cycles + -> IORef DupesMap + -- ^ The duplicates seen so far, used to defer reporting on duplicates -> FilePath -- ^ The directory of the project configuration, typically the directory of cabal.project -> ProjectConfigPath @@ -276,7 +304,7 @@ parseProjectSkeleton -> ProjectConfigToParse -- ^ The contents of the file to parse -> IO (ProjectParseResult ProjectConfigSkeleton) -parseProjectSkeleton cacheDir httpTransport verbosity projectDir source (ProjectConfigToParse bs) = +parseProjectSkeleton cacheDir httpTransport verbosity importsBy dupesMap projectDir source (ProjectConfigToParse bs) = (sanityWalkPCS False =<<) <$> liftPR source (go []) (ParseUtils.readFields bs) where go :: [ParseUtils.Field] -> [ParseUtils.Field] -> IO (ProjectParseResult ProjectConfigSkeleton) @@ -284,10 +312,14 @@ parseProjectSkeleton cacheDir httpTransport verbosity projectDir source (Project (ParseUtils.F _ "import" importLoc) -> do let importLocPath = importLoc `consProjectConfigPath` source - -- Once we canonicalize the import path, we can check for cyclical imports + -- Once we canonicalize the import path, we can check for cyclical and duplicate imports normSource <- canonicalizeConfigPath projectDir source - normLocPath <- canonicalizeConfigPath projectDir importLocPath + normLocPath@(ProjectConfigPath (uniqueImport :| _)) <- canonicalizeConfigPath projectDir importLocPath + seenImportsBy@(fmap fst -> seenImports) <- fromNubList <$> atomicModifyIORef' importsBy (\ibs -> (toNubList [(uniqueImport, normLocPath)] <> ibs, ibs)) debug verbosity $ "\nimport path, normalized\n=======================\n" ++ render (docProjectConfigPath normLocPath) + debug verbosity "\nseen unique paths\n=================" + mapM_ (debug verbosity) seenImports + debug verbosity "\n" if isCyclicConfigPath normLocPath then pure . projectParseFail Nothing (Just normSource) $ ParseUtils.FromString (render $ cyclicalImportMsg normLocPath) Nothing @@ -296,8 +328,10 @@ parseProjectSkeleton cacheDir httpTransport verbosity projectDir source (Project (isUntrimmedUriConfigPath importLocPath) (noticeDoc verbosity $ untrimmedUriImportMsg (Disp.text "Warning:") importLocPath) let fs = (\z -> CondNode z [normLocPath] mempty) <$> fieldsToConfig normSource (reverse acc) - res <- parseProjectSkeleton cacheDir httpTransport verbosity projectDir importLocPath . ProjectConfigToParse =<< fetchImportConfig normLocPath - rest <- go [] xs + let uniqueFields = if uniqueImport `elem` seenImports then [] else xs + atomicModifyIORef' dupesMap $ \dm -> (Map.insertWith (++) uniqueImport [Dupes uniqueImport normLocPath seenImportsBy] dm, ()) + res <- parseProjectSkeleton cacheDir httpTransport verbosity importsBy dupesMap projectDir importLocPath . ProjectConfigToParse =<< fetchImportConfig normLocPath + rest <- go [] uniqueFields pure . fmap mconcat . sequence $ [projectParse Nothing normSource fs, res, rest] (ParseUtils.Section l "if" p xs') -> do normSource <- canonicalizeConfigPath projectDir source diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.out b/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.out index 2ddeff77281..3f7b8d6c539 100644 --- a/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.out +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.out @@ -254,43 +254,123 @@ Could not resolve dependencies: (constraint from oops-0.project requires ==1.4.3.0) [__1] fail (backjumping, conflict set: hashable, oops) After searching the rest of the dependency tree exhaustively, these were the goals I've had most trouble fulfilling: hashable (3), oops (2) -# checking if we detect when the same config is imported via many different paths (we don't) +# checking that we detect when the same config is imported via many different paths # cabal v2-build -Configuration is affected by the following files: -- yops-0.project -- yops-2.config +Warning: 2 imports of yops-4.config; + yops-4.config + imported by: yops/yops-3.config + imported by: yops-0.project + yops-4.config + imported by: yops/yops-3.config + imported by: yops-2.config imported by: yops/yops-1.config imported by: yops-0.project -- yops-4.config +Warning: 2 imports of yops-6.config; + yops-6.config + imported by: yops/yops-5.config + imported by: yops-4.config imported by: yops/yops-3.config imported by: yops-0.project -- yops-4.config + yops-6.config + imported by: yops/yops-5.config + imported by: yops-4.config imported by: yops/yops-3.config imported by: yops-2.config imported by: yops/yops-1.config imported by: yops-0.project -- yops-6.config +Warning: 2 imports of yops-8.config; + yops-8.config + imported by: yops/yops-7.config + imported by: yops-6.config imported by: yops/yops-5.config + imported by: yops-4.config + imported by: yops/yops-3.config imported by: yops-0.project -- yops-6.config + yops-8.config + imported by: yops/yops-7.config + imported by: yops-6.config imported by: yops/yops-5.config imported by: yops-4.config imported by: yops/yops-3.config + imported by: yops-2.config + imported by: yops/yops-1.config imported by: yops-0.project -- yops-6.config +Warning: 2 imports of yops/yops-3.config; + yops/yops-3.config + imported by: yops-0.project + yops/yops-3.config + imported by: yops-2.config + imported by: yops/yops-1.config + imported by: yops-0.project +Warning: 2 imports of yops/yops-5.config; + yops/yops-5.config + imported by: yops-4.config + imported by: yops/yops-3.config + imported by: yops-0.project + yops/yops-5.config + imported by: yops-4.config + imported by: yops/yops-3.config + imported by: yops-2.config + imported by: yops/yops-1.config + imported by: yops-0.project +Warning: 2 imports of yops/yops-7.config; + yops/yops-7.config + imported by: yops-6.config + imported by: yops/yops-5.config + imported by: yops-4.config + imported by: yops/yops-3.config + imported by: yops-0.project + yops/yops-7.config + imported by: yops-6.config imported by: yops/yops-5.config imported by: yops-4.config imported by: yops/yops-3.config imported by: yops-2.config imported by: yops/yops-1.config imported by: yops-0.project -- yops-8.config +Warning: 2 imports of yops/yops-9.config; + yops/yops-9.config + imported by: yops-8.config imported by: yops/yops-7.config + imported by: yops-6.config + imported by: yops/yops-5.config + imported by: yops-4.config + imported by: yops/yops-3.config imported by: yops-0.project -- yops-8.config + yops/yops-9.config + imported by: yops-8.config imported by: yops/yops-7.config imported by: yops-6.config imported by: yops/yops-5.config + imported by: yops-4.config + imported by: yops/yops-3.config + imported by: yops-2.config + imported by: yops/yops-1.config + imported by: yops-0.project +Configuration is affected by the following files: +- yops-0.project +- yops-2.config + imported by: yops/yops-1.config + imported by: yops-0.project +- yops-4.config + imported by: yops/yops-3.config + imported by: yops-0.project +- yops-4.config + imported by: yops/yops-3.config + imported by: yops-2.config + imported by: yops/yops-1.config + imported by: yops-0.project +- yops-6.config + imported by: yops/yops-5.config + imported by: yops-4.config + imported by: yops/yops-3.config + imported by: yops-0.project +- yops-6.config + imported by: yops/yops-5.config + imported by: yops-4.config + imported by: yops/yops-3.config + imported by: yops-2.config + imported by: yops/yops-1.config imported by: yops-0.project - yops-8.config imported by: yops/yops-7.config @@ -316,8 +396,6 @@ Configuration is affected by the following files: imported by: yops-2.config imported by: yops/yops-1.config imported by: yops-0.project -- yops/yops-5.config - imported by: yops-0.project - yops/yops-5.config imported by: yops-4.config imported by: yops/yops-3.config @@ -328,12 +406,6 @@ Configuration is affected by the following files: imported by: yops-2.config imported by: yops/yops-1.config imported by: yops-0.project -- yops/yops-7.config - imported by: yops-0.project -- yops/yops-7.config - imported by: yops-6.config - imported by: yops/yops-5.config - imported by: yops-0.project - yops/yops-7.config imported by: yops-6.config imported by: yops/yops-5.config @@ -348,18 +420,6 @@ Configuration is affected by the following files: imported by: yops-2.config imported by: yops/yops-1.config imported by: yops-0.project -- yops/yops-9.config - imported by: yops-0.project -- yops/yops-9.config - imported by: yops-8.config - imported by: yops/yops-7.config - imported by: yops-0.project -- yops/yops-9.config - imported by: yops-8.config - imported by: yops/yops-7.config - imported by: yops-6.config - imported by: yops/yops-5.config - imported by: yops-0.project - yops/yops-9.config imported by: yops-8.config imported by: yops/yops-7.config @@ -379,6 +439,330 @@ Configuration is affected by the following files: imported by: yops/yops-1.config imported by: yops-0.project Up to date +# checking that we detect when the same config is imported via many different paths +# cabal v2-build +Warning: 10 imports of https://www.stackage.org/lts-21.25/cabal.config; + https://www.stackage.org/lts-21.25/cabal.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project + https://www.stackage.org/lts-21.25/cabal.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project + https://www.stackage.org/lts-21.25/cabal.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project + https://www.stackage.org/lts-21.25/cabal.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project + https://www.stackage.org/lts-21.25/cabal.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project + https://www.stackage.org/lts-21.25/cabal.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project + https://www.stackage.org/lts-21.25/cabal.config + imported by: woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project + https://www.stackage.org/lts-21.25/cabal.config + imported by: woops/woops-9.config + imported by: woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +Warning: 2 imports of woops-4.config; + woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project + woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +Warning: 2 imports of woops-6.config; + woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project + woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +Warning: 2 imports of woops-8.config; + woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project + woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +Warning: 2 imports of woops/woops-3.config; + woops/woops-3.config + imported by: woops-0.project + woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +Warning: 2 imports of woops/woops-5.config; + woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project + woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +Warning: 2 imports of woops/woops-7.config; + woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project + woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +Warning: 2 imports of woops/woops-9.config; + woops/woops-9.config + imported by: woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project + woops/woops-9.config + imported by: woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +Configuration is affected by the following files: +- woops-0.project +- woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project +- woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project +- woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project +- woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- woops/woops-1.config + imported by: woops-0.project +- woops/woops-3.config + imported by: woops-0.project +- woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project +- woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project +- woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- woops/woops-9.config + imported by: woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project +- woops/woops-9.config + imported by: woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- https://www.stackage.org/lts-21.25/cabal.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- https://www.stackage.org/lts-21.25/cabal.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- https://www.stackage.org/lts-21.25/cabal.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- https://www.stackage.org/lts-21.25/cabal.config + imported by: woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- https://www.stackage.org/lts-21.25/cabal.config + imported by: woops/woops-1.config + imported by: woops-0.project +- https://www.stackage.org/lts-21.25/cabal.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- https://www.stackage.org/lts-21.25/cabal.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- https://www.stackage.org/lts-21.25/cabal.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +- https://www.stackage.org/lts-21.25/cabal.config + imported by: woops/woops-9.config + imported by: woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-0.project +- https://www.stackage.org/lts-21.25/cabal.config + imported by: woops/woops-9.config + imported by: woops-8.config + imported by: woops/woops-7.config + imported by: woops-6.config + imported by: woops/woops-5.config + imported by: woops-4.config + imported by: woops/woops-3.config + imported by: woops-2.config + imported by: woops/woops-1.config + imported by: woops-0.project +Resolving dependencies... +Build profile: -w ghc-9.4.8 -O1 +In order, the following will be built: + - my-0.1 (lib:my) (first run) +Configuring my-0.1... +Preprocessing library for my-0.1... +Building library for my-0.1... # checking bad conditional # cabal v2-build Error: [Cabal-7090] diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs b/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs index 681865df2e5..c1eb3f338cf 100644 --- a/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs @@ -159,11 +159,13 @@ main = cabalTest . withRepo "repo" . recordMode RecordMarked $ do -- +-- yops-8.config -- +-- yops/yops-9.config (no further imports) -- +-- yops/yops-9.config (no further imports) - -- - -- We don't check and don't error or warn on the same config being imported - -- via many different paths. - log "checking if we detect when the same config is imported via many different paths (we don't)" + log "checking that we detect when the same config is imported via many different paths" yopping <- cabal' "v2-build" [ "--project-file=yops-0.project" ] + assertOutputContains "Warning: 2 imports" yopping + + log "checking that we detect when the same config is imported via many different paths" + wooping <- cabal' "v2-build" [ "--project-file=woops-0.project" ] + assertOutputContains "Warning: 10 imports" wooping log "checking bad conditional" badIf <- fails $ cabal' "v2-build" [ "--project-file=bad-conditional.project" ] diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/woops-0.project b/cabal-testsuite/PackageTests/ConditionalAndImport/woops-0.project new file mode 100644 index 00000000000..79933c8c1cb --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/woops-0.project @@ -0,0 +1,7 @@ +packages: . + +import: woops/woops-1.config +import: woops/woops-3.config +import: woops/woops-5.config +import: woops/woops-7.config +import: woops/woops-9.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/woops-2.config b/cabal-testsuite/PackageTests/ConditionalAndImport/woops-2.config new file mode 100644 index 00000000000..50deddaaef5 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/woops-2.config @@ -0,0 +1,2 @@ +import: woops/woops-3.config +import: https://www.stackage.org/lts-21.25/cabal.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/woops-4.config b/cabal-testsuite/PackageTests/ConditionalAndImport/woops-4.config new file mode 100644 index 00000000000..6ff8dfb3013 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/woops-4.config @@ -0,0 +1,2 @@ +import: woops/woops-5.config +import: https://www.stackage.org/lts-21.25/cabal.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/woops-6.config b/cabal-testsuite/PackageTests/ConditionalAndImport/woops-6.config new file mode 100644 index 00000000000..f32758b83e4 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/woops-6.config @@ -0,0 +1,2 @@ +import: woops/woops-7.config +import: https://www.stackage.org/lts-21.25/cabal.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/woops-8.config b/cabal-testsuite/PackageTests/ConditionalAndImport/woops-8.config new file mode 100644 index 00000000000..b9043ce7c5d --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/woops-8.config @@ -0,0 +1,2 @@ +import: woops/woops-9.config +import: https://www.stackage.org/lts-21.25/cabal.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-1.config b/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-1.config new file mode 100644 index 00000000000..1151046199a --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-1.config @@ -0,0 +1,2 @@ +import: ../woops-2.config +import: https://www.stackage.org/lts-21.25/cabal.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-3.config b/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-3.config new file mode 100644 index 00000000000..9bbcedeb506 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-3.config @@ -0,0 +1,2 @@ +import: ../woops-4.config +import: https://www.stackage.org/lts-21.25/cabal.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-5.config b/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-5.config new file mode 100644 index 00000000000..181577c4dfe --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-5.config @@ -0,0 +1,2 @@ +import: ../woops-6.config +import: https://www.stackage.org/lts-21.25/cabal.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-7.config b/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-7.config new file mode 100644 index 00000000000..c2d9821826a --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-7.config @@ -0,0 +1,2 @@ +import: ../woops-8.config +import: https://www.stackage.org/lts-21.25/cabal.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-9.config b/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-9.config new file mode 100644 index 00000000000..44d1cc5e562 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/woops/woops-9.config @@ -0,0 +1,2 @@ +-- No imports here +import: https://www.stackage.org/lts-21.25/cabal.config diff --git a/changelog.d/pr-9933 b/changelog.d/pr-9933 new file mode 100644 index 00000000000..3e94a44609a --- /dev/null +++ b/changelog.d/pr-9933 @@ -0,0 +1,23 @@ +synopsis: Detect non-cyclical duplicate project imports +description: + Detect and report on duplicate imports that are non-cyclical. Give more detail + when reporting cyclical imports. Be more explicit and consistent with + non-cyclical duplicate reporting. + + ``` + $ cabal build --project-file=cabal.project + ... + Error: [Cabal-7090] + Error parsing project file cabal.project: + duplicate import of config/config-3.config; + config/config-3.config + imported by: cabal.project + config/config-3.config + imported by: config-2.config + imported by: config/config-1.config + imported by: cabal.project + ``` + +packages: cabal-install-solver cabal-install +prs: #9578 #9933 +issues: #9562 From b61e03133035b58e7c537e5efbed002b1048841d Mon Sep 17 00:00:00 2001 From: Phil de Joux Date: Sat, 26 Apr 2025 20:49:59 -0400 Subject: [PATCH 2/7] Fewer imports from PrettyPrint qualified as Disp --- .../Client/ProjectConfig/Legacy.hs | 47 ++++++++----------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs index ee175d72ce3..550ac58ca06 100644 --- a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs +++ b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs @@ -201,15 +201,8 @@ import qualified Data.Set as Set import Network.URI (URI (..), nullURIAuth, parseURI) import System.Directory (createDirectoryIfMissing, makeAbsolute) import System.FilePath (isAbsolute, isPathSeparator, makeValid, splitFileName, ()) -import Text.PrettyPrint - ( Doc - , render - , semi - , text - , vcat - , ($+$) - ) -import qualified Text.PrettyPrint as Disp (empty, int, render, text) +import Text.PrettyPrint (Doc, int, render, semi, text, vcat, ($+$)) +import qualified Text.PrettyPrint as Disp (empty) ------------------------------------------------------------------ -- Handle extended project config files with conditionals and imports. @@ -286,7 +279,7 @@ type DupesMap = Map FilePath [Dupes] dupesMsg :: (FilePath, [Dupes]) -> Doc dupesMsg (duplicate, ds@(take 1 . sortOn dupesNormLocPath -> dupes)) = vcat $ - ((text "Warning:" <+> Disp.int (length ds) <+> text "imports of" <+> text duplicate) <> semi) + ((text "Warning:" <+> int (length ds) <+> text "imports of" <+> text duplicate) <> semi) : ((\Dupes{..} -> duplicateImportMsg Disp.empty dupesUniqueImport dupesNormLocPath dupesSeenImportsBy) <$> dupes) parseProjectSkeleton @@ -326,7 +319,7 @@ parseProjectSkeleton cacheDir httpTransport verbosity importsBy dupesMap project else do when (isUntrimmedUriConfigPath importLocPath) - (noticeDoc verbosity $ untrimmedUriImportMsg (Disp.text "Warning:") importLocPath) + (noticeDoc verbosity $ untrimmedUriImportMsg (text "Warning:") importLocPath) let fs = (\z -> CondNode z [normLocPath] mempty) <$> fieldsToConfig normSource (reverse acc) let uniqueFields = if uniqueImport `elem` seenImports then [] else xs atomicModifyIORef' dupesMap $ \dm -> (Map.insertWith (++) uniqueImport [Dupes uniqueImport normLocPath seenImportsBy] dm, ()) @@ -1324,13 +1317,13 @@ parseLegacyProjectConfig rootConfig bs = showLegacyProjectConfig :: LegacyProjectConfig -> String showLegacyProjectConfig config = - Disp.render $ + render $ showConfig (legacyProjectConfigFieldDescrs constraintSrc) legacyPackageConfigSectionDescrs legacyPackageConfigFGSectionDescrs config - $+$ Disp.text "" + $+$ text "" where -- Note: ConstraintSource is unused when pretty-printing. We fake -- it here to avoid having to pass it on call-sites. It's not great @@ -1341,13 +1334,13 @@ legacyProjectConfigFieldDescrs :: ConstraintSource -> [FieldDescr LegacyProjectC legacyProjectConfigFieldDescrs constraintSrc = [ newLineListField "packages" - (Disp.text . renderPackageLocationToken) + (text . renderPackageLocationToken) parsePackageLocationTokenQ legacyPackages (\v flags -> flags{legacyPackages = v}) , newLineListField "optional-packages" - (Disp.text . renderPackageLocationToken) + (text . renderPackageLocationToken) parsePackageLocationTokenQ legacyPackagesOptional (\v flags -> flags{legacyPackagesOptional = v}) @@ -1458,7 +1451,7 @@ legacySharedConfigFieldDescrs constraintSrc = . addFields [ commaNewLineListFieldParsec "package-dbs" - (Disp.text . showPackageDb) + (text . showPackageDb) (fmap readPackageDb parsecToken) configPackageDBs (\v conf -> conf{configPackageDBs = v}) @@ -1751,8 +1744,8 @@ legacyPackageConfigFieldDescrs = in FieldDescr name ( \f -> case f of - Flag NoDumpBuildInfo -> Disp.text "False" - Flag DumpBuildInfo -> Disp.text "True" + Flag NoDumpBuildInfo -> text "False" + Flag DumpBuildInfo -> text "True" _ -> Disp.empty ) ( \line str _ -> case () of @@ -1779,9 +1772,9 @@ legacyPackageConfigFieldDescrs = in FieldDescr name ( \f -> case f of - Flag NoOptimisation -> Disp.text "False" - Flag NormalOptimisation -> Disp.text "True" - Flag MaximumOptimisation -> Disp.text "2" + Flag NoOptimisation -> text "False" + Flag NormalOptimisation -> text "True" + Flag MaximumOptimisation -> text "2" _ -> Disp.empty ) ( \line str _ -> case () of @@ -1804,10 +1797,10 @@ legacyPackageConfigFieldDescrs = in FieldDescr name ( \f -> case f of - Flag NoDebugInfo -> Disp.text "False" - Flag MinimalDebugInfo -> Disp.text "1" - Flag NormalDebugInfo -> Disp.text "True" - Flag MaximalDebugInfo -> Disp.text "3" + Flag NoDebugInfo -> text "False" + Flag MinimalDebugInfo -> text "1" + Flag NormalDebugInfo -> text "True" + Flag MaximalDebugInfo -> text "3" _ -> Disp.empty ) ( \line str _ -> case () of @@ -2132,6 +2125,6 @@ monoidFieldParsec name showF readF get' set = -- otherwise are special syntax. showTokenQ :: String -> Doc showTokenQ "" = Disp.empty -showTokenQ x@('-' : '-' : _) = Disp.text (show x) -showTokenQ x@('.' : []) = Disp.text (show x) +showTokenQ x@('-' : '-' : _) = text (show x) +showTokenQ x@('.' : []) = text (show x) showTokenQ x = showToken x From 1accbca6149dcc41a26a91edacf976cb87974b7c Mon Sep 17 00:00:00 2001 From: Phil de Joux Date: Sat, 26 Apr 2025 21:21:41 -0400 Subject: [PATCH 3/7] Add data ProjectImport replacing tuples --- .../Solver/Types/ProjectConfigPath.hs | 21 +++++++++++++------ .../Client/ProjectConfig/Legacy.hs | 8 +++---- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs b/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs index 21e3a0e8ef6..8d8cbc2bab1 100644 --- a/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs +++ b/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs @@ -1,10 +1,12 @@ {-# LANGUAGE DeriveGeneric #-} +{-# LANGUAGE NamedFieldPuns #-} {-# LANGUAGE ViewPatterns #-} module Distribution.Solver.Types.ProjectConfigPath ( -- * Project Config Path Manipulation - ProjectConfigPath(..) + ProjectImport(..) + , ProjectConfigPath(..) , projectConfigPathRoot , nullProjectConfigPath , consProjectConfigPath @@ -45,6 +47,13 @@ import Text.PrettyPrint import Distribution.Simple.Utils (ordNub) import Distribution.System (OS(Windows), buildOS) +data ProjectImport = + ProjectImport + { importOf :: FilePath + , importBy :: ProjectConfigPath + } + deriving (Eq, Ord) + -- | Path to a configuration file, either a singleton project root, or a longer -- list representing a path to an import. The path is a non-empty list that we -- build up by prepending relative imports with @consProjectConfigPath@. @@ -180,18 +189,18 @@ cyclicalImportMsg path@(ProjectConfigPath (duplicate :| _)) = -- | A message for a duplicate import, a "duplicate import of". If a check for -- cyclical imports has already been made then this would report a duplicate -- import by two different paths. -duplicateImportMsg :: Doc -> FilePath -> ProjectConfigPath -> [(FilePath, ProjectConfigPath)] -> Doc +duplicateImportMsg :: Doc -> FilePath -> ProjectConfigPath -> [ProjectImport] -> Doc duplicateImportMsg intro = seenImportMsg intro -seenImportMsg :: Doc -> FilePath -> ProjectConfigPath -> [(FilePath, ProjectConfigPath)] -> Doc -seenImportMsg intro duplicate path seenImportsBy = +seenImportMsg :: Doc -> FilePath -> ProjectConfigPath -> [ProjectImport] -> Doc +seenImportMsg intro duplicate path seenImports = vcat [ intro , nest 2 (docProjectConfigPath path) , nest 2 $ vcat - [ docProjectConfigPath dib - | (_, dib) <- filter ((duplicate ==) . fst) seenImportsBy + [ docProjectConfigPath importBy + | ProjectImport{importBy} <- filter ((duplicate ==) . importOf) seenImports ] ] diff --git a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs index 550ac58ca06..1cc031ceb80 100644 --- a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs +++ b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs @@ -257,7 +257,7 @@ parseProject rootPath cacheDir httpTransport verbosity configToParse = do let (dir, projectFileName) = splitFileName rootPath projectDir <- makeAbsolute dir projectPath@(ProjectConfigPath (canonicalRoot :| _)) <- canonicalizeConfigPath projectDir (ProjectConfigPath $ projectFileName :| []) - importsBy <- newIORef $ toNubList [(canonicalRoot, projectPath)] + importsBy <- newIORef $ toNubList [ProjectImport canonicalRoot projectPath] dupesMap <- newIORef mempty result <- parseProjectSkeleton cacheDir httpTransport verbosity importsBy dupesMap projectDir projectPath configToParse dupes <- Map.filter ((> 1) . length) <$> readIORef dupesMap @@ -267,7 +267,7 @@ parseProject rootPath cacheDir httpTransport verbosity configToParse = do data Dupes = Dupes { dupesUniqueImport :: FilePath , dupesNormLocPath :: ProjectConfigPath - , dupesSeenImportsBy :: [(FilePath, ProjectConfigPath)] + , dupesSeenImportsBy :: [ProjectImport] } deriving (Eq) @@ -286,7 +286,7 @@ parseProjectSkeleton :: FilePath -> HttpTransport -> Verbosity - -> IORef (NubList (FilePath, ProjectConfigPath)) + -> IORef (NubList ProjectImport) -- ^ The imports seen so far, used to report on cycles and duplicates and to detect duplicates that are not cycles -> IORef DupesMap -- ^ The duplicates seen so far, used to defer reporting on duplicates @@ -308,7 +308,7 @@ parseProjectSkeleton cacheDir httpTransport verbosity importsBy dupesMap project -- Once we canonicalize the import path, we can check for cyclical and duplicate imports normSource <- canonicalizeConfigPath projectDir source normLocPath@(ProjectConfigPath (uniqueImport :| _)) <- canonicalizeConfigPath projectDir importLocPath - seenImportsBy@(fmap fst -> seenImports) <- fromNubList <$> atomicModifyIORef' importsBy (\ibs -> (toNubList [(uniqueImport, normLocPath)] <> ibs, ibs)) + seenImportsBy@(fmap importOf -> seenImports) <- fromNubList <$> atomicModifyIORef' importsBy (\ibs -> (toNubList [ProjectImport uniqueImport normLocPath] <> ibs, ibs)) debug verbosity $ "\nimport path, normalized\n=======================\n" ++ render (docProjectConfigPath normLocPath) debug verbosity "\nseen unique paths\n=================" mapM_ (debug verbosity) seenImports From a6d24a10ba4138f90eb5e63b49721bd3136e4c3f Mon Sep 17 00:00:00 2001 From: Phil de Joux Date: Sat, 26 Apr 2025 21:32:23 -0400 Subject: [PATCH 4/7] Combine fields as ProjectImport --- .../Distribution/Solver/Types/ProjectConfigPath.hs | 11 +++++++---- .../src/Distribution/Client/ProjectConfig/Legacy.hs | 9 ++++----- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs b/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs index 8d8cbc2bab1..ac80823f30e 100644 --- a/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs +++ b/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs @@ -184,16 +184,19 @@ docProjectConfigFiles ps = vcat -- | A message for a cyclical import, a "cyclical import of". cyclicalImportMsg :: ProjectConfigPath -> Doc cyclicalImportMsg path@(ProjectConfigPath (duplicate :| _)) = - seenImportMsg (text "cyclical import of" <+> text duplicate <> semi) duplicate path [] + seenImportMsg + (text "cyclical import of" <+> text duplicate <> semi) + (ProjectImport duplicate path) + [] -- | A message for a duplicate import, a "duplicate import of". If a check for -- cyclical imports has already been made then this would report a duplicate -- import by two different paths. -duplicateImportMsg :: Doc -> FilePath -> ProjectConfigPath -> [ProjectImport] -> Doc +duplicateImportMsg :: Doc -> ProjectImport -> [ProjectImport] -> Doc duplicateImportMsg intro = seenImportMsg intro -seenImportMsg :: Doc -> FilePath -> ProjectConfigPath -> [ProjectImport] -> Doc -seenImportMsg intro duplicate path seenImports = +seenImportMsg :: Doc -> ProjectImport -> [ProjectImport] -> Doc +seenImportMsg intro ProjectImport{importOf = duplicate, importBy = path} seenImports = vcat [ intro , nest 2 (docProjectConfigPath path) diff --git a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs index 1cc031ceb80..2dd4ae3fdbb 100644 --- a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs +++ b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs @@ -265,8 +265,7 @@ parseProject rootPath cacheDir httpTransport verbosity configToParse = do return result data Dupes = Dupes - { dupesUniqueImport :: FilePath - , dupesNormLocPath :: ProjectConfigPath + { dupesImport :: ProjectImport , dupesSeenImportsBy :: [ProjectImport] } deriving (Eq) @@ -277,10 +276,10 @@ instance Ord Dupes where type DupesMap = Map FilePath [Dupes] dupesMsg :: (FilePath, [Dupes]) -> Doc -dupesMsg (duplicate, ds@(take 1 . sortOn dupesNormLocPath -> dupes)) = +dupesMsg (duplicate, ds@(take 1 . sortOn (importBy . dupesImport) -> dupes)) = vcat $ ((text "Warning:" <+> int (length ds) <+> text "imports of" <+> text duplicate) <> semi) - : ((\Dupes{..} -> duplicateImportMsg Disp.empty dupesUniqueImport dupesNormLocPath dupesSeenImportsBy) <$> dupes) + : ((\Dupes{..} -> duplicateImportMsg Disp.empty dupesImport dupesSeenImportsBy) <$> dupes) parseProjectSkeleton :: FilePath @@ -322,7 +321,7 @@ parseProjectSkeleton cacheDir httpTransport verbosity importsBy dupesMap project (noticeDoc verbosity $ untrimmedUriImportMsg (text "Warning:") importLocPath) let fs = (\z -> CondNode z [normLocPath] mempty) <$> fieldsToConfig normSource (reverse acc) let uniqueFields = if uniqueImport `elem` seenImports then [] else xs - atomicModifyIORef' dupesMap $ \dm -> (Map.insertWith (++) uniqueImport [Dupes uniqueImport normLocPath seenImportsBy] dm, ()) + atomicModifyIORef' dupesMap $ \dm -> (Map.insertWith (++) uniqueImport [Dupes (ProjectImport uniqueImport normLocPath) seenImportsBy] dm, ()) res <- parseProjectSkeleton cacheDir httpTransport verbosity importsBy dupesMap projectDir importLocPath . ProjectConfigToParse =<< fetchImportConfig normLocPath rest <- go [] uniqueFields pure . fmap mconcat . sequence $ [projectParse Nothing normSource fs, res, rest] From 87414e4b83115c86558fb71105fc5fa94ce7b0c3 Mon Sep 17 00:00:00 2001 From: Phil de Joux Date: Sat, 26 Apr 2025 21:37:02 -0400 Subject: [PATCH 5/7] Rename field to dupesImports --- .../src/Distribution/Client/ProjectConfig/Legacy.hs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs index 2dd4ae3fdbb..ebf20495f82 100644 --- a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs +++ b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs @@ -266,12 +266,12 @@ parseProject rootPath cacheDir httpTransport verbosity configToParse = do data Dupes = Dupes { dupesImport :: ProjectImport - , dupesSeenImportsBy :: [ProjectImport] + , dupesImports :: [ProjectImport] } deriving (Eq) instance Ord Dupes where - compare = compare `on` length . dupesSeenImportsBy + compare = compare `on` length . dupesImports type DupesMap = Map FilePath [Dupes] @@ -279,7 +279,7 @@ dupesMsg :: (FilePath, [Dupes]) -> Doc dupesMsg (duplicate, ds@(take 1 . sortOn (importBy . dupesImport) -> dupes)) = vcat $ ((text "Warning:" <+> int (length ds) <+> text "imports of" <+> text duplicate) <> semi) - : ((\Dupes{..} -> duplicateImportMsg Disp.empty dupesImport dupesSeenImportsBy) <$> dupes) + : ((\Dupes{..} -> duplicateImportMsg Disp.empty dupesImport dupesImports) <$> dupes) parseProjectSkeleton :: FilePath From 5f6ac16bdf00492d1ec41d2418f139988ff5de98 Mon Sep 17 00:00:00 2001 From: Phil de Joux Date: Sat, 26 Apr 2025 22:05:01 -0400 Subject: [PATCH 6/7] Add haddocks to Dupes fields --- cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs index ebf20495f82..b2f1bc3ce8b 100644 --- a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs +++ b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs @@ -266,7 +266,9 @@ parseProject rootPath cacheDir httpTransport verbosity configToParse = do data Dupes = Dupes { dupesImport :: ProjectImport + -- ^ The import that we're checking for duplicates. , dupesImports :: [ProjectImport] + -- ^ All the imports of this file. } deriving (Eq) From a926d99f7d7d5ab3066e72e04dbd1c21a1b2cfe5 Mon Sep 17 00:00:00 2001 From: Phil de Joux Date: Sun, 27 Apr 2025 05:51:15 -0400 Subject: [PATCH 7/7] Mark test as flaky - Any test accessing stackage seems susceptible --- .../ProjectImport/DedupUsingConfigFromSimple/cabal.test.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cabal-testsuite/PackageTests/ProjectImport/DedupUsingConfigFromSimple/cabal.test.hs b/cabal-testsuite/PackageTests/ProjectImport/DedupUsingConfigFromSimple/cabal.test.hs index cf37d1621a6..3287c84521a 100644 --- a/cabal-testsuite/PackageTests/ProjectImport/DedupUsingConfigFromSimple/cabal.test.hs +++ b/cabal-testsuite/PackageTests/ProjectImport/DedupUsingConfigFromSimple/cabal.test.hs @@ -1,6 +1,6 @@ import Test.Cabal.Prelude -main = cabalTest . recordMode RecordMarked $ do +main = cabalTest . flakyIfCI 10927. recordMode RecordMarked $ do let log = recordHeader . pure out <- fails $ cabal' "v2-build" [ "all", "--dry-run" ]