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.

Sign up for GitHub

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

Support .tar archives #3647 #3655

Merged
merged 2 commits into from
Dec 15, 2017
+28 −14
Merged

Support .tar archives #3647 #3655

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8de003
Support .tar archives #3647
snoyberg Dec 14, 2017
563a5f8
Share file read for archives
snoyberg Dec 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ChangeLog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ Behaviour changes:

Other enhancements:

* In addition to supporting `.tar.gz` and `.zip` files as remote archives,
plain `.tar` files are now accepted too. This will additionally help with
cases where HTTP servers mistakenly set the transfer encoding to `gzip`. See
[#3647](https://github.com/commercialhaskell/stack/issues/3647).

Bug fixes:

* For versions of Cabal before 1.24, ensure that the dependencies of
Expand Down
12 changes: 9 additions & 3 deletions src/Stack/PackageLocation.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ resolveSinglePackageLocation projRoot (PLArchive (Archive url subdir msha)) = do

let fp = toFilePath file

let tryTar = do
logDebug $ "Trying to untar " <> T.pack fp
let tryTargz = do
logDebug $ "Trying to ungzip/untar " <> T.pack fp
liftIO $ withBinaryFile fp ReadMode $ \h -> do
lbs <- L.hGetContents h
let entries = Tar.read $ GZip.decompress lbs
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, makes sense!

Why merge this into stable instead of master?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me to have it in the patch version to, just curious

lbs <- L.hGetContents h
let entries = Tar.read lbs
Tar.unpack (toFilePath dirTmp) entries
err = throwM $ UnableToExtractArchive url file

catchAnyLog goodpath handler =
catchAny goodpath $ \e -> do
logDebug $ "Got exception: " <> T.pack (show e)
handler

tryTar `catchAnyLog` tryZip `catchAnyLog` err
tryTargz `catchAnyLog` tryZip `catchAnyLog` tryTar `catchAnyLog` err
renameDir dirTmp dir

x <- listDir dir
Expand Down