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

Support GHC 9.2. #436

Merged
merged 11 commits into from
Dec 27, 2021
Merged

Support GHC 9.2. #436

merged 11 commits into from
Dec 27, 2021

Conversation

patrickt
Copy link
Contributor

@patrickt patrickt commented Nov 1, 2021

Widens template-haskell bounds and removes mappend definitions in favor of defaulting to <>, as per -Wnoncanonical-monoid-instances.

Widens template-haskell bounds and removes `mappend` definitions in
favor of defaulting to `<>`, as per `-Wnoncanonical-monoid-instances`.
@patrickt
Copy link
Contributor Author

patrickt commented Nov 7, 2021

This is ready for review.

@patrickt
Copy link
Contributor Author

@jacobstanley ping

@weebl2000
Copy link

Cool.

@tmcgilchrist
Copy link
Contributor

Could you add this GHC version to the GH CI setup so it gets fully tested.
https://github.com/hedgehogqa/haskell-hedgehog/blob/master/.github/workflows/ci.yaml#L17

Seems like a maintainer needs to approve the PR so CI gets kicked off.

@patrickt
Copy link
Contributor Author

patrickt commented Dec 3, 2021

@jacobstanley Any word on this?

@smunix
Copy link

smunix commented Dec 4, 2021

@moodmosaic ping

Copy link
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

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

👍 🚀 🍻

@moodmosaic
Copy link
Member

It looks like some builds are failing. Have to investigate on that.

@gwils
Copy link
Contributor

gwils commented Dec 5, 2021

It looks as though you will need to use CPP to give mappend an explicit definition on GHC 8.2 and 8.0, in order to make -Werror happy.

@moodmosaic
Copy link
Member

It looks as though you will need to use CPP to give mappend an explicit definition on GHC 8.2 and 8.0, in order to make -Werror happy.

Could you do that, @patrickt? Otherwise we'll get to it as soon as possible.

@patrickt
Copy link
Contributor Author

patrickt commented Dec 5, 2021

@moodmosaic On it!

@ysangkok
Copy link
Contributor

ysangkok commented Dec 5, 2021

@patrickt It is better to define mappend = (<>) on versions of base where Semigroup is a superclass of Monoid. Otherwise this warning is triggered: https://downloads.haskell.org/~ghc/9.2.1/docs/html/users_guide/using-warnings.html#ghc-flag--Wnoncanonical-monoid-instances . I believe this would be GHC 8.0 and 8.2 according to the linked docs. EDIT: The last sentence was meant to refer to versions where it is not a superclass.

@patrickt
Copy link
Contributor Author

patrickt commented Dec 5, 2021

@moodmosaic I’ve tested this with 8.2 and 8.0 locally; can I get another approval/merge if it looks good to you?

@moodmosaic
Copy link
Member

@moodmosaic I’ve tested this with 8.2 and 8.0 locally; can I get another approval/merge if it looks good to you?

Sure, I just run it. 🍻

@moodmosaic
Copy link
Member

@patrickt, some are still failing but we're almost there.

@patrickt
Copy link
Contributor Author

patrickt commented Dec 5, 2021

@moodmosaic Ugh, my bad. This last one should be okay.

@moodmosaic
Copy link
Member

@moodmosaic Ugh, my bad. This last one should be okay.

No problem! Hmm, I think there's one more left to go 👀

@moodmosaic
Copy link
Member

@patrickt, I know this is getting boring at this point 😴 It looks like some checks are still failing.

@moodmosaic
Copy link
Member

Yep, looking at the build error it looks like changes in 94dd5b8 must also be applied in Registry.hs. @patrickt, let me know if you'd want me to do that.

@ysangkok
Copy link
Contributor

@patrickt looks like a new warning in 9.2 makes the build fail:

src\Test\Example\Resource.hs:174:7: error: [-Wincomplete-uni-patterns, -Werror=incomplete-uni-patterns]
Error:     Pattern match(es) are non-exhaustive
    In a pattern binding:
        Patterns of type ‘([String], [String])’ not matched:
            ([], [])
            ((_:_), [])
    |
174 |       (xs, x : ys) =

    |       ^^^^^^^^^^^^^^^...

@patrickt
Copy link
Contributor Author

Sorry for the churn, everyone. I’ve turned on -Werror locally and observed a clean build. Hopefully we’re good to go.

@gwils
Copy link
Contributor

gwils commented Dec 27, 2021

Perhaps a maintainer could trigger a CI build to verify this? @moodmosaic

@moodmosaic
Copy link
Member

Sorry for the churn, everyone. I’ve turned on -Werror locally and observed a clean build. Hopefully we’re good to go.

No problem at all! 🍻

@moodmosaic
Copy link
Member

Perhaps a maintainer could trigger a CI build to verify this? @moodmosaic

@gwils, CI build started 🚀

@moodmosaic moodmosaic merged commit 21a5131 into hedgehogqa:master Dec 27, 2021
@moodmosaic
Copy link
Member

Thank you, @patrickt 🍻 Also thank you @ysangkok, @gwils, @tmcgilchrist for the great feedback 🍺

@ysangkok
Copy link
Contributor

ysangkok commented Jan 27, 2022

I think this was released as part of https://github.com/hedgehogqa/haskell-hedgehog/releases/tag/1.1, with bounds similar to the ones in this PR at https://hackage.haskell.org/package/hedgehog

@patrickt patrickt deleted the ghc-9.2 branch January 27, 2022 16:40
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.

None yet

7 participants