From 8e6e31536def3cf481f5bdfd26a99197cf248dad Mon Sep 17 00:00:00 2001 From: Jappie Klooster Date: Wed, 3 Aug 2022 18:30:20 +0200 Subject: [PATCH 1/8] Add check for upper bound on any package fixes https://github.com/haskell/cabal/issues/8291 presumably this will make it nag at anyone for forgetting to add upper bounds to their packages. add changelog (presumably) wait what? move toDependencyVersionsMap to utils section add nicer error message simplify checking logic, add more comments only emit missing upper bounds if bigger then one don't add bound to internal libraries filter out self from the dependency map I think this is an external library so it needs an upper bound now? add test for multilib fix test suite by ignoring the warning ... probably not the best approach change link to pvp instead of parsonsmatt better wording on missing upper bound error remove spurious parenthesis change map creation from monad to list comprehension use foldmap to get rid of maybe, fix compile error rewrite from do notation to list comprehension fix test suite failing fix compile error factor out double filter call in a && expression --- Cabal-tests/tests/CheckTests.hs | 1 + .../regressions/public-multilib-2.cabal | 2 +- .../regressions/public-multilib-3.cabal | 14 ++++ .../regressions/public-multilib-3.check | 3 + .../Distribution/PackageDescription/Check.hs | 69 +++++++++++-------- .../Distribution/Solver/Modular/DSL.hs | 6 +- changelog.d/pr-8339 | 4 ++ 7 files changed, 67 insertions(+), 32 deletions(-) create mode 100644 Cabal-tests/tests/ParserTests/regressions/public-multilib-3.cabal create mode 100644 Cabal-tests/tests/ParserTests/regressions/public-multilib-3.check create mode 100644 changelog.d/pr-8339 diff --git a/Cabal-tests/tests/CheckTests.hs b/Cabal-tests/tests/CheckTests.hs index b9b5d6d94f2..01370fc2395 100644 --- a/Cabal-tests/tests/CheckTests.hs +++ b/Cabal-tests/tests/CheckTests.hs @@ -45,6 +45,7 @@ checkTests = testGroup "regressions" , checkTest "assoc-cpp-options.cabal" , checkTest "public-multilib-1.cabal" , checkTest "public-multilib-2.cabal" + , checkTest "public-multilib-3.cabal" , checkTest "issue-6288-a.cabal" , checkTest "issue-6288-b.cabal" , checkTest "issue-6288-c.cabal" diff --git a/Cabal-tests/tests/ParserTests/regressions/public-multilib-2.cabal b/Cabal-tests/tests/ParserTests/regressions/public-multilib-2.cabal index fe0d60a561c..13d6c72f2de 100644 --- a/Cabal-tests/tests/ParserTests/regressions/public-multilib-2.cabal +++ b/Cabal-tests/tests/ParserTests/regressions/public-multilib-2.cabal @@ -9,6 +9,6 @@ library default-language: Haskell2010 build-depends: , base ^>=4.14 - , somelib:internal + , somelib:internal ^>=1.0 exposed-modules: Foo diff --git a/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.cabal b/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.cabal new file mode 100644 index 00000000000..cde78b61b52 --- /dev/null +++ b/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.cabal @@ -0,0 +1,14 @@ +cabal-version: 3.0 +name: public-multilib3 +version: 0 +synopsis: public-multilibs +category: Tests +license: MIT + +library + default-language: Haskell2010 + build-depends: + , base ^>=4.14 + , somelib + + exposed-modules: Foo diff --git a/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.check b/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.check new file mode 100644 index 00000000000..aa1d95e299c --- /dev/null +++ b/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.check @@ -0,0 +1,3 @@ +No 'maintainer' field. +No 'description' field. +These packages miss upper bounds 'somelib' please add them using `cabal gen-bounds` for suggestions. For more information see: https://pvp.haskell.org/ diff --git a/Cabal/src/Distribution/PackageDescription/Check.hs b/Cabal/src/Distribution/PackageDescription/Check.hs index b364979bd20..bf9ad9d760b 100644 --- a/Cabal/src/Distribution/PackageDescription/Check.hs +++ b/Cabal/src/Distribution/PackageDescription/Check.hs @@ -227,6 +227,7 @@ data CheckExplanation = | UnknownArch [String] | UnknownCompiler [String] | BaseNoUpperBounds + | MissingUpperBounds [PackageName] | SuspiciousFlagName [String] | DeclaredUsedFlags (Set FlagName) (Set FlagName) | NonASCIICustomField [String] @@ -666,6 +667,12 @@ ppExplanation (UnknownArch unknownArches) = "Unknown architecture name " ++ commaSep (map quote unknownArches) ppExplanation (UnknownCompiler unknownImpls) = "Unknown compiler name " ++ commaSep (map quote unknownImpls) +ppExplanation (MissingUpperBounds names) = + "These packages miss upper bounds '" + ++ (intercalate "','" (unPackageName <$> names)) ++ "'" + ++ " please add them using `cabal gen-bounds` for suggestions." + ++ " For more information see: " + ++ " https://pvp.haskell.org/" ppExplanation BaseNoUpperBounds = "The dependency 'build-depends: base' does not specify an upper " ++ "bound on the version number. Each major release of the 'base' " @@ -1813,29 +1820,20 @@ checkCabalVersion pkg = -- checkPackageVersions :: GenericPackageDescription -> [PackageCheck] checkPackageVersions pkg = - catMaybes [ - - -- Check that the version of base is bounded above. - -- For example this bans "build-depends: base >= 3". - -- It should probably be "build-depends: base >= 3 && < 4" - -- which is the same as "build-depends: base == 3.*" - check (not (hasUpperBound baseDependency)) $ - PackageDistInexcusable BaseNoUpperBounds - - ] + if length others > 0 + then + PackageDistSuspiciousWarn (MissingUpperBounds others) : baseErrors + else + baseErrors where - baseDependency = case typicalPkg pkg of - Right (pkg', _) | not (null baseDeps) -> - foldr intersectVersionRanges anyVersion baseDeps - where - baseDeps = - [ vr | Dependency pname vr _ <- allBuildDepends pkg' - , pname == mkPackageName "base" ] - - -- Just in case finalizePD fails for any reason, - -- or if the package doesn't depend on the base package at all, - -- then we will just skip the check, since hasUpperBound noVersion = True - _ -> noVersion + baseErrors = PackageDistInexcusable BaseNoUpperBounds <$ bases + deps = toDependencyVersionsMap allBuildDepends pkg + -- base gets special treatment (it's more critical) + (bases, others) = partition (("base" ==) . unPackageName) $ + [ name + | (name, vr) <- Map.toList deps + , not (hasUpperBound vr) + ] checkConditionals :: GenericPackageDescription -> [PackageCheck] checkConditionals pkg = @@ -2409,14 +2407,7 @@ checkSetupVersions pkg = ] where criticalPkgs = ["Cabal", "base"] - deps = case typicalPkg pkg of - Right (pkgs', _) -> - Map.fromListWith intersectVersionRanges - [ (pname, vr) - | sbi <- maybeToList $ setupBuildInfo pkgs' - , Dependency pname vr _ <- setupDepends sbi - ] - _ -> Map.empty + deps = toDependencyVersionsMap (foldMap setupDepends . setupBuildInfo) pkg emitError nm = PackageDistInexcusable (UpperBoundSetup nm) @@ -2455,6 +2446,24 @@ checkDuplicateModules pkg = -- * Utils -- ------------------------------------------------------------ +toDependencyVersionsMap :: (PackageDescription -> [Dependency]) -> GenericPackageDescription -> Map PackageName VersionRange +toDependencyVersionsMap selectDependencies pkg = case typicalPkg pkg of + Right (pkgs', _) -> + let + self :: PackageName + self = pkgName $ package pkgs' + in + Map.fromListWith intersectVersionRanges $ + [ (pname, vr) + | Dependency pname vr _ <- selectDependencies pkgs' + , pname /= self + ] + -- Just in case finalizePD fails for any reason, + -- or if the package doesn't depend on the base package at all, + -- no deps is no checks. + _ -> Map.empty + + quote :: String -> String quote s = "'" ++ s ++ "'" diff --git a/cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs b/cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs index 157f5c3cc2f..71ec7889808 100644 --- a/cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs +++ b/cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs @@ -478,7 +478,7 @@ exAvSrcPkg ex = -- solver allows unknown extensions/languages when the compiler -- supports them. let checks = C.checkPackage (srcpkgDescription package) Nothing - in filter (not . isUnknownLangExt) checks + in filter (\x -> not (isMissingUpperBound x) && not (isUnknownLangExt x)) checks in if null pkgCheckErrors then package else error $ "invalid GenericPackageDescription for package " @@ -671,6 +671,10 @@ exAvSrcPkg ex = C.UnknownExtensions {} -> True C.UnknownLanguages {} -> True _ -> False + isMissingUpperBound :: C.PackageCheck -> Bool + isMissingUpperBound pc = case C.explanation pc of + C.MissingUpperBounds {} -> True + _ -> False mkSimpleVersion :: ExamplePkgVersion -> C.Version diff --git a/changelog.d/pr-8339 b/changelog.d/pr-8339 new file mode 100644 index 00000000000..1e602c1e985 --- /dev/null +++ b/changelog.d/pr-8339 @@ -0,0 +1,4 @@ +synopsis: Add check for upper bound on any dependency in cabal check +report, list, init, fetch, info, upload, get. +prs: #8339 +issues: #8291 From bf270de94239d3edb35a779c42cd96d41725ae61 Mon Sep 17 00:00:00 2001 From: Jappie Klooster Date: Wed, 28 Sep 2022 10:45:42 -0400 Subject: [PATCH 2/8] set expectation --- .../tests/ParserTests/regressions/public-multilib-3.cabal | 3 +++ .../tests/ParserTests/regressions/public-multilib-3.check | 7 ++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.cabal b/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.cabal index cde78b61b52..e96fe3e2f5f 100644 --- a/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.cabal +++ b/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.cabal @@ -10,5 +10,8 @@ library build-depends: , base ^>=4.14 , somelib + , alphalib + , betalib + , deltalib exposed-modules: Foo diff --git a/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.check b/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.check index aa1d95e299c..32a880d1100 100644 --- a/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.check +++ b/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.check @@ -1,3 +1,8 @@ No 'maintainer' field. No 'description' field. -These packages miss upper bounds 'somelib' please add them using `cabal gen-bounds` for suggestions. For more information see: https://pvp.haskell.org/ +These packages miss upper bounds: + - somelib + - alphalib + - betalib + - deltalib +Please add them, using `cabal gen-bounds` for suggestions. For more information see: https://pvp.haskell.org/ From 4ebb72bad4d237244b08490aaf1c687272e42df9 Mon Sep 17 00:00:00 2001 From: Jappie Klooster Date: Wed, 28 Sep 2022 11:09:00 -0400 Subject: [PATCH 3/8] Modify test so it gives a list of expected libs rename public-multilib-3 to all-upper-bound the test had little to do with multilib. --- Cabal-tests/tests/CheckTests.hs | 2 +- .../{public-multilib-3.cabal => all-upper-bound.cabal} | 4 ++-- .../{public-multilib-3.check => all-upper-bound.check} | 2 +- Cabal/src/Distribution/PackageDescription/Check.hs | 8 +++++--- 4 files changed, 9 insertions(+), 7 deletions(-) rename Cabal-tests/tests/ParserTests/regressions/{public-multilib-3.cabal => all-upper-bound.cabal} (78%) rename Cabal-tests/tests/ParserTests/regressions/{public-multilib-3.check => all-upper-bound.check} (100%) diff --git a/Cabal-tests/tests/CheckTests.hs b/Cabal-tests/tests/CheckTests.hs index 01370fc2395..603827e108e 100644 --- a/Cabal-tests/tests/CheckTests.hs +++ b/Cabal-tests/tests/CheckTests.hs @@ -45,7 +45,7 @@ checkTests = testGroup "regressions" , checkTest "assoc-cpp-options.cabal" , checkTest "public-multilib-1.cabal" , checkTest "public-multilib-2.cabal" - , checkTest "public-multilib-3.cabal" + , checkTest "all-upper-bound.cabal" , checkTest "issue-6288-a.cabal" , checkTest "issue-6288-b.cabal" , checkTest "issue-6288-c.cabal" diff --git a/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.cabal b/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal similarity index 78% rename from Cabal-tests/tests/ParserTests/regressions/public-multilib-3.cabal rename to Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal index e96fe3e2f5f..05cc9d4cf62 100644 --- a/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.cabal +++ b/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal @@ -1,7 +1,7 @@ cabal-version: 3.0 -name: public-multilib3 +name: all-upper-bound version: 0 -synopsis: public-multilibs +synopsis: all-upper-bound category: Tests license: MIT diff --git a/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.check b/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.check similarity index 100% rename from Cabal-tests/tests/ParserTests/regressions/public-multilib-3.check rename to Cabal-tests/tests/ParserTests/regressions/all-upper-bound.check index 32a880d1100..fd738cd955a 100644 --- a/Cabal-tests/tests/ParserTests/regressions/public-multilib-3.check +++ b/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.check @@ -1,8 +1,8 @@ No 'maintainer' field. No 'description' field. These packages miss upper bounds: - - somelib - alphalib - betalib - deltalib + - somelib Please add them, using `cabal gen-bounds` for suggestions. For more information see: https://pvp.haskell.org/ diff --git a/Cabal/src/Distribution/PackageDescription/Check.hs b/Cabal/src/Distribution/PackageDescription/Check.hs index bf9ad9d760b..3b5df37045b 100644 --- a/Cabal/src/Distribution/PackageDescription/Check.hs +++ b/Cabal/src/Distribution/PackageDescription/Check.hs @@ -668,9 +668,11 @@ ppExplanation (UnknownArch unknownArches) = ppExplanation (UnknownCompiler unknownImpls) = "Unknown compiler name " ++ commaSep (map quote unknownImpls) ppExplanation (MissingUpperBounds names) = - "These packages miss upper bounds '" - ++ (intercalate "','" (unPackageName <$> names)) ++ "'" - ++ " please add them using `cabal gen-bounds` for suggestions." + let separator = "\n - " + in + "These packages miss upper bounds:" ++ separator + ++ (intercalate separator (unPackageName <$> names)) ++ "\n" + ++ "Please add them, using `cabal gen-bounds` for suggestions." ++ " For more information see: " ++ " https://pvp.haskell.org/" ppExplanation BaseNoUpperBounds = From 705463adc60030f25f817b1173cd43a76c7c6d59 Mon Sep 17 00:00:00 2001 From: Jappie Klooster Date: Wed, 12 Oct 2022 10:08:44 -0400 Subject: [PATCH 4/8] Update comment on ignoring warnigns --- .../tests/UnitTests/Distribution/Solver/Modular/DSL.hs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs b/cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs index 71ec7889808..0d22d5fe758 100644 --- a/cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs +++ b/cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs @@ -474,9 +474,12 @@ exAvSrcPkg ex = } } pkgCheckErrors = - -- We ignore these warnings because some unit tests test that the - -- solver allows unknown extensions/languages when the compiler - -- supports them. + -- We ignore unknown extensions/languages warnings because + -- some there are some unit tests test in which the solver allows + -- unknown extensions/languages when the compiler supports them. + -- Furthermore we ignore missing upper bound warnings because + -- they are not related to this test suite, and are tested + -- with golden tests. let checks = C.checkPackage (srcpkgDescription package) Nothing in filter (\x -> not (isMissingUpperBound x) && not (isUnknownLangExt x)) checks in if null pkgCheckErrors From 637e29afcfbdaa0bf40a02a09ee2d44e1decc114 Mon Sep 17 00:00:00 2001 From: Jappie Klooster Date: Wed, 12 Oct 2022 12:30:43 -0400 Subject: [PATCH 5/8] use map instead of if --- Cabal/src/Distribution/PackageDescription/Check.hs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Cabal/src/Distribution/PackageDescription/Check.hs b/Cabal/src/Distribution/PackageDescription/Check.hs index 3b5df37045b..98b4855040c 100644 --- a/Cabal/src/Distribution/PackageDescription/Check.hs +++ b/Cabal/src/Distribution/PackageDescription/Check.hs @@ -1822,11 +1822,7 @@ checkCabalVersion pkg = -- checkPackageVersions :: GenericPackageDescription -> [PackageCheck] checkPackageVersions pkg = - if length others > 0 - then - PackageDistSuspiciousWarn (MissingUpperBounds others) : baseErrors - else - baseErrors + map (PackageDistSuspiciousWarn . MissingUpperBounds) others : baseErrors where baseErrors = PackageDistInexcusable BaseNoUpperBounds <$ bases deps = toDependencyVersionsMap allBuildDepends pkg From 920e2a319cd5a4f9bda03e43ff7360baed3e5c4b Mon Sep 17 00:00:00 2001 From: Jappie Klooster Date: Wed, 12 Oct 2022 13:08:10 -0400 Subject: [PATCH 6/8] Undo map change add comment on why that doesn't work --- Cabal/src/Distribution/PackageDescription/Check.hs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Cabal/src/Distribution/PackageDescription/Check.hs b/Cabal/src/Distribution/PackageDescription/Check.hs index 98b4855040c..1908b6a412e 100644 --- a/Cabal/src/Distribution/PackageDescription/Check.hs +++ b/Cabal/src/Distribution/PackageDescription/Check.hs @@ -1822,7 +1822,14 @@ checkCabalVersion pkg = -- checkPackageVersions :: GenericPackageDescription -> [PackageCheck] checkPackageVersions pkg = - map (PackageDistSuspiciousWarn . MissingUpperBounds) others : baseErrors + -- if others is empty, + -- the error will still fire but listing no dependencies. + -- so we have to check + if length others > 0 + then + PackageDistSuspiciousWarn (MissingUpperBounds others) : baseErrors + else + baseErrors where baseErrors = PackageDistInexcusable BaseNoUpperBounds <$ bases deps = toDependencyVersionsMap allBuildDepends pkg From 2d96210e97ac5852d6769502cb915fa2b1cac524 Mon Sep 17 00:00:00 2001 From: Jappie Klooster Date: Wed, 19 Oct 2022 11:47:59 -0400 Subject: [PATCH 7/8] Add description and maintainer fields --- Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal | 2 ++ Cabal-tests/tests/ParserTests/regressions/all-upper-bound.check | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal b/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal index 05cc9d4cf62..5c1850f0d1c 100644 --- a/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal +++ b/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal @@ -4,6 +4,8 @@ version: 0 synopsis: all-upper-bound category: Tests license: MIT +maintainer: someone@example.com +description: test package. library default-language: Haskell2010 diff --git a/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.check b/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.check index fd738cd955a..0da0e871ebb 100644 --- a/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.check +++ b/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.check @@ -1,5 +1,3 @@ -No 'maintainer' field. -No 'description' field. These packages miss upper bounds: - alphalib - betalib From 37102b9524f7cfad7a4e0c2adc2b4323d5f7ddfb Mon Sep 17 00:00:00 2001 From: Jappie Klooster Date: Thu, 1 Dec 2022 19:21:50 -0400 Subject: [PATCH 8/8] make description longer then synopsis in all-upper-bound.cabal --- Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal b/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal index 5c1850f0d1c..2b12c477fcf 100644 --- a/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal +++ b/Cabal-tests/tests/ParserTests/regressions/all-upper-bound.cabal @@ -5,7 +5,7 @@ synopsis: all-upper-bound category: Tests license: MIT maintainer: someone@example.com -description: test package. +description: all-upper-bound test package. library default-language: Haskell2010