Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make it compile with MicroHs #190

Closed
wants to merge 3 commits into from
Closed

Conversation

augustss
Copy link

@augustss augustss commented Jan 4, 2025

Two changes

  • MicroCabal cannot parse everything cabal can
  • MicroHs always has MonoLocalBinds, so some type signatures are required.

@@ -86,7 +85,7 @@ library
UndecidableInstances

ghc-options: -Wall
if impl(ghc >= 8.0)
if impl(ghc >= 8.0) || impl(mhs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually dropped the support for pre GHC-8.0, so this conditional's else-branch can be removed. Do you mind?

I'm not sure whether Hackage is using new enough Cabal to accept impl(mhs) .

Copy link
Author

Choose a reason for hiding this comment

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

You can remove the else branch if you want.

Cabal accepts mhs as a valid compiler now, I believe.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm. Maybe the CI is using some older cabal?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cabal accepts mhs as a valid compiler now, I believe.

newer Cabal does. But hackage-server still uses old Cabal, so this change could not be released. (And it might take quite a long time).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

@phadej phadej Jan 4, 2025

Choose a reason for hiding this comment

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

@Bodigrim good to know. But given a thought, if the thing can be written without impl(mhs), it should.

You help me, I help you. I'm sorry, but I don't care about MicroHS, so if I can be unaware I will.

That said, the first moment the CI breaks, I will remove it; so adding CI for microhs is somewhat unnecessary too. I won't put any effort into it.

TL;DR my stance is: GHC is the only compiler out there. The ship for multi-compiler ecosystem has long sailed away.

Copy link
Author

Choose a reason for hiding this comment

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

If there is no support for pre-8.0 GHC then the condition can be simplified and it will work with MicroHs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I started this discussion with saying

I actually dropped the support for pre GHC-8.0, so this conditional's else-branch can be removed. Do you mind?


This left a bad taste. I removed the conditional, so AFAICT this patch has only MonoLocalBinds changes which are relevant.

Copy link
Collaborator

@phadej phadej left a comment

Choose a reason for hiding this comment

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

For a time being, I won't accept changes using impl(mhs).

@phadej
Copy link
Collaborator

phadej commented Jan 5, 2025

... but I don't really care anymore. I see there are a forks of transformers and mtl, so I guess you will have to maintain a fork of parsec too.

@phadej phadej closed this Jan 5, 2025
@augustss
Copy link
Author

augustss commented Jan 5, 2025

The MicroHs changes to transformers have been accepted a long time ago. But since the master copy of the source is in darcs I have a copy in my GitHub repo.
I've not submitted a PR for mtl yet, but I will soon.
I only keep a (divergent) fork until the changes have been accepted. It's worked smoothly with other packages like containers and time.

@augustss
Copy link
Author

augustss commented Jan 5, 2025

Ah, I miunderstood you comment about dropping support for 8.0. I thought you'd done it, but not yet changed the parsec.cabal file, but it was just that I hadn't synced my repo. My bad.
Thanks for the change, that's excellent!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants