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

Various changes to make it compile with MicroHs. #1043

Merged
merged 12 commits into from
Oct 4, 2024

Conversation

augustss
Copy link
Contributor

Mostly #ifdefs and some type signatures.

Mostly #ifdefs and some type signatures.
@treeowl
Copy link
Contributor

treeowl commented Sep 19, 2024

Thank you! This is a very welcome development. I don't see any changes to CI. Could we at least compile the library on MicroHS in CI, even if dependencies don't yet let us compile the test suite?

@augustss
Copy link
Contributor Author

augustss commented Sep 19, 2024 via email

@treeowl
Copy link
Contributor

treeowl commented Sep 19, 2024

I imagine so, though I'm not at all an expert in the ways of haskell-ci.

@meooow25
Copy link
Contributor

What exactly are the differences between getting something to compile on GHC and MicroHs. It seems to be mostly about certain modules not available in MicroHs base?

And +1 on having a CI so we don't break this in the future.

@meooow25
Copy link
Contributor

By the way, I am having to manually approve these CI runs. It seems that this is the default for "first time contributors" and I don't see a way to change it. treeowl could you peek at the settings to see if we can change this (perhaps temporarily) to make things easier?

@treeowl
Copy link
Contributor

treeowl commented Sep 19, 2024

I don't think I have the authority to change that. I think we'd need a haskell owner or something.

@augustss
Copy link
Contributor Author

Yes, getting it to run with MHS is mostly a matter of #ifdef-ing out things that base does not support (and cannot support right now, since it needs extensions like QualifiedConstraints).
There's also a semantics difference to the scoping of type variables in signatures. I might change that for easier porting.

@treeowl
Copy link
Contributor

treeowl commented Sep 19, 2024

Would it make sense on MicroHs to have a local copyish of Control.Category to limit the CPP and offer that interface more universally?

@augustss
Copy link
Contributor Author

I've added CI for MHS. It's a little clunky right now, but I hope to improve it.

@augustss
Copy link
Contributor Author

I'll add Control.Category and update the PR.

Copy link
Contributor

@meooow25 meooow25 left a comment

Choose a reason for hiding this comment

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

If QuantifiedConstraints is a problem, MicroHs could define the pre-QuantifiedConstraints versions of the classes (Eq1, Ord1, Bifoldable, etc). This would let us get rid of most of the CPP.

containers/src/Data/Map/Internal.hs Outdated Show resolved Hide resolved
containers/src/Data/Graph.hs Outdated Show resolved Hide resolved
@augustss
Copy link
Contributor Author

augustss commented Sep 19, 2024

Just for your amusement: containers has 25kloc of Haskell code and MHS has 11kloc. :)

@augustss
Copy link
Contributor Author

I'm making the MHS base library more GHC compatible. The PR will be updated soon.

@treeowl
Copy link
Contributor

treeowl commented Sep 20, 2024

Don't compromise your principles too much, but the less CPP we have to deal with the better.

@augustss
Copy link
Contributor Author

I've made my changes. Please have a look again.

Copy link
Contributor

@meooow25 meooow25 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, this is looking much nicer!

.github/workflows/mhs-ci.yml Show resolved Hide resolved
containers/src/Data/IntMap/Internal.hs Show resolved Hide resolved
containers/src/Data/Map/Internal.hs Show resolved Hide resolved
containers/src/Data/Sequence/Internal.hs Outdated Show resolved Hide resolved
containers/src/Data/Sequence/Internal.hs Show resolved Hide resolved
containers/src/Data/Tree.hs Outdated Show resolved Hide resolved
containers/src/Utils/Containers/Internal/Prelude.hs Outdated Show resolved Hide resolved
containers/src/Data/IntMap/Internal.hs Show resolved Hide resolved
@meooow25 meooow25 merged commit 24b0b3a into haskell:master Oct 4, 2024
12 checks passed
@meooow25 meooow25 mentioned this pull request Oct 4, 2024
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.

3 participants