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.8 and GHC 9.10 #130

Merged
merged 5 commits into from
Feb 7, 2025
Merged

Support GHC 9.8 and GHC 9.10 #130

merged 5 commits into from
Feb 7, 2025

Conversation

DigitalBrains1
Copy link
Member

@DigitalBrains1 DigitalBrains1 commented Jan 27, 2025

This PR includes the commits of PR #110 (which I initially had not noticed when I opened this PR).

Updates the circuit-notation plugin to current master.

Protocols.Internal actually contains quite some orphan instances regardless of clash-prelude version now.

Rowan or Tim, could you verify I did the right thing in the Fix partial function warnings in PacketStream commit? By the time I got to makePropPacketArbiter I was frankly spent and my error message is pretty non-helpful.

Funny thing: head has a warning but last doesn't. So we could just rewrite every use of head to last . reverse. For some reason I didn't.

@DigitalBrains1
Copy link
Member Author

Yeah, I can see how I screwed up with depacketizerModel. I'd still really like a review from someone who has more than read-only experience with PacketStream.

@DigitalBrains1 DigitalBrains1 force-pushed the add-ghc910 branch 6 times, most recently from 6b49557 to e66339f Compare January 28, 2025 01:45
Copy link
Member

@rowanG077 rowanG077 left a comment

Choose a reason for hiding this comment

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

Seems very uncontroversial to me. Once you got CI working it can be merged from my perspective.

@DigitalBrains1 DigitalBrains1 force-pushed the add-ghc910 branch 11 times, most recently from 1ef4e3e to be19a10 Compare January 28, 2025 02:26
@bgamari
Copy link
Collaborator

bgamari commented Jan 28, 2025

I suppose this supercedes #110?

@DigitalBrains1
Copy link
Member Author

I suppose this supercedes #110?

Oh! I've hardly ever worked on this repository and completely forgot to look at existing PR's.

My suggestion is to incorporate those commits into this PR, do you agree? I can also do it the other way around but that would require pushing to your fork; it seems simpler to use this branch.

bgamari and others added 4 commits January 28, 2025 13:01

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Only clash-protocols-base needs to depend upon ghc.
GHC 9.8 and 9.10 give a lot of Haddock warnings. We used to check the
Haddock for all GHC versions, but this is overkill. Just check it on GHC
9.6 only.

We used to first invoke `cabal build` and then
`cabal build --enable-documentation`. But this rebuilds all
dependencies, effectively doubling running time.

The `allow-newer` on `circuit-notation` is superfluous.

Further details: Haddock for GHC 9.8 and up generate more information
about type family instances. However, this also leads to "could not find
link destinations for" warnings. Given an `instance C A`, it would seem
that when it lists the instance under the Haddock of the definition of
`C`, it tries to create a link to the listing of the instance under the
Haddock of the definition of `A`, and fails somehow. I suspect a bug in
Haddock rather than something we can fix.
@bgamari
Copy link
Collaborator

bgamari commented Jan 28, 2025

I suppose this supercedes #110?

Oh! I've hardly ever worked on this repository and completely forgot to look at existing PR's.

My suggestion is to incorporate those commits into this PR, do you agree? I can also do it the other way around but that would require pushing to your fork; it seems simpler to use this branch.

Yes, feel free the take my changes. I just wanted to make sure we avoided duplication of work where possible.

This was referenced Jan 28, 2025
Copy link
Member

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

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

Seems uncontroversial :)

Comment on lines +7 to +8
sed <.ci/cabal.project.local.in >cabal.project.local "
s/__CHECK_HADDOCK__/$check_haddock/"
Copy link
Member

Choose a reason for hiding this comment

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

Why not sed -i?

Copy link
Member Author

Choose a reason for hiding this comment

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

But then you'd still have to copy the file, right?

If I try to use --in-place, I end up with something like:

sed .ci/cabal.project.local.in -i -e "
    s/__CHECK_HADDOCK__/$check_haddock/"
cp .ci/cabal.project.local.in cabal.project.local

@DigitalBrains1 DigitalBrains1 merged commit fb44438 into main Feb 7, 2025
10 checks passed
@DigitalBrains1 DigitalBrains1 deleted the add-ghc910 branch February 7, 2025 08:58
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

4 participants