Skip to content

Fix for GHC 9.2 / template-haskell-2.18 #1338

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

Closed
wants to merge 7 commits into from
Closed

Fix for GHC 9.2 / template-haskell-2.18 #1338

wants to merge 7 commits into from

Conversation

brandon-leapyear
Copy link
Contributor

@brandon-leapyear brandon-leapyear commented Nov 15, 2021

This is an old work account. Please reference @brandonchinn178 for all future communication


Forked off of #1335 to fix Database.Persist.TH errors and CI errors. Feel free to merge this back into the other PR

Comment on lines +55 to +62
# When the following PRs are merged, add a source-repository-package
# stanza in cabal.project for them.
# * https://github.com/yesodweb/shakespeare/pull/260
# * https://github.com/DanBurton/haskell-src-meta/pull/23
# * https://github.com/haskell-foundation/foundation/pull/555
# * https://github.com/vincenthz/hs-memory/pull/87
# * https://github.com/haskell-crypto/cryptonite/pull/354
# - "9.2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cabal doesn't support this like Stack does: haskell/cabal#7821. I don't want to specify the forked repos in cabal.project, though, because when these PRs are merged, the forked repos could be deleted

I've tested this manually with stack build --test --no-run-tests --bench --no-run-benchmarks and it builds

@hololeap
Copy link

hololeap commented Nov 29, 2021

I'm getting a test failure on this branch with ghc-9.2.1 and aeson-2.0.2.0:

Failures:

  test/Database/Persist/ClassSpec.hs:14:21:
  1) Database.Persist.Class.PersistField.UTCTime fromPersistValue with format
       expected: Right 2018-02-27 10:49:42.123 UTC
        but got: Right 2018-02-27 10:49:42 UTC

  To rerun use: --match "/Database/Persist/Class/PersistField/UTCTime/fromPersistValue with format/"

Randomized with seed 947093686

@brandon-leapyear
Copy link
Contributor Author

brandon-leapyear commented Nov 29, 2021 via email

@hololeap
Copy link

It's hard to know because master will not build with ghc-9.2 and aeson-2. I can say that master builds fine with ghc-8.10.6 and aeson-1.5.6.0 and all the tests pass. I am not necessarily saying that the changes made in this PR are the source of this failing test, but I wanted to point it out.

@brandon-leapyear
Copy link
Contributor Author

brandon-leapyear commented Nov 29, 2021

This is an old work account. Please reference @brandonchinn178 for all future communication


Seems like ghc-9.2.1 is bundled with time-1.11.1.1, which changed the Read UTCTime instance.

>>> reads "2018-02-27 10:49:42.123 UTC" :: [(UTCTime, String)]
[(2018-02-27 10:49:42 UTC,"123"),(2018-02-27 10:49:42.1 UTC,"23"),(2018-02-27 10:49:42.12 UTC,"3"),(2018-02-27 10:49:42.123 UTC,"")]

Updated the code and tested that the test now passes.

case parse8601 s <|> parsePretty s of
Nothing -> Left $ fromPersistValueParseError "UTCTime" x
Just x' -> Right x'
matches -> Right $ fst $ last matches
Copy link

@andreasabel andreasabel Dec 17, 2021

Choose a reason for hiding this comment

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

Leafing through this PR, this was the only change that stuck out as not obvious.

  • Why switch from first match to last match?
  • Could this application of last be made statically error-free by using Data.List.NonEmpty.last?

I'd say a comment in the code, justifying the choice of last match, would be in order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I mentioned this in the commit message, but the new time package bundled with GHC 9.2 changed the Read UTCTime instance to be more forgiving and provide partial matches.

-- old time
>>> reads "2018-02-27 10:49:42.12 UTC" :: [(UTCTime, String)]
[(2018-02-27 10:49:42.12 UTC,"")]

-- new time
>>> reads "2018-02-27 10:49:42.12" :: [(UTCTime, String)]
[(2018-02-27 10:49:42 UTC,"12"),(2018-02-27 10:49:42.1 UTC,"2"),(2018-02-27 10:49:42.12 UTC,"")]

I'll add a comment and use NonEmpty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Excellent!

@bergmark
Copy link
Contributor

This can be rebased now since the aeson 2 changes were merged separately.

@brandon-leapyear brandon-leapyear changed the title Fix for aeson-2 and template-haskell-2.18 Fix for GHC 9.2 / template-haskell-2.18 Jan 21, 2022
@brandon-leapyear
Copy link
Contributor Author

brandon-leapyear commented Jan 21, 2022

This is an old work account. Please reference @brandonchinn178 for all future communication


@bergmark thanks! Done

Comment on lines +25 to +34
- github: haskell-foundation/foundation
commit: 0bb195e1fea06d144dafc5af9a0ff79af0a5f4a0
subdirs:
- basement
# https://github.com/vincenthz/hs-memory/pull/87
- github: vincenthz/hs-memory
commit: 3cf661a8a9a8ac028df77daa88e8d65c55a3347a
# https://github.com/haskell-crypto/cryptonite/pull/354
- github: haskell-crypto/cryptonite
commit: 3b081e3ad027b0550fc87f171dffecbb20dedafe
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unfortunate. I wonder if we can drop these dependencies...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like commenting out persistent-redis and persistent-mongoDB from stack.yaml builds without these dependencies, but I can't figure out how those libraries use these libraries

Copy link
Collaborator

Choose a reason for hiding this comment

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

using stack dot --external and grepping the file, looks like persistent-mongoDB depends on mongoDB depends on bson depends on cryptohash-md5 depends on crpyothash depends on cryptonite.

Copy link
Contributor Author

@brandon-leapyear brandon-leapyear Jan 22, 2022

Choose a reason for hiding this comment

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

Ah and persistent-redis depends on hedis which depends on tls which depends on cryptonite

Maybe those two should be broken out into a separate repo?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, no, it's mongoDB -> cryptohash -> cryptonite. cryptohash-md5 appears to be factored out to avoid that dependency entirely, but mongoDB appears to use hmac and sha1 as well

Copy link
Contributor

Choose a reason for hiding this comment

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

There is cryptohash-sha1. And there is crypto-api which is maintained by @TomMD , I think.

@parsonsmatt
Copy link
Collaborator

Closing from #1367 - thanks for the contribution! ❤️

@brandon-leapyear brandon-leapyear deleted the chinn/aeson2 branch March 14, 2022 19:00
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.

7 participants