axman6 / amazonka-s3-streaming

Provides a conduit based interface to uploading data to S3 using the Multipart API
MIT License
20 stars 23 forks source link

Support conduit-1.3 #12

Closed fumieval closed 5 years ago

fumieval commented 6 years ago

They changed the API to use MonadUnliftIO rather than MonadBaseControl IO.

axman6 commented 6 years ago

Hi @fumieval, is it possible to keep this working for older versions of conduit too? The changes look minor so CPP should be sufficient.

fumieval commented 6 years ago

@axman6 Sure, I've just added conditionals

domenkozar commented 6 years ago

@fumieval CI seems to fail with:


src/Network/AWS/S3/StreamingUpload.hs:252:20: error:
    • Could not deduce (MonadUnliftIO
                          (Control.Monad.Trans.AWS.AWST'
                             Network.AWS.Env.Env
                             (Control.Monad.Trans.Resource.Internal.ResourceT IO)))
        arising from a use of ‘forConcurrently’
      from the context: (MonadAWS m, MonadUnliftIO m)
        bound by the type signature for:
                   concurrentUpload :: (MonadAWS m, MonadUnliftIO m) =>
                                       Maybe ChunkSize
                                       -> Maybe NumThreads
                                       -> UploadLocation
                                       -> CreateMultipartUpload
                                       -> m CompleteMultipartUploadResponse
        at src/Network/AWS/S3/StreamingUpload.hs:(214,1)-(222,53)
    • In the expression:
        forConcurrently (zip [1 .. ] $ chunksOf chunkSize bs)
      In the expression:
        forConcurrently (zip [1 .. ] $ chunksOf chunkSize bs)
        $ \ (partnum, b)
            -> do { liftIO $ waitQSem sem;
                    logStr $ "Starting part: " ++ show partnum;
                    .... }
      In the expression:
        let chunkSize = calcChunkSize $ BS.length bs
        in
          forConcurrently (zip [1 .. ] $ chunksOf chunkSize bs)
          $ \ (partnum, b)
              -> do { liftIO $ waitQSem sem;
                      .... }
fumieval commented 6 years ago

I'm not sure how to deal with this until a newer version of amazonka containing #461 is released.

axman6 commented 6 years ago

yeah I haven't got around to releasing anything because I wasn't sure how to keep this library working with older and newer conduit versions.

axman6 commented 6 years ago

Hmm, somehow I had missed the changes you made to the cabal file, which looks like they should work. I really wish there was a way to conditionally do things in cabal files based on package versions, rather than needing a manual flag.

AlexeyRaga commented 6 years ago

So what is the strategy now? I think cabal flags is a better alternative than forking this library, applying this patch and using it from GitHub (seems that it is what I'll have to do now) Another way would be to release a new major version that only supports Amazonka >=1.6 and then it is up to users to make a choice which version of amazonka-s3-streaming to use?

Either way it would be very helpful to have the release.

domenkozar commented 5 years ago

I've opened https://github.com/brendanhay/amazonka/issues/494 in hope to move this forward.

domenkozar commented 5 years ago

What if we add an orphan instance for MonadUnliftIO when using amazonka 1.6 and lower? Maintenance of amazonka seems to have stalled.

domenkozar commented 5 years ago

@axman6 what if only conduit 1.3 and newer is supported? If someone needs lower version, they can use old releases of amazonka-s3-streaming.

domenkozar commented 5 years ago

@axman6 would you accept a PR if I go ahead with that approach?

axman6 commented 5 years ago

Hey mate, sorry for the delay, I think this package could actually do with a pretty substantial rewrite, it's got far more dependencies than it should given what it does. I think in the process of doing that I'd likely make it conduit >= 1.3 only. Not sure when I'll have time for that in the next few weeks though.

domenkozar commented 5 years ago

Thanks. Unless someone beats me to it, I'll do the conduit >= 1.3 in a couple of weeks.

axman6 commented 5 years ago

I've released v1.0 which only supports conduit >= 1.3. Would love to hear how this is being used by people, turns out it's more widely used than I knew.