-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add support for summoning GHC 9.2.8, 9.4.8, and 9.6.6 #570
Conversation
13accba
to
1a84038
Compare
@willbasky I've implemented your suggestion, thanks. @vrom911 can you please review this? |
@noughtmare The tests need the updates. |
I've used |
What a nice tool! |
1a1c758
to
f7094af
Compare
It seems the CI also needed updating. |
Oh, of course summoner itself doesn't build on 9.6.2 |
And that's blocked on shellmet, at least. |
< 9.4 yes, building summoner with 9.4.5 and 9.6.2 has no ways right now. Also, instead of |
But if we can't build on 9.4.5 and 9.6.2 then we also can't test if summoning those versions work, except perhaps if we would build summoner with a different version of GHC than what we use to test the summoned project. |
Do the golden tests check if the 'project that is made with specific ghc in configs' is built with that ghc?
|
I think you're right that the tests only check things like 1, but then I don't understand why it did not work before I changed the CI. I'll revert that and we can try again. |
f7094af
to
d50b371
Compare
Neat, the old results are still here. Ah, so it's actually a separate CI action that tries to build the minimal project. |
e8ee0f3
to
467c2c3
Compare
If checks within |
I've now added 9.4.5 too |
Now it should work... |
975b811
to
c45c02c
Compare
I've squashed the commits |
@willbasky there will be a 9.4.7 soon, so I think we should skip 9.4.6. |
Hi, GHC 9.4.7 is now available, and slated to be the final release of the 9.4 series. Anything blocking this from merging? |
@kowainik @vrom911 @noughtmare @willbasky What is the plan to proceed with this PR to enable summoner to support GHC 9.2, 9.4, and 9.6? A new version of |
@Vekhir I think this package has been abandoned. I'm personally not motivated enough to fork and initiate the package takeover process. So I think not much is going to change any time soon. |
Alright, thanks for your reply. |
.github/workflows/ci.yml
Outdated
|
||
- name: Configure | ||
- if: matrix.ghc != '9.4.5' && matrix.ghc != '9.6.2' | ||
name: Configure |
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 am a kowainik assistant maintainer and I can merge this. However, could you please first explain why some of the steps have been disabled for 9.4.5 and 9.6.2?
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.
@tomjaguarpaw Not OP, but see this comment here: #570 (comment)
Also see the summoner.cabal
:
summoner/summoner-cli/summoner.cabal
Line 77 in 30b5604
build-depends: base >= 4.11 && < 4.17 |
and
summoner-tui.cabal
:summoner/summoner-tui/summoner-tui.cabal
Line 31 in 30b5604
build-depends: base >= 4.11 && < 4.17 |
In essence, summoner
doesn't build with 9.4 and 9.6 due to too restrictive base
(<4.17). It can still summon them, just doesn't build with them. 9.4 can probably be just bumped, but 9.6 has a compile issue: #575
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.
OK, let's fix summoner first to build with 9.4 and 9.6 rather than adding these special cases.
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.
Yeah, @Vekhir is right. At least at the time some of summoner's dependencies were not updated yet to those GHC versions.
@tomjaguarpaw do you want me to try building it again (and I could add some newer GHC versions while I'm at it)? Or do we want to merge this first and fix the other things later?
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'll get summoner building on the latest GHCs, and when that's done you can rebase this MR on top (hopefully without special cases).
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.
OK, main
now builds on up to 9.6. Could you please rebase your branch and remove the special cases?
c45c02c
to
a03cdaf
Compare
I've rebased and changed 9.4.5 to 9.4.8 and 9.6.2 to 9.6.6 with the latest stackage lts. |
Oh, now the golden tests have changed again |
a03cdaf
to
7720ada
Compare
Now it might work. There are still two things that could be updated:
|
Cool, thanks! If you feel like making the other updates then please do, but for things like tested-with I find them too difficult to keep up to date if they're not checked by tooling. |
Feel free to create another PR with the updates above. @noughtmare @willbasky @Vekhir thanks for pushing this through! |
@tomjaguarpaw Thanks for merging! Quick question: Do you also have the permissions to push Hackage releases? That would be quite useful in combination with this PR. |
I've asked Veronika and she says she can give me Hackage permissions in a couple of weeks. |
Alright. |
I've also updated the latest Stackage snapshots of 8.10.7 and 9.0.2.