Make check comply with Hackage requirements (#8897)

* Abstract `isDistError`

There are two places in cabal codebase where `isDistError` is defined
(would Hackage refuse the package based on this error?), plus another
one in hackage-server (src/Distribution/Server/Packages/Unpack.hs).

This commit refactors the definitions into a singel `isHackageDistError`
(in Distribution.Client.Check) so everything is in sync. The function
is exported, for the benefit of third-party tools.

* Make `check` comply with Hackage requirements

`cabal check` exits with 0 or 1. As now the `1` exit is slightly
stricter than “Hackage would reject this”, as even PackageDistSuspicious
errors will trigger a `1` exit.

This patch rectifies this and also specifies the behaviour in the
documentation. Notice the usage of “should” and “will”: we cannot
be sure Hackage will accept a package (e.g.: maybe there are name
clashes) but there are no type I errors: if it fails `cabal check`,
Hackage will refuse it.

* Make testsuite pass

All the modified tests returned 1 on exit, now they return 0.

* Add changelog for #8897

* Expand catchall into constructors

(Andreas’ review) For `isHackageDistError`, so that in the future, when
constructors are added, the compiler will warn about missing them missing and
a thoughtful choice will be made.

* Use LambdaCase

(Artem’s review) Saves typing `isHackageDistError` four times more.

* Reword help strings

(Artem’s review) Make clear that errors will make `check` return 1.

* Fix typo

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Author: ffaf1

Cabal/src/Distribution/PackageDescription/Check.hs

@@ -1,3 +1,5 @@
+{-# LANGUAGE LambdaCase #-}
+
 -----------------------------------------------------------------------------
 -- |
 -- Module      :  Distribution.PackageDescription.Check
@@ -28,6 +30,7 @@ module Distribution.PackageDescription.Check (
         checkConfiguredPackage,
         wrapParseWarning,
         ppPackageCheck,
+        isHackageDistError,
 
         -- ** Checking package contents
         checkPackageFiles,
@@ -846,6 +849,15 @@ data PackageCheck =
      | PackageDistInexcusable { explanation :: CheckExplanation }
   deriving (Eq, Ord)
 
+-- | Would Hackage refuse a package because of this error?
+isHackageDistError :: PackageCheck -> Bool
+isHackageDistError = \case
+    (PackageBuildImpossible {}) -> True
+    (PackageBuildWarning {}) -> True
+    (PackageDistInexcusable {}) -> True
+    (PackageDistSuspicious {}) -> False
+    (PackageDistSuspiciousWarn {}) -> False
+
 -- | Pretty printing 'PackageCheck'.
 --
 ppPackageCheck :: PackageCheck -> String

Cabal/src/Distribution/Simple/SrcDist.hs

@@ -501,10 +501,7 @@ printPackageProblems :: Verbosity -> PackageDescription -> IO ()
 printPackageProblems verbosity pkg_descr = do
   ioChecks      <- checkPackageFiles verbosity pkg_descr "."
   let pureChecks = checkConfiguredPackage pkg_descr
-      isDistError (PackageDistSuspicious     _) = False
-      isDistError (PackageDistSuspiciousWarn _) = False
-      isDistError _                             = True
-      (errors, warnings) = partition isDistError (pureChecks ++ ioChecks)
+      (errors, warnings) = partition isHackageDistError (pureChecks ++ ioChecks)
   unless (null errors) $
       notice verbosity $ "Distribution quality errors:\n"
                       ++ unlines (map ppPackageCheck errors)

cabal-install/src/Distribution/Client/Check.hs

@@ -48,7 +48,9 @@ readGenericPackageDescriptionCheck verbosity fpath = do
             die' verbosity "parse error"
         Right x  -> return (warnings, x)
 
--- | Note: must be called with the CWD set to the directory containing
+-- | Checks a packge for common errors. Returns @True@ if the package
+-- is fit to upload to Hackage, @False@ otherwise.
+-- Note: must be called with the CWD set to the directory containing
 -- the '.cabal' file.
 check :: Verbosity -> IO Bool
 check verbosity = do
@@ -94,20 +96,15 @@ check verbosity = do
         warn verbosity "The following errors will cause portability problems on other environments:"
         printCheckMessages distInexusable
 
-    let isDistError (PackageDistSuspicious     {}) = False
-        isDistError (PackageDistSuspiciousWarn {}) = False
-        isDistError _                              = True
-        isCheckError (PackageDistSuspiciousWarn {}) = False
-        isCheckError _                              = True
-        errors = filter isDistError packageChecks
+    let errors = filter isHackageDistError packageChecks
 
     unless (null errors) $
         warn verbosity "Hackage would reject this package."
 
     when (null packageChecks) $
         notice verbosity "No errors or warnings could be found in the package."
 
-    return (not . any isCheckError $ packageChecks)
+    return (null errors)
 
   where
     printCheckMessages :: [PackageCheck] -> IO ()

cabal-install/src/Distribution/Client/Setup.hs

@@ -1201,8 +1201,9 @@ checkCommand = CommandUI {
          "Expects a .cabal package file in the current directory.\n"
       ++ "\n"
       ++ "The checks correspond to the requirements to packages on Hackage. "
-      ++ "If no errors and warnings are reported, Hackage will accept this "
-      ++ "package.\n",
+      ++ "If no errors and warnings are reported, Hackage should accept the "
+      ++ "package. If errors are present, `check` exits with 1 and Hackage "
+      ++ "will refuse the package.\n",
     commandNotes        = Nothing,
     commandUsage        = usageFlags "check",
     commandDefaultFlags = toFlag normal,

cabal-testsuite/PackageTests/Check/ConfiguredPackage/COptions/CxxOs/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- `cxx-options`, do not use `-O1`.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/CabalVersion/DefaultExtension/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- `default-extensions` need ≥1.10.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/CabalVersion/DefaultLanguage/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- `default-language` need ≥1.10.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/CabalVersion/ExtraDynamicLibraryFlavour/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- `extra-dynamic-library-flavour` need ≥3.0.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/CabalVersion/Mixins/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- `mixins` need ≥2.0.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/CabalVersion/Sources/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- `cmm-sources` and friends need ≥3.0.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/CabalVersion/VirtualModules/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- `virtual-modules` need ≥2.2.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/Fields/DeprecatedExtension/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- Deprecated extension.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/Fields/NoCategory/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- No category.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/Fields/NoDescription/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- No description.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/Fields/NoMaintainer/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- No maintainer.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/Fields/NoSynopsis/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- No synopsis.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/Fields/ShortDescription/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- Description should be longer than synopsis.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/GHCOptions/GHCSharedOptions/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- Tricky option in `ghc-shared-options`.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/License/NoFileSpecified/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- `licence-file` missing.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/License/SuspiciousLicense/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- Suspicious license BSD4.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/License/SuspiciousVersion/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- Suspicious license version.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/License/WarnAllRightsReserved/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- Dubious AllRightsReserved.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/ConfiguredPackage/Sanity/VersionSignatures/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- `signatures` field used with cabal-version < 2.0
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/NonConfCheck/DevOnlyFlags/Profiling/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- Profiling flags unsuited for distribution.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/NonConfCheck/UnusedFlags/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- Unused flag.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

cabal-testsuite/PackageTests/Check/PackageFiles/VCSInfo/cabal.test.hs

@@ -2,4 +2,4 @@ import Test.Cabal.Prelude
 
 -- Missing VCS info.
 main = cabalTest $
-  fails $ cabal "check" []
+  cabal "check" []

changelog.d/pr-8897

@@ -0,0 +1,14 @@
+synopsis: Make check comply with Hackage requirements
+packages: Cabal cabal-install
+prs: #8897
+
+description: {
+
+- `cabal check` will only return exitcode 1 when the package is not fit
+  for Hackage. E.g. it will not error anymore when your `synopsis:` is
+  larger than `description:`, just emit a warning.
+- Cabal: Distribution.Client.Check now exports `isHackageDistError`, for
+  third-party tools to know if a specific error will preclude a package
+  from being uploaded to Hacakge.
+
+}

doc/cabal-commands.rst

@@ -977,7 +977,8 @@ Run ``cabal check`` in the folder where your ``.cabal`` package file is.
     Set verbosity level (0–3, default is 1).
 
 ``cabal check`` mimics Hackage's requirements: if no error or warning
-is reported, Hackage should accept your package.
+is reported, Hackage should accept your package. If errors are present
+``cabal check`` exits with ``1`` and Hackage will refuse the package.
 
 cabal sdist
 ^^^^^^^^^^^