-
Notifications
You must be signed in to change notification settings - Fork 187
Switch from template-haskell to template-haskell-lift #1162
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
base: master
Are you sure you want to change the base?
Conversation
4ccd3a1 to
ad2e782
Compare
|
Why doesn't the new package support older GHC versions? The CPP seems a bit blecherous. |
|
template-haskell-lift is compatible with GHC>=8.10. But it is only a boot library with GHC>=9.14. So you wouldn't be able to backport new releases of containers to older GHCs as a boot lib (they would build fine from Hackage though). Let me know what you'd prefer.
Eventually though, |
|
That's only a potential issue for new releases on old GHC branches, right? I suppose it would be okay to have some CPP temporarily to support that as long as such branches are "supported" by GHC HQ. But wouldn't it be cleaner to just add |
|
Yeah I agree that would be optimal. Unfortunately I don't think we can add new boot libs in minor releases (but I'll ask) since that can be disruptive. I can commit to removing this CPP when it's no longer necessary though. |
150bcfc to
cb380cd
Compare
|
Now that GHC-9.14 is released we can test this. |
|
Any idea what's going wrong with MucroHs? It smells like a CPP formatting issue, but I can't find it. |
I think that it doesn't like the |
meooow25
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MicroHs version was updated in #1177. Hopefully the CI will pass after a rebase.
| import Control.Monad.Fix (MonadFix (..)) | ||
| import Test.Tasty.HUnit | ||
| import Test.ChasingBottoms.IsBottom (isBottom) | ||
| # if __GLASGOW_HASKELL__ >= 914 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is minor, but let's not have spaces after #. I think it would be less surprising (I'm surprised to learn that this is acceptable syntax).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it doesn't belong here, but in nested conditions it's very helpful to see what's going on visually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot. That was a copy-paste error
| # if __GLASGOW_HASKELL__ >= 914 | ||
| import Language.Haskell.TH.Lift (Lift(..)) | ||
| # else | ||
| import Language.Haskell.TH.Syntax (Lift(..)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change from Lift to Lift(..)? Same for other files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's needed yeah.
containers/src/Data/Set/Internal.hs
Outdated
| import Language.Haskell.TH () | ||
| import Data.Coerce (coerce) | ||
| # endif | ||
| import Data.Coerce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo. Fixed now
| # else | ||
| import Language.Haskell.TH.Syntax (Lift(..)) | ||
| -- See Note [ Template Haskell Dependencies ] | ||
| import Language.Haskell.TH () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we don't need this any more if we depend on template-haskell-lift? The current reason is documented in note-template-haskell-deps.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep in fact we haven't needed it since GHC 9.12, I think, when these things moved to ghc-internal from template-haskell
This new boot library should be more stable than template-haskell and should eventually allow us to remove much of the CPP around TH. It will also make it easier for end-users to reinstall template-haskell as it will no longer be used by any boot libraries
This new boot library should be more stable than template-haskell and
should eventually allow us to remove much of the CPP around TH.
It will also make it easier for end-users to reinstall template-haskell
as it will no longer be used by any boot libraries
This GHC MR tests this PR against GHC-HEAD: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/14978