Support .tar archives #3647 #3655
Conversation
src/Stack/PackageLocation.hs
Outdated
| @@ -120,14 +120,20 @@ resolveSinglePackageLocation projRoot (PLArchive (Archive url subdir msha)) = do | |||
| archive <- fmap Zip.toArchive $ liftIO $ L.readFile fp | |||
| liftIO $ Zip.extractFilesFromArchive [Zip.OptDestination | |||
| (toFilePath dirTmp)] archive | |||
| tryTar = do | |||
| logDebug $ "Trying to untar (no ungzip) " <> T.pack fp | |||
| liftIO $ withBinaryFile fp ReadMode $ \h -> do | |||
There was a problem hiding this comment.
I think the current code will end up reading the file 3 times. Though perhaps it wouldn't be as bad as that, since the decoding should fail quickly.
Might be worth pulling withBinaryFile + hGetContents out to the top level. Could increase memory residency, but that seems fine.
I'd actually lean towards using something like L.readFile, but I guess the exception handling isn't so great there w.r.t. closing the handle. This is a side note, but I'm a bit surprised the code wouldn't use a weak pointer and also close the handle of the lbs is GCed.
Note that L.readFile is used above in the unzip code.
There was a problem hiding this comment.
I'd much rather have multiple file reads than potentially unbounded memory usage, though in this case unbounded is basically impossible: parsing should fail immediately. I'll update this to do one read. Note that on master, I've already changed this to use a new helper withLazyFile instead of explicit withBinaryFile and hGetContents.
There was a problem hiding this comment.
Cool, makes sense!
Why merge this into stable instead of master?
There was a problem hiding this comment.
I was thinking that it's a small enough change to include in a patch. Since we're actively planning on such a patch, I'm trying to include as many minor, backwards compatible changes as I can. You think it's better to put this one on master?
There was a problem hiding this comment.
Seems fine to me to have it in the patch version to, just curious
Note: Documentation fixes for https://docs.haskellstack.org/en/stable/ should target the "stable" branch, not master.
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!