Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exe:cabal doesn't properly ignore .cabal files with unsupported cabal-version: #4624

Closed
hvr opened this issue Jul 24, 2017 · 23 comments
Closed

Comments

@hvr
Copy link
Member

hvr commented Jul 24, 2017

This is causing problems for e.g. cabal-install-1.24 (and older versions as well), which fails to parse bleeding edge .cabal files.

So e.g.

cabal-1.24 info base-noprelude-4.10.0.0

succeeds, even though base-noprelude-4.10.0.0 would require cabal-2.0 to be properly decodable.

But there's more harmful cases, like e.g. for

cabal-1.24 info haddock-library
cabal: internal error when reading package index: failed to parse .cabal file
The package index or index cache is probably corrupt. Running cabal update might fix it.

or even cabal-1.24 list which fails as soon as such a .cabal file is encountered.

Or even worse, cause breakages such as in jgm/pandoc#3814 which render cabal-1.24 unusable.

In the past this would have been mitigated by having cabal update remind users to update, e.g.

$ cabal-1.20 update
Downloading the latest package list from hackage.haskell.org
Skipping download: Local and remote files match.
Note: there is a new version of cabal-install available.
To upgrade, run: cabal install cabal-install

How to fix (imho)

Quickfix

  • ignore .cabal files in the index we can't parse (i.e. which the parser currently fails with an "internal error")
  • possibly, for those .cabal files we can parse but have a too-new cabal-version, ignore them as well

(see also #4624 (comment))

Proper fix

For packages that are newer than cabal-install supports, we can only know the package name & version, and possibly the specified cabal-version. Everything beyond that would require the parser to know how to parse and interpret the .cabal contents (note that semantics may change depending on the cabal-version, so we really cannot know what a .cabal files means if we don't actively support the declared cabal-version).

I've looked at the code in D.C.IndexUtils, and one way to go about this is to do something along the lines of

--- a/cabal-install/Distribution/Client/IndexUtils.hs
+++ b/cabal-install/Distribution/Client/IndexUtils.hs
@@ -56,7 +56,7 @@ import Distribution.Simple.Program
 import qualified Distribution.Simple.Configure as Configure
          ( getInstalledPackages, getInstalledPackagesMonitorFiles )
 import Distribution.ParseUtils
-         ( ParseResult(..) )
+         ( ParseResult(..), PError(FromString) )
 import Distribution.Version
          ( Version(Version), intersectVersionRanges )
 import Distribution.Text
@@ -254,7 +254,7 @@ whenCacheOutOfDate index action = do
 
 -- | An index entry is either a normal package, or a local build tree reference.
 data PackageEntry =
-  NormalPackage  PackageId GenericPackageDescription ByteString BlockNo
+  NormalPackage  PackageId (Maybe GenericPackageDescription) ByteString BlockNo
   | BuildTreeRef BuildTreeRefType
                  PackageId GenericPackageDescription FilePath   BlockNo
 
@@ -541,13 +542,16 @@ packageListFromCache mkPkg hnd Cache{..} mode = accum mempty [] cacheEntries
           -> return content
         _ -> interror "unexpected tar entry type"
 
-    readPackageDescription :: ByteString -> IO GenericPackageDescription
+    readPackageDescription :: ByteString -> IO (Maybe GenericPackageDescription)
     readPackageDescription content =
       case parsePackageDescription . ignoreBOM . fromUTF8 . BS.Char8.unpack $ content of
-        ParseOk _ d -> return d
-        _           -> interror "failed to parse .cabal file"
+        ParseOk _ d -> return (Just $! d)
+        ParseFailed (FromString e _)
+            | "This package requires at least Cabal version" `isPrefixOf` e
+               -> return Nothing
+        _      -> interror "failed to parse .cabal file"

I.e. when a GenericPackageDescription cannot be parsed, turn it into a Nothing (or something with more information, providing more details about the minimum required version).

Then we could have the solver handle "future" packages by placing them into a blacklist
(as if they had an unconditional fail: this package requires a newer cabal version, see #393) - or we could just fake a minimal GenericPackageDescription which consists merely of an empty lib and exe with such a fail directive. Actually, would it work already now if we just faked a dummy GenericPackageDescription with a too-new spec-cabal-version?

Anyway, then the solver would avoid selecting such packages, and if forced to (freeze file, --constraints, or merely cabal install foo-1.2.3), would present the user with an informative error message which points to a possible resolution.

Commands like cabal info or cabal list would have to gracefully skip, ignore, or if forced to (cabal info foo-1.2.3), emit useful messages.

/cc @grayjay

@jgm
Copy link

jgm commented Jul 24, 2017

Note that this also seems to affect cabal-1.22 and cabal-1.18, as the pandoc travis builds show.

@hvr
Copy link
Member Author

hvr commented Jul 24, 2017

Indeed. We may require to release hotfixes to a couple of older major versions of cabal-install, so Linux distributions can pick it up.

/cc @23Skidoo

@23Skidoo
Copy link
Member

:-(

I'll try to fix this (and the remaining exe:cabal-install-2.0 blockers) during this week.

@23Skidoo 23Skidoo self-assigned this Jul 24, 2017
@hvr
Copy link
Member Author

hvr commented Jul 25, 2017

@23Skidoo here's an alternative & complementary idea: older cabals (i.e. cabal-install-1.22 and older) only know about the 00-index.tar.gz; so we could remove cabal-version:>=2.0 files from the 00-index.tar.gz server-side; so only 01-index.tar.gz would contain .cabal files requiring cabal>=2. I've looked at hackage-server's code, and this requires a bit of work to get implemented...

Then we'd only need a quickfix for cabal-install-1.24, and a proper fix for cabal-install-2.0+ (so we won't run into this when we extend the .cabal format again in 2.2)

@sol
Copy link
Member

sol commented Jul 26, 2017

@RyanGlScott I'm not sure if ekmett/semigroupoids@ef085d1 is a viable solution. As I understand it masks the problem on Travis, but users that use a released version of cabal-install are still affected.

@hvr for now I'm avoiding to run cabal update. Which packages on Hackage cause this issue? Wouldn't we want to blacklist (or revise) those packages for now?

@gallais
Copy link

gallais commented Jul 26, 2017

Which packages on Hackage cause this issue?

Yesterday I ran a small script untarring and grepping for these cabal-version: >=2.0 constraints and the packages having them were: haddock-api, haddock-library, haddock, and base-noprelude

@ezyang
Copy link
Contributor

ezyang commented Jul 27, 2017

I don't think this is a blocker for 2.0. We can always get the fix which solves cabal-version: >= 2.2 into cabal-install-2.0 into a bugfix release before Cabal-2.2 comes out. In the meantime, not having a cabal-install-2.0 means that it's not possible to work around this bug by installing the latest cabal-install.

@RyanGlScott
Copy link
Member

@sol, to be clear, I don't intend for ekmett/semigroupoids@ef085d1 to be a permanent fix. I only committed that to repair Travis breakage after a previous commit I made, and I needed to bandage the wound to save face.

That being said, I've pretty much stopped making commits to most of my Haskell projects until this issue is fixed so as not to break things further.

@hvr
Copy link
Member Author

hvr commented Jul 28, 2017

@RyanGlScott et al., I've deployed a server-side workaround for legacy cabal-install clients (i.e. before 2.0). Can you give it a try and see if this fixes more than it breaks?

RyanGlScott added a commit to ekmett/semigroupoids that referenced this issue Jul 28, 2017
asr added a commit to asr/my-agda that referenced this issue Jul 28, 2017
@RyanGlScott
Copy link
Member

I can confirm that the cabal-install-1.24-based Travis build for semigroupoids is working again. Thanks, @hvr!

@asr
Copy link
Contributor

asr commented Jul 28, 2017

I also confirm that the issue is fixed (cabal-install 1.24, GHC 7.8.4, Travis). Thanks!

asr added a commit to agda/agda that referenced this issue Jul 28, 2017
@jgm
Copy link

jgm commented Jul 30, 2017 via email

@23Skidoo 23Skidoo added this to the 2.0.1 milestone Jul 31, 2017
@robinp
Copy link

robinp commented Aug 25, 2017

@hvr was the server-side fix on hackage? Interestingly the default Travis config (which used fpcomplete mirror) still failed (workaround in robinp/haskell-indexer@9f9eddb). Is this possible?

robinp added a commit to robinp/haskell-indexer that referenced this issue Aug 25, 2017
Actually the issue is worked around on hackage-side, but it seems not in
the fpcomplete mirror.
See haskell/cabal#4624 (comment).
@hvr
Copy link
Member Author

hvr commented Aug 25, 2017

@robinp it appears that the fpcomplete mirror doesn't faithfully mirror the legacy package index 00-index.tar.gz; in fact if you compare

to each other, you'll notice they have different contents, and in fact fpcomplete's one is 25MiB large, while hackage's is 15MiB large.

robinp added a commit to google/haskell-indexer that referenced this issue Aug 25, 2017
Actually the issue is worked around on hackage-side, but it seems not in
the fpcomplete mirror.
See haskell/cabal#4624 (comment).
psibi added a commit to psibi/xmonad-extras that referenced this issue Sep 3, 2017
snoyberg added a commit to commercialhaskell/all-cabal-tool that referenced this issue Sep 7, 2017
@snoyberg
Copy link
Collaborator

snoyberg commented Sep 7, 2017

I was just pointed at this issue. To give an update on the FP Complete mirror and explanation of what's happening:

  • Our mirror previously simply copied all relevant files from hackage.haskell.org to S3
  • I was informed at some point that, despite connecting to hackage.haskell.org over HTTPS, the connection between the CDN hosting that domain and the Hackage origin server was an insecure HTTP connection
  • Therefore, we moved over to @hvr's Hackage mirror tool, which used Hackage Security (HS) to verify the downloads
  • Unfortunately, that tool cannot mirror 00-index.tar.gz using HS, since HS only deals with the 01-index.tar.gz file
  • Therefore, we created our own 00-index.tar.gz file from the contents of 01-index.tar.gz. At the time I discussed this with @hvr, this would "result in the same filesystem contents."

Two things have changed since that solution was put in place:

  • It's now possible to connect to Hackage Origin securely (either directly, or via the CDN which now uses a secure connection too)
  • This issue changed the invariant that allowed 00-index.tar.gz to be created from 01-index.tar.gz

So to address this, I've pushed a small patch to our all-cabal-tool utility (the thing responsible for 00-index.tar.gz and a few other things) to just securely download and upload that file: commercialhaskell/all-cabal-tool@9bfe3e6.

I've just confirmed, and now the FP Complete S3 mirror's 00-index.tar.gz matches the file on Hackage.

@snoyberg
Copy link
Collaborator

snoyberg commented Sep 7, 2017

As a side note: I mentioned on Reddit the idea of having a grace period after a new release of the Cabal library to allow tooling a chance to catch up before the new format is allowed on Hackage. There was a mixed reaction there (but completely negative from the Cabal devs that spoke up). This issue seems like a good argument in favor of that. I'd be happy to open up an issue if there's more support for the idea.

@23Skidoo
Copy link
Member

23Skidoo commented Sep 7, 2017

For reference, the server-side fix referred to above is haskell/hackage-server@9a6be09.

@hvr
Copy link
Member Author

hvr commented Sep 7, 2017

I'm not sure what gave you the perception there were "a mixed reaction", but from my POV it was a polite but clearly negative reaction. I can imagine why you'd want this, but I see no reason for a "grace period", our development cycles are already overly prolonged since we're coupled to GHC releases; we also have rather long pre-release and RC phases for that already which is intended to give tooling the chance to adapt and test things, and I try to field-test & dogfood as much as possible myself. Moreover, a grade period would most likely not have helped to prevent the problems at hand and I've learned from this; and I have a different strategy to prevent issues like the one in this issue in future. Consequently, I strongly oppose any "grace period" as I see no compelling reason for it, as the main result of such a protectionism measure would be to delay features becoming available to Hackage/Cabal users while waiting for 3rd party tools to slowly catch up.

@23Skidoo
Copy link
Member

23Skidoo commented Sep 7, 2017

I'm also not too hot on the grace period idea, but that's more of a hackage-server issue, so let's keep the discussion on-topic.

@23Skidoo 23Skidoo modified the milestones: 2.0.1, 2.0.2 Sep 19, 2017
@23Skidoo 23Skidoo assigned hvr and unassigned 23Skidoo Sep 19, 2017
@bgamari
Copy link
Contributor

bgamari commented Oct 6, 2017

What is the status of this? The next GHC release candidate will likely be in a few weeks and I would prefer to avoid submodule bumps after it goes out.

@hvr
Copy link
Member Author

hvr commented Oct 6, 2017

@bgamari I've got a working proof of concept, and it doesn't require changes to lib:Cabal; you should however keep an eye on the #4808 regression which can be considered a release-blocker for Cabal-2.0.1

@23Skidoo 23Skidoo modified the milestones: 2.0.1, 2.0.2 Dec 4, 2017
@23Skidoo
Copy link
Member

23Skidoo commented Dec 8, 2017

@hvr I guess this can be closed in favour of #4899?

@hvr
Copy link
Member Author

hvr commented Dec 8, 2017

@23Skidoo Indeed; everything's been put in place for the officially released cabal versions; it's just cabal-2.2 that needs a little bit of work.

@hvr hvr closed this as completed Dec 8, 2017
ZenithalHourlyRate added a commit to tuna/tunasync-scripts that referenced this issue Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests