Improve "Cannot read .cabal file inside ..." errors

This error message was very confusing -- it doesn't tell you what
`.cabal` file it's looking for or why it cannot read the `.cabal` file.
(Did it fail to parse the `.tar` archive itself? Did parsing the
`.cabal` file fail? If so, why?)

This patch improves the error message to include the name of the
`.cabal` file being searched for. Additionally, parse errors and
warnings are printed, as are format failures in the tarball itself.

I ran into this error while I was writing tests for Cabal and it
confused the hell out of me; this is an expanded version of the changes
I made to help debug that failure.

Before:

```
Error: [Cabal-7046]
Cannot read .cabal file inside my-lib-1.0.tar.gz
```

After:

```
Error: In <ROOT>/cabal.dist/repo/my-lib-1.0.tar.gz: Errors encountered when parsing cabal file my-lib-1.0/my-lib.cabal:

my-lib-1.0/my-lib.cabal:4:22: error:
unexpected Unknown SPDX license identifier: 'puppy'

    3 | version:        1.0
    4 | license:        puppy license :)
      |                      ^

my-lib-1.0/my-lib.cabal:5:1: warning:
Unknown field: "puppy"

    4 | license:        puppy license :)
    5 | puppy:          teehee!
      | ^
Error: [Cabal-7046]
Failed to read my-lib-1.0/my-lib.cabal from archive <ROOT>/cabal.dist/repo/my-lib-1.0.tar.gz
```

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

@@ -70,7 +70,7 @@ data CabalInstallException
   | ReportTargetProblems String
   | ListBinTargetException String
   | ResolveWithoutDependency String
-  | CannotReadCabalFile FilePath
+  | CannotReadCabalFile FilePath FilePath
   | ErrorUpdatingIndex FilePath IOException
   | InternalError FilePath
   | ReadIndexCache FilePath
@@ -390,7 +390,11 @@ exceptionMessageCabalInstall e = case e of
   ReportTargetProblems problemsMsg -> problemsMsg
   ListBinTargetException errorStr -> errorStr
   ResolveWithoutDependency errorStr -> errorStr
-  CannotReadCabalFile file -> "Cannot read .cabal file inside " ++ file
+  CannotReadCabalFile expect file ->
+    "Failed to read "
+      ++ expect
+      ++ " from archive "
+      ++ file
   ErrorUpdatingIndex name ioe -> "Error while updating index for " ++ name ++ " repository " ++ show ioe
   InternalError msg ->
     "internal error when reading package index: "

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

@@ -66,6 +66,10 @@ import Distribution.Client.Types
 import Distribution.Parsec (simpleParsecBS)
 import Distribution.Verbosity
 
+import Distribution.Client.ProjectConfig
+  ( CabalFileParseError
+  , readSourcePackageCabalFile'
+  )
 import Distribution.Client.Setup
   ( RepoContext (..)
   )
@@ -97,6 +101,7 @@ import Distribution.Simple.Utils
   , fromUTF8LBS
   , info
   , warn
+  , warnError
   )
 import Distribution.Types.Dependency
 import Distribution.Types.PackageName (PackageName)
@@ -880,14 +885,22 @@ withIndexEntries verbosity (RepoIndex _repoCtxt (RepoLocalNoIndex (LocalRepo nam
         where
           cabalPath = prettyShow pkgid ++ ".cabal"
       Just pkgId -> do
+        let tarFile = localDir </> file
         -- check for the right named .cabal file in the compressed tarball
-        tarGz <- BS.readFile (localDir </> file)
+        tarGz <- BS.readFile tarFile
         let tar = GZip.decompress tarGz
             entries = Tar.read tar
+            expectFilename = prettyShow pkgId FilePath.</> prettyShow (packageName pkgId) ++ ".cabal"
 
-        case Tar.foldEntries (readCabalEntry pkgId) Nothing (const Nothing) entries of
+        tarballPackageDescription <-
+          Tar.foldEntries
+            (readCabalEntry tarFile expectFilename)
+            (pure Nothing)
+            (handleTarFormatError tarFile)
+            entries
+        case tarballPackageDescription of
           Just ce -> return (Just ce)
-          Nothing -> dieWithException verbosity $ CannotReadCabalFile file
+          Nothing -> dieWithException verbosity $ CannotReadCabalFile expectFilename tarFile
 
   let (prefs, gpds) =
         partitionEithers $
@@ -918,16 +931,55 @@ withIndexEntries verbosity (RepoIndex _repoCtxt (RepoLocalNoIndex (LocalRepo nam
 
     stripSuffix sfx str = fmap reverse (stripPrefix (reverse sfx) (reverse str))
 
-    -- look for <pkgid>/<pkgname>.cabal inside the tarball
-    readCabalEntry :: PackageIdentifier -> Tar.Entry -> Maybe NoIndexCacheEntry -> Maybe NoIndexCacheEntry
-    readCabalEntry pkgId entry Nothing
-      | filename == Tar.entryPath entry
-      , Tar.NormalFile contents _ <- Tar.entryContent entry =
-          let bs = BS.toStrict contents
-           in ((`CacheGPD` bs) <$> parseGenericPackageDescriptionMaybe bs)
-      where
-        filename = prettyShow pkgId FilePath.</> prettyShow (packageName pkgId) ++ ".cabal"
-    readCabalEntry _ _ x = x
+    handleTarFormatError :: FilePath -> Tar.FormatError -> IO (Maybe NoIndexCacheEntry)
+    handleTarFormatError tarFile formatError = do
+      -- This is printed at warning-level because malformed `.tar.gz`s in
+      -- indexes are unexpected and we want users to tell us about them.
+      warnError verbosity $
+        "Failed to parse "
+          <> tarFile
+          <> ": "
+          <> displayException formatError
+      pure Nothing
+
+    -- look for `expectFilename` inside the tarball
+    readCabalEntry
+      :: FilePath
+      -> FilePath
+      -> Tar.Entry
+      -> IO (Maybe NoIndexCacheEntry)
+      -> IO (Maybe NoIndexCacheEntry)
+    readCabalEntry tarFile expectFilename entry previous' = do
+      previous <- previous'
+      case previous of
+        Just _entry -> pure previous
+        Nothing -> do
+          if expectFilename /= Tar.entryPath entry
+            then pure Nothing
+            else case Tar.entryContent entry of
+              Tar.NormalFile contents _fileSize -> do
+                let bytes = BS.toStrict contents
+                maybePackageDescription
+                  :: Either CabalFileParseError GenericPackageDescription <-
+                  -- Warnings while parsing `.cabal` files are only shown in
+                  -- verbose mode because people are allowed to upload packages
+                  -- with warnings to Hackage (and we may _add_ warnings by
+                  -- shipping new versions of Cabal). You probably don't care
+                  -- if there's a warning in some random package in your
+                  -- transitive dependency tree, as long as it's not causing
+                  -- issues. If it is causing issues, you can add `-v` to see
+                  -- the warnings!
+                  try $ readSourcePackageCabalFile' (info verbosity) expectFilename bytes
+                case maybePackageDescription of
+                  Left exception -> do
+                    -- Here we show the _failure_ to parse the `.cabal` file as
+                    -- a warning. This will impact which versions/packages are
+                    -- available in your index, so users should know!
+                    warn verbosity $ "In " <> tarFile <> ": " <> displayException exception
+                    pure Nothing
+                  Right genericPackageDescription ->
+                    pure $ Just $ CacheGPD genericPackageDescription bytes
+              _ -> pure Nothing
 withIndexEntries verbosity index callback _ = do
   -- non-secure repositories
   withFile (indexFile index) ReadMode $ \h -> do

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

@@ -38,6 +38,9 @@ module Distribution.Client.ProjectConfig
   , writeProjectConfigFile
   , commandLineFlagsToProjectConfig
   , onlyTopLevelProvenance
+  , readSourcePackageCabalFile
+  , readSourcePackageCabalFile'
+  , CabalFileParseError (..)
 
     -- * Packages within projects
   , ProjectPackageLocation (..)
@@ -174,7 +177,6 @@ import Distribution.Simple.Setup
 import Distribution.Simple.Utils
   ( createDirectoryIfMissingVerbose
   , dieWithException
-  , info
   , maybeExit
   , notice
   , rawSystemIOWithEnv
@@ -1612,10 +1614,22 @@ readSourcePackageCabalFile
   -> BS.ByteString
   -> IO GenericPackageDescription
 readSourcePackageCabalFile verbosity pkgfilename content =
+  readSourcePackageCabalFile' (warn verbosity) pkgfilename content
+
+-- | Like `readSourcePackageCabalFile`, but the `warn` function is an argument.
+--
+-- This is used when reading @.cabal@ files in indexes, where warnings should
+-- generally be ignored.
+readSourcePackageCabalFile'
+  :: (String -> IO ())
+  -> FilePath
+  -> BS.ByteString
+  -> IO GenericPackageDescription
+readSourcePackageCabalFile' logWarnings pkgfilename content =
   case runParseResult (parseGenericPackageDescription content) of
     (warnings, Right pkg) -> do
       unless (null warnings) $
-        info verbosity (formatWarnings warnings)
+        logWarnings (formatWarnings warnings)
       return pkg
     (warnings, Left (mspecVersion, errors)) ->
       throwIO $ CabalFileParseError pkgfilename content errors mspecVersion warnings

cabal-testsuite/PackageTests/BuildTargets/UseLocalPackageForSetup/pkg/pkg.cabal

@@ -1,7 +1,7 @@
 name: pkg
 version: 1.0
 build-type: Custom
-cabal-version: >= 1.20
+cabal-version: 1.20
 
 custom-setup
   setup-depends: base, Cabal, setup-dep == 2.*

cabal-testsuite/PackageTests/BuildTargets/UseLocalPackageForSetup/repo/setup-dep-1.0/setup-dep.cabal

@@ -1,7 +1,7 @@
 name: setup-dep
 version: 1.0
 build-type: Simple
-cabal-version: >= 1.20
+cabal-version: 1.20
 
 library
   build-depends: base

cabal-testsuite/PackageTests/BuildTargets/UseLocalPackageForSetup/setup-dep/setup-dep.cabal

@@ -1,7 +1,7 @@
 name: setup-dep
 version: 2.0
 build-type: Simple
-cabal-version: >= 1.20
+cabal-version: 1.20
 
 library
   build-depends: base

cabal-testsuite/PackageTests/IndexCabalFileParseError/cabal.out

@@ -0,0 +1,19 @@
+# cabal v2-update
+Downloading the latest package list from test-local-repo
+Warning: In <ROOT>/cabal.dist/repo/my-lib-1.0.tar.gz: Errors encountered when parsing cabal file my-lib-1.0/my-lib.cabal:
+
+my-lib-1.0/my-lib.cabal:4:22: error:
+unexpected Unknown SPDX license identifier: 'puppy' 
+
+    3 | version:        1.0
+    4 | license:        puppy license :)
+      |                      ^
+
+my-lib-1.0/my-lib.cabal:5:1: warning:
+Unknown field: "puppy"
+
+    4 | license:        puppy license :)
+    5 | puppy:          teehee!
+      | ^
+Error: [Cabal-7046]
+Failed to read my-lib-1.0/my-lib.cabal from archive <ROOT>/cabal.dist/repo/my-lib-1.0.tar.gz

cabal-testsuite/PackageTests/IndexCabalFileParseError/cabal.test.hs

@@ -0,0 +1,4 @@
+import Test.Cabal.Prelude
+
+main = cabalTest $ withRepoNoUpdate "repo" $ do
+    fails $ cabal "v2-update" []

cabal-testsuite/PackageTests/IndexCabalFileParseError/repo/my-lib-1.0/my-lib.cabal

@@ -0,0 +1,5 @@
+cabal-version:  3.0
+name:           my-lib
+version:        1.0
+license:        puppy license :)
+puppy:          teehee!

cabal-testsuite/PackageTests/NewBuild/CmdRun/Script/cabal.out

@@ -1,4 +1,5 @@
 # cabal v2-run
+Warning: The package description file ./script.cabal has warnings: script.cabal:0:0: A package using 'cabal-version: >=1.10' must use section syntax. See the Cabal user guide for details.
 Configuration is affected by the following files:
 - cabal.project
 Resolving dependencies...

cabal-testsuite/PackageTests/NewBuild/CmdRun/ScriptBad/cabal.out

@@ -1,4 +1,5 @@
 # cabal v2-run
+Warning: The package description file ./script.cabal has warnings: script.cabal:0:0: A package using 'cabal-version: >=1.10' must use section syntax. See the Cabal user guide for details.
 Configuration is affected by the following files:
 - cabal.project
 Error: [Cabal-7121]

cabal-testsuite/PackageTests/NewBuild/CmdRun/ScriptLiterate/cabal.out

@@ -1,4 +1,5 @@
 # cabal v2-run
+Warning: The package description file ./script.cabal has warnings: script.cabal:0:0: A package using 'cabal-version: >=1.10' must use section syntax. See the Cabal user guide for details.
 Configuration is affected by the following files:
 - cabal.project
 Resolving dependencies...

cabal-testsuite/PackageTests/NewConfigure/ConfigFile/cabal.out

@@ -1,4 +1,5 @@
 # cabal v2-configure
+Warning: The package description file ./ConfigFile.cabal has warnings: ConfigFile.cabal:0:0: A package using 'cabal-version: >=1.10' must use section syntax. See the Cabal user guide for details.
 Configuration is affected by the following files:
 - cabal.project
 Config file not written due to flag(s).

cabal-testsuite/PackageTests/NewFreeze/BuildTools/new_freeze.out

@@ -1,6 +1,7 @@
 # cabal v2-update
 Downloading the latest package list from test-local-repo
 # cabal v2-build
+Warning: The package description file ./my-local-package.cabal has warnings: my-local-package.cabal:3:23: Packages with 'cabal-version: 1.12' or later should specify a specific version of the Cabal spec of the form 'cabal-version: x.y'. Use 'cabal-version: 1.20'.
 Configuration is affected by the following files:
 - cabal.project
 Resolving dependencies...
@@ -16,6 +17,7 @@ Configuration is affected by the following files:
 Resolving dependencies...
 Wrote freeze file: <ROOT>/cabal.project.freeze
 # cabal v2-build
+Warning: The package description file ./my-local-package.cabal has warnings: my-local-package.cabal:3:23: Packages with 'cabal-version: 1.12' or later should specify a specific version of the Cabal spec of the form 'cabal-version: x.y'. Use 'cabal-version: 1.20'.
 Configuration is affected by the following files:
 - cabal.project
 - cabal.project.freeze

cabal-testsuite/PackageTests/Regression/T7234/Fail/cabal.out

@@ -1,4 +1,5 @@
 # cabal v2-build
+Warning: The package description file ./issue7234.cabal has warnings: issue7234.cabal:13:3: The field "other-extensions" is available only since the Cabal specification version 1.10.
 Configuration is affected by the following files:
 - cabal.project
 Resolving dependencies...

cabal-testsuite/PackageTests/Regression/T7234/Success/cabal.out

@@ -1,4 +1,5 @@
 # cabal v2-build
+Warning: The package description file ./issue7234.cabal has warnings: issue7234.cabal:14:3: The field "other-extensions" is available only since the Cabal specification version 1.10.
 Configuration is affected by the following files:
 - cabal.project
 Resolving dependencies...

cabal-testsuite/PackageTests/Regression/T9640/cabal.out

@@ -1,6 +1,7 @@
 # cabal v2-update
 Downloading the latest package list from test-local-repo
 # cabal build
+Warning: The package description file ./depend-on-custom-with-exe.cabal has warnings: depend-on-custom-with-exe.cabal:16:1: Ignoring trailing fields after sections: "ghc-options"
 Configuration is affected by the following files:
 - cabal.project
 Resolving dependencies...

cabal-testsuite/src/Test/Cabal/Prelude.hs

@@ -564,11 +564,9 @@ src `archiveTo` dst = do
 
 infixr 4 `archiveTo`
 
--- | Given a directory (relative to the 'testCurrentDir') containing
--- a series of directories representing packages, generate an
--- external repository corresponding to all of these packages
-withRepo :: FilePath -> TestM a -> TestM a
-withRepo repo_dir m = do
+-- | Like 'withRepo', but doesn't run @cabal update@.
+withRepoNoUpdate :: FilePath -> TestM a -> TestM a
+withRepoNoUpdate repo_dir m = do
     env <- getTestEnv
 
     -- 1. Initialize repo directory
@@ -606,11 +604,7 @@ withRepo repo_dir m = do
     liftIO $ print $ testUserCabalConfigFile env
     liftIO $ print =<< readFile (testUserCabalConfigFile env)
 
-    -- 4. Update our local index
-    -- Note: this doesn't do anything for file+noindex repositories.
-    cabal "v2-update" ["-z"]
-
-    -- 5. Profit
+    -- 4. Profit
     withReaderT (\env' -> env' { testHaveRepo = True }) m
     -- TODO: Arguably should undo everything when we're done...
   where
@@ -620,6 +614,17 @@ withRepo repo_dir m = do
                                             _ -> x)
                                         else id) (testRepoDir env)
 
+-- | Given a directory (relative to the 'testCurrentDir') containing
+-- a series of directories representing packages, generate an
+-- external repository corresponding to all of these packages
+withRepo :: FilePath -> TestM a -> TestM a
+withRepo repo_dir m = do
+    withRepoNoUpdate repo_dir $ do
+        -- Update our local index
+        -- Note: this doesn't do anything for file+noindex repositories.
+        cabal "v2-update" ["-z"]
+        m
+
 -- | Given a directory (relative to the 'testCurrentDir') containing
 -- a series of directories representing packages, generate an
 -- remote repository corresponding to all of these packages
@@ -798,17 +803,31 @@ recordMode mode = withReaderT (\env -> env {
 assertOutputContains :: MonadIO m => WithCallStack (String -> Result -> m ())
 assertOutputContains needle result =
     withFrozenCallStack $
-    unless (needle `isInfixOf` (concatOutput output)) $
+    unless (needle `isInfixOf` concatOutput output) $
     assertFailure $ " expected: " ++ needle
   where output = resultOutput result
 
 assertOutputDoesNotContain :: MonadIO m => WithCallStack (String -> Result -> m ())
 assertOutputDoesNotContain needle result =
     withFrozenCallStack $
-    when (needle `isInfixOf` (concatOutput output)) $
+    when (needle `isInfixOf` concatOutput output) $
     assertFailure $ "unexpected: " ++ needle
   where output = resultOutput result
 
+assertOutputMatches :: MonadIO m => WithCallStack (String -> Result -> m ())
+assertOutputMatches regex result =
+    withFrozenCallStack $
+    unless (concatOutput output =~ regex) $
+    assertFailure $ "expected regex match: " ++ regex
+  where output = resultOutput result
+
+assertOutputDoesNotMatch :: MonadIO m => WithCallStack (String -> Result -> m ())
+assertOutputDoesNotMatch regex result =
+    withFrozenCallStack $
+    when (concatOutput output =~ regex) $
+    assertFailure $ "unexpected regex match: " ++ regex
+  where output = resultOutput result
+
 assertFindInFile :: MonadIO m => WithCallStack (String -> FilePath -> m ())
 assertFindInFile needle path =
     withFrozenCallStack $

changelog.d/pr-10647

@@ -0,0 +1,10 @@
+---
+synopsis: 'Improve "Cannot read .cabal file inside ..." errors'
+packages: [cabal-install]
+prs: 10647
+---
+
+The error message printed when Cabal fails to read a `.cabal` file inside an
+index (like the Hackage index) has been greatly improved. Additionally,
+warnings in `.cabal` files in indexes are printed instead of being silently
+discarded.