From 067b957cf43f2c9c3d5768ed1148717ebe21d746 Mon Sep 17 00:00:00 2001 From: Phil de Joux Date: Tue, 13 Feb 2024 14:10:39 -0500 Subject: [PATCH] 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 --- .../Solver/Types/ProjectConfigPath.hs | 20 +++++++-- .../Client/ProjectConfig/Legacy.hs | 43 ++++++++++++------- .../ConditionalAndImport/cabal.out | 11 +++++ .../ConditionalAndImport/cabal.test.hs | 31 +++++++++++++ .../ConditionalAndImport/yops-0.project | 7 +++ .../ConditionalAndImport/yops-2.config | 1 + .../ConditionalAndImport/yops-4.config | 1 + .../ConditionalAndImport/yops-6.config | 1 + .../ConditionalAndImport/yops-8.config | 1 + .../ConditionalAndImport/yops/yops-1.config | 1 + .../ConditionalAndImport/yops/yops-3.config | 1 + .../ConditionalAndImport/yops/yops-5.config | 1 + .../ConditionalAndImport/yops/yops-7.config | 1 + .../ConditionalAndImport/yops/yops-9.config | 1 + changelog.d/pr-9933 | 23 ++++++++++ 15 files changed, 125 insertions(+), 19 deletions(-) create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/yops-0.project create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/yops-2.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/yops-4.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/yops-6.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/yops-8.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-1.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-3.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-5.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-7.config create mode 100644 cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-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 c57ade0c3e3..872a6d6cf44 100644 --- a/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs +++ b/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs @@ -12,6 +12,7 @@ module Distribution.Solver.Types.ProjectConfigPath , docProjectConfigPath , docProjectConfigPaths , cyclicalImportMsg + , duplicateImportMsg , docProjectConfigPathFailReason -- * Checks and Normalization @@ -101,13 +102,24 @@ docProjectConfigPaths :: [ProjectConfigPath] -> Doc docProjectConfigPaths ps = vcat [ text "-" <+> text p | ProjectConfigPath (p :| _) <- ps ] --- | A message for a cyclical import, assuming the head of the path is the --- duplicate. +-- | A message for a cyclical import, a "cyclical import of". cyclicalImportMsg :: ProjectConfigPath -> Doc -cyclicalImportMsg path@(ProjectConfigPath (duplicate :| _)) = +cyclicalImportMsg path@(ProjectConfigPath (duplicate :| _)) = seenImportMsg "cyclical" duplicate path [] + +-- | A message for a duplicate import, a "duplicate import of". +duplicateImportMsg :: FilePath -> ProjectConfigPath -> [(FilePath, ProjectConfigPath)] -> Doc +duplicateImportMsg = seenImportMsg "duplicate" + +seenImportMsg :: String -> FilePath -> ProjectConfigPath -> [(FilePath, ProjectConfigPath)] -> Doc +seenImportMsg seen duplicate path seenImportsBy = vcat - [ text "cyclical import of" <+> text duplicate <> semi + [ text seen <+> text "import of" <+> text duplicate <> semi , nest 2 (docProjectConfigPath path) + , nest 2 $ + vcat + [ docProjectConfigPath dib + | (_, dib) <- filter ((duplicate ==) . fst) seenImportsBy + ] ] docProjectConfigPathFailReason :: VR -> ProjectConfigPath -> Doc diff --git a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs index 7ed13df1232..1dc7f9c3fde 100644 --- a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs +++ b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs @@ -1,6 +1,7 @@ {-# LANGUAGE ConstraintKinds #-} {-# LANGUAGE DataKinds #-} {-# LANGUAGE DeriveGeneric #-} +{-# LANGUAGE MultiWayIf #-} {-# LANGUAGE NamedFieldPuns #-} {-# LANGUAGE RecordWildCards #-} {-# LANGUAGE ScopedTypeVariables #-} @@ -33,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) @@ -137,7 +139,8 @@ import Distribution.Types.CondTree ) import Distribution.Types.SourceRepo (RepoType) import Distribution.Utils.NubList - ( fromNubList + ( NubList + , fromNubList , overNubList , toNubList ) @@ -246,13 +249,16 @@ parseProject 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 + projectPath@(ProjectConfigPath (canonicalRoot :| _)) <- canonicalizeConfigPath projectDir (ProjectConfigPath $ projectFileName :| []) + importsBy <- newIORef $ toNubList [(canonicalRoot, projectPath)] + parseProjectSkeleton cacheDir httpTransport verbosity importsBy projectDir projectPath configToParse parseProjectSkeleton :: FilePath -> HttpTransport -> Verbosity + -> IORef (NubList (FilePath, ProjectConfigPath)) + -- ^ The imports seen so far, useful for reporting on duplicates and to detect duplicates that are not cycles -> FilePath -- ^ The directory of the project configuration, typically the directory of cabal.project -> ProjectConfigPath @@ -260,7 +266,7 @@ parseProjectSkeleton -> ProjectConfigToParse -- ^ The contents of the file to parse -> IO (ParseResult ProjectConfigSkeleton) -parseProjectSkeleton cacheDir httpTransport verbosity projectDir source (ProjectConfigToParse bs) = +parseProjectSkeleton cacheDir httpTransport verbosity importsBy projectDir source (ProjectConfigToParse bs) = (sanityWalkPCS False =<<) <$> liftPR (go []) (ParseUtils.readFields bs) where go :: [ParseUtils.Field] -> [ParseUtils.Field] -> IO (ParseResult ProjectConfigSkeleton) @@ -268,19 +274,26 @@ 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 - normLocPath <- canonicalizeConfigPath projectDir importLocPath + -- Once we canonicalize the import path, we can check for cyclical and duplicate imports + 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) - - if isCyclicConfigPath normLocPath - then pure . parseFail $ ParseUtils.FromString (render $ cyclicalImportMsg normLocPath) Nothing - else do - normSource <- canonicalizeConfigPath projectDir source - let fs = (\z -> CondNode z [normLocPath] mempty) <$> fieldsToConfig normSource (reverse acc) - res <- parseProjectSkeleton cacheDir httpTransport verbosity projectDir importLocPath . ProjectConfigToParse =<< fetchImportConfig normLocPath - rest <- go [] xs - pure . fmap mconcat . sequence $ [fs, res, rest] + debug verbosity "\nseen unique paths\n=================" + mapM_ (debug verbosity) seenImports + debug verbosity "\n" + + if + | isCyclicConfigPath normLocPath -> + pure . parseFail $ ParseUtils.FromString (render $ cyclicalImportMsg normLocPath) Nothing + | uniqueImport `elem` seenImports -> do + pure . parseFail $ ParseUtils.FromString (render $ duplicateImportMsg uniqueImport normLocPath seenImportsBy) Nothing + | otherwise -> do + normSource <- canonicalizeConfigPath projectDir source + let fs = (\z -> CondNode z [normLocPath] mempty) <$> fieldsToConfig normSource (reverse acc) + res <- parseProjectSkeleton cacheDir httpTransport verbosity importsBy projectDir importLocPath . ProjectConfigToParse =<< fetchImportConfig normLocPath + rest <- go [] xs + pure . fmap mconcat . sequence $ [fs, res, rest] (ParseUtils.Section l "if" p xs') -> do subpcs <- go [] xs' let fs = singletonProjectConfigSkeleton <$> fieldsToConfig source (reverse acc) diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.out b/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.out index 7a6333d21bc..cb129d39855 100644 --- a/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.out +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.out @@ -124,6 +124,17 @@ 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 that we detect when the same config is imported via many different paths +# cabal v2-build +Error: [Cabal-7090] +Error parsing project file /yops-0.project: +duplicate import 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 # 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 c1aea47ce85..b98162cfdce 100644 --- a/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs @@ -225,6 +225,37 @@ main = cabalTest . withRepo "repo" . recordMode RecordMarked $ do \ imported by: oops-0.project") oopsing + -- The project is named yops as it is like hops but with y's for forks. + -- +-- yops-0.project + -- +-- yops/yops-1.config + -- +-- yops-2.config + -- +-- yops/yops-3.config + -- +-- yops-4.config + -- +-- yops/yops-5.config + -- +-- yops-6.config + -- +-- yops/yops-7.config + -- +-- yops-8.config + -- +-- yops/yops-9.config (no further imports) + -- +-- yops/yops-3.config + -- +-- yops-4.config + -- +-- yops/yops-5.config + -- +-- yops-6.config + -- +-- yops/yops-7.config + -- +-- yops-8.config + -- +-- yops/yops-9.config (no further imports) + -- +-- yops/yops-5.config + -- +-- yops-6.config + -- +-- yops/yops-7.config + -- +-- yops-8.config + -- +-- yops/yops-9.config (no further imports) + -- +-- yops/yops-7.config + -- +-- yops-8.config + -- +-- yops/yops-9.config (no further imports) + -- +-- yops/yops-9.config (no further imports) + log "checking that we detect when the same config is imported via many different paths" + yopping <- fails $ cabal' "v2-build" [ "--project-file=yops-0.project" ] + assertOutputContains "duplicate import of yops/yops-3.config" yopping + log "checking bad conditional" badIf <- fails $ cabal' "v2-build" [ "--project-file=bad-conditional.project" ] assertOutputContains "Cannot set compiler in a conditional clause of a cabal project file" badIf diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/yops-0.project b/cabal-testsuite/PackageTests/ConditionalAndImport/yops-0.project new file mode 100644 index 00000000000..28f582bab91 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/yops-0.project @@ -0,0 +1,7 @@ +packages: . + +import: yops/yops-1.config +import: yops/yops-3.config +import: yops/yops-5.config +import: yops/yops-7.config +import: yops/yops-9.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/yops-2.config b/cabal-testsuite/PackageTests/ConditionalAndImport/yops-2.config new file mode 100644 index 00000000000..c8535704f13 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/yops-2.config @@ -0,0 +1 @@ +import: yops/yops-3.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/yops-4.config b/cabal-testsuite/PackageTests/ConditionalAndImport/yops-4.config new file mode 100644 index 00000000000..024d1c94d1e --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/yops-4.config @@ -0,0 +1 @@ +import: yops/yops-5.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/yops-6.config b/cabal-testsuite/PackageTests/ConditionalAndImport/yops-6.config new file mode 100644 index 00000000000..4df25a90a59 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/yops-6.config @@ -0,0 +1 @@ +import: yops/yops-7.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/yops-8.config b/cabal-testsuite/PackageTests/ConditionalAndImport/yops-8.config new file mode 100644 index 00000000000..a2d27ab2b16 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/yops-8.config @@ -0,0 +1 @@ +import: yops/yops-9.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-1.config b/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-1.config new file mode 100644 index 00000000000..5ad1fe73bd3 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-1.config @@ -0,0 +1 @@ +import: ../yops-2.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-3.config b/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-3.config new file mode 100644 index 00000000000..6cbaeb3fa87 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-3.config @@ -0,0 +1 @@ +import: ../yops-4.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-5.config b/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-5.config new file mode 100644 index 00000000000..ae0901cedd6 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-5.config @@ -0,0 +1 @@ +import: ../yops-6.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-7.config b/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-7.config new file mode 100644 index 00000000000..18edcb74c20 --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-7.config @@ -0,0 +1 @@ +import: ../yops-8.config diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-9.config b/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-9.config new file mode 100644 index 00000000000..61813df4e2c --- /dev/null +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/yops/yops-9.config @@ -0,0 +1 @@ +-- No imports here diff --git a/changelog.d/pr-9933 b/changelog.d/pr-9933 new file mode 100644 index 00000000000..d8a6c1d2337 --- /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 and expand the + detail in cyclical import reporting, being more explicit and consistent with + non-cyclical duplicate reporting. + + ``` + $ cabal build --project-file=yops-0.project + ... + Error: [Cabal-7090] + Error parsing project file yops-0.project: + duplicate import 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 + ``` + +packages: cabal-install-solver cabal-install +prs: #9578 #9933 +issues: #9562