klappvisor / haskell-telegram-api

Telegram Bot API for Haskell
BSD 3-Clause "New" or "Revised" License
201 stars 67 forks source link

GHC 8.10 and 9.0 Compatibility #131

Open DrewFenwick opened 3 years ago

DrewFenwick commented 3 years ago

This PR does four things:

  1. Repairs some tests
  2. Opens up compatibility with more servant-client versions
  3. Ups the cabal file spec version
  4. Updates dependency bounds

Thanks mostly thanks to the loosening of the servant constraints, I have been able to successfully build this package under GHC 8.6.5, 8.8.4, 8.10.7 and 9.0.1

Tests

The test repairs cover 3 problems:

  1. runIntegrationSpec expected SpecWith to satisfy the MonadFail constraint which isn't the case (at least with with modern hspec versions).
  2. The /getFile test assumed that it would get a successful response (which wasn't happening).
  3. Sticker tests were checking that a returned sticker was correct with an id field that can no-longer be considered unchanging for each sticker, so I added the sticker_file_unique_id field from the current version of the telegram API.

⚠The addition of the sticker_file_unique_id adds to the interface so at least a minor version bump will be required on release.

What this doesn't do is guarantee that all tests are working nor fix all failing tests that may be a result from incompatibilities with the current version of the Telegram API.

Servant-Client Compatibility

Most of the incompatibility seemed to come from dependence on internal servant modules which subsequently underwent minor changes like renamings.

The fixes should be simple and self-explanatory. All tests except 1 passed, which I believe to be is unrelated and caused by an outdated hardcoded file id

  test/TestCore.hs:15:15: 
  1) Main integration tests, /getFile, should get file
       predicate failed on: Left (FailureResponse (Request {requestPath = (BaseUrl {baseUrlScheme = Https, baseUrlHost = "api.telegram.org", baseUrlPort = 443, baseUrlPath = ""},"/bot1159302136:AAG8MeT_d3Ds2gPlso1Zur0yIBVOhR8bztk/getFile"), requestQueryString = fromList [("file_id",Just "AAQEABMXDZEwAARC0Kj3twkzNcMkAAIC")], requestBody = Nothing, requestAccept = fromList [application/json;charset=utf-8,application/json], requestHeaders = fromList []), requestHttpVersion = HTTP/1.1, requestMethod = "GET"} (Response {responseStatusCode = Status {statusCode = 400, statusMessage = "Bad Request"}, responseHeaders = fromList [("Server","nginx/1.18.0"),("Date","Mon, 18 Oct 2021 15:19:34 GMT"),("Content-Type","application/json"),("Content-Length","111"),("Connection","keep-alive"),("Strict-Transport-Security","max-age=31536000; includeSubDomains; preload"),("Access-Control-Allow-Origin","*"),("Access-Control-Expose-Headers","Content-Length,Content-Type,Date,Server,Connection")], responseHttpVersion = HTTP/1.1, responseBody = "{\"ok\":false,\"error_code\":400,\"description\":\"Bad Request: wrong file_id or the file is temporarily unavailable\"}"}))

This fixes #132

To maintain compatibility between both old and new versions of servant I had to resort to a little CPP usage. I'd prefer to avoid it since in my experience it tends to cause problems with tooling, but dropping compatability with servant-client 0.16 would also drop support with stackage LTS 14, 15 and 16 and therefore support for GHC 8.6 and 8.8 for stackage users.

I expect it may be possible to avoid depending on the internal modules to make this less of an issue.

Upping the Cabal Spec Version

The cabal file was originally on cabal-version >=1.10 Later cabal versions add convenient improvements to cabal file syntax and also provide more information so that tools lik HLS can function, IIRC, such as through the autogen-modules field.

I don't think it likely that there are any users out there who desperately need support for old versions of cabal or stack so I don't think it's too much of an issue to drop support for them.

As such I've upped the cabal-version to 3.4, and made some adjustments to make it compliant.

Dependency bounds.

I have added upper and lower bounds to every library dependency in accordance with the Hackage PVP. This will avoid tools resolving build plans that seem valid but lead to build errors.

These bounds are broad enough to build on GHC 8.6.5 through to GHC 9.0.1 according to my testing.

This fixes #130.

The dependency bounds on servant libs currently look a little odd:

, servant (>= 0.16 && < 0.18) || ^>= 0.18
, servant-client (>= 0.16 && < 0.18) || ^>= 0.18
, servant-client-core (>= 0.16 && < 0.18) || ^>= 0.18

This is because the ^>= 0.18 has subtly different semantics from < 0.19

I could've just done this:

, servant ^>= {0.16, 0.17, 0.18}
, servant-client ^>= {0.16, 0.17, 0.18}
, servant-client-core ^>= {0.16, 0.17, 0.18}

But as more major versions come along those sets are only going to get larger. I'm happy to change it if preferred.

ppmdo commented 3 years ago

Hey! Thanks a lot for the effort.

Are there any plans to merge this? The repo seems a bit abandonded.

nomeata commented 2 years ago

Thanks for this PR! I’m trying to use it with nixpks-22.05, but am hitting

[ 1 of 16] Compiling Servant.Client.MultipartFormData ( src/Servant/Client/MultipartFormData.hs, dist/build/Servant/Client/MultipartFormData.o )

src/Servant/Client/MultipartFormData.hs:94:49: error:
    Ambiguous occurrence ‘//’
    It could refer to
       either ‘Network.HTTP.Media.//’,
              imported from ‘Network.HTTP.Media’ at src/Servant/Client/MultipartFormData.hs:34:1-35
              (and originally defined in ‘Network.HTTP.Media.MediaType’)
           or ‘Servant.Client.//’,
              imported from ‘Servant.Client’ at src/Servant/Client/MultipartFormData.hs:39:1-31
              (and originally defined in ‘Servant.Client.Core.HasClient’)
   |
94 |                  Nothing -> pure $ "application"//"octet-stream"
   |                                                 ^^

(Ok, granted, I am forcing the use of the dependencies provided by nixpkgs-22.05, so I should probably investigate myself…)

As @ppmdo says, this repo seems to be relatively abandonned. Do you maybe want to take over the package, @DrewFenwick?

nomeata commented 2 years ago

Ok, that one is easy to fix;

Patch ```patch ~/build/haskell/haskell-telegram-api $ git diff diff --git a/src/Servant/Client/MultipartFormData.hs b/src/Servant/Client/MultipartFormData.hs index 1b6d17d..01e8f75 100644 --- a/src/Servant/Client/MultipartFormData.hs +++ b/src/Servant/Client/MultipartFormData.hs @@ -36,7 +36,7 @@ import Network.HTTP.Types import qualified Network.HTTP.Types as H import qualified Network.HTTP.Types.Header as HTTP import Servant.API -import Servant.Client +import Servant.Client hiding ((//)) import qualified Servant.Client.Core as Core import Servant.Client.Internal.HttpClient (catchConnectionError, clientResponseToResponse) #if !MIN_VERSION_servant_client(0,17,0) diff --git a/telegram-api.cabal b/telegram-api.cabal index 5d54cd7..e23ca17 100644 --- a/telegram-api.cabal +++ b/telegram-api.cabal @@ -47,13 +47,13 @@ library , Web.Telegram.API.Bot.API.Core , Servant.Client.MultipartFormData build-depends: base >= 4.7 && < 5 - , aeson ^>= {1.4.4, 1.5.6} - , containers ^>= 0.6.0 + , aeson ^>= {1.4.4, 1.5.6, 2.0.2} + , containers ^>= 0.6.0 , http-api-data ^>= 0.4.1 , http-client ^>= {0.6.4, 0.7.0} - , servant (>= 0.16 && < 0.18) || ^>= 0.18 - , servant-client (>= 0.16 && < 0.18) || ^>= 0.18 - , servant-client-core (>= 0.16 && < 0.18) || ^>= 0.18 + , servant (>= 0.16 && < 0.18) || ^>= {0.18, 0.19} + , servant-client (>= 0.16 && < 0.18) || ^>= {0.18, 0.19} + , servant-client-core (>= 0.16 && < 0.18) || ^>= {0.18, 0.19} , mtl ^>= 2.2.2 , text ^>= 1.2.3 , transformers ^>= 0.5.6 ```

But I am not sure what to do about

[ 1 of 16] Compiling Servant.Client.MultipartFormData ( src/Servant/Client/MultipartFormData.hs, dist/build/Servant/Client/MultipartFormData.o )

src/Servant/Client/MultipartFormData.hs:55:10: warning: [-Wmissing-methods]
    • No explicit implementation for
        ‘hoistClientMonad’
    • In the instance declaration for
        ‘HasClient m (MultipartFormDataReqBody b :> Post cts' a)’
   |
55 | instance (Core.RunClient m, ToMultipartFormData b, MimeUnrender ct a, cts' ~ (ct ': cts)
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...

and unfortunately, https://hackage.haskell.org/package/servant-client-core-0.19/changelog does not give any migration advise.

nomeata commented 2 years ago

This might work? It typechecks :-)

#if MIN_VERSION_servant_client(0,19,0)
  hoistClientMonad _ _ _ ma = ma
#endif