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.

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

GHC 8.10 and 9.0 Compatibility #131

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DrewFenwick
Copy link

@DrewFenwick DrewFenwick commented Oct 19, 2021

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.

Add a field to the stickers object datatype,
which will require a minor version bump.

Fix an assumption of successful response in /getFile test.

Fix an outdated dependence on SpecWith satisfying MonadFail.
Add compatibility with servant-client 0.17 and 0.18

Update to newer version of cabal file spec.

Add upper and lower bounds on all dependencies.
Builds under GHC 8.6.5 through 9.0.1
Should be compatible with Stackage lts 14.0 through 18.16
@DrewFenwick DrewFenwick changed the title Bound bump GHC 8.10 and 9.0 Compatibility Oct 19, 2021
@ppmdo
Copy link

ppmdo commented Nov 6, 2021

Hey! Thanks a lot for the effort.

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

@nomeata
Copy link

nomeata commented Jun 12, 2022

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
Copy link

nomeata commented Jun 12, 2022

Ok, that one is easy to fix;

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
Copy link

nomeata commented Jun 12, 2022

This might work? It typechecks :-)

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

nomeata added a commit to nomeata/kaleidogen that referenced this pull request Jun 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GHC 8.10 and 9.0 support Too strict version bounds on aeson
3 participants