-
Notifications
You must be signed in to change notification settings - Fork 164
Update flake #3112
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?
Update flake #3112
Conversation
1ba5d8c to
76353ee
Compare
|
I also ran into: #2823 in nixpkgs it's also patched out on >=9.6: https://github.com/NixOS/nixpkgs/blob/4cb741a91fd65971689b6272fd1fdc67355cd3e5/pkgs/development/compilers/ghc/ghc-9.6-llvm-use-new-pass-manager.patch So I applied @christiaanb suggestion to remove it entirely. |
cf6bc92 to
1b9d874
Compare
|
Looks good overall! I thought it maybe had to do with the removal of |
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 really thought we deliberately didn't use 9.10.3. Also please see #3112 (comment). Without adding it to CI, I really think we can't put it in our flake. And I suspect CI will fail.
[edit]
I'll run GitLab CI on it.
[/edit]
Perhaps it's the Lines 43 to 46 in 2423678
clash-compiler/.ci/gitlab/test.yml Lines 157 to 160 in 2423678
|
|
Heh, silly me. If I would like to test 9.10.3 on GitLab CI, I'd first need to create a Docker image for that. I'll try a local run. @martijnbastiaan It wasn't an accident that we did not use 9.10.3, right? Something was up with it? I've forgotten what, but if we'd supported 9.10.3, we'd have done so to stay on Stackage. [edit] [edit 2] [edit 3] |
|
I have no idea @DigitalBrains1, sorry. It wouldn't surprise me if it was deliberate though, every other GHC release seems to be broken in some way. |
|
Maybe I'm just mixing stuff up with GHC 9.12, though. GHC 9.12 is broken for us, and that is why we are not in Stackage nightly. But I'm not sure what the situation is for 9.10. I think we can switch to 9.10.3, provided we add it to GitLab CI, because we want to know when we break something. @christiaanb Do you agree? Or do you remember why we're still on 9.10.2? |
|
IMO just do it. If it didn't work, we should have left a note.. |
|
Right, I wasn't very explicit, but I also meant, let's upgrade to 9.10.3. |
|
By the way, I'm going to do a Hackage revision of That way, we can get into Stack LTS 24 without any fussing about with disabling tests because the version is out-of-bounds. Which brings me to this question: we're going to backport this PR to 1.8, right? Our own downstream projects use the flake from the stable Clash release, so it'd be good to get this change into an upcoming stable release. We definitely don't want the next release to shrink its bounds on |
|
@rowanG077 , I've created a branch peter/nix-ghc-minor-update where I did all the things needed to bump to 9.10.3 in CI. I created and published new Docker images, which is the major part of it. I also noted that the script wasn't updated to drop old versions of GHC we no longer support in I suggest you include the commit in this PR. Of course, the BTW, the backport to 1.8 of this PR will need some more work for the changes to CI. I'll do that as well (it's not much work if you've done it before); just don't expect a backport to work right away. |
|
@DigitalBrains1 Awesome! Will take a look at this later this week and finalize this! |
1b9d874 to
da40b2e
Compare
DigitalBrains1
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.
My requested changes have been taken care of, approved.
However, I think you will still need to do something about 9.8.4 on Nix. I suspect ffi-interface-tests is your culprit #3112 (comment).
da40b2e to
a103740
Compare
|
The reason the clash-ffi build succeeds on GHC 9.10.3 and not on GHC 9.8.4 seems to be that cabal errors on empty package definition with the cabal that is used with GHC 9.8.4 but no longer with GHC 9.10.3. What I have done now is marked @jaschutte To do this I had to change the order of the overlays. With I think if @jaschutte is oke with this and CI passes it can be merged. |
08e1ee0 to
bf88deb
Compare
bf88deb to
feb0ff3
Compare
|
Scratch that, It seems that
|
There seems to be no reason this was not a test-suite to begin with. Making it a test-suite makes it automatically build and ran for nix builds of clash-ffi.
7774a32 to
973e91a
Compare
I don't remember the exact issue, but it was some problem with adding C sources as part of the test-suite. Happy to hear if the issue got fixed such that we can convert it to a proper test-suite now. |
This reverts commit 1e6967a.
This reverts commit 973e91a.
It is just for `clash-ffi`, but we are about to disable that
Co-authored-by: Rowan Goemans <[email protected]>
e3a7e78 to
2330bb4
Compare
| -- TODO: remove this once https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12264#note_602406 | ||
| -- is fixed | ||
| if impl(ghc < 9.8.3) || impl(ghc == 9.10.1) | ||
| if impl(ghc < 9.8.3) || impl(ghc == 9.10.1) || impl(ghc > 9.10.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.
Why not:
if impl(ghc < 9.8.3) || impl(ghc >= 9.10.1)
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.
Because GHC 9.10.2 has the issue.
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.
Of course we could do
if impl(ghc < 9.8.3) || impl(ghc > 9.10.2)
and just ignore the fact that 9.10.1 would actually work.
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.
Ah, perhaps we should reverse the logic and do:
-- GHC 9.8.4 and 9.10.2 have issues, see https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12264#note_602406
if impl(ghc == 9.8.4) || impl(ghc == 9.10.2)
buildable: False
else
buildable: True
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.
You're thinking nobody will be trying 9.8.3 anymore? In that case, it would indeed match everything in practice.
What happened to 9.8.3? :-) Do you know why it was pulled from GHCup? I couldn't find anything at a really quick glance, and my dinner is cooling.
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.
Ah, sorry, I missed that 9.8.3 also has the bug... so yeah, then it would be:
-- GHC 9.8.3, 9.8.4 and 9.10.2 have issues, see https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12264#note_602406
if impl(ghc == 9.8.3) || impl(ghc == 9.8.4) || impl(ghc == 9.10.2)
buildable: False
else
buildable: True
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.
Perhaps we can just leave the logic as is, but update the comment to specify which GHCs are affected
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.
Fine by me! What do we do with
-- TODO: remove this once https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12264#note_602406
-- is fixedI'm not sure what it suggests we do, but I think it's saying: once GHC 9.8 and GHC 9.10 both have received a minor version update that fixes the bug, remove this stanza. I wonder whether that would even be a good idea.
Also, after reading the discussion about 9.8.3 I linked to in my previous message, I doubt there will ever be another 9.8 release.
You wrote the comment; what did you intend?
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.
Ah, I meant that when a fix was released, we should properly fix the bounds. The original version didn’t allow building on e.g. GHC 9.12
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.
Okay I've updated the comments from
-- TODO: remove this once https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12264#note_602406
-- is fixed
to
-- GHC 9.8.3, 9.8.4 and 9.10.2 have issues, see https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12264#note_602406
I note that in .ci/build.sh the phrasing was a bit different:
Lines 42 to 44 in d0f65c4
| # TODO: remove this and put it back into tests when | |
| # https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12264#note_602406 | |
| # is fixed |
I think it'll be quite a while before we can actually put it back into tests; I suppose that will be once we drop 9.8 support from Clash. In the interest of clarity and maintainability, I've rewritten it a bit so it doesn't duplicate shell code and is easier to grok.
Are you satisfied with how it is now, @christiaanb ?
| -- TODO: remove this once https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12264#note_602406 | ||
| -- is fixed | ||
| if impl(ghc < 9.8.3) || impl(ghc == 9.10.1) | ||
| if impl(ghc < 9.8.3) || impl(ghc == 9.10.1) || impl(ghc > 9.10.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.
Why not
if impl(ghc < 9.8.3) || impl(ghc >= 9.10.1)
Nope! That should be good. I don't know what depends on Just to be sure I |
This updates the nix flake to the newest nixos-unstable. Fully updates to the most recent GHC minor version as well as bump the typelit plugins.