search

snoyberg / tar-conduit

Conduit based tar extraction mechanism
MIT License
8 stars 9 forks source link

Discard payload following unsupported headers #18

Closed anakos closed 6 years ago

anakos commented 6 years ago

As per https://github.com/snoyberg/tar-conduit/issues/17, drops subsequent ChunkPayloads following a discarded ChunkHeader.

lehins commented 6 years ago

Implementation itself looks legit, but the test suite is broken for older versions of conduit:

/home/travis/build/snoyberg/tar-conduit/tests/Spec.hs:249:6: error:
    Not in scope: type constructor or class ‘ConduitT’
    Perhaps you meant one of these:
      ‘Conduit’ (imported from Conduit),
      ‘ConduitM’ (imported from Conduit)
snoyberg commented 6 years ago

@anakos Switching to ConduitM instead of ConduitT should make it compatible with both versions of the library.

anakos commented 6 years ago

Will update and push changes ASAP

anakos commented 6 years ago

@snoyberg / @lehins - I have pushed changes, but having some trouble with the build on Travis. There are 2 distinct sets of failures so far as I can tell:

  1. The GHC builds consistently fail because the tar file used by the test I introduced does not exist:

    uncaught exception: IOException of type NoSuchThing
./tests/files/libpq-0.3.tar.gz: openBinaryFile: does not exist (No such file or directory)

    Is this because cabal is using the tarball that would be fetched from hackage (and thusly would not contain the test resources)? Any pointers on how best to resolve this?

  2. Two of the stack builds fail due to the conduit-combinators and safe-exceptions libraries not being present (lts-2, lts-3). Is it best to explicitly add these dependencies as per the recommendations from Stack or is there some other preferred approach to deal explicitly with these lts versions?

Thanks! Alex

snoyberg commented 6 years ago

For (1): you need to add the test tarball to the extra-source-files section in the .cabal file so that the test suite can use it. For (2): It's basically a spurious error because cabal's dependency solver is getting confused. I think we should simply prune our list of supported GHC versions. If you fix (1), I'd merge this PR and then clean up the dependency information to address (2) myself. (Unless you want to address (2) yourself as well, happy to give some more information on the glories of dependency management if you'd like 😄.)

anakos commented 6 years ago

@snoyberg - happy to take a stab at (2) if you can provide the relevant info. Thanks! Alex

snoyberg commented 6 years ago

Alright, brain dump on (2).

Stack requires that all of a package's dependencies be present in the stack.yaml file. For the older LTS snapshots, some of the dependencies are not present. To work with this situation, the Travis configuration uses the stack solver command to ask cabal-install to find a plan that'll work. This brings in all the flaws of dependency solving, such as non-determinism. Previously, this worked for lts-2 and lts-3. Recently, it stopped, for reasons I'm frankly not interested in (this is a common problem with dep solving).

There are a few different solutions we can take here:

I prefer going the third route, since it's essentially busywork to continue supporting older GHC releases. Even if people are using them, they have usually already chosen their set of dependencies and aren't expanding significantly.

So the steps here are:

  1. Determine which versions of GHC we support. I'd say let's take GHC 8 and up.

  2. Add a stanza like the following to the library section:

    if impl(ghc < 8)
  buildable: false
  3. Just to be extra careful, also ban base versions that shipped with GHC < 8. Using my helper page, that would mean base >= 4.9.
  4. Remove the relevant older LTSes and cabal configs from Travis.

Does that make sense?

Pinging @vincenthz: we should really write up that blog post we'd been discussing on dropping support for older GHC versions :)

anakos commented 6 years ago

@snoyberg - apologies for taking til now to get this done!

As per your guidance, following the steps listed above, I dropped all versions of GHC < 8. Had a bit of trouble with getting Travis to build, but got it working in the end.

I did add a build target for GHC 8.01 since it was missing - not sure if this was purposely omitted, but I added it just in case. Can remove if necessary.

Should build targets also be added for stack lts-11? Also, do you want me to squash the commits in this branch prior to merge?

Thanks for all your assistance with this! Alex

snoyberg commented 6 years ago

Looks good! I typically ignore older point releases of GHC, so having 8.0.2 means I won't include 8.0.1. But I have no objection to including here.

Adding in LTS 9 and 11 targets would definitely make sense here, I hadn't noticed that they were missing. Your call if they get included in this PR or I merge this and you open a new PR to add them. Thanks for all the work here!

anakos commented 6 years ago

I've added the targets here - probably best to just get it done at once. Thanks! Alex